On Wed, 19 Apr 2023 17:25:17 +0100
Peter Maydell <peter.mayd...@linaro.org> wrote:
> 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.
Absolutely. Was a case of lazily throwing it over the wall before running out
the door.
>
> > -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.)
I'll fix that up and send out properly. Thanks for the
quick feedback.
Jonathan
>
> > +
> > +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
