Re: [PULL 70/73] hw/pxb-cxl: Support passthrough HDM Decoders unless overridden

[Jonathan Cameron]

On Wed, 19 Apr 2023 17:25:17 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Wed, 19 Apr 2023 at 15:50, Jonathan Cameron
> <Jonathan.Cameron@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.maydell@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.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>  
> 
> 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