[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 70/73] hw/pxb-cxl: Support passthrough HDM Decoders unless ove
|
From: |
Jonathan Cameron |
|
Subject: |
Re: [PULL 70/73] hw/pxb-cxl: Support passthrough HDM Decoders unless overridden |
|
Date: |
Wed, 19 Apr 2023 15:49:57 +0100 |
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
>
>
>