Re: [PULL 70/73] hw/pxb-cxl: Support passthrough HDM Decoders unless overridden

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Wed, 19 Apr 2023 14:57:54 +0100
Jonathan Cameron via <qemu-devel@nongnu.org> wrote:

> On Tue, 11 Apr 2023 11:26:16 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> > On Wed, 8 Mar 2023 at 01:14, Michael S. Tsirkin <mst@redhat.com> wrote:  
> > >
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >
> > > The CXL r3.0 specification allows for there to be no HDM decoders on CXL
> > > Host Bridges if they have only a single root port. Instead, all accesses
> > > directed to the host bridge (as specified in CXL Fixed Memory Windows)
> > > are assumed to be routed to the single root port.    
> > 
> > Hi; in issue https://gitlab.com/qemu-project/qemu/-/issues/1586
> > it's been pointed out that this commit causes an assertion
> > failure during 'make check' if you configure with
> > --enable-qom-cast-debug. You can repro by doing that and running:
> > 
> >  qemu-system-i386 -display none -machine q35,cxl=on -device pxb-cxl,bus=pcie.0
> > 
> > Here's a backtrace:
> > 
> > Thread 1 "qemu-system-i38" received signal SIGABRT, Aborted.
> > __pthread_kill_implementation (no_tid=0, signo=6,
> > threadid=140737217810816) at ./nptl/pthread_kill.c:44
> > 44      ./nptl/pthread_kill.c: No such file or directory.
> > (gdb) bt
> > #0  __pthread_kill_implementation (no_tid=0, signo=6,
> > threadid=140737217810816) at ./nptl/pthread_kill.c:44
> > #1  __pthread_kill_internal (signo=6, threadid=140737217810816) at
> > ./nptl/pthread_kill.c:78
> > #2  __GI___pthread_kill (threadid=140737217810816,
> > signo=signo@entry=6) at ./nptl/pthread_kill.c:89
> > #3  0x00007ffff4b1c476 in __GI_raise (sig=sig@entry=6) at
> > ../sysdeps/posix/raise.c:26
> > #4  0x00007ffff4b027f3 in __GI_abort () at ./stdlib/abort.c:79
> > #5  0x0000555555cecfab in object_dynamic_cast_assert
> >     (obj=obj@entry=0x555557a70b60, typename=0x555555f80406 "pxb",
> > file=0x555555f80357 "../../hw/pci-bridge/pci_expander_bridge.c",
> > line=line@entry=55, func=0x555555f8040a "PXB_DEV") at
> > ../../qom/object.c:890
> > #6  0x00005555559c7bbd in PXB_DEV (obj=0x555557a70b60) at
> > ../../hw/pci-bridge/pci_expander_bridge.c:54
> > #7  pxb_cxl_dev_reset (dev=0x555557a70b60) at
> > ../../hw/pci-bridge/pci_expander_bridge.c:314
> > #8  0x00005555559bd624 in pci_qdev_realize (qdev=0x555557a70b60,
> > errp=0x7fffffffdd28) at ../../hw/pci/pci.c:2098
> > #9  0x0000555555ce8ada in device_set_realized (obj=<optimised out>,
> > value=true, errp=0x7fffffffdea8) at ../../hw/core/qdev.c:510
> > #10 0x0000555555cf0219 in property_set_bool
> >      (obj=obj@entry=0x555557a70b60, v=v@entry=0x555557a727b0,
> > name=name@entry=0x55555601db04 "realized", opaque=0x55555687b780,
> > errp=errp@entry=0x7fffffffdea8) at ../../qom/object.c:2285
> > #11 0x0000555555cee4e5 in object_property_set
> >      (obj=obj@entry=0x555557a70b60, name=name@entry=0x55555601db04
> > "realized", v=0x555557a727b0, errp=errp@entry=0x7fffffffdea8) at
> > ../../qom/object.c:1420
> > #12 0x0000555555cf23cd in object_property_set_qobject
> >     (obj=obj@entry=0x555557a70b60, name=name@entry=0x55555601db04
> > "realized", value=<optimised out>, errp=errp@entry=0x7fffffffdea8) at
> > ../../qom/qom-qobject.c:28
> > #13 0x0000555555cee93b in object_property_set_bool
> > (obj=0x555557a70b60, name=0x55555601db04 "realized", value=<optimised  
> > out>, errp=0x7fffffffdea8)    
> >     at ../../qom/object.c:1489
> > #14 0x0000555555a6ae42 in qdev_device_add_from_qdict
> > (opts=0x555557a6fb40, from_json=false, errp=0x7fffffffdea8,
> > errp@entry=0x555556765830 <error_fatal>)
> >     at ../../softmmu/qdev-monitor.c:714
> > #15 0x0000555555a6b202 in qdev_device_add
> > (opts=opts@entry=0x5555568776f0, errp=errp@entry=0x555556765830
> > <error_fatal>) at ../../softmmu/qdev-monitor.c:733
> > #16 0x0000555555a7367f in device_init_func (opaque=opaque@entry=0x0,
> > opts=0x3cd16a, opts@entry=0x5555568776f0, errp=0x6,
> > errp@entry=0x555556765830 <error_fatal>)
> >     at ../../softmmu/vl.c:1140
> > #17 0x0000555555e78331 in qemu_opts_foreach
> >     (list=<optimised out>, func=0x555555a73670 <device_init_func>,
> > opaque=opaque@entry=0x0, errp=0x555556765830 <error_fatal>) at
> > ../../util/qemu-option.c:1135
> > #18 0x0000555555a6dd61 in qemu_create_cli_devices () at ../../softmmu/vl.c:2542
> > #19 qmp_x_exit_preconfig (errp=<optimised out>) at ../../softmmu/vl.c:2610
> > #20 0x0000555555a7177b in qemu_init (argc=<optimised out>,
> > argv=<optimised out>) at ../../softmmu/vl.c:3612
> > #21 0x000055555587b656 in main (argc=3985770, argv=0x3cd16a) at
> > ../../softmmu/main.c:47
> > 
> > The problem is here:
> >   
> > > -static void pxb_dev_reset(DeviceState *dev)
> > > +static void pxb_cxl_dev_reset(DeviceState *dev)    
> > 
> > This function is called from  pxb_cxl_dev_realize(),
> > which is the realize function for TYPE_PXB_CXL_DEVICE.
> > That type's parent is TYPE_PCI_DEVICE.
> >   
> > >  {
> > >      CXLHost *cxl = PXB_CXL_DEV(dev)->cxl.cxl_host_bridge;
> > >      CXLComponentState *cxl_cstate = &cxl->cxl_cstate;
> > > +    PCIHostState *hb = PCI_HOST_BRIDGE(cxl);
> > >      uint32_t *reg_state = cxl_cstate->crb.cache_mem_registers;
> > >      uint32_t *write_msk = cxl_cstate->crb.cache_mem_regs_write_mask;
> > > +    int dsp_count = 0;
> > >
> > >      cxl_component_register_init_common(reg_state, write_msk, CXL2_ROOT_PORT);
> > > -    ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, TARGET_COUNT, 8);
> > > +    /*
> > > +     * The CXL specification allows for host bridges with no HDM decoders
> > > +     * if they only have a single root port.
> > > +     */
> > > +    if (!PXB_DEV(dev)->hdm_for_passthrough) {    
> > 
> > However, here we try to cast the device pointer to PXB_DEV.
> > That is not permitted because dev is not of type TYPE_PXB_DEVICE
> > (either directly or as a parent class). So if you have the QOM
> > debugging enabled then the attempt to cast causes an assertion
> > failure.
> >   
> > > +        dsp_count = pcie_count_ds_ports(hb->bus);
> > > +    }
> > > +    /* Initial reset will have 0 dsp so wait until > 0 */
> > > +    if (dsp_count == 1) {
> > > +        cxl->passthrough = true;
> > > +        /* Set Capability ID in header to NONE */
> > > +        ARRAY_FIELD_DP32(reg_state, CXL_HDM_CAPABILITY_HEADER, ID, 0);
> > > +    } else {
> > > +        ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, TARGET_COUNT,
> > > +                         8);
> > > +    }
> > >  }    
> > 
> > What was the intention here with the type hierarchy?
> > Should TYPE_PXB_CXL_DEVICE be a subclass of TYPE_PXB_DEVICE,
> > or should the cxl-related functions not be trying to treat
> > it as a PXB device ?  
> 
> I can't immediately recall why, but PXB_DEV and PXB_CXL_DEV use the
> same struct PXBDev so here switching to PXB_CXL_DEV(dev)->hdm_for_passthrough
> looks to be the minimum fix.
> 
> I'll dig into why / if there is a good reason for why PXB_CXL_DEV doesn't
> simply inherit from PXB_DEV then use runtime type checking in the few places it
> will matter.

Ah. Looks to be cut and paste from what TYPE_PXB_PCIE_DEV was doing. 
We probably never considered if that was a good path to take or not.
Not clear why they can't both just inherit from TYPE_PXB_DEV.
At least superficially it seems to work + is cleaner.

Following only lightly tested... May eat babies etc.

From 995226fcdfe196e010c5a3850bfca2f97a384307 Mon Sep 17 00:00:00 2001
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Date: Wed, 19 Apr 2023 15:41:44 +0100
Subject: [PATCH] hw/pci-bridge: Make PCIe and CXL PXB Devices inherit from
 PXB_DEVICE

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/acpi/cxl.c                       |  9 +++---
 hw/cxl/cxl-host.c                   |  2 +-
 hw/pci-bridge/pci_expander_bridge.c | 50 +++++++++--------------------
 include/hw/cxl/cxl.h                |  4 +--
 include/hw/pci/pci_bridge.h         | 26 +++++++++++----
 5 files changed, 43 insertions(+), 48 deletions(-)

diff --git a/hw/acpi/cxl.c b/hw/acpi/cxl.c
index 17dc0bdc9c..ad8faef48b 100644
--- a/hw/acpi/cxl.c
+++ b/hw/acpi/cxl.c
@@ -87,9 +87,10 @@ void build_cxl_dsm_method(Aml *dev)
     aml_append(dev, method);
 }
 
-static void cedt_build_chbs(GArray *table_data, PXBDev *cxl)
+static void cedt_build_chbs(GArray *table_data, PXBCXLDev *cxl)
 {
-    SysBusDevice *sbd = SYS_BUS_DEVICE(cxl->cxl.cxl_host_bridge);
+    PXBDev *pxb = PXB_DEV(cxl);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(cxl->cxl_host_bridge);
     struct MemoryRegion *mr = sbd->mmio[0].memory;
 
     /* Type */
@@ -102,7 +103,7 @@ static void cedt_build_chbs(GArray *table_data, PXBDev *cxl)
     build_append_int_noprefix(table_data, 32, 2);
 
     /* UID - currently equal to bus number */
-    build_append_int_noprefix(table_data, cxl->bus_nr, 4);
+    build_append_int_noprefix(table_data, pxb->bus_nr, 4);
 
     /* Version */
     build_append_int_noprefix(table_data, 1, 4);
@@ -169,7 +170,7 @@ static void cedt_build_cfmws(GArray *table_data, CXLState *cxls)
         /* Host Bridge List (list of UIDs - currently bus_nr) */
         for (i = 0; i < fw->num_targets; i++) {
             g_assert(fw->target_hbs[i]);
-            build_append_int_noprefix(table_data, fw->target_hbs[i]->bus_nr, 4);
+            build_append_int_noprefix(table_data, PXB_DEV(fw->target_hbs[i])->bus_nr, 4);
         }
     }
 }
diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
index d75f673e6d..d6d510803f 100644
--- a/hw/cxl/cxl-host.c
+++ b/hw/cxl/cxl-host.c
@@ -167,7 +167,7 @@ static PCIDevice *cxl_cfmws_find_device(CXLFixedWindow *fw, hwaddr addr)
     addr += fw->base;
 
     rb_index = (addr / cxl_decode_ig(fw->enc_int_gran)) % fw->num_targets;
-    hb = PCI_HOST_BRIDGE(fw->target_hbs[rb_index]->cxl.cxl_host_bridge);
+    hb = PCI_HOST_BRIDGE(fw->target_hbs[rb_index]->cxl_host_bridge);
     if (!hb || !hb->bus || !pci_bus_is_cxl(hb->bus)) {
         return NULL;
     }
diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index ead33f0c05..2af942dc10 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -50,25 +50,10 @@ struct PXBBus {
     char bus_path[8];
 };
 
-#define TYPE_PXB_DEVICE "pxb"
-DECLARE_INSTANCE_CHECKER(PXBDev, PXB_DEV,
-                         TYPE_PXB_DEVICE)
-
 #define TYPE_PXB_PCIE_DEVICE "pxb-pcie"
 DECLARE_INSTANCE_CHECKER(PXBDev, PXB_PCIE_DEV,
                          TYPE_PXB_PCIE_DEVICE)
 
-static PXBDev *convert_to_pxb(PCIDevice *dev)
-{
-    /* A CXL PXB's parent bus is PCIe, so the normal check won't work */
-    if (object_dynamic_cast(OBJECT(dev), TYPE_PXB_CXL_DEVICE)) {
-        return PXB_CXL_DEV(dev);
-    }
-
-    return pci_bus_is_express(pci_get_bus(dev))
-        ? PXB_PCIE_DEV(dev) : PXB_DEV(dev);
-}
-
 static GList *pxb_dev_list;
 
 #define TYPE_PXB_HOST "pxb-host"
@@ -89,14 +74,14 @@ bool cxl_get_hb_passthrough(PCIHostState *hb)
 
 static int pxb_bus_num(PCIBus *bus)
 {
-    PXBDev *pxb = convert_to_pxb(bus->parent_dev);
+    PXBDev *pxb = PXB_DEV(bus->parent_dev);
 
     return pxb->bus_nr;
 }
 
 static uint16_t pxb_bus_numa_node(PCIBus *bus)
 {
-    PXBDev *pxb = convert_to_pxb(bus->parent_dev);
+    PXBDev *pxb = PXB_DEV(bus->parent_dev);
 
     return pxb->numa_node;
 }
@@ -154,7 +139,7 @@ static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
 
     pxb_host = PCI_HOST_BRIDGE(dev);
     pxb_bus = pxb_host->bus;
-    pxb_dev = convert_to_pxb(pxb_bus->parent_dev);
+    pxb_dev = PXB_DEV(pxb_bus->parent_dev);
     position = g_list_index(pxb_dev_list, pxb_dev);
     assert(position >= 0);
 
@@ -212,8 +197,8 @@ static void pxb_cxl_realize(DeviceState *dev, Error **errp)
  */
 void pxb_cxl_hook_up_registers(CXLState *cxl_state, PCIBus *bus, Error **errp)
 {
-    PXBDev *pxb =  PXB_CXL_DEV(pci_bridge_get_device(bus));
-    CXLHost *cxl = pxb->cxl.cxl_host_bridge;
+    PXBCXLDev *pxb =  PXB_CXL_DEV(pci_bridge_get_device(bus));
+    CXLHost *cxl = pxb->cxl_host_bridge;
     CXLComponentState *cxl_cstate = &cxl->cxl_cstate;
     struct MemoryRegion *mr = &cxl_cstate->crb.component_registers;
     hwaddr offset;
@@ -299,7 +284,7 @@ static int pxb_map_irq_fn(PCIDevice *pci_dev, int pin)
 
 static void pxb_cxl_dev_reset(DeviceState *dev)
 {
-    CXLHost *cxl = PXB_CXL_DEV(dev)->cxl.cxl_host_bridge;
+    CXLHost *cxl = PXB_CXL_DEV(dev)->cxl_host_bridge;
     CXLComponentState *cxl_cstate = &cxl->cxl_cstate;
     PCIHostState *hb = PCI_HOST_BRIDGE(cxl);
     uint32_t *reg_state = cxl_cstate->crb.cache_mem_registers;
@@ -311,7 +296,7 @@ static void pxb_cxl_dev_reset(DeviceState *dev)
      * The CXL specification allows for host bridges with no HDM decoders
      * if they only have a single root port.
      */
-    if (!PXB_DEV(dev)->hdm_for_passthrough) {
+    if (!PXB_CXL_DEV(dev)->hdm_for_passthrough) {
         dsp_count = pcie_count_ds_ports(hb->bus);
     }
     /* Initial reset will have 0 dsp so wait until > 0 */
@@ -337,7 +322,7 @@ static gint pxb_compare(gconstpointer a, gconstpointer b)
 static void pxb_dev_realize_common(PCIDevice *dev, enum BusType type,
                                    Error **errp)
 {
-    PXBDev *pxb = convert_to_pxb(dev);
+    PXBDev *pxb = PXB_DEV(dev);
     DeviceState *ds, *bds = NULL;
     PCIBus *bus;
     const char *dev_name = NULL;
@@ -365,7 +350,7 @@ static void pxb_dev_realize_common(PCIDevice *dev, enum BusType type,
     } else if (type == CXL) {
         bus = pci_root_bus_new(ds, dev_name, NULL, NULL, 0, TYPE_PXB_CXL_BUS);
         bus->flags |= PCI_BUS_CXL;
-        PXB_CXL_DEV(dev)->cxl.cxl_host_bridge = PXB_CXL_HOST(ds);
+        PXB_CXL_DEV(dev)->cxl_host_bridge = PXB_CXL_HOST(ds);
     } else {
         bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS);
         bds = qdev_new("pci-bridge");
@@ -418,7 +403,7 @@ static void pxb_dev_realize(PCIDevice *dev, Error **errp)
 
 static void pxb_dev_exitfn(PCIDevice *pci_dev)
 {
-    PXBDev *pxb = convert_to_pxb(pci_dev);
+    PXBDev *pxb = PXB_DEV(pci_dev);
 
     pxb_dev_list = g_list_remove(pxb_dev_list, pxb);
 }
@@ -481,15 +466,14 @@ static void pxb_pcie_dev_class_init(ObjectClass *klass, void *data)
     k->class_id = PCI_CLASS_BRIDGE_HOST;
 
     dc->desc = "PCI Express Expander Bridge";
-    device_class_set_props(dc, pxb_dev_properties);
     dc->hotpluggable = false;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
 }
 
 static const TypeInfo pxb_pcie_dev_info = {
     .name          = TYPE_PXB_PCIE_DEVICE,
-    .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(PXBDev),
+    .parent        = TYPE_PXB_DEVICE,
+    .instance_size = sizeof(PXBPCIEDev),
     .class_init    = pxb_pcie_dev_class_init,
     .interfaces = (InterfaceInfo[]) {
         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
@@ -510,11 +494,7 @@ static void pxb_cxl_dev_realize(PCIDevice *dev, Error **errp)
 }
 
 static Property pxb_cxl_dev_properties[] = {
-    /* Note: 0 is not a legal PXB bus number. */
-    DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0),
-    DEFINE_PROP_UINT16("numa_node", PXBDev, numa_node, NUMA_NODE_UNASSIGNED),
-    DEFINE_PROP_BOOL("bypass_iommu", PXBDev, bypass_iommu, false),
-    DEFINE_PROP_BOOL("hdm_for_passthrough", PXBDev, hdm_for_passthrough, false),
+    DEFINE_PROP_BOOL("hdm_for_passthrough", PXBCXLDev, hdm_for_passthrough, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -541,8 +521,8 @@ static void pxb_cxl_dev_class_init(ObjectClass *klass, void *data)
 
 static const TypeInfo pxb_cxl_dev_info = {
     .name          = TYPE_PXB_CXL_DEVICE,
-    .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(PXBDev),
+    .parent        = TYPE_PXB_PCIE_DEVICE,
+    .instance_size = sizeof(PXBCXLDev),
     .class_init    = pxb_cxl_dev_class_init,
     .interfaces =
         (InterfaceInfo[]){
diff --git a/include/hw/cxl/cxl.h b/include/hw/cxl/cxl.h
index 97c1bc2c8a..028e78a40d 100644
--- a/include/hw/cxl/cxl.h
+++ b/include/hw/cxl/cxl.h
@@ -26,12 +26,12 @@
 
 #define CXL_WINDOW_MAX 10
 
-typedef struct PXBDev PXBDev;
+typedef struct PXBCXLDev PXBCXLDev;
 
 typedef struct CXLFixedWindow {
     uint64_t size;
     char **targets;
-    PXBDev *target_hbs[8];
+    PXBCXLDev *target_hbs[8];
     uint8_t num_targets;
     uint8_t enc_int_ways;
     uint8_t enc_int_gran;
diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
index 0aac8e9db0..57f66da5bd 100644
--- a/include/hw/pci/pci_bridge.h
+++ b/include/hw/pci/pci_bridge.h
@@ -85,7 +85,7 @@ struct PCIBridge {
 #define PCI_BRIDGE_DEV_PROP_SHPC       "shpc"
 typedef struct CXLHost CXLHost;
 
-struct PXBDev {
+typedef struct PXBDev {
     /*< private >*/
     PCIDevice parent_obj;
     /*< public >*/
@@ -93,14 +93,28 @@ struct PXBDev {
     uint8_t bus_nr;
     uint16_t numa_node;
     bool bypass_iommu;
+} PXBDev;
+
+typedef struct PXBPCIEDev {
+    /*< private >*/
+    PXBDev parent_obj;
+} PXBPCIEDev;
+
+#define TYPE_PXB_DEVICE "pxb"
+DECLARE_INSTANCE_CHECKER(PXBDev, PXB_DEV,
+                         TYPE_PXB_DEVICE)
+
+typedef struct PXBCXLDev {
+    /*< private >*/
+    PXBPCIEDev parent_obj;
+    /*< public >*/
+
     bool hdm_for_passthrough;
-    struct cxl_dev {
-        CXLHost *cxl_host_bridge; /* Pointer to a CXLHost */
-    } cxl;
-};
+    CXLHost *cxl_host_bridge; /* Pointer to a CXLHost */
+} PXBCXLDev;
 
 #define TYPE_PXB_CXL_DEVICE "pxb-cxl"
-DECLARE_INSTANCE_CHECKER(PXBDev, PXB_CXL_DEV,
+DECLARE_INSTANCE_CHECKER(PXBCXLDev, PXB_CXL_DEV,
                          TYPE_PXB_CXL_DEVICE)
 
 int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
-- 
2.37.2


> 
> Jonathan
> 
> > 
> > thanks
> > -- PMM  
> 
> 
>