Re: [PATCH 3/3] hw/nvme: Add SPDM over DOE support

[Jonathan Cameron]

On Mon, 2 Oct 2023 09:15:58 +0200
Klaus Jensen <its@irrelevant.dk> wrote:

> On Sep 15 21:27, Alistair Francis wrote:
> > From: Wilfred Mallawa <wilfred.mallawa@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.mallawa@wdc.com>
> > Signed-off-by: Alistair Francis <alistair.francis@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