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

[Peter Maydell]

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.

> -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