Re: [PATCH kernel 1/9] pci/tsm: Add TDISP report blob and helpers to parse it

[Alexey Kardashevskiy]

On 25/2/26 17:16, dan.j.williams@xxxxxxxxx wrote:
> Alexey Kardashevskiy wrote:
> > The TDI interface report is defined in PCIe r7.0,
> > chapter "11.3.11 DEVICE_INTERFACE_REPORT". The report enumerates
> > MMIO resources and their properties which will take effect upon
> > transitioning to the RUN state.
> >
> > Store the report in pci_tsm.
> >
> > Define macros and helpers to parse the binary blob.
> >
> > Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxx>
> > ---
> >
> > Probably pci_tsm::report could be struct tdi_report_header*?
> [..]
> > +struct tdi_report_header {
> > +	__u16 interface_info; /* TSM_TDI_REPORT_xxx */
> > +	__u16 reserved2;
> > +	__u16 msi_x_message_control;
> > +	__u16 lnr_control;
> > +	__u32 tph_control;
> > +	__u32 mmio_range_count;
> > +} __packed;
> > +
> > +/*
> > + * Each MMIO Range of the TDI is reported with the MMIO reporting offset added.
> > + * Base and size in units of 4K pages
> > + */
> > +#define TSM_TDI_REPORT_MMIO_MSIX_TABLE		BIT(0)
> > +#define TSM_TDI_REPORT_MMIO_PBA			BIT(1)
> > +#define TSM_TDI_REPORT_MMIO_IS_NON_TEE		BIT(2)
> > +#define TSM_TDI_REPORT_MMIO_IS_UPDATABLE	BIT(3)
> > +#define TSM_TDI_REPORT_MMIO_RESERVED		GENMASK(15, 4)
> > +#define TSM_TDI_REPORT_MMIO_RANGE_ID		GENMASK(31, 16)
> > +
> > +struct tdi_report_mmio_range {
> > +	__u64 first_page;		/* First 4K page with offset added */
> > +	__u32 num;			/* Number of 4K pages in this range */
> > +	__u32 range_attributes;		/* TSM_TDI_REPORT_MMIO_xxx */
>
> Those should be __le64 and le32, right? 

Oh yes.

> But see below for another
> option...
>
> > +} __packed;
> > +
> > +struct tdi_report_footer {
> > +	__u32 device_specific_info_len;
> > +	__u8 device_specific_info[];
> > +} __packed;
> > +
> > +#define TDI_REPORT_HDR(rep)		((struct tdi_report_header *) ((rep)->data))
> > +#define TDI_REPORT_MR_NUM(rep)		(TDI_REPORT_HDR(rep)->mmio_range_count)
> > +#define TDI_REPORT_MR_OFF(rep)		((struct tdi_report_mmio_range *) (TDI_REPORT_HDR(rep) + 1))
> > +#define TDI_REPORT_MR(rep, rangeid)	TDI_REPORT_MR_OFF(rep)[rangeid]
> > +#define TDI_REPORT_FTR(rep)		((struct tdi_report_footer *) &TDI_REPORT_MR((rep), \
> > +					TDI_REPORT_MR_NUM(rep)))
> > +
>
> So we all have a version of a patch like this and the general style
> suggestion I have is to just parse this layout with typical
> offsets+bitfield definitions.
>
> This follows the precedent, admittedly tiny, of the DOE definitions in
> pci_regs.h. See:
>
> 	/* DOE Data Object - note not actually registers */
>
> I have a patch that parses the TDISP report with these defines:
>
> /*
>   * PCIe ECN TEE Device Interface Security Protocol (TDISP)
>   *
>   * Device Interface Report data object layout as defined by PCIe r7.0 section
>   * 11.3.11
>   */
> #define PCI_TSM_DEVIF_REPORT_INFO 0
> #define PCI_TSM_DEVIF_REPORT_MSIX 4
> #define PCI_TSM_DEVIF_REPORT_LNR 6
> #define PCI_TSM_DEVIF_REPORT_TPH 8
> #define PCI_TSM_DEVIF_REPORT_MMIO_COUNT 12
> #define  PCI_TSM_DEVIF_REPORT_MMIO_PFN 0 /* An interface report 'pfn' is 4K in size */
> #define  PCI_TSM_DEVIF_REPORT_MMIO_NR_PFNS 8
> #define  PCI_TSM_DEVIF_REPORT_MMIO_ATTR 12


I cannot easily see from these what the sizes are. And how many of each.

> #define  PCI_TSM_DEVIF_REPORT_MMIO_ATTR_MSIX_TABLE BIT(0)
> #define  PCI_TSM_DEVIF_REPORT_MMIO_ATTR_MSIX_PBA BIT(1)
> #define  PCI_TSM_DEVIF_REPORT_MMIO_ATTR_IS_NON_TEE BIT(2)
> #define  PCI_TSM_DEVIF_REPORT_MMIO_ATTR_IS_UPDATABLE BIT(3)
> #define  PCI_TSM_DEVIF_REPORT_MMIO_ATTR_RANGE_ID GENMASK(31, 16)
> #define  PCI_TSM_DEVIF_REPORT_MMIO_SIZE (16)
> #define PCI_TSM_DEVIF_REPORT_BASE_SIZE(nr_mmio) (16 + nr_mmio * PCI_TSM_DEVIF_REPORT_MMIO_SIZE)
>
> Any strong feelings one way or the other? I have a mild preference for
> this offset+bitfields approach.


My variant is just like this (may be need to put it in the comment):

tdi_report_header
tdi_report_mmio_range[]
tdi_report_footer

imho easier on eyes. I can live with either if the majority votes for it. Thanks.

--
Alexey