[PATCH qemu 0/6] hw/cxl: Link speed and width control

[PATCH qemu 0/6] hw/cxl: Link speed and width control

[Jonathan Cameron via]

Changes since RFC:
- rebase

Question:
- I could enable this for all PCIe device (including ports).
  Does that makes sense, or is it better to limit this to my cases?
  It is quite easy to build broken setups (downstream device reports
  faster link than the port etc) because QEMU 'link' training' is
  simplistic.  I'm not sure it is worth making it more clever.

The Generic Ports support added the ability to describe the bandwidth and
Latency within a host to a CXL host bridge.  To be able to test the of the
discovery path used by Linux [1] we also need to be able to create
bottlenecks at difference places in the topology. There are two parts to
this
* CXL link characteristics as described by PCI Express Capability Link
  status etc.
* Bandwidth and latency across CXL Switches (via CDAT data from the switch
  USP)
* Bandwidth and latency from the CXL type 3 device port to the actual
  memory (Via CDAT data from the EP).

Currently we have fixed values for the CXL CDAT tables, and to test this
I recommend changing those as per the patch at the end of this cover letter
(so they aren't always the bottleneck). Making those configurable will be
handled in a future patch set.

RFC has a set of examples but those were to help testing the kernel code
rather than providing much info for QEMU review so I haven't repeated
them ehre.

https://lore.kernel.org/qemu-devel/20240712122414.1448284-1-Jonathan.Cameron@huawei.com/

Jonathan Cameron (6):
  hw/pci-bridge/cxl_root_port: Provide x-speed and x-width properties.
  hw/pci-bridge/cxl_upstream: Provide x-speed and x-width properties.
  hw/pcie: Factor out PCI Express link register filling common to EP.
  hw/pcie: Provide a utility function for control of EP / SW USP link
  hw/mem/cxl-type3: Add properties to control link speed and width
  hw/pci-bridge/cxl-upstream: Add properties to control link speed and
    width

 include/hw/cxl/cxl_device.h               |   4 +
 include/hw/pci-bridge/cxl_upstream_port.h |   4 +
 include/hw/pci/pcie.h                     |   2 +
 hw/mem/cxl_type3.c                        |   6 ++
 hw/pci-bridge/cxl_downstream.c            |  23 +++--
 hw/pci-bridge/cxl_root_port.c             |   5 ++
 hw/pci-bridge/cxl_upstream.c              |   6 ++
 hw/pci/pcie.c                             | 105 ++++++++++++++--------
 8 files changed, 103 insertions(+), 52 deletions(-)

-- 
2.43.0

[PATCH 1/6] hw/pci-bridge/cxl_root_port: Provide x-speed and x-width properties.

[Jonathan Cameron via]

Approach copied from gen_pcie_root_port.c
Previously the link defaulted to a maximum of 2.5GT/s and 1x.  Enable setting
it's maximum values.  The actual value after 'training' will depend on the
downstream device configuration.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/pci-bridge/cxl_root_port.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/pci-bridge/cxl_root_port.c b/hw/pci-bridge/cxl_root_port.c
index 2dd10239bd..5e2156d7ba 100644
--- a/hw/pci-bridge/cxl_root_port.c
+++ b/hw/pci-bridge/cxl_root_port.c
@@ -24,6 +24,7 @@
 #include "hw/pci/pcie_port.h"
 #include "hw/pci/msi.h"
 #include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
 #include "hw/sysbus.h"
 #include "qapi/error.h"
 #include "hw/cxl/cxl.h"
@@ -206,6 +207,10 @@ static Property gen_rp_props[] = {
                      -1),
     DEFINE_PROP_SIZE("pref64-reserve", CXLRootPort, res_reserve.mem_pref_64,
                      -1),
+    DEFINE_PROP_PCIE_LINK_SPEED("x-speed", PCIESlot,
+                                speed, PCIE_LINK_SPEED_64),
+    DEFINE_PROP_PCIE_LINK_WIDTH("x-width", PCIESlot,
+                                width, PCIE_LINK_WIDTH_32),
     DEFINE_PROP_END_OF_LIST()
 };
 
-- 
2.43.0

[PATCH 2/6] hw/pci-bridge/cxl_upstream: Provide x-speed and x-width properties.

[Jonathan Cameron via]

Copied from gen_pcie_root_port.c
Drop the previous code that ensured a valid value in s->width, s->speed
as now a default is provided so this will always be set.

Note this changes the default settings but it is unlikely to have a negative
effect on software as will only affect ports with now downstream device.
All other ports will use the settings from that device.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/pci-bridge/cxl_downstream.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/hw/pci-bridge/cxl_downstream.c b/hw/pci-bridge/cxl_downstream.c
index 4b42984360..c347ac06f3 100644
--- a/hw/pci-bridge/cxl_downstream.c
+++ b/hw/pci-bridge/cxl_downstream.c
@@ -13,6 +13,8 @@
 #include "hw/pci/msi.h"
 #include "hw/pci/pcie.h"
 #include "hw/pci/pcie_port.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
 #include "hw/cxl/cxl.h"
 #include "qapi/error.h"
 
@@ -210,24 +212,20 @@ static void cxl_dsp_exitfn(PCIDevice *d)
     pci_bridge_exitfn(d);
 }
 
-static void cxl_dsp_instance_post_init(Object *obj)
-{
-    PCIESlot *s = PCIE_SLOT(obj);
-
-    if (!s->speed) {
-        s->speed = QEMU_PCI_EXP_LNK_2_5GT;
-    }
-
-    if (!s->width) {
-        s->width = QEMU_PCI_EXP_LNK_X1;
-    }
-}
+static Property cxl_dsp_props[] = {
+    DEFINE_PROP_PCIE_LINK_SPEED("x-speed", PCIESlot,
+                                speed, PCIE_LINK_SPEED_64),
+    DEFINE_PROP_PCIE_LINK_WIDTH("x-width", PCIESlot,
+                                width, PCIE_LINK_WIDTH_16),
+    DEFINE_PROP_END_OF_LIST()
+};
 
 static void cxl_dsp_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(oc);
 
+    device_class_set_props(dc, cxl_dsp_props);
     k->config_write = cxl_dsp_config_write;
     k->realize = cxl_dsp_realize;
     k->exit = cxl_dsp_exitfn;
@@ -243,7 +241,6 @@ static const TypeInfo cxl_dsp_info = {
     .name = TYPE_CXL_DSP,
     .instance_size = sizeof(CXLDownstreamPort),
     .parent = TYPE_PCIE_SLOT,
-    .instance_post_init = cxl_dsp_instance_post_init,
     .class_init = cxl_dsp_class_init,
     .interfaces = (InterfaceInfo[]) {
         { INTERFACE_PCIE_DEVICE },
-- 
2.43.0

[PATCH 3/6] hw/pcie: Factor out PCI Express link register filling common to EP.

[Jonathan Cameron via]

Whilst not all link related registers are common between RP / Switch DSP
and EP / Switch USP many of them are.  Factor that group out to save
on duplication when adding EP / Swtich USP configurability.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/pci/pcie.c | 87 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 48 insertions(+), 39 deletions(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 4b2f0805c6..1ac6d89dcf 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -105,46 +105,18 @@ pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t type, uint8_t version)
     pci_set_word(cmask + PCI_EXP_LNKSTA, 0);
 }
 
-static void pcie_cap_fill_slot_lnk(PCIDevice *dev)
+/* Includes setting the target speed default */
+static void pcie_cap_fill_lnk(uint8_t *exp_cap, PCIExpLinkWidth width,
+                              PCIExpLinkSpeed speed)
 {
-    PCIESlot *s = (PCIESlot *)object_dynamic_cast(OBJECT(dev), TYPE_PCIE_SLOT);
-    uint8_t *exp_cap = dev->config + dev->exp.exp_cap;
-
-    /* Skip anything that isn't a PCIESlot */
-    if (!s) {
-        return;
-    }
-
     /* Clear and fill LNKCAP from what was configured above */
     pci_long_test_and_clear_mask(exp_cap + PCI_EXP_LNKCAP,
                                  PCI_EXP_LNKCAP_MLW | PCI_EXP_LNKCAP_SLS);
     pci_long_test_and_set_mask(exp_cap + PCI_EXP_LNKCAP,
-                               QEMU_PCI_EXP_LNKCAP_MLW(s->width) |
-                               QEMU_PCI_EXP_LNKCAP_MLS(s->speed));
-
-    /*
-     * Link bandwidth notification is required for all root ports and
-     * downstream ports supporting links wider than x1 or multiple link
-     * speeds.
-     */
-    if (s->width > QEMU_PCI_EXP_LNK_X1 ||
-        s->speed > QEMU_PCI_EXP_LNK_2_5GT) {
-        pci_long_test_and_set_mask(exp_cap + PCI_EXP_LNKCAP,
-                                   PCI_EXP_LNKCAP_LBNC);
-    }
-
-    if (s->speed > QEMU_PCI_EXP_LNK_2_5GT) {
-        /*
-         * Hot-plug capable downstream ports and downstream ports supporting
-         * link speeds greater than 5GT/s must hardwire PCI_EXP_LNKCAP_DLLLARC
-         * to 1b.  PCI_EXP_LNKCAP_DLLLARC implies PCI_EXP_LNKSTA_DLLLA, which
-         * we also hardwire to 1b here.  2.5GT/s hot-plug slots should also
-         * technically implement this, but it's not done here for compatibility.
-         */
-        pci_long_test_and_set_mask(exp_cap + PCI_EXP_LNKCAP,
-                                   PCI_EXP_LNKCAP_DLLLARC);
-        /* the PCI_EXP_LNKSTA_DLLLA will be set in the hotplug function */
+                               QEMU_PCI_EXP_LNKCAP_MLW(width) |
+                               QEMU_PCI_EXP_LNKCAP_MLS(speed));
 
+    if (speed > QEMU_PCI_EXP_LNK_2_5GT) {
         /*
          * Target Link Speed defaults to the highest link speed supported by
          * the component.  2.5GT/s devices are permitted to hardwire to zero.
@@ -152,7 +124,7 @@ static void pcie_cap_fill_slot_lnk(PCIDevice *dev)
         pci_word_test_and_clear_mask(exp_cap + PCI_EXP_LNKCTL2,
                                      PCI_EXP_LNKCTL2_TLS);
         pci_word_test_and_set_mask(exp_cap + PCI_EXP_LNKCTL2,
-                                   QEMU_PCI_EXP_LNKCAP_MLS(s->speed) &
+                                   QEMU_PCI_EXP_LNKCAP_MLS(speed) &
                                    PCI_EXP_LNKCTL2_TLS);
     }
 
@@ -161,27 +133,64 @@ static void pcie_cap_fill_slot_lnk(PCIDevice *dev)
      * actually a reference to the highest bit supported in this register.
      * We assume the device supports all link speeds.
      */
-    if (s->speed > QEMU_PCI_EXP_LNK_5GT) {
+    if (speed > QEMU_PCI_EXP_LNK_5GT) {
         pci_long_test_and_clear_mask(exp_cap + PCI_EXP_LNKCAP2, ~0U);
         pci_long_test_and_set_mask(exp_cap + PCI_EXP_LNKCAP2,
                                    PCI_EXP_LNKCAP2_SLS_2_5GB |
                                    PCI_EXP_LNKCAP2_SLS_5_0GB |
                                    PCI_EXP_LNKCAP2_SLS_8_0GB);
-        if (s->speed > QEMU_PCI_EXP_LNK_8GT) {
+        if (speed > QEMU_PCI_EXP_LNK_8GT) {
             pci_long_test_and_set_mask(exp_cap + PCI_EXP_LNKCAP2,
                                        PCI_EXP_LNKCAP2_SLS_16_0GB);
         }
-        if (s->speed > QEMU_PCI_EXP_LNK_16GT) {
+        if (speed > QEMU_PCI_EXP_LNK_16GT) {
             pci_long_test_and_set_mask(exp_cap + PCI_EXP_LNKCAP2,
                                        PCI_EXP_LNKCAP2_SLS_32_0GB);
         }
-        if (s->speed > QEMU_PCI_EXP_LNK_32GT) {
+        if (speed > QEMU_PCI_EXP_LNK_32GT) {
             pci_long_test_and_set_mask(exp_cap + PCI_EXP_LNKCAP2,
                                        PCI_EXP_LNKCAP2_SLS_64_0GB);
         }
     }
 }
 
+static void pcie_cap_fill_slot_lnk(PCIDevice *dev)
+{
+    PCIESlot *s = (PCIESlot *)object_dynamic_cast(OBJECT(dev), TYPE_PCIE_SLOT);
+    uint8_t *exp_cap = dev->config + dev->exp.exp_cap;
+
+    /* Skip anything that isn't a PCIESlot */
+    if (!s) {
+        return;
+    }
+
+    /*
+     * Link bandwidth notification is required for all root ports and
+     * downstream ports supporting links wider than x1 or multiple link
+     * speeds.
+     */
+    if (s->width > QEMU_PCI_EXP_LNK_X1 ||
+        s->speed > QEMU_PCI_EXP_LNK_2_5GT) {
+        pci_long_test_and_set_mask(exp_cap + PCI_EXP_LNKCAP,
+                                   PCI_EXP_LNKCAP_LBNC);
+    }
+
+    if (s->speed > QEMU_PCI_EXP_LNK_2_5GT) {
+        /*
+         * Hot-plug capable downstream ports and downstream ports supporting
+         * link speeds greater than 5GT/s must hardwire PCI_EXP_LNKCAP_DLLLARC
+         * to 1b.  PCI_EXP_LNKCAP_DLLLARC implies PCI_EXP_LNKSTA_DLLLA, which
+         * we also hardwire to 1b here.  2.5GT/s hot-plug slots should also
+         * technically implement this, but it's not done here for compatibility.
+         */
+        pci_long_test_and_set_mask(exp_cap + PCI_EXP_LNKCAP,
+                                   PCI_EXP_LNKCAP_DLLLARC);
+        /* the PCI_EXP_LNKSTA_DLLLA will be set in the hotplug function */
+    }
+
+    pcie_cap_fill_lnk(exp_cap, s->width, s->speed);
+}
+
 int pcie_cap_init(PCIDevice *dev, uint8_t offset,
                   uint8_t type, uint8_t port,
                   Error **errp)
-- 
2.43.0

[PATCH 4/6] hw/pcie: Provide a utility function for control of EP / SW USP link

[Jonathan Cameron via]

Whilst similar to existing PCIESlot link configuration a few registers
need to be set differently so that the downstream device presents
a 'configured' state that is then used to 'train' the upstream port
on the link.  Basically that means setting the status register to
reflect it succeeding in training up to target settings.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 include/hw/pci/pcie.h |  2 ++
 hw/pci/pcie.c         | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 5eddb90976..b8d59732bc 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -141,6 +141,8 @@ void pcie_acs_reset(PCIDevice *dev);
 void pcie_ari_init(PCIDevice *dev, uint16_t offset);
 void pcie_dev_ser_num_init(PCIDevice *dev, uint16_t offset, uint64_t ser_num);
 void pcie_ats_init(PCIDevice *dev, uint16_t offset, bool aligned);
+void pcie_cap_fill_link_ep_usp(PCIDevice *dev, PCIExpLinkWidth width,
+                               PCIExpLinkSpeed speed);
 
 void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                                Error **errp);
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 1ac6d89dcf..2738dbb28d 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -154,6 +154,24 @@ static void pcie_cap_fill_lnk(uint8_t *exp_cap, PCIExpLinkWidth width,
     }
 }
 
+void pcie_cap_fill_link_ep_usp(PCIDevice *dev, PCIExpLinkWidth width,
+                               PCIExpLinkSpeed speed)
+{
+    uint8_t *exp_cap = dev->config + dev->exp.exp_cap;
+
+    /*
+     * For an end point or USP need to set the current status as well
+     * as the capabilities.
+     */
+    pci_long_test_and_clear_mask(exp_cap + PCI_EXP_LNKSTA,
+                                 PCI_EXP_LNKSTA_CLS | PCI_EXP_LNKSTA_NLW);
+    pci_long_test_and_set_mask(exp_cap + PCI_EXP_LNKSTA,
+                               QEMU_PCI_EXP_LNKSTA_NLW(width) |
+                               QEMU_PCI_EXP_LNKSTA_CLS(speed));
+
+    pcie_cap_fill_lnk(exp_cap, width, speed);
+}
+
 static void pcie_cap_fill_slot_lnk(PCIDevice *dev)
 {
     PCIESlot *s = (PCIESlot *)object_dynamic_cast(OBJECT(dev), TYPE_PCIE_SLOT);
-- 
2.43.0

[PATCH 5/6] hw/mem/cxl-type3: Add properties to control link speed and width

[Jonathan Cameron via]

To establish performance characteristics of a CXL device when used via a
particular CXL topology (root ports, switches, end points) it is necessary
to set the appropriate link speed and width in the PCI Express capability
structure.  Provide x-speed and x-link properties for this.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 include/hw/cxl/cxl_device.h | 4 ++++
 hw/mem/cxl_type3.c          | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index fdd0f4e62b..e14e56ae4b 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -549,6 +549,10 @@ struct CXLType3Dev {
     CXLCCI vdm_fm_owned_ld_mctp_cci;
     CXLCCI ld0_cci;
 
+    /* PCIe link characteristics */
+    PCIExpLinkSpeed speed;
+    PCIExpLinkWidth width;
+
     /* DOE */
     DOECap doe_cdat;
 
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 235ac40aeb..44d491d8f6 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -17,6 +17,7 @@
 #include "hw/mem/pc-dimm.h"
 #include "hw/pci/pci.h"
 #include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
 #include "qapi/error.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
@@ -1200,6 +1201,7 @@ static void ct3d_reset(DeviceState *dev)
     uint32_t *reg_state = ct3d->cxl_cstate.crb.cache_mem_registers;
     uint32_t *write_msk = ct3d->cxl_cstate.crb.cache_mem_regs_write_mask;
 
+    pcie_cap_fill_link_ep_usp(PCI_DEVICE(dev), ct3d->width, ct3d->speed);
     cxl_component_register_init_common(reg_state, write_msk, CXL2_TYPE3_DEVICE);
     cxl_device_register_init_t3(ct3d);
 
@@ -1229,6 +1231,10 @@ static Property ct3_props[] = {
     DEFINE_PROP_UINT8("num-dc-regions", CXLType3Dev, dc.num_regions, 0),
     DEFINE_PROP_LINK("volatile-dc-memdev", CXLType3Dev, dc.host_dc,
                      TYPE_MEMORY_BACKEND, HostMemoryBackend *),
+    DEFINE_PROP_PCIE_LINK_SPEED("x-speed", CXLType3Dev,
+                                speed, PCIE_LINK_SPEED_32),
+    DEFINE_PROP_PCIE_LINK_WIDTH("x-width", CXLType3Dev,
+                                width, PCIE_LINK_WIDTH_16),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.43.0

[PATCH 6/6] hw/pci-bridge/cxl-upstream: Add properties to control link speed and width

[Jonathan Cameron via]

To establish performance characteristics of a CXL device when used via a
particular CXL topology (root ports, switches, end points) it is necessary
to set the appropriate link speed and width in the PCI Express capability
structure.  Provide x-speed and x-link properties for this.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 include/hw/pci-bridge/cxl_upstream_port.h | 4 ++++
 hw/pci-bridge/cxl_upstream.c              | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/include/hw/pci-bridge/cxl_upstream_port.h b/include/hw/pci-bridge/cxl_upstream_port.h
index 12635139f6..f208397ffe 100644
--- a/include/hw/pci-bridge/cxl_upstream_port.h
+++ b/include/hw/pci-bridge/cxl_upstream_port.h
@@ -12,6 +12,10 @@ typedef struct CXLUpstreamPort {
     /*< public >*/
     CXLComponentState cxl_cstate;
     CXLCCI swcci;
+
+    PCIExpLinkSpeed speed;
+    PCIExpLinkWidth width;
+
     DOECap doe_cdat;
     uint64_t sn;
 } CXLUpstreamPort;
diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c
index a5a39cc524..55f8b0053f 100644
--- a/hw/pci-bridge/cxl_upstream.c
+++ b/hw/pci-bridge/cxl_upstream.c
@@ -11,6 +11,7 @@
 #include "qemu/osdep.h"
 #include "qemu/log.h"
 #include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
 #include "hw/pci/msi.h"
 #include "hw/pci/pcie.h"
 #include "hw/pci/pcie_port.h"
@@ -100,6 +101,7 @@ static void cxl_usp_reset(DeviceState *qdev)
 
     pci_bridge_reset(qdev);
     pcie_cap_deverr_reset(d);
+    pcie_cap_fill_link_ep_usp(d, usp->width, usp->speed);
     latch_registers(usp);
 }
 
@@ -363,6 +365,10 @@ 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_PCIE_LINK_SPEED("x-speed", CXLUpstreamPort,
+                                speed, PCIE_LINK_SPEED_32),
+    DEFINE_PROP_PCIE_LINK_WIDTH("x-width", CXLUpstreamPort,
+                                width, PCIE_LINK_WIDTH_16),
     DEFINE_PROP_END_OF_LIST()
 };
 
-- 
2.43.0

Re: [PATCH qemu 0/6] hw/cxl: Link speed and width control

[Jonathan Cameron via]

Gentle ping.

I'm less worried about hitting this cycle for this than the Generic Port
work but this is the missing piece for ensuring we can test the kernel
code that builds NUMA description for CXL devices. I'd like to have
the full solution for that upstream.

The bulk of this is a refactor with functional changes limited to
the CXL components.

Jonathan


On Mon, 16 Sep 2024 18:35:12 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> Changes since RFC:
> - rebase
> 
> Question:
> - I could enable this for all PCIe device (including ports).
>   Does that makes sense, or is it better to limit this to my cases?
>   It is quite easy to build broken setups (downstream device reports
>   faster link than the port etc) because QEMU 'link' training' is
>   simplistic.  I'm not sure it is worth making it more clever.
> 
> The Generic Ports support added the ability to describe the bandwidth and
> Latency within a host to a CXL host bridge.  To be able to test the of the
> discovery path used by Linux [1] we also need to be able to create
> bottlenecks at difference places in the topology. There are two parts to
> this
> * CXL link characteristics as described by PCI Express Capability Link
>   status etc.
> * Bandwidth and latency across CXL Switches (via CDAT data from the switch
>   USP)
> * Bandwidth and latency from the CXL type 3 device port to the actual
>   memory (Via CDAT data from the EP).
> 
> Currently we have fixed values for the CXL CDAT tables, and to test this
> I recommend changing those as per the patch at the end of this cover letter
> (so they aren't always the bottleneck). Making those configurable will be
> handled in a future patch set.
> 
> RFC has a set of examples but those were to help testing the kernel code
> rather than providing much info for QEMU review so I haven't repeated
> them ehre.
> 
> https://lore.kernel.org/qemu-devel/20240712122414.1448284-1-Jonathan.Cameron@huawei.com/
> 
> Jonathan Cameron (6):
>   hw/pci-bridge/cxl_root_port: Provide x-speed and x-width properties.
>   hw/pci-bridge/cxl_upstream: Provide x-speed and x-width properties.
>   hw/pcie: Factor out PCI Express link register filling common to EP.
>   hw/pcie: Provide a utility function for control of EP / SW USP link
>   hw/mem/cxl-type3: Add properties to control link speed and width
>   hw/pci-bridge/cxl-upstream: Add properties to control link speed and
>     width
> 
>  include/hw/cxl/cxl_device.h               |   4 +
>  include/hw/pci-bridge/cxl_upstream_port.h |   4 +
>  include/hw/pci/pcie.h                     |   2 +
>  hw/mem/cxl_type3.c                        |   6 ++
>  hw/pci-bridge/cxl_downstream.c            |  23 +++--
>  hw/pci-bridge/cxl_root_port.c             |   5 ++
>  hw/pci-bridge/cxl_upstream.c              |   6 ++
>  hw/pci/pcie.c                             | 105 ++++++++++++++--------
>  8 files changed, 103 insertions(+), 52 deletions(-)
> 

Re: [PATCH 1/6] hw/pci-bridge/cxl_root_port: Provide x-speed and x-width properties.

[Fan Ni]

On Mon, Sep 16, 2024 at 06:35:13PM +0100, Jonathan Cameron wrote:
> Approach copied from gen_pcie_root_port.c
> Previously the link defaulted to a maximum of 2.5GT/s and 1x.  Enable setting
> it's maximum values.  The actual value after 'training' will depend on the
> downstream device configuration.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  hw/pci-bridge/cxl_root_port.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/pci-bridge/cxl_root_port.c b/hw/pci-bridge/cxl_root_port.c
> index 2dd10239bd..5e2156d7ba 100644
> --- a/hw/pci-bridge/cxl_root_port.c
> +++ b/hw/pci-bridge/cxl_root_port.c
> @@ -24,6 +24,7 @@
>  #include "hw/pci/pcie_port.h"
>  #include "hw/pci/msi.h"
>  #include "hw/qdev-properties.h"
> +#include "hw/qdev-properties-system.h"
>  #include "hw/sysbus.h"
>  #include "qapi/error.h"
>  #include "hw/cxl/cxl.h"
> @@ -206,6 +207,10 @@ static Property gen_rp_props[] = {
>                       -1),
>      DEFINE_PROP_SIZE("pref64-reserve", CXLRootPort, res_reserve.mem_pref_64,
>                       -1),
> +    DEFINE_PROP_PCIE_LINK_SPEED("x-speed", PCIESlot,
> +                                speed, PCIE_LINK_SPEED_64),
> +    DEFINE_PROP_PCIE_LINK_WIDTH("x-width", PCIESlot,
> +                                width, PCIE_LINK_WIDTH_32),
>      DEFINE_PROP_END_OF_LIST()
>  };
LGTM.

Reviewed-by: Fan Ni <fan.ni@samsung.com>

Fan
>  
> -- 
> 2.43.0
> 

-- 
Fan Ni

Re: [PATCH 2/6] hw/pci-bridge/cxl_upstream: Provide x-speed and x-width properties.

[Fan Ni]

On Mon, Sep 16, 2024 at 06:35:14PM +0100, Jonathan Cameron wrote:
> Copied from gen_pcie_root_port.c
> Drop the previous code that ensured a valid value in s->width, s->speed
> as now a default is provided so this will always be set.
> 
> Note this changes the default settings but it is unlikely to have a negative
> effect on software as will only affect ports with now downstream device.
> All other ports will use the settings from that device.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  hw/pci-bridge/cxl_downstream.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/pci-bridge/cxl_downstream.c b/hw/pci-bridge/cxl_downstream.c
> index 4b42984360..c347ac06f3 100644
> --- a/hw/pci-bridge/cxl_downstream.c
> +++ b/hw/pci-bridge/cxl_downstream.c
> @@ -13,6 +13,8 @@
>  #include "hw/pci/msi.h"
>  #include "hw/pci/pcie.h"
>  #include "hw/pci/pcie_port.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/qdev-properties-system.h"
>  #include "hw/cxl/cxl.h"
>  #include "qapi/error.h"
>  
> @@ -210,24 +212,20 @@ static void cxl_dsp_exitfn(PCIDevice *d)
>      pci_bridge_exitfn(d);
>  }
>  
> -static void cxl_dsp_instance_post_init(Object *obj)
> -{
> -    PCIESlot *s = PCIE_SLOT(obj);
> -
> -    if (!s->speed) {
> -        s->speed = QEMU_PCI_EXP_LNK_2_5GT;
> -    }
> -
> -    if (!s->width) {
> -        s->width = QEMU_PCI_EXP_LNK_X1;
> -    }
> -}
> +static Property cxl_dsp_props[] = {
> +    DEFINE_PROP_PCIE_LINK_SPEED("x-speed", PCIESlot,
> +                                speed, PCIE_LINK_SPEED_64),
> +    DEFINE_PROP_PCIE_LINK_WIDTH("x-width", PCIESlot,
> +                                width, PCIE_LINK_WIDTH_16),

Not sure why. For the root port, we use PCIE_LINK_WIDTH_32, and here it is
PCIE_LINK_WIDTH_16?

Fan

> +    DEFINE_PROP_END_OF_LIST()
> +};
>  
>  static void cxl_dsp_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(oc);
>  
> +    device_class_set_props(dc, cxl_dsp_props);
>      k->config_write = cxl_dsp_config_write;
>      k->realize = cxl_dsp_realize;
>      k->exit = cxl_dsp_exitfn;
> @@ -243,7 +241,6 @@ static const TypeInfo cxl_dsp_info = {
>      .name = TYPE_CXL_DSP,
>      .instance_size = sizeof(CXLDownstreamPort),
>      .parent = TYPE_PCIE_SLOT,
> -    .instance_post_init = cxl_dsp_instance_post_init,
>      .class_init = cxl_dsp_class_init,
>      .interfaces = (InterfaceInfo[]) {
>          { INTERFACE_PCIE_DEVICE },
> -- 
> 2.43.0
> 

-- 
Fan Ni

Re: [PATCH 2/6] hw/pci-bridge/cxl_upstream: Provide x-speed and x-width properties.

[Jonathan Cameron via]

On Tue, 29 Oct 2024 09:37:59 -0700
Fan Ni <nifan.cxl@gmail.com> wrote:

> On Mon, Sep 16, 2024 at 06:35:14PM +0100, Jonathan Cameron wrote:
> > Copied from gen_pcie_root_port.c
> > Drop the previous code that ensured a valid value in s->width, s->speed
> > as now a default is provided so this will always be set.
> > 
> > Note this changes the default settings but it is unlikely to have a negative
> > effect on software as will only affect ports with now downstream device.
> > All other ports will use the settings from that device.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  hw/pci-bridge/cxl_downstream.c | 23 ++++++++++-------------
> >  1 file changed, 10 insertions(+), 13 deletions(-)
> > 
> > diff --git a/hw/pci-bridge/cxl_downstream.c b/hw/pci-bridge/cxl_downstream.c
> > index 4b42984360..c347ac06f3 100644
> > --- a/hw/pci-bridge/cxl_downstream.c
> > +++ b/hw/pci-bridge/cxl_downstream.c
> > @@ -13,6 +13,8 @@
> >  #include "hw/pci/msi.h"
> >  #include "hw/pci/pcie.h"
> >  #include "hw/pci/pcie_port.h"
> > +#include "hw/qdev-properties.h"
> > +#include "hw/qdev-properties-system.h"
> >  #include "hw/cxl/cxl.h"
> >  #include "qapi/error.h"
> >  
> > @@ -210,24 +212,20 @@ static void cxl_dsp_exitfn(PCIDevice *d)
> >      pci_bridge_exitfn(d);
> >  }
> >  
> > -static void cxl_dsp_instance_post_init(Object *obj)
> > -{
> > -    PCIESlot *s = PCIE_SLOT(obj);
> > -
> > -    if (!s->speed) {
> > -        s->speed = QEMU_PCI_EXP_LNK_2_5GT;
> > -    }
> > -
> > -    if (!s->width) {
> > -        s->width = QEMU_PCI_EXP_LNK_X1;
> > -    }
> > -}
> > +static Property cxl_dsp_props[] = {
> > +    DEFINE_PROP_PCIE_LINK_SPEED("x-speed", PCIESlot,
> > +                                speed, PCIE_LINK_SPEED_64),
> > +    DEFINE_PROP_PCIE_LINK_WIDTH("x-width", PCIESlot,
> > +                                width, PCIE_LINK_WIDTH_16),  
> 
> Not sure why. For the root port, we use PCIE_LINK_WIDTH_32, and here it is
> PCIE_LINK_WIDTH_16?

Random choice. There is no obvious default to choose.

I'm fine changing them to another choice if that makes more sense.

Jonathan


> 
> Fan
> 
> > +    DEFINE_PROP_END_OF_LIST()
> > +};
> >  
> >  static void cxl_dsp_class_init(ObjectClass *oc, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(oc);
> >      PCIDeviceClass *k = PCI_DEVICE_CLASS(oc);
> >  
> > +    device_class_set_props(dc, cxl_dsp_props);
> >      k->config_write = cxl_dsp_config_write;
> >      k->realize = cxl_dsp_realize;
> >      k->exit = cxl_dsp_exitfn;
> > @@ -243,7 +241,6 @@ static const TypeInfo cxl_dsp_info = {
> >      .name = TYPE_CXL_DSP,
> >      .instance_size = sizeof(CXLDownstreamPort),
> >      .parent = TYPE_PCIE_SLOT,
> > -    .instance_post_init = cxl_dsp_instance_post_init,
> >      .class_init = cxl_dsp_class_init,
> >      .interfaces = (InterfaceInfo[]) {
> >          { INTERFACE_PCIE_DEVICE },
> > -- 
> > 2.43.0
> >   
>