On Mon, 2 Oct 2023 09:15:58 +0200 Klaus Jensen <i...@irrelevant.dk> wrote:
> On Sep 15 21:27, Alistair Francis wrote: > > From: Wilfred Mallawa <wilfred.mall...@wdc.com> > > > > Setup Data Object Exchance (DOE) as an extended capability for the NVME > > controller and connect SPDM to it (CMA) to it. > > > > Signed-off-by: Wilfred Mallawa <wilfred.mall...@wdc.com> > > Signed-off-by: Alistair Francis <alistair.fran...@wdc.com> > > --- > > docs/specs/index.rst | 1 + > > docs/specs/spdm.rst | 56 +++++++++++++++++++++++++++++++++++++ > > include/hw/pci/pci_device.h | 5 ++++ > > include/hw/pci/pcie_doe.h | 3 ++ > > hw/nvme/ctrl.c | 52 ++++++++++++++++++++++++++++++++++ > > hw/nvme/trace-events | 1 + > > 6 files changed, 118 insertions(+) > > create mode 100644 docs/specs/spdm.rst > > > > This looks reasonable enough, but could this not be implemented at the > PCI layer? I do not see anything that is tied specifically to the nvme > device, why can the spdm parameter not be a PCIDevice parameter such > that all PCIDevice-derived devices gains this functionality? > Whilst a few parts of this feel like they'd reasonably move to PCI core library code (though may not be worth it as they are trivial wrappers). I can look at that when we add CXL support on top of this (which will be simple given Alastair used the SPDM sockets stuff the CXL stuff is based on.) I'm not sure it makes sense to have it as a property for all devices. If we did go that way possible issues would include. 1) Need find some extended config space to put it in and extended config space layouts are per device. We'd need to do something like pcie_endpoint_cap_init() that provided a suitable address range. So other than stashing the address int he PCI core to allow the default config_read / config_write to work we don't gain much. 2) This will get more complex and more device specific as there are a whole load of device specific protocols that will get layered on top. Sure we could do all that with callbacks, but seems simpler just to stick to a bit of repeating boiler plate. 3) There will probably be real devices with multiple instances. Whether we emulate the in QEMU is a whole different question. Virtualizing real instances of this is going to be interest (if we don't just hide them completely in the host). Anyhow I'd go with a 'maybe' but not yet. Easy to move this later I think if it does make sense to move where it is implemented? Jonathan
