On Wed, 19 Apr 2023 at 15:50, Jonathan Cameron
<jonathan.came...@huawei.com> wrote:
>
> 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.mayd...@linaro.org> wrote:
> > > 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.came...@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.came...@huawei.com>
You probably want to send this out as a proper top-level patch:
both humans and automated tooling tend to not notice patches
buried inside other list threads.
> -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);
> -}
This function looks super-dubious, so the fact that it
goes away in this patch is a good sign :-)
> 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)
The documentation for DECLARE_INSTANCE_CHECKER()
says "Direct usage of this macro should be avoided, and
the complete OBJECT_DECLARE_TYPE() macro is recommended
instead. So we should do that. (That will also mean you don't
need the explicit "typedef"s on your struct declarations,
because OBJECT_DECLARE_TYPE() will define typedefs for you.)
> +
> +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,
thanks
-- PMM
