set-features should be idempotent?

[iximeow]

I have barely tested this, but I built a propolis-standalone and threw it + a Windows Server 2022 onto a Cosmo and I'm no longer seeing the results of Windows repeatedly resetting the device. so this gets #1052 and apparently #1048 with it.

this probably should just be a mutex that we take somewhere in managing virtio device configuration but this was simple enough to throw together for a quick PoC/validation..

[hawkw]

would like a comment on this explaining what it means and why it's tracked here?

[hawkw]

i found the description of the suspected issue in #1048 and #1052 quite useful and felt like something like that (though not necessarily from the angle of "it was broken") would be nice to capture closer to the code.

[iximeow]

yeah, i wasn't even really sure this is a great place to put it. i mostly just took what seemed to get windows happy, threw it in a commit, and put it up as a draft in case someone haad the time to take it further (also why a clippy lint survived to the push lol)

[hawkw]

clippy is kind of obnoxiously mad about alphabetization here :|

[dancrossnyc]

I think that Windows may be in error here; or, at least, there's some ambiguity in the spec (VirtIO 1.2).

Section 2.2 of the spec reads, in part:

Each virtio device offers all the features it understands. During device initialization, the driver reads this and tells the device the subset that it accepts. The only way to renegotiate is to reset the device.

(Emphasis added)

However, sec. 2.2.2 goes on to say,

If a device has successfully negotiated a set of features at least once (by accepting the FEATURES_OK device status bit during device initialization), then it SHOULD NOT fail re-negotiation of the same set of features after a device or system reset.

(Again, emphasis added)

What this says to me is that, once the driver has written the FEATURES_OK bit in the device status register, and this has been acknowledged by the device by similarly reflecting that bit back to the driver on a read, then the driver should not write to the features register again.

However, I could also imagine that most devices should be able to tolerate a read that doesn't change the value of the register; that is, if I read the register and write it right back, perhaps that should be a tragically complicated no-op. However, if the driver tries to change anything, then that ought to fail, either by clearing (and refusing to set) FEATURES_OK or by resetting.

[iximeow]

I read those sections a bit differently: taken together it implies to me that if you want to change features then you must reset (via writing device_status or system reset), but that if you were to read driver features and write back the same pattern, that's.. "fine? just, why would you do that" (and that the section you've emphasized is more of the spirit: "if this configuration worked when I hibernated a VM, it better work when I wake the VM back up")

either way, I think my initial understanding about who was set_features'ing when was incorrect. back in #1033 I'd tied set_use_pairs() and set_pairs() to resetting the PCI device. what I think happens here is: something negotiates multiqueue (maybe OVMF, maybe a bootloader, I don't think it's important what exactly), boot into the desired OS, and then that OS sets up its VirtIO driver. from the VirtIO spec that driver will write RESET to the device status register, and end up at set_status() -> virtio_reset(). that does not set_use_pairs()/set_pairs() back down to 1.

then if that driver tries to also negotiate multiqueue, we're back at #1052. we'll try to SET_PAIRS while some rings are out of reset, get EBUSY, and set the device to NEEDS_RESET. that seems much more clearly an issue on our part!

(if you're going only by the viona ioctl output then you do see two set_features() of the same bits when Windows boots .. partially 'cause there's no probe to see a device reset in the middle. so I'm going to add one of those here too and when I stop panicking my workstation I'll follow up on if the set_features() are separated by a reset or not here...)

[iximeow]

well, I stand by my reading that we're not handling a reset of a virtio device via device_status write correctly, but the Windows circumstance is a lot more straightforward. we're handling the 3.1.1-described device initialization poorly.

note that we dev.set_features() any time a device status is set with FEATURES_OK.

then, 3.1.1 says:

The driver MUST follow this sequence to initialize a device:

1. Reset the device.
2. Set the ACKNOWLEDGE status bit: the guest OS has noticed the device.
3. Set the DRIVER status bit: the guest OS knows how to drive the device.
4. Read device feature bits, and write the subset of feature bits understood by the OS and driver to the device. During this step the driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it.
5. Set the FEATURES_OK status bit. The driver MUST NOT accept new feature bits after this step.
6. Re-read device status to ensure the FEATURES_OK bit is still set: otherwise, the device does not support our subset of features and the device is unusable.
7. Perform device-specific setup, including discovery of virtqueues for the device, optional per-bus setup,
   reading and possibly writing the device’s virtio configuration space, and population of virtqueues.
8. Set the DRIVER_OK status bit. At this point the device is “live”.

so when a guest driver reaches step 5, we'll set_features and commit the negotiated features to the device. then when a guest reaches step 8, it sets a status including FEATURES_OK and set the same negotiated features again. then we see that MQ is an enabled feature and the whole use_pairs() on rings outside of reset thing happens. a "MUST NOT" tangent, if a guest wrote additional features between FEATURES_OK and DRIVER_OK, we'd accept them - we should probably warn and go to NEEDS_RESET for that case..

a lightly adjusted viona.d with probes for device status and reset events made this a lot more clear:

... nothing interesting ...
--- DEVICE STATUS SET: 00 ---
--- DEVICE STATUS SET: 01 ---
--- DEVICE STATUS SET: 03 ---
--- DEVICE STATUS SET: 0b ---
SET_FEATURES 0x1104388ab
SET_PAIRS 0xb
SET_USEPAIRS 0xb
RING_INIT_MODERN 0x0 234e15000/234e1d000/234e1e040 800
RING_SET_MSI 0x0 addr=0 msg=0
RING_INIT_MODERN 0x1 233258000/233259000/233259240 100
RING_SET_MSI 0x1 addr=0 msg=0
RING_INIT_MODERN 0x2 231289000/231291000/231292040 800
RING_INIT_MODERN 0x3 22d0f0000/22d0f1000/22d0f1240 100
RING_INIT_MODERN 0x4 22f8ce000/22f8d6000/22f8d7040 800
RING_INIT_MODERN 0x5 2294fd000/2294fe000/2294fe240 100
RING_INIT_MODERN 0x6 22bd5c000/22bd64000/22bd65040 800
RING_INIT_MODERN 0x7 21f5fd000/21f5fe000/21f5fe240 100
RING_SET_MSI 0x1 addr=fee0100c msg=49a1
RING_SET_MSI 0x0 addr=fee0100c msg=4991
RING_SET_MSI 0x3 addr=fee0200c msg=49b1
RING_SET_MSI 0x2 addr=fee0200c msg=49a1
RING_SET_MSI 0x5 addr=fee0400c msg=49b1
RING_SET_MSI 0x4 addr=fee0400c msg=49a1
RING_SET_MSI 0x7 addr=fee0800c msg=49b1
RING_SET_MSI 0x6 addr=fee0800c msg=49a1
--- DEVICE STATUS SET: 0f ---
SET_FEATURES 0x1104388ab
SET_PAIRS 0xb

but this also explains why I was seeing the "redundant" SET_FEATURES in a Linux guest and failing to find where in the Linux virtio driver that would actually happen..

[dancrossnyc]

Oh yeah, I think you're right; it's write OK, read OK, write OK to acknowledge.

[iximeow]

closing this in favor of #1064, as I no longer think it matters if set_features() is idempotent, 'cause we should only call it once in the first place