Re: [PATCH v2 27/31] coco/tdx-host: Implement SPDM session setup

Xu Yilun
2026-04-22T10:15:52Z

> > +#define TDISP_FUNC_ID GENMASK(15, 0)
> > +#define TDISP_FUNC_ID_SEGMENT GENMASK(23, 16)
> > +#define TDISP_FUNC_ID_SEG_VALID BIT(24)
> > +
> > +static inline u32 tdisp_func_id(struct pci_dev *pdev)
> > +{
> > + u32 func_id;
> > +
> > + func_id = FIELD_PREP(TDISP_FUNC_ID_SEGMENT, pci_domain_nr(pdev->bus));
> > + if (func_id)
> > + func_id |= TDISP_FUNC_ID_SEG_VALID;
>
> This check implies pci_domain_nr returning 0 is considered invalid. Other
> callers in the kernel seem to not care, they just use the domain nr, so is
> this check spurious or intentional ?

This is the func_id format defined in TDISP SPEC, bit 24 is a must. It
is not the linux defined SBDF format.

>
> > + func_id |= FIELD_PREP(TDISP_FUNC_ID,
> > + PCI_DEVID(pdev->bus->number, pdev->devfn));
> > +
> > + return func_id;
> > +}
> > +
> > +struct spdm_config_info_t {
> > + u32 vmm_spdm_cap;
> > +#define SPDM_CAP_HBEAT BIT(13)
> > +#define SPDM_CAP_KEY_UPD BIT(14)
>
> nit: move those defines above the struct definition, they just break the
> reading flow as it is.

Yes.

...

> > +static void *tdx_dup_array_data(struct tdx_page_array *array,
> > + unsigned int data_size)
> > +{
> > + unsigned int npages = (data_size + PAGE_SIZE - 1) / PAGE_SIZE;
>
> nit: There's DIV_ROUND_UP

Yes.

...

> > +DEFINE_FREE(tdx_spdm_session_teardown, struct tdx_tsm_link *,
> > + if (!IS_ERR_OR_NULL(_T)) tdx_spdm_session_teardown(_T))
> > +
> > static int tdx_tsm_link_connect(struct pci_dev *pdev)
> > {
> > - return -ENXIO;
> > + struct tdx_tsm_link *tlink = to_tdx_tsm_link(pdev->tsm);
> > +
> > + struct tdx_tsm_link *tlink_spdm __free(tdx_spdm_session_teardown) =
> > + tdx_spdm_session_setup(tlink);
>
> Is the free() really needed here, either the session is correctly setup and
> tlink_spdm is returned. But if session_setup() files then what about calling
> spdm_session_disconnect() on an unestablished session?

Ah, we have more steps to add, the __free() will take function when the
following steps fail.

We may add __free() when we add more steps, but I think that makes the
diff harder to read, so I want to keep this style.

Thanks.

>
>
> > + if (IS_ERR(tlink_spdm)) {
> > + pci_err(pdev, "fail to setup spdm session\n");
> > + return PTR_ERR(tlink_spdm);
> > + }
> > +
> > + retain_and_null_ptr(tlink_spdm);
> > +
> > + return 0;
> > }
>
> <snip>
>