nvme: allow PCI ID overrides

[mattkur]

I have found it useful to ask the nvme device to pretend to be a different PCI device type. This is useful for testing and prototyping.

Validated via unit tests.

[mattkur]

Note for reviewers: these changes are to match the terminology in PCI Code and ID Assignment Specification from the PCI SIG standards body.

[mattkur]

While this primitive is useful for me in the NVMe context, I could see a generalized override make sense. In the current architecture, each device owns the ConfigSpaceType0Emulator emulator for that device, which is where these HardwareIds are set. I didn't immediately come up with an elegant way of making this a general primitive.

[daprilik]

In the current architecture, each device owns the ConfigSpaceType0Emulator emulator for that device, which is where these HardwareIds are set. I didn't immediately come up with an elegant way of making this a general primitive.

I can offer some context on why this is modeled the way it is.

In a nutshell - we wanted to keep the "core" PCI interconnect trait as simple (and as flexible) as possible, so we could decouple our PCI root controller implementations (e.g: vPCI, legacy PCI, and in the future - PCIe) from the various PCI devices that plugged into them (e.g: legacy gen1 PCI devices, virtio-pci, modern pci deviecs, etc...). Pushing the details of config space emulation / layout into the devices themselves (aided by a common shared "helper" emulator that those devices could leverage) enables the core interface to be as simple as "read/write bytes from a device's PCI config space", which significantly simplifies the wiring required to mix-and-match PCI devices / busses / topologies.

Of course, this means there's no centralized place to set up these sorts of overrides... and you are forced to plumb through this config info manually.

That said - I might have some ideas on how we can simplify some of the plumbing here. I'll leave some more comments on the PR

[jstarks]

If this is just for testing/prototyping, I wonder if it would be better to instead make a wrapper PCI device that allows for various config space functionality to be tweaked before/after calling through to the real device.

[daprilik]

Hah, and here I was just about to leave a different suggestion (something along the lines of adding a new field to ResolvePciDeviceHandleParams, where you can stuff these overrides). I do like John's a lot more.

The caveat is that you end up paying extra virtual dispatch cost when doing chipset device operations (IO, MMIO, PCI, etc...), which is a bit unfortunate.

My follow-on suggestion is that you could implement overrides as a feature on the PCI "bus" itself. i.e: when attaching a device to the vPCI bus, give it the ability to associate ID overrides with particular devices, and have it do an extra intercept when incoming/outbound pci_{read/write} to devices.

[mattkur]

@jstarks, @daprilik Thank you for the suggestions, and I agree with the course of action. If I'm following correctly, then I'd:

• Create a wrapper type that impl ChipsetDevice, C
• Create a wrapper type that impl PciConfigSpace, P
• C's supports_pci method would return a P, with whatever overrides the creator sets.
• Everything else gets passed through to the underlying device.
• Still wire up the override in impl AsyncResolveResource<PciDeviceHandleKind, NvmeControllerHandle> for NvmeControllerResolver (with the actual config space overrides coming from ResolvePciDeviceHandleParams).

Is this what you had in mind?

This raises a separate question for me: where is the machinery to decide how to offer this device (e.g. over virtio, vpci, or on the emulated pci bus)?

I don't think I'll get to them in a timely manner. I'll move this PR back to draft and pick it back up when I am able.

[jstarks]

If you take my suggestion (as opposed to doing this in the bus, which is also a reasonable suggestion)...

C and P can be the same struct.

Then, instead of doing something in the NVMe resolver, you'd make a new resource:

struct DisguisedPciDeviceHandle {
    device: Resource<PciDeviceHandleKind>,
    prog_id: Option<Whatever>,
    ...
}

The resolver for this thing would resolve device to a device and instantiate a C/P that wraps it.

So there wouldn't be any NVMe-related changes here.

[mattkur]

If you take my suggestion (as opposed to doing this in the bus, which is also a reasonable suggestion)...

C and P can be the same struct.

Then, instead of doing something in the NVMe resolver, you'd make a new resource:

struct DisguisedPciDeviceHandle {
    device: Resource<PciDeviceHandleKind>,
    prog_id: Option<Whatever>,
    ...
}

The resolver for this thing would resolve device to a device and instantiate a C/P that wraps it.

So there wouldn't be any NVMe-related changes here.

Got it, thanks. This is helpful.

[daprilik]

And if you take my suggestion (pushing this override logic into the vpci bus itself)

I believe the key function to work around is build_vpci_device. If you follow the code leading into / out-of that function, it should be pretty straightforward to add in the right hooks / logic to teach the underlying VpciBus how to do overrides (by updating its impl MmioIntercept for VpciBus implementation).

[mattkur]

Will repopen or post a new pr if I complete this work.