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

[Jonathan Cameron]

On Tue, 5 Sep 2023 05:02:47 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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.

The chosen default is also the one that the relevant specifications says
means 'NULL' for a EUI64 code so is at least a valid hack...
https://standards.ieee.org/wp-content/uploads/import/documents/tutorials/eui.pdf
"Unassigned and NULL EUI values"
specifically recommend NULL values in that section.

However, it's obscure enough that we probably don't want it in a generic
header.

> 
> 
> 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.

Agreed that would be generally useful but here there is a reasonable
default value so I don't think we need this.

> 
> 
> > >   #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()
> > >   };  
>