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

[Philippe Mathieu-Daudé]

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?

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