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

[Xu Yilun]

> > +#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>
>