| Message ID | 20240201060228.3070928-2-aik@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | pci/doe/ide: Capabilities, protocols | expand |
On Thu, Feb 01, 2024 at 05:02:25PM +1100, Alexey Kardashevskiy wrote: > Already public pci_doe() takes a protocol type argument. > PCIe 6.0 defines three, define them in a header for use with pci_doe(). [...] > --- a/include/linux/pci-doe.h > +++ b/include/linux/pci-doe.h > @@ -13,6 +13,10 @@ > #ifndef LINUX_PCI_DOE_H > #define LINUX_PCI_DOE_H > > +#define PCI_DOE_PROTOCOL_DISCOVERY 0 > +#define PCI_DOE_PROTOCOL_CMA_SPDM 1 > +#define PCI_DOE_PROTOCOL_SECURED_CMA_SPDM 2 These are deliberately defined in the .c files which actually need them, i.e. DISCOVERY is defined in drivers/pci/doe.c and CMA_SPDM is defined in drivers/pci/cma.c: https://lore.kernel.org/all/7721bfa3b4f8a99a111f7808ad8890c3c13df56d.1695921657.git.lukas@wunner.de/ I don't see why they would have to be public if they're each only needed in a single .c file. Thanks, Lukas
On 1/2/24 17:50, Lukas Wunner wrote: > On Thu, Feb 01, 2024 at 05:02:25PM +1100, Alexey Kardashevskiy wrote: >> Already public pci_doe() takes a protocol type argument. >> PCIe 6.0 defines three, define them in a header for use with pci_doe(). > [...] >> --- a/include/linux/pci-doe.h >> +++ b/include/linux/pci-doe.h >> @@ -13,6 +13,10 @@ >> #ifndef LINUX_PCI_DOE_H >> #define LINUX_PCI_DOE_H >> >> +#define PCI_DOE_PROTOCOL_DISCOVERY 0 >> +#define PCI_DOE_PROTOCOL_CMA_SPDM 1 >> +#define PCI_DOE_PROTOCOL_SECURED_CMA_SPDM 2 > > These are deliberately defined in the .c files which actually need them, > i.e. DISCOVERY is defined in drivers/pci/doe.c and CMA_SPDM is defined in > drivers/pci/cma.c: > > https://lore.kernel.org/all/7721bfa3b4f8a99a111f7808ad8890c3c13df56d.1695921657.git.lukas@wunner.de/ > > I don't see why they would have to be public if they're each only needed > in a single .c file. SEV TIO needs both CMA and SECURED_CMA for DOE device<->firmware bouncing and it is going to use pci_doe() for this. I should have put this into the commit log, sorry about that. Or the plan is to add pci_doe_secure_transport() to cma.c and force everyone use that? The PCI SIG DOE protocol numbers (discovery, CMA, secure CMA) are all defined in one place in the PCIe spec and defining them in different places (doe.c, cma.c) is weird imho. > > Thanks, > > Lukas
On Thu, Feb 01, 2024 at 05:02:25PM +1100, Alexey Kardashevskiy wrote: > Already public pci_doe() takes a protocol type argument. > PCIe 6.0 defines three, define them in a header for use with pci_doe(). > > Signed-off-by: Alexey Kardashevskiy <aik@amd.com> > --- > include/linux/pci-doe.h | 4 ++++ > drivers/pci/doe.c | 2 -- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h > index 1f14aed4354b..5c33f7dc978b 100644 > --- a/include/linux/pci-doe.h > +++ b/include/linux/pci-doe.h > @@ -13,6 +13,10 @@ > #ifndef LINUX_PCI_DOE_H > #define LINUX_PCI_DOE_H > > +#define PCI_DOE_PROTOCOL_DISCOVERY 0 > +#define PCI_DOE_PROTOCOL_CMA_SPDM 1 > +#define PCI_DOE_PROTOCOL_SECURED_CMA_SPDM 2 So far these are only needed inside drivers/pci/. I don't want to expose them to the rest of the kernel via include/linux/pci-doe.h until they're needed elsewhere. > struct pci_doe_mb; > > struct pci_doe_mb *pci_find_doe_mailbox(struct pci_dev *pdev, u16 vendor, > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c > index e3aab5edaf70..61f0531d2b1d 100644 > --- a/drivers/pci/doe.c > +++ b/drivers/pci/doe.c > @@ -22,8 +22,6 @@ > > #include "pci.h" > > -#define PCI_DOE_PROTOCOL_DISCOVERY 0 > - > /* Timeout of 1 second from 6.30.2 Operation, PCI Spec r6.0 */ > #define PCI_DOE_TIMEOUT HZ > #define PCI_DOE_POLL_INTERVAL (PCI_DOE_TIMEOUT / 128) > -- > 2.41.0 >
On Thu, Feb 01, 2024 at 09:16:37PM +1100, Alexey Kardashevskiy wrote: > Or the plan is to add pci_doe_secure_transport() to cma.c and force everyone > use that? Right, the plan is to pass an additional callback to spdm_create() which performs a secure transmission. And cma.c would define that to use the separate DOE type. Thanks, Lukas
On 9/2/24 19:52, Lukas Wunner wrote: > On Thu, Feb 01, 2024 at 09:16:37PM +1100, Alexey Kardashevskiy wrote: >> Or the plan is to add pci_doe_secure_transport() to cma.c and force everyone >> use that? > > Right, the plan is to pass an additional callback to spdm_create() > which performs a secure transmission. And cma.c would define that > to use the separate DOE type. But it is no use for the TDISP case which won't be calling spdm_create() and will have to define CMA_SPDM and SECURED_CMA_SPDM, again.
diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h index 1f14aed4354b..5c33f7dc978b 100644 --- a/include/linux/pci-doe.h +++ b/include/linux/pci-doe.h @@ -13,6 +13,10 @@ #ifndef LINUX_PCI_DOE_H #define LINUX_PCI_DOE_H +#define PCI_DOE_PROTOCOL_DISCOVERY 0 +#define PCI_DOE_PROTOCOL_CMA_SPDM 1 +#define PCI_DOE_PROTOCOL_SECURED_CMA_SPDM 2 + struct pci_doe_mb; struct pci_doe_mb *pci_find_doe_mailbox(struct pci_dev *pdev, u16 vendor, diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c index e3aab5edaf70..61f0531d2b1d 100644 --- a/drivers/pci/doe.c +++ b/drivers/pci/doe.c @@ -22,8 +22,6 @@ #include "pci.h" -#define PCI_DOE_PROTOCOL_DISCOVERY 0 - /* Timeout of 1 second from 6.30.2 Operation, PCI Spec r6.0 */ #define PCI_DOE_TIMEOUT HZ #define PCI_DOE_POLL_INTERVAL (PCI_DOE_TIMEOUT / 128)
Already public pci_doe() takes a protocol type argument. PCIe 6.0 defines three, define them in a header for use with pci_doe(). Signed-off-by: Alexey Kardashevskiy <aik@amd.com> --- include/linux/pci-doe.h | 4 ++++ drivers/pci/doe.c | 2 -- 2 files changed, 4 insertions(+), 2 deletions(-)