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

[dan.j.williams]

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? 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
#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.