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

Alexey Kardashevskiy
2026-02-26T00:09:41Z

On 25/2/26 17:16, dan.j.williams@intel.com 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@amd.com>
>> ---
>>
>> 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