[PATCH 0/2] hw/pci-bridge: pci_expander_bridge: Fix wrong type and rework inheritance.

[PATCH 0/2] hw/pci-bridge: pci_expander_bridge: Fix wrong type and rework inheritance.

[Jonathan Cameron via]

Peter Maydell highlighted an incorrect conversion to TYPE_PXB_DEVICE from
a device that didn't have that a an ancestor type. PXB_DEV() used instead of
PXB_CXL_DEV()/

https://lore.kernel.org/qemu-devel/CAFEAcA-+de+eeLCE4YsAw1O-Qyd_4W1Ra05mGDsU_-3a6d92qw@mail.gmail.com/

During the discussion it became clear that the inheritance of the various
TYPE_PXB*_DEVICE was unusual. This patchset first provides the minimal
fix then cleans up the inheritance of types based on functionality.

There is also a rename to TYPE_PXB*_DEV to allow removal of some boilerplate.

Before this series
TYPE_PXB_DEVICE, TYPE_PXB_PCIE_DEVICE and TYPE_PXB_CXL_DEVICE all
had TYPE_PCI_DEVICE as their direct parent though they shared a common
struct PXBDev for their state.  As a result this state contained
some data that was irrelevant for some the types.

This series changes to
TYPE_PXB_CXL_DEV has a parent of TYPE_PXB_PCIE_DEV
TYPE_PXB_PCIE_DEV has a parent of TYPE_PXB_DEV
TYPE_PXB_DEV continues to have a parent of TYPE_PCI_DEVICE.

Each of the TYPE_PXB*_DEV has a state structure adding those elements
to their parent that they need. This also allowed dropping a wrapping
structure for the CXL state as the PXBCXLDev structure already provides
the equivalent grouping.

Patches are similar to those posted in the thread but rebased on v8.0.0.

Jonathan Cameron (2):
  hw/pci-bridge: pci_expander_bridge fix type in pxb_cxl_dev_reset()
  hw/pci-bridge: Make PCIe and CXL PXB Devices inherit from TYPE_PXB_DEV

 hw/acpi/cxl.c                       | 11 +++---
 hw/cxl/cxl-host.c                   |  4 +-
 hw/pci-bridge/pci_expander_bridge.c | 61 ++++++++++-------------------
 include/hw/cxl/cxl.h                |  4 +-
 include/hw/pci/pci_bridge.h         | 28 +++++++++----
 5 files changed, 50 insertions(+), 58 deletions(-)

-- 
2.37.2

[PATCH 1/2] hw/pci-bridge: pci_expander_bridge fix type in pxb_cxl_dev_reset()

[Jonathan Cameron via]

Reproduce issue with

configure --enable-qom-cast-debug ...

qemu-system-x86_64 -display none -machine q35,cxl=on -device pxb-cxl,bus=pcie.0

  hw/pci-bridge/pci_expander_bridge.c:54:PXB_DEV: Object 0x5570e0b1ada0 is not an instance of type pxb
  Aborted

The type conversion results in the right state structure, but PXB_DEV is
not a parent of PXB_CXL_DEV hence the error. Rather than directly
cleaning up the inheritance, this is the minimal fix which will be
followed by the cleanup.

Fixes: 154070eaf6 ("hw/pxb-cxl: Support passthrough HDM Decoders unless overridden")
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/pci-bridge/pci_expander_bridge.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index ead33f0c05..a78327b5f2 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -311,7 +311,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 */
-- 
2.37.2

[PATCH 2/2] hw/pci-bridge: Make PCIe and CXL PXB Devices inherit from TYPE_PXB_DEV

[Jonathan Cameron via]

Previously, PXB_CXL_DEVICE, PXB_PCIE_DEVICE and PXB_DEVICE all
have PCI_DEVICE as their direct parent but share a common state
struct PXBDev. convert_to_pxb() is used to get the PXBDev
instance from which ever of these types it is called on.

This patch switches to an explicit heirarchy based on shared
functionality.  To allow use of OBJECT_DECLARE_SIMPLE_TYPE()
whilst minimizing code changes, all types are renamed to have
the postfix _DEV rather than _DEVICE.  The new heirarchy
has PXB_CXL_DEV with parent PXB_PCIE_DEV which in turn
has parent PXB_DEV which continues to have parent PCI_DEVICE.

This allows simple use of PXB_DEV() etc rather than a custom function
+ removal of duplicated properties and moving the CXL specific
elements out of struct PXBDev.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/acpi/cxl.c                       | 11 +++---
 hw/cxl/cxl-host.c                   |  4 +-
 hw/pci-bridge/pci_expander_bridge.c | 59 ++++++++++-------------------
 include/hw/cxl/cxl.h                |  4 +-
 include/hw/pci/pci_bridge.h         | 28 ++++++++++----
 5 files changed, 49 insertions(+), 57 deletions(-)

diff --git a/hw/acpi/cxl.c b/hw/acpi/cxl.c
index 2bf8c07993..92b46bc932 100644
--- a/hw/acpi/cxl.c
+++ b/hw/acpi/cxl.c
@@ -30,9 +30,10 @@
 #include "qapi/error.h"
 #include "qemu/uuid.h"
 
-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 */
@@ -45,7 +46,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);
@@ -112,7 +113,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);
         }
     }
 }
@@ -121,7 +122,7 @@ static int cxl_foreach_pxb_hb(Object *obj, void *opaque)
 {
     Aml *cedt = opaque;
 
-    if (object_dynamic_cast(obj, TYPE_PXB_CXL_DEVICE)) {
+    if (object_dynamic_cast(obj, TYPE_PXB_CXL_DEV)) {
         cedt_build_chbs(cedt->buf, PXB_CXL_DEV(obj));
     }
 
diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
index 6e923ceeaf..034c7805b3 100644
--- a/hw/cxl/cxl-host.c
+++ b/hw/cxl/cxl-host.c
@@ -84,7 +84,7 @@ void cxl_fmws_link_targets(CXLState *cxl_state, Error **errp)
                 bool ambig;
 
                 o = object_resolve_path_type(fw->targets[i],
-                                             TYPE_PXB_CXL_DEVICE,
+                                             TYPE_PXB_CXL_DEV,
                                              &ambig);
                 if (!o) {
                     error_setg(errp, "Could not resolve CXLFM target %s",
@@ -141,7 +141,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 a78327b5f2..613857b601 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -50,24 +50,8 @@ 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);
-}
+#define TYPE_PXB_PCIE_DEV "pxb-pcie"
+OBJECT_DECLARE_SIMPLE_TYPE(PXBPCIEDev, PXB_PCIE_DEV)
 
 static GList *pxb_dev_list;
 
@@ -89,14 +73,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 +138,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 +196,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 +283,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;
@@ -337,7 +321,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 +349,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 +402,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);
 }
@@ -449,7 +433,7 @@ static void pxb_dev_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo pxb_dev_info = {
-    .name          = TYPE_PXB_DEVICE,
+    .name          = TYPE_PXB_DEV,
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PXBDev),
     .class_init    = pxb_dev_class_init,
@@ -481,15 +465,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),
+    .name          = TYPE_PXB_PCIE_DEV,
+    .parent        = TYPE_PXB_DEV,
+    .instance_size = sizeof(PXBPCIEDev),
     .class_init    = pxb_pcie_dev_class_init,
     .interfaces = (InterfaceInfo[]) {
         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
@@ -510,11 +493,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(),
 };
 
@@ -540,9 +519,9 @@ 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),
+    .name          = TYPE_PXB_CXL_DEV,
+    .parent        = TYPE_PXB_PCIE_DEV,
+    .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 b2cffbb364..c453983e83 100644
--- a/include/hw/cxl/cxl.h
+++ b/include/hw/cxl/cxl.h
@@ -23,12 +23,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 1677176b2a..01670e9e65 100644
--- a/include/hw/pci/pci_bridge.h
+++ b/include/hw/pci/pci_bridge.h
@@ -84,7 +84,7 @@ struct PCIBridge {
 #define PCI_BRIDGE_DEV_PROP_SHPC       "shpc"
 typedef struct CXLHost CXLHost;
 
-struct PXBDev {
+typedef struct PXBDev {
     /*< private >*/
     PCIDevice parent_obj;
     /*< public >*/
@@ -92,15 +92,27 @@ 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_DEV "pxb"
+OBJECT_DECLARE_SIMPLE_TYPE(PXBDev, PXB_DEV)
+
+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,
-                         TYPE_PXB_CXL_DEVICE)
+#define TYPE_PXB_CXL_DEV "pxb-cxl"
+OBJECT_DECLARE_SIMPLE_TYPE(PXBCXLDev, PXB_CXL_DEV)
 
 int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
                           uint16_t svid, uint16_t ssid,
-- 
2.37.2

Re: [PATCH 0/2] hw/pci-bridge: pci_expander_bridge: Fix wrong type and rework inheritance.

[Michael S. Tsirkin]

On Thu, Apr 20, 2023 at 03:27:48PM +0100, Jonathan Cameron wrote:
> Peter Maydell highlighted an incorrect conversion to TYPE_PXB_DEVICE from
> a device that didn't have that a an ancestor type. PXB_DEV() used instead of
> PXB_CXL_DEV()/
> 
> https://lore.kernel.org/qemu-devel/CAFEAcA-+de+eeLCE4YsAw1O-Qyd_4W1Ra05mGDsU_-3a6d92qw@mail.gmail.com/
> 
> During the discussion it became clear that the inheritance of the various
> TYPE_PXB*_DEVICE was unusual. This patchset first provides the minimal
> fix then cleans up the inheritance of types based on functionality.
> 
> There is also a rename to TYPE_PXB*_DEV to allow removal of some boilerplate.
> 
> Before this series
> TYPE_PXB_DEVICE, TYPE_PXB_PCIE_DEVICE and TYPE_PXB_CXL_DEVICE all
> had TYPE_PCI_DEVICE as their direct parent though they shared a common
> struct PXBDev for their state.  As a result this state contained
> some data that was irrelevant for some the types.
> 
> This series changes to
> TYPE_PXB_CXL_DEV has a parent of TYPE_PXB_PCIE_DEV
> TYPE_PXB_PCIE_DEV has a parent of TYPE_PXB_DEV
> TYPE_PXB_DEV continues to have a parent of TYPE_PCI_DEVICE.
> 
> Each of the TYPE_PXB*_DEV has a state structure adding those elements
> to their parent that they need. This also allowed dropping a wrapping
> structure for the CXL state as the PXBCXLDev structure already provides
> the equivalent grouping.
> 
> Patches are similar to those posted in the thread but rebased on v8.0.0.

this conflicts with
    Revert "hw/pxb-cxl: Support passthrough HDM Decoders unless overridden"

I think you acked that one?

> Jonathan Cameron (2):
>   hw/pci-bridge: pci_expander_bridge fix type in pxb_cxl_dev_reset()
>   hw/pci-bridge: Make PCIe and CXL PXB Devices inherit from TYPE_PXB_DEV
> 
>  hw/acpi/cxl.c                       | 11 +++---
>  hw/cxl/cxl-host.c                   |  4 +-
>  hw/pci-bridge/pci_expander_bridge.c | 61 ++++++++++-------------------
>  include/hw/cxl/cxl.h                |  4 +-
>  include/hw/pci/pci_bridge.h         | 28 +++++++++----
>  5 files changed, 50 insertions(+), 58 deletions(-)
> 
> -- 
> 2.37.2

Re: [PATCH 0/2] hw/pci-bridge: pci_expander_bridge: Fix wrong type and rework inheritance.

[Peter Maydell]

On Fri, 21 Apr 2023 at 09:19, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Apr 20, 2023 at 03:27:48PM +0100, Jonathan Cameron wrote:
> > Peter Maydell highlighted an incorrect conversion to TYPE_PXB_DEVICE from
> > a device that didn't have that a an ancestor type. PXB_DEV() used instead of
> > PXB_CXL_DEV()/
> >
> > https://lore.kernel.org/qemu-devel/CAFEAcA-+de+eeLCE4YsAw1O-Qyd_4W1Ra05mGDsU_-3a6d92qw@mail.gmail.com/
> >
> > During the discussion it became clear that the inheritance of the various
> > TYPE_PXB*_DEVICE was unusual. This patchset first provides the minimal
> > fix then cleans up the inheritance of types based on functionality.
> >
> > There is also a rename to TYPE_PXB*_DEV to allow removal of some boilerplate.
> >
> > Before this series
> > TYPE_PXB_DEVICE, TYPE_PXB_PCIE_DEVICE and TYPE_PXB_CXL_DEVICE all
> > had TYPE_PCI_DEVICE as their direct parent though they shared a common
> > struct PXBDev for their state.  As a result this state contained
> > some data that was irrelevant for some the types.
> >
> > This series changes to
> > TYPE_PXB_CXL_DEV has a parent of TYPE_PXB_PCIE_DEV
> > TYPE_PXB_PCIE_DEV has a parent of TYPE_PXB_DEV
> > TYPE_PXB_DEV continues to have a parent of TYPE_PCI_DEVICE.
> >
> > Each of the TYPE_PXB*_DEV has a state structure adding those elements
> > to their parent that they need. This also allowed dropping a wrapping
> > structure for the CXL state as the PXBCXLDev structure already provides
> > the equivalent grouping.
> >
> > Patches are similar to those posted in the thread but rebased on v8.0.0.
>
> this conflicts with
>     Revert "hw/pxb-cxl: Support passthrough HDM Decoders unless overridden"
>
> I think you acked that one?

We should take one or the other, but not both. If this patchset
is good then it's probably better to fix the bug rather than
revert the feature, I think.

-- PMM

Re: [PATCH 0/2] hw/pci-bridge: pci_expander_bridge: Fix wrong type and rework inheritance.

[Jonathan Cameron via]

On Fri, 21 Apr 2023 09:59:57 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Fri, 21 Apr 2023 at 09:19, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Apr 20, 2023 at 03:27:48PM +0100, Jonathan Cameron wrote:  
> > > Peter Maydell highlighted an incorrect conversion to TYPE_PXB_DEVICE from
> > > a device that didn't have that a an ancestor type. PXB_DEV() used instead of
> > > PXB_CXL_DEV()/
> > >
> > > https://lore.kernel.org/qemu-devel/CAFEAcA-+de+eeLCE4YsAw1O-Qyd_4W1Ra05mGDsU_-3a6d92qw@mail.gmail.com/
> > >
> > > During the discussion it became clear that the inheritance of the various
> > > TYPE_PXB*_DEVICE was unusual. This patchset first provides the minimal
> > > fix then cleans up the inheritance of types based on functionality.
> > >
> > > There is also a rename to TYPE_PXB*_DEV to allow removal of some boilerplate.
> > >
> > > Before this series
> > > TYPE_PXB_DEVICE, TYPE_PXB_PCIE_DEVICE and TYPE_PXB_CXL_DEVICE all
> > > had TYPE_PCI_DEVICE as their direct parent though they shared a common
> > > struct PXBDev for their state.  As a result this state contained
> > > some data that was irrelevant for some the types.
> > >
> > > This series changes to
> > > TYPE_PXB_CXL_DEV has a parent of TYPE_PXB_PCIE_DEV
> > > TYPE_PXB_PCIE_DEV has a parent of TYPE_PXB_DEV
> > > TYPE_PXB_DEV continues to have a parent of TYPE_PCI_DEVICE.
> > >
> > > Each of the TYPE_PXB*_DEV has a state structure adding those elements
> > > to their parent that they need. This also allowed dropping a wrapping
> > > structure for the CXL state as the PXBCXLDev structure already provides
> > > the equivalent grouping.
> > >
> > > Patches are similar to those posted in the thread but rebased on v8.0.0.  
> >
> > this conflicts with
> >     Revert "hw/pxb-cxl: Support passthrough HDM Decoders unless overridden"
> >
> > I think you acked that one?  
> 
> We should take one or the other, but not both. If this patchset
> is good then it's probably better to fix the bug rather than
> revert the feature, I think.

If it's easy to drop the revert that would be my preference.

If not, then I'm fine spinning a new version of that patch without
the bug (so with patch 1 of this squashed in).  Patch 2 is somewhat related
refactoring.  Not necessary to fix the issue even though it was motivated
by that bug.

Jonathan

> 
> -- PMM

Re: [PATCH 0/2] hw/pci-bridge: pci_expander_bridge: Fix wrong type and rework inheritance.

[Thomas Huth]

On 21/04/2023 14.19, Jonathan Cameron via wrote:
> On Fri, 21 Apr 2023 09:59:57 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
> 
>> On Fri, 21 Apr 2023 at 09:19, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>
>>> On Thu, Apr 20, 2023 at 03:27:48PM +0100, Jonathan Cameron wrote:
>>>> Peter Maydell highlighted an incorrect conversion to TYPE_PXB_DEVICE from
>>>> a device that didn't have that a an ancestor type. PXB_DEV() used instead of
>>>> PXB_CXL_DEV()/
>>>>
>>>> https://lore.kernel.org/qemu-devel/CAFEAcA-+de+eeLCE4YsAw1O-Qyd_4W1Ra05mGDsU_-3a6d92qw@mail.gmail.com/
>>>>
>>>> During the discussion it became clear that the inheritance of the various
>>>> TYPE_PXB*_DEVICE was unusual. This patchset first provides the minimal
>>>> fix then cleans up the inheritance of types based on functionality.
>>>>
>>>> There is also a rename to TYPE_PXB*_DEV to allow removal of some boilerplate.
>>>>
>>>> Before this series
>>>> TYPE_PXB_DEVICE, TYPE_PXB_PCIE_DEVICE and TYPE_PXB_CXL_DEVICE all
>>>> had TYPE_PCI_DEVICE as their direct parent though they shared a common
>>>> struct PXBDev for their state.  As a result this state contained
>>>> some data that was irrelevant for some the types.
>>>>
>>>> This series changes to
>>>> TYPE_PXB_CXL_DEV has a parent of TYPE_PXB_PCIE_DEV
>>>> TYPE_PXB_PCIE_DEV has a parent of TYPE_PXB_DEV
>>>> TYPE_PXB_DEV continues to have a parent of TYPE_PCI_DEVICE.
>>>>
>>>> Each of the TYPE_PXB*_DEV has a state structure adding those elements
>>>> to their parent that they need. This also allowed dropping a wrapping
>>>> structure for the CXL state as the PXBCXLDev structure already provides
>>>> the equivalent grouping.
>>>>
>>>> Patches are similar to those posted in the thread but rebased on v8.0.0.
>>>
>>> this conflicts with
>>>      Revert "hw/pxb-cxl: Support passthrough HDM Decoders unless overridden"
>>>
>>> I think you acked that one?
>>
>> We should take one or the other, but not both. If this patchset
>> is good then it's probably better to fix the bug rather than
>> revert the feature, I think.
> 
> If it's easy to drop the revert that would be my preference.

Yes, the patch has not been picked up yet, so we can simply forget about the 
revert.

  Thomas

Re: [PATCH 1/2] hw/pci-bridge: pci_expander_bridge fix type in pxb_cxl_dev_reset()

[Thomas Huth]

On 20/04/2023 16.27, Jonathan Cameron via wrote:
> Reproduce issue with
> 
> configure --enable-qom-cast-debug ...
> 
> qemu-system-x86_64 -display none -machine q35,cxl=on -device pxb-cxl,bus=pcie.0
> 
>    hw/pci-bridge/pci_expander_bridge.c:54:PXB_DEV: Object 0x5570e0b1ada0 is not an instance of type pxb
>    Aborted
> 
> The type conversion results in the right state structure, but PXB_DEV is
> not a parent of PXB_CXL_DEV hence the error. Rather than directly
> cleaning up the inheritance, this is the minimal fix which will be
> followed by the cleanup.
> 
> Fixes: 154070eaf6 ("hw/pxb-cxl: Support passthrough HDM Decoders unless overridden")
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>   hw/pci-bridge/pci_expander_bridge.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> index ead33f0c05..a78327b5f2 100644
> --- a/hw/pci-bridge/pci_expander_bridge.c
> +++ b/hw/pci-bridge/pci_expander_bridge.c
> @@ -311,7 +311,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 */

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1586

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 1/2] hw/pci-bridge: pci_expander_bridge fix type in pxb_cxl_dev_reset()

[Peter Maydell]

On Thu, 20 Apr 2023 at 15:28, Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> Reproduce issue with
>
> configure --enable-qom-cast-debug ...
>
> qemu-system-x86_64 -display none -machine q35,cxl=on -device pxb-cxl,bus=pcie.0
>
>   hw/pci-bridge/pci_expander_bridge.c:54:PXB_DEV: Object 0x5570e0b1ada0 is not an instance of type pxb
>   Aborted
>
> The type conversion results in the right state structure, but PXB_DEV is
> not a parent of PXB_CXL_DEV hence the error. Rather than directly
> cleaning up the inheritance, this is the minimal fix which will be
> followed by the cleanup.
>
> Fixes: 154070eaf6 ("hw/pxb-cxl: Support passthrough HDM Decoders unless overridden")
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> --

We should add:
Cc: qemu-stable@nongnu.org

so downstreams don't have to disable the QOM cast asserts.

-- PMM

Re: [PATCH 2/2] hw/pci-bridge: Make PCIe and CXL PXB Devices inherit from TYPE_PXB_DEV

[Thomas Huth]

On 20/04/2023 16.27, Jonathan Cameron via wrote:
> Previously, PXB_CXL_DEVICE, PXB_PCIE_DEVICE and PXB_DEVICE all
> have PCI_DEVICE as their direct parent but share a common state
> struct PXBDev. convert_to_pxb() is used to get the PXBDev
> instance from which ever of these types it is called on.
> 
> This patch switches to an explicit heirarchy based on shared
> functionality.  To allow use of OBJECT_DECLARE_SIMPLE_TYPE()
> whilst minimizing code changes, all types are renamed to have
> the postfix _DEV rather than _DEVICE.  The new heirarchy

s/heirarchy/hierarchy/

> has PXB_CXL_DEV with parent PXB_PCIE_DEV which in turn
> has parent PXB_DEV which continues to have parent PCI_DEVICE.
> 
> This allows simple use of PXB_DEV() etc rather than a custom function
> + removal of duplicated properties and moving the CXL specific
> elements out of struct PXBDev.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>   hw/acpi/cxl.c                       | 11 +++---
>   hw/cxl/cxl-host.c                   |  4 +-
>   hw/pci-bridge/pci_expander_bridge.c | 59 ++++++++++-------------------
>   include/hw/cxl/cxl.h                |  4 +-
>   include/hw/pci/pci_bridge.h         | 28 ++++++++++----
>   5 files changed, 49 insertions(+), 57 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 0/2] hw/pci-bridge: pci_expander_bridge: Fix wrong type and rework inheritance.

[Michael S. Tsirkin]

On Fri, Apr 21, 2023 at 01:19:08PM +0100, Jonathan Cameron wrote:
> On Fri, 21 Apr 2023 09:59:57 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> > On Fri, 21 Apr 2023 at 09:19, Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Thu, Apr 20, 2023 at 03:27:48PM +0100, Jonathan Cameron wrote:  
> > > > Peter Maydell highlighted an incorrect conversion to TYPE_PXB_DEVICE from
> > > > a device that didn't have that a an ancestor type. PXB_DEV() used instead of
> > > > PXB_CXL_DEV()/
> > > >
> > > > https://lore.kernel.org/qemu-devel/CAFEAcA-+de+eeLCE4YsAw1O-Qyd_4W1Ra05mGDsU_-3a6d92qw@mail.gmail.com/
> > > >
> > > > During the discussion it became clear that the inheritance of the various
> > > > TYPE_PXB*_DEVICE was unusual. This patchset first provides the minimal
> > > > fix then cleans up the inheritance of types based on functionality.
> > > >
> > > > There is also a rename to TYPE_PXB*_DEV to allow removal of some boilerplate.
> > > >
> > > > Before this series
> > > > TYPE_PXB_DEVICE, TYPE_PXB_PCIE_DEVICE and TYPE_PXB_CXL_DEVICE all
> > > > had TYPE_PCI_DEVICE as their direct parent though they shared a common
> > > > struct PXBDev for their state.  As a result this state contained
> > > > some data that was irrelevant for some the types.
> > > >
> > > > This series changes to
> > > > TYPE_PXB_CXL_DEV has a parent of TYPE_PXB_PCIE_DEV
> > > > TYPE_PXB_PCIE_DEV has a parent of TYPE_PXB_DEV
> > > > TYPE_PXB_DEV continues to have a parent of TYPE_PCI_DEVICE.
> > > >
> > > > Each of the TYPE_PXB*_DEV has a state structure adding those elements
> > > > to their parent that they need. This also allowed dropping a wrapping
> > > > structure for the CXL state as the PXBCXLDev structure already provides
> > > > the equivalent grouping.
> > > >
> > > > Patches are similar to those posted in the thread but rebased on v8.0.0.  
> > >
> > > this conflicts with
> > >     Revert "hw/pxb-cxl: Support passthrough HDM Decoders unless overridden"
> > >
> > > I think you acked that one?  
> > 
> > We should take one or the other, but not both. If this patchset
> > is good then it's probably better to fix the bug rather than
> > revert the feature, I think.
> 
> If it's easy to drop the revert that would be my preference.

OK, I'll do that. Thanks!

> If not, then I'm fine spinning a new version of that patch without
> the bug (so with patch 1 of this squashed in).  Patch 2 is somewhat related
> refactoring.  Not necessary to fix the issue even though it was motivated
> by that bug.
> 
> Jonathan
> 
> > 
> > -- PMM