Re: [PATCH] hw/pci-bridge/cxl-upstream: Add serial number extended capability support

[Michael S. Tsirkin]

On Tue, Sep 05, 2023 at 10:48:54AM +0200, Philippe Mathieu-Daudé wrote:
> Hi Jonathan,
> 
> On 4/9/23 19:57, Jonathan Cameron wrote:
> > Will be needed so there is a defined serial number for
> > information queries via the Switch CCI.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> > No ordering dependencies wrt to other CXL patch sets.
> > 
> > Whilst we 'need' it for the Switch CCI set it is valid without
> > it and aligns with existing EP serial number support. Seems sensible
> > to upstream this first and reduce my out of tree backlog a little!
> > 
> >   hw/pci-bridge/cxl_upstream.c | 15 +++++++++++++--
> >   1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c
> > index 2b9cf0cc97..15c4d84a56 100644
> > --- a/hw/pci-bridge/cxl_upstream.c
> > +++ b/hw/pci-bridge/cxl_upstream.c
> > @@ -14,6 +14,11 @@
> >   #include "hw/pci/msi.h"
> >   #include "hw/pci/pcie.h"
> >   #include "hw/pci/pcie_port.h"
> > +/*
> > + * Null value of all Fs suggested by IEEE RA guidelines for use of
> > + * EU, OUI and CID
> > + */
> > +#define UI64_NULL (~0ULL)
> 
> Already defined in hw/mem/cxl_type3.c, can we move it to some common
> CXL header? Or include/qemu/units.h?

not the last one I think - this is a cxl specific hack to detect that
user has changed the property.


I think we really should have a variant of DEFINE_PROP_XXX that sets a
flag allowing us to detect whether a property has been set manually.
This would be a generalization of DEFINE_PROP_ON_OFF_AUTO.


> >   #define CXL_UPSTREAM_PORT_MSI_NR_VECTOR 2
> > @@ -30,6 +35,7 @@ typedef struct CXLUpstreamPort {
> >       /*< public >*/
> >       CXLComponentState cxl_cstate;
> >       DOECap doe_cdat;
> > +    uint64_t sn;
> >   } CXLUpstreamPort;
> >   CXLComponentState *cxl_usp_to_cstate(CXLUpstreamPort *usp)
> > @@ -326,8 +332,12 @@ static void cxl_usp_realize(PCIDevice *d, Error **errp)
> >       if (rc) {
> >           goto err_cap;
> >       }
> > -
> > -    cxl_cstate->dvsec_offset = CXL_UPSTREAM_PORT_DVSEC_OFFSET;
> > +    if (usp->sn != UI64_NULL) {
> > +        pcie_dev_ser_num_init(d, CXL_UPSTREAM_PORT_DVSEC_OFFSET, usp->sn);
> > +        cxl_cstate->dvsec_offset = CXL_UPSTREAM_PORT_DVSEC_OFFSET + 0x0c;
> 
> Could it be clearer to have:
> 
> diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c
> @@ -23,2 +23,2 @@
> -#define CXL_UPSTREAM_PORT_DVSEC_OFFSET \
> -    (CXL_UPSTREAM_PORT_AER_OFFSET + PCI_ERR_SIZEOF)
> +#define CXL_UPSTREAM_PORT_DVSEC_OFFSET(offset) \
> +    (CXL_UPSTREAM_PORT_AER_OFFSET + PCI_ERR_SIZEOF + offset)
> 
> ?
> 
> > +    } else {
> > +        cxl_cstate->dvsec_offset = CXL_UPSTREAM_PORT_DVSEC_OFFSET;
> > +    }
> >       cxl_cstate->pdev = d;
> >       build_dvsecs(cxl_cstate);
> >       cxl_component_register_block_init(OBJECT(d), cxl_cstate, 
> > TYPE_CXL_USP);
> > @@ -366,6 +376,7 @@ static void cxl_usp_exitfn(PCIDevice *d)
> >   }
> >   static Property cxl_upstream_props[] = {
> > +    DEFINE_PROP_UINT64("sn", CXLUpstreamPort, sn, UI64_NULL),
> >       DEFINE_PROP_STRING("cdat", CXLUpstreamPort, cxl_cstate.cdat.filename),
> >       DEFINE_PROP_END_OF_LIST()
> >   };