Revert 8c3988dff9 and de54c58838 This simplifies a larger following patchset. The intent is that these set of patches will be committed either as a single change, or back-back, to avoid any regressions.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Everything those commits did is necessary for V1. It's not sufficient per the spec, granted, but having the commits to support _some_ V1 implementations is strictly better than supporting _none_. If you have commits to introduce the missing parts of the V1 feature negotiation sequence then add them, but don't revert what's already there only to reintroduce it later. Otherwise you will break TinyEMU and the FPGA-based system we have built atop its VirtIO implementation.
Comment Actions
It is so wrong and incomplete that I have zero interest in trying to sort out the merge conflicts in the patches series I have for V1 support. This is so far away from _some_ V1 support that it is effectively _none_. Fix TinyEMU: it would appear it otherwise supports legacy but has a bug that requires VIRTIO_F_VERSION_1 to be negotiated.
I pointed out to you privately at the time that the change to vtnet_rx_header broke other assumptions but I never got a response, so there is little that is salvageable from these commits.
Comment Actions
That is grossly incorrect. There are a couple of deficiencies, and as I just said, everything I did is required for V1.
I pointed out to you privately at the time that the change to vtnet_rx_header broke other assumptions but I never got a response, so there is little that is salvageable from these commits.
There was a regression that I fixed very quickly for one specific case that bhyve hit. I started to draft a response to your private email but quite frankly it was extremely rude and condescending so I gave up and decided it was best to not engage in such conversation. And saying it's "wrong and incomplete", "effectively _none_" and "there is little that is salvageable" just continues in that manner and I am not enthused about working with you to improve VirtIO support if that's how you want to treat my contributions.
Comment Actions
No, it is not just "a couple". I would advise you to read the VirtIO V1 spec. Or look at D27856 that starts to add V1 support. The OASIS TC did not spend years to develop a V1 specification that just ... negotiates VIRTIO_F_VERSION_1 on top of legacy VirtIO. Doing so here is so far from any V1 support that it is wrong & pointless.
What I don't understand is why not just fix TinyEMU? It must already basically support legacy because that is ultimately what ends up being used here, but it would appear to have a bug that requires VIRTIO_F_VERSION_1 to be negotiated. This is NOT "_some_ V1 implementation"; it is V-TinyEMU, with no good reason.
I pointed out to you privately at the time that the change to vtnet_rx_header broke other assumptions but I never got a response, so there is little that is salvageable from these commits.
There was a regression that I fixed very quickly for one specific case that bhyve hit. I started to draft a response to your private email but quite frankly it was extremely rude and condescending so I gave up and decided it was best to not engage in such conversation. And saying it's "wrong and incomplete", "effectively _none_" and "there is little that is salvageable" just continues in that manner and I am not enthused about working with you to improve VirtIO support if that's how you want to treat my contributions.
My email was not about that regression. For the record, here is the email I sent you:
Please be sure to add me (bryanv@) on any VirtIO code reviews in the future. I'm working on rebasing some older work I have to add actual V1 support.
Note that you broke the previous assumption around the RX padding by adding 2 bytes to the structure. And for this change in general, we should not be advertising half-baked V1 support like this. This is not non-legacy support. At quick glance, this looks to be exploiting non spec compliant behavior in the TinyEMU.
Please try to minimize changes in this area because it just makes rebasing V1 support harder.
I'm not quite sure what part is "extremely rude and condescending".
Yes, "... you broke the previous assumption around the RX padding by adding 2 bytes": by adding virtio_net_hdr_mrg_rxbuf to the vtnet_rx_header WITHOUT adjusting VTNET_RX_HEADER_PAD, the IP header no longer lands on a word boundary. We all introduce breakages, but being told so isn't rude.
That "we should not be advertising half-baked V1 support like this" is correct because this not spec compliant by any imagination, despite the commit message claiming otherwise. And, yes, "looks to be exploiting non spec compliant behavior in the TinyEMU." is correct because TinyEMU is using legacy but with VIRTIO_F_VERSION_1 flag for some reason.
"Please try to minimize changes in this area because it just makes rebasing V1 support harder." is just a request to limit any future changes in this code. I had spent a lot of time dealing with gratuitous diffs trying to rebase on top of 2 years of changes. I made a similar request to vmaffione@, and I am grateful that he was responsive and accommodating, while still being able to accomplish what he needed.
Since the change to vtnet_rx_header needs to be reverted, the majority of the if_vtnet changes fall out too, so there really isn't much to be salvaged. Much of the init/deinit & Tx/Rx paths have been rewritten, and by reverting these changes, it makes the diffs that actually add functional V1 support much clearer to a later reader.
"... if that's how you want to treat my contributions". The commits were wrong. But you really need to learn how to separate yourself from code and not take a revert as some personal affront.
Comment Actions
I have read the spec. MMIO v1 is not all that different from MMIO v0.95; PCI changed a fair amount, but not MMIO. And a lot of v1 is just optional additional flexibility/features that FreeBSD does not try to negotiate, so are irrelevant. As for the changes to the configuration registers themselves, those have already been implemented. The only thing missing is proper endianness handling for big-endian systems, something which VirtIO has a poor track record of anyway, and is only needed if you have both a modern device and a big-endian guest, something which wouldn't have worked before anyway.
What I don't understand is why not just fix TinyEMU? It must already basically support legacy because that is ultimately what ends up being used here, but it would appear to have a bug that requires VIRTIO_F_VERSION_1 to be negotiated. This is NOT "_some_ V1 implementation"; it is V-TinyEMU, with no good reason.
It's not legacy; see below.
I pointed out to you privately at the time that the change to vtnet_rx_header broke other assumptions but I never got a response, so there is little that is salvageable from these commits.
There was a regression that I fixed very quickly for one specific case that bhyve hit. I started to draft a response to your private email but quite frankly it was extremely rude and condescending so I gave up and decided it was best to not engage in such conversation. And saying it's "wrong and incomplete", "effectively _none_" and "there is little that is salvageable" just continues in that manner and I am not enthused about working with you to improve VirtIO support if that's how you want to treat my contributions.
My email was not about that regression. For the record, here is the email I sent you:
Please be sure to add me (bryanv@) on any VirtIO code reviews in the future. I'm working on rebasing some older work I have to add actual V1 support.
Note that you broke the previous assumption around the RX padding by adding 2 bytes to the structure. And for this change in general, we should not be advertising half-baked V1 support like this. This is not non-legacy support. At quick glance, this looks to be exploiting non spec compliant behavior in the TinyEMU.
Please try to minimize changes in this area because it just makes rebasing V1 support harder.
I'm not quite sure what part is "extremely rude and condescending".
Yes, "... you broke the previous assumption around the RX padding by adding 2 bytes": by adding virtio_net_hdr_mrg_rxbuf to the vtnet_rx_header WITHOUT adjusting VTNET_RX_HEADER_PAD, the IP header no longer lands on a word boundary. We all introduce breakages, but being told so isn't rude.
That's not a breakage though, that's just a performance regression, and can trivially be fixed by changing the 4 to a 2 (or, better, replacing it with 4 - (sizeof(union) % 4) so it's never wrong).
That "we should not be advertising half-baked V1 support like this" is correct because this not spec compliant by any imagination, despite the commit message claiming otherwise. And, yes, "looks to be exploiting non spec compliant behavior in the TinyEMU." is correct because TinyEMU is using legacy but with VIRTIO_F_VERSION_1 flag for some reason.
Yes the commit message could have been better. But it was a strict improvement in functionality. TinyEMU is not spec-noncompliant, it's just lenient in what it accepts in this case; whilst the spec says devices shouldn't consume descriptors until DRIVER_OK is set, is says nothing of the sort for FEATURES_OK. There's a difference between "exploiting non-spec-compliant behaviour" and "exploiting device-specific behaviour". And no, TinyEMU is definitely *not* legacy VirtIO, it's modern VirtIO with a limited feature set (and, in this case, MMIO specifically). The whole point behind these commits was that TinyEMU is using modern VirtIO and hence needs the mergeable RX buffers-style v1 header. It advertises itself as version 2 in the MMIO header, has VIRTIO_F_VERSION_1 in the device features, uses the modern RX buffer layout and uses the modern MMIO queue configuration registers (split, and with 64-bit address not a 32-bit PFN). It is not legacy.
"Please try to minimize changes in this area because it just makes rebasing V1 support harder." is just a request to limit any future changes in this code. I had spent a lot of time dealing with gratuitous diffs trying to rebase on top of 2 years of changes. I made a similar request to vmaffione@, and I am grateful that he was responsive and accommodating, while still being able accomplished what he needed.
See now this is conflicting. My change was a minimal one already (but you phrased that in such a way as to imply that this wasn't), and you're telling me to minimise changes (which I was already doing because I don't want to do more than is needed, and continued to do because it's good practice regardless), yet you're also telling me off for having an incomplete commit, so either way it seems I can't win and what you really want is for nobody to touch the VirtIO code at all. But we cannot sit around waiting for you to get round to rebasing a 2 year old patch set and finally submit it for review when we have a small commit that is needed for FreeBSD to support a platform we are using. I appreciate that it creates a small amount of additional effort when rebasing, but it should not be all that significant compared with the time taken to go through code review for such a large patch set.
Since the change to vtnet_rx_header needs to be reverted, the majority of the if_vtnet changes fall out too, so there really isn't much to be salvaged. Much of the init/deinit & Tx/Rx paths have been rewritten, and by reverting these changes, it makes the diffs that actually add functional V1 support much clearer to a later reader.
"... if that's how you want to treat my contributions". The commits were wrong. But you really need to learn how to separate yourself from code and not take a revert as some personal affront.
My response is not because of the revert in and of itself. It's because of (a) the content/tone of the commit message and (b) the fact that it's being put up for review without a follow-up to implement full v1 support. I really do not care if you want to rewrite everything I did in a way that's cleaner for your v1 code. I would however much prefer that that refactor just be folded into the commit that adds v1 support to vtnet like any other feature addition. However, if you really do not want to do that, then I am not going to object to the revert, provided:
- The commit message of the revert be changed to drop the accusations and instead just something along the lines of "These only implemented the subset of v1 support needed for TinyEMU. Revert these to allow for a cleaner full implementation of v1 support in the following commit."
- The revert commit be immediately followed by the vtnet v1 support commit (or at least in the same git push) so as to not regress TinyEMU support, and thus support for our FPGA development platform, in the tree
I don't want to keep arguing about this, it's not how I like spending my time. Are either of those approaches acceptable to you? If not, do you have suggestions for an alternative approach that doesn't involve inaccurate accusations?
Comment Actions
No. virtio_net_hdr_mrg_rxbuf does not belong in there in the first place. It was not needed for legacy mergeable buffers, and is not needed for V1 which effectively uses the same mergeable header.
That "we should not be advertising half-baked V1 support like this" is correct because this not spec compliant by any imagination, despite the commit message claiming otherwise. And, yes, "looks to be exploiting non spec compliant behavior in the TinyEMU." is correct because TinyEMU is using legacy but with VIRTIO_F_VERSION_1 flag for some reason.
Yes the commit message could have been better. But it was a strict improvement in functionality. TinyEMU is not spec-noncompliant, it's just lenient in what it accepts in this case; whilst the spec says devices shouldn't consume descriptors until DRIVER_OK is set, is says nothing of the sort for FEATURES_OK. There's a difference between "exploiting non-spec-compliant behaviour" and "exploiting device-specific behaviour". And no, TinyEMU is definitely *not* legacy VirtIO, it's modern VirtIO with a limited feature set (and, in this case, MMIO specifically). The whole point behind these commits was that TinyEMU is using modern VirtIO and hence needs the mergeable RX buffers-style v1 header. It advertises itself as version 2 in the MMIO header, has VIRTIO_F_VERSION_1 in the device features, uses the modern RX buffer layout and uses the modern MMIO queue configuration registers (split, and with 64-bit address not a 32-bit PFN). It is not legacy.
From the network _driver_ perspective, it is still behaving as legacy while claiming V1. Note that those were the only commits, not MMIO, reverted here.
"Please try to minimize changes in this area because it just makes rebasing V1 support harder." is just a request to limit any future changes in this code. I had spent a lot of time dealing with gratuitous diffs trying to rebase on top of 2 years of changes. I made a similar request to vmaffione@, and I am grateful that he was responsive and accommodating, while still being able accomplished what he needed.
See now this is conflicting. My change was a minimal one already (but you phrased that in such a way as to imply that this wasn't), and you're telling me to minimise changes (which I was already doing because I don't want to do more than is needed, and continued to do because it's good practice regardless), yet you're also telling me off for having an incomplete commit, so either way it seems I can't win and what you really want is for nobody to touch the VirtIO code at all. But we cannot sit around waiting for you to get round to rebasing a 2 year old patch set and finally submit it for review when we have a small commit that is needed for FreeBSD to support a platform we are using. I appreciate that it creates a small amount of additional effort when rebasing, but it should not be all that significant compared with the time taken to go through code review for such a large patch set.
There is a wide gulf between minimal changes, and wrong, incomplete. and non-compliant changes. I would have been fine, and grateful, if somebody else beat me to actual V1 support.
Since the change to vtnet_rx_header needs to be reverted, the majority of the if_vtnet changes fall out too, so there really isn't much to be salvaged. Much of the init/deinit & Tx/Rx paths have been rewritten, and by reverting these changes, it makes the diffs that actually add functional V1 support much clearer to a later reader.
"... if that's how you want to treat my contributions". The commits were wrong. But you really need to learn how to separate yourself from code and not take a revert as some personal affront.
My response is not because of the revert in and of itself. It's because of (a) the content/tone of the commit message and (b) the fact that it's being put up for review without a follow-up to implement full v1 support. I really do not care if you want to rewrite everything I did in a way that's cleaner for your v1 code. I would however much prefer that that refactor just be folded into the commit that adds v1 support to vtnet like any other feature addition. However, if you really do not want to do that, then I am not going to object to the revert, provided:
- The commit message of the revert be changed to drop the accusations and instead just something along the lines of "These only implemented the subset of v1 support needed for TinyEMU. Revert these to allow for a cleaner full implementation of v1 support in the following commit."
- The revert commit be immediately followed by the vtnet v1 support commit (or at least in the same git push) so as to not regress TinyEMU support, and thus support for our FPGA development platform, in the tree
I don't want to keep arguing about this, it's not how I like spending my time. Are either of those approaches acceptable to you? If not, do you have suggestions for an alternative approach that doesn't involve inaccurate accusations?
This is the first commit in a long patch series that does just that. Loading a series into Phab isn't a great process. The series will be, and was always going to be, committed together.