Re: [PATCH 06/11] samples/devsec: PCI device-security bus / endpoint sample

[Dan Williams <dan.j.williams@intel.com>]

Jonathan Cameron wrote:
> On Thu, 05 Dec 2024 14:23:51 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Establish just enough emulated PCI infrastructure to register a sample
> > TSM (platform security manager) driver and have it discover an IDE + TEE
> > (link encryption + device-interface security protocol (TDISP)) capable
> > device.
> > 
> > Use the existing a CONFIG_PCI_BRIDGE_EMUL to emulate an IDE capable root
> > port, and open code the emulation of an endpoint device via simulated
> > configuration cycle responses.
> > 
> > The devsec_tsm driver responds to the PCI core TSM operations as if it
> > successfully exercised the given interface security protocol message.
> > 
> > The devsec_bus and devsec_tsm drivers can be loaded in either order to
> > reflect cases like SEV-TIO where the TSM is PCI-device firmware, and
> > cases like TDX Connect where the TSM is a software agent running on the
> > host CPU.
> > 
> > Follow-on patches add common code for TSM managed IDE establishment. For
> > now, just successfully complete setup and teardown of the DSM (device
> > security manager) context as a building block for management of TDI
> > (trusted device interface) instances.
> > 
> >  # modprobe devsec_bus
> >     devsec_bus devsec_bus: PCI host bridge to bus 10000:00
> >     pci_bus 10000:00: root bus resource [bus 00-01]
> >     pci_bus 10000:00: root bus resource [mem 0xf000000000-0xffffffffff 64bit]
> >     pci 10000:00:00.0: [8086:7075] type 01 class 0x060400 PCIe Root Port
> >     pci 10000:00:00.0: PCI bridge to [bus 00]
> >     pci 10000:00:00.0:   bridge window [io  0x0000-0x0fff]
> >     pci 10000:00:00.0:   bridge window [mem 0x00000000-0x000fffff]
> >     pci 10000:00:00.0:   bridge window [mem 0x00000000-0x000fffff 64bit pref]
> >     pci 10000:00:00.0: bridge configuration invalid ([bus 00-00]), reconfiguring
> >     pci 10000:01:00.0: [8086:ffff] type 00 class 0x000000 PCIe Endpoint
> >     pci 10000:01:00.0: BAR 0 [mem 0xf000000000-0xf0001fffff 64bit pref]
> >     pci_doe_abort: pci 10000:01:00.0: DOE: [100] Issuing Abort
> >     pci_doe_cache_protocols: pci 10000:01:00.0: DOE: [100] Found protocol 0 vid: 1 prot: 1
> >     pci 10000:01:00.0: disabling ASPM on pre-1.1 PCIe device.  You can enable it with 'pcie_aspm=force'
> >     pci 10000:00:00.0: PCI bridge to [bus 01]
> >     pci_bus 10000:01: busn_res: [bus 01] end is updated to 01
> >  # modprobe devsec_tsm
> >     devsec_tsm_pci_probe: pci 10000:01:00.0: devsec: tsm enabled
> >     __pci_tsm_init: pci 10000:01:00.0: TSM: Device security capabilities detected ( ide tee ), TSM attach
> > 
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Lukas Wunner <lukas@wunner.de>
> > Cc: Samuel Ortiz <sameo@rivosinc.com>
> > Cc: Alexey Kardashevskiy <aik@amd.com>
> > Cc: Xu Yilun <yilun.xu@linux.intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Hi Dan,
> 
> A few minor comments as I was reading this. Mostly just trying
> to get my head around it hence they are all fairly superficial things.
> 
> Jonathan
> 
> > diff --git a/samples/devsec/bus.c b/samples/devsec/bus.c
> > new file mode 100644
> > index 000000000000..47dbe4e1b648
> > --- /dev/null
> > +++ b/samples/devsec/bus.c
> 
> 
> > +static void destroy_iomem_pool(void *data)
> 
> There is a devm_gen_pool_create you can probably use.

Indeed there is, thanks.

[..]
> Similar to the case below. I'd rather see a per dev devm_ cleanup
> than relying on unified cleanup and that array having null entrees.
> Should end up easier to follow.  Might require devsec dev to have
> a reference back to the pool though.

Done for ports and devs.

The arrays are used during PCI bus operations. This made me realize that
I should be putting the device and port allocation *before* the PCI bus
creation to make sure those arrays are dead and idle before the they are
invalidated by the port and dev devres actions.

[..]
> > +static int init_port(struct devsec_port *devsec_port)
> > +{
> > +	struct pci_bridge_emul *bridge = &devsec_port->bridge;
> > +	int rc;
> > +
> > +	bridge->conf.vendor = cpu_to_le16(0x8086);
> > +	bridge->conf.device = cpu_to_le16(0x7075);
> > +	bridge->subsystem_vendor_id = cpu_to_le16(0x8086);
> > +	bridge->conf.class_revision = cpu_to_le32(0x1);
> > +
> > +	bridge->conf.pref_mem_base = cpu_to_le16(PCI_PREF_RANGE_TYPE_64);
> > +	bridge->conf.pref_mem_limit = cpu_to_le16(PCI_PREF_RANGE_TYPE_64);
> > +
> > +	bridge->has_pcie = true;
> > +	bridge->pcie_conf.devcap = cpu_to_le16(PCI_EXP_DEVCAP_FLR);
> > +	bridge->pcie_conf.lnksta = cpu_to_le16(PCI_EXP_LNKSTA_CLS_2_5GB);
> > +
> > +	bridge->data = devsec_port;
> > +	bridge->ops = &devsec_bridge_ops;
> Maybe 
> 	*bridge = (struct pci_bridge_emul) {
> 	};
> appropriate here. 	

Sure.

> > +
> > +	init_ide(&devsec_port->ide);
> > +
> > +	rc = pci_bridge_emul_init(bridge, 0);
> 
> return pci_bridge_emul_init() unless a later patch is going to add more here.

Ok.