On Wed, 19 Apr 2023 14:57:54 +0100 Jonathan Cameron via <[email protected]> wrote:
> On Tue, 11 Apr 2023 11:26:16 +0100 > Peter Maydell <[email protected]> wrote: > > > On Wed, 8 Mar 2023 at 01:14, Michael S. Tsirkin <[email protected]> wrote: > > > > > > From: Jonathan Cameron <[email protected]> > > > > > > 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 <[email protected]> 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 <[email protected]> --- 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 > > >
