[pci] guard set_features+handle mask drift in legacy & common interfaces

[zeeshanlakhani]

This work fixes a known issue where legacy interface writes could arrive after feature negotiation ended or a DRIVER_OK flag was set, causing side-effects and unwanted reconfiguration.

We fix this with a guard before set_features that checks that negotiation has closed first, setting NEEDS_RESET (discarding the write) if a different mask is requested. A write that produces the same mask is just a no-op here.

For modern, common interfaces, writes after FEATURES_OK could mutate the state's negotiated_features without calling set_features itself, causing the mask to diverge from what the device had already applied. This came up in iximeow's comment, which proposed NEEDS_RESET for feature writes between FEATURES_OK and DRIVER_OK.

To handle this, we apply a similar methodology, but for CommonConfigReg::DriverFeature once negotiation is closed. We also reject retried FEATURES_OK flag sets while NEEDS_RESET status is already set.

We need these changes for upcoming viona work involving setting MAC filters.

[iximeow]

I closed #1053 because I proposed it not knowing the mechanism by which it fixed the issue I was observing, and I later determined that the issue was different and motivated a different fix (the fourth paragraph in #1053) - I did not close #1053 leaving a known issue in the tree, beyond that a guest with a driver that is explicitly misbehaving against modern registers can cause a bit worse misbehavior than we'd like.

the issue here is more subtle, in that I'd overlooked that writes to the legacy features register went directly to set_features too, and that the lack of a FEATURES_OK in legacy negotiation meant that worked kind of by accident since we even started supporting modern devices. legacy drivers would probably write a feature set once, get the correct number of set_features() calls as a result, and then continue with negotiation. it was really not brought along with the FSM I'd hoped to set up in #1064.

so there are kind of two bugs here: the device retains writes to state.negotiated_features after FEATURES_OK which, yes, is pretty wonky. for example writing F_CTRL_RX into the features after FEATURES_OK will mean that migrating such a device will have the destination's device operate with a different feature set than was actually initially set up. weird, not ideal, and all not in line with the VirtIO spec. additionally, writes to LegacyConfigReg::DriverFeature after the first can cause set_features() to run multiple times on an initialized device, which is also, I'd say, "pretty wonky".

what I'm confused about is: how does this relate to #1210? I don't see a way that that PR should have issues or not have issues based on this being in the Propolis, and I don't see how this could lead to reapplying set_features() on a device that was already configured. this is all under a pretty clear MUST NOT from the spec (what I was gesturing at in the comment you linked to), is there a guest that is setting additional features after negotiation, which we need to tolerate?

I have looked at the diff itself and I think it's probably not how we'd want to go about fixing these issues, but I think it's more important to understand what issues we have or will run into than getting into the change itself.