* [PATCH] hw/pci-bridge/cxl-upstream: Add serial number extended capability support
@ 2023-09-04 17:57 Jonathan Cameron via
2023-09-05 8:48 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Cameron via @ 2023-09-04 17:57 UTC (permalink / raw)
To: qemu-devel, Michael Tsirkin, Fan Ni, linux-cxl
Cc: Philippe Mathieu-Daudé, linuxarm
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)
#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;
+ } 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()
};
--
2.39.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/pci-bridge/cxl-upstream: Add serial number extended capability support
2023-09-04 17:57 [PATCH] hw/pci-bridge/cxl-upstream: Add serial number extended capability support Jonathan Cameron via
@ 2023-09-05 8:48 ` Philippe Mathieu-Daudé
2023-09-05 9:02 ` Michael S. Tsirkin
2023-09-05 16:22 ` Jonathan Cameron via
0 siblings, 2 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-09-05 8:48 UTC (permalink / raw)
To: Jonathan Cameron, qemu-devel, Michael Tsirkin, Fan Ni, linux-cxl; +Cc: linuxarm
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()
> };
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/pci-bridge/cxl-upstream: Add serial number extended capability support
2023-09-05 8:48 ` Philippe Mathieu-Daudé
@ 2023-09-05 9:02 ` Michael S. Tsirkin
2023-09-05 16:21 ` Jonathan Cameron via
2023-09-05 16:22 ` Jonathan Cameron via
1 sibling, 1 reply; 5+ messages in thread
From: Michael S. Tsirkin @ 2023-09-05 9:02 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Jonathan Cameron, qemu-devel, Fan Ni, linux-cxl, linuxarm
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()
> > };
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/pci-bridge/cxl-upstream: Add serial number extended capability support
2023-09-05 9:02 ` Michael S. Tsirkin
@ 2023-09-05 16:21 ` Jonathan Cameron via
0 siblings, 0 replies; 5+ messages in thread
From: Jonathan Cameron via @ 2023-09-05 16:21 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Philippe Mathieu-Daudé, qemu-devel, Fan Ni, linux-cxl,
linuxarm
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()
> > > };
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/pci-bridge/cxl-upstream: Add serial number extended capability support
2023-09-05 8:48 ` Philippe Mathieu-Daudé
2023-09-05 9:02 ` Michael S. Tsirkin
@ 2023-09-05 16:22 ` Jonathan Cameron via
1 sibling, 0 replies; 5+ messages in thread
From: Jonathan Cameron via @ 2023-09-05 16:22 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, Michael Tsirkin, Fan Ni, linux-cxl, linuxarm
On Tue, 5 Sep 2023 10:48:54 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> 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?
>
> > #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)
>
> ?
The naming is going to be very confusing if we do as it becomes
an offset of an offset.
Given we've never yet cared that much about keeping these devices
looking stable to a guest, I can just leave a gap if this cap
not defined and use fixed offsets instead thus avoiding this
complexity.
>
> > + } 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()
> > };
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-09-05 16:22 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-04 17:57 [PATCH] hw/pci-bridge/cxl-upstream: Add serial number extended capability support Jonathan Cameron via
2023-09-05 8:48 ` Philippe Mathieu-Daudé
2023-09-05 9:02 ` Michael S. Tsirkin
2023-09-05 16:21 ` Jonathan Cameron via
2023-09-05 16:22 ` Jonathan Cameron via
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).