On Tue, 5 Sep 2023 05:02:47 -0400 "Michael S. Tsirkin" <m...@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.came...@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() > > > }; >
