Re: [RFC PATCH 22/27] coco/tdx-host: Implement SPDM session setup

[Xu Yilun <yilun.xu@linux.intel.com>]

> > +struct spdm_config_info_t {
> > +	u32 vmm_spdm_cap;
> > +#define SPDM_CAP_HBEAT          BIT(13)
> > +#define SPDM_CAP_KEY_UPD        BIT(14)
> > +	u8 spdm_session_policy;
> > +	u8 certificate_slot_mask;
> > +	u8 raw_bitstream_requested;
> > +	u8 reserved[];
> Given the only use in here that I can immediately spot is on the
> stack with nothing in reserved (so zero size) + you then memcpy that into
> another buffer, why bother having reserved in this declaration?

I'll delete the reserved.

> > +} __packed;
> > +
> >  static struct pci_tsm_ops tdx_link_ops;
> >  
> >  static struct pci_tsm *tdx_link_pf0_probe(struct tsm_dev *tsm_dev,
> >  					  struct pci_dev *pdev)
> >  {
> > +	const struct spdm_config_info_t spdm_config_info = {
> > +		/* use a default configuration, may require user input later */
> > +		.vmm_spdm_cap = SPDM_CAP_KEY_UPD,
> > +		.certificate_slot_mask = 0xff,
> > +	};
> >  	int rc;
> >  
> >  	struct tdx_link *tlink __free(kfree) =
> > @@ -176,6 +423,29 @@ static struct pci_tsm *tdx_link_pf0_probe(struct tsm_dev *tsm_dev,
> >  	if (rc)
> >  		return NULL;
> >  
> > +	tlink->func_id = tdisp_func_id(pdev);
> > +
> > +	struct page *in_msg_page __free(__free_page) =
> > +		alloc_page(GFP_KERNEL | __GFP_ZERO);
> > +	if (!in_msg_page)
> > +		return NULL;
> > +
> > +	struct page *out_msg_page __free(__free_page) =
> > +		alloc_page(GFP_KERNEL | __GFP_ZERO);
> > +	if (!out_msg_page)
> > +		return NULL;
> > +
> > +	struct page *spdm_conf __free(__free_page) =
> > +		alloc_page(GFP_KERNEL | __GFP_ZERO);
> > +	if (!spdm_conf)
> > +		return NULL;
> > +
> > +	memcpy(page_address(spdm_conf), &spdm_config_info, sizeof(spdm_config_info));
> > +
> > +	tlink->in_msg = no_free_ptr(in_msg_page);
> > +	tlink->out_msg = no_free_ptr(out_msg_page);
> > +	tlink->spdm_conf = no_free_ptr(spdm_conf);
> > +
> >  	return &no_free_ptr(tlink)->pci.base_tsm;
> >  }
> >  
> > @@ -183,6 +453,9 @@ static void tdx_link_pf0_remove(struct pci_tsm *tsm)
> >  {
> >  	struct tdx_link *tlink = to_tdx_link(tsm);
> >  
> > +	__free_page(tlink->in_msg);
> > +	__free_page(tlink->out_msg);
> > +	__free_page(tlink->spdm_conf);
> Trivial but I'd prefer to see these freed in reverse order of allocation
> of where they are set. Just makes reviewing a tiny bit easier as the code
> evolves.

Will do.

> 
> >  	pci_tsm_pf0_destructor(&tlink->pci);
> >  	kfree(tlink);
> >  }
>