Re: [RFC PATCH 11/25] hw/pxb: Allow creation of a CXL PXB (host bridge)

[Jonathan Cameron]

On Tue, 10 Nov 2020 21:47:10 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> This works like adding a typical pxb device, except the name is
> 'pxb-cxl' instead of 'pxb-pcie'. An example command line would be as
> follows:
>   -device pxb-cxl,id=cxl.0,bus="pcie.0",bus_nr=1
> 
> A CXL PXB is backward compatible with PCIe. What this means in practice
> is that an operating system that is unaware of CXL should still be able
> to enumerate this topology as if it were PCIe.
> 
> One can create multiple CXL PXB host bridges, but a host bridge can only
> be connected to the main root bus. Host bridges cannot appear elsewhere
> in the topology.
> 
> Note that as of this patch, the ACPI tables needed for the host bridge
> (specifically, an ACPI object in _SB named ACPI0016 and the CEDT) aren't
> created. So while this patch internally creates it, it cannot be
> properly used by an operating system or other system software.
> 
> Upcoming patches will allow creating multiple host bridges.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
Hi Ben,

Few minor things inline.

Jonathan

> ---
>  hw/pci-bridge/pci_expander_bridge.c | 67 ++++++++++++++++++++++++++++-
>  hw/pci/pci.c                        |  7 +++
>  include/hw/pci/pci.h                |  6 +++
>  3 files changed, 78 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-bridge/pci_expander_bridge.c 
> b/hw/pci-bridge/pci_expander_bridge.c
> index 88c45dc3b5..3a8d815231 100644
> --- a/hw/pci-bridge/pci_expander_bridge.c
> +++ b/hw/pci-bridge/pci_expander_bridge.c
> @@ -56,6 +56,10 @@ DECLARE_INSTANCE_CHECKER(PXBDev, PXB_DEV,
>  DECLARE_INSTANCE_CHECKER(PXBDev, PXB_PCIE_DEV,
>                           TYPE_PXB_PCIE_DEVICE)
>  
> +#define TYPE_PXB_CXL_DEVICE "pxb-cxl"
> +DECLARE_INSTANCE_CHECKER(PXBDev, PXB_CXL_DEV,
> +                         TYPE_PXB_CXL_DEVICE)
> +
>  struct PXBDev {
>      /*< private >*/
>      PCIDevice parent_obj;
> @@ -67,6 +71,11 @@ struct PXBDev {
>  
>  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);
>  }
> @@ -111,11 +120,20 @@ static const TypeInfo pxb_pcie_bus_info = {
>      .class_init    = pxb_bus_class_init,
>  };
>  
> +static const TypeInfo pxb_cxl_bus_info = {
> +    .name          = TYPE_PXB_CXL_BUS,
> +    .parent        = TYPE_CXL_BUS,
> +    .instance_size = sizeof(PXBBus),
> +    .class_init    = pxb_bus_class_init,
> +};
> +
>  static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,
>                                            PCIBus *rootbus)
>  {
> -    PXBBus *bus = pci_bus_is_express(rootbus) ?
> -                  PXB_PCIE_BUS(rootbus) : PXB_BUS(rootbus);
> +    PXBBus *bus = pci_bus_is_cxl(rootbus) ?
> +                      PXB_CXL_BUS(rootbus) :
> +                      pci_bus_is_express(rootbus) ? PXB_PCIE_BUS(rootbus) :
> +                                                    PXB_BUS(rootbus);

There comes a point where if / else is much more readable.

>  
>      snprintf(bus->bus_path, 8, "0000:%02x", pxb_bus_num(rootbus));
>      return bus->bus_path;
> @@ -380,13 +398,58 @@ static const TypeInfo pxb_pcie_dev_info = {
>      },
>  };
>  
> +static void pxb_cxl_dev_realize(PCIDevice *dev, Error **errp)
> +{
> +    /* A CXL PXB's parent bus is still PCIe */
> +    if (!pci_bus_is_express(pci_get_bus(dev))) {
> +        error_setg(errp, "pxb-cxl devices cannot reside on a PCI bus");
> +        return;
> +    }
> +
> +    pxb_dev_realize_common(dev, CXL, errp);
> +}
> +
> +static void pxb_cxl_dev_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc   = DEVICE_CLASS(klass);
> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> +    k->realize             = pxb_cxl_dev_realize;
> +    k->exit                = pxb_dev_exitfn;
> +    k->vendor_id           = PCI_VENDOR_ID_INTEL;
> +    k->device_id           = 0xabcd;

Just to check, is that an officially assigned device_id that we will never
have a clash with?  Nice ID to get if it is :)


> +    k->class_id            = PCI_CLASS_BRIDGE_HOST;
> +    k->subsystem_vendor_id = PCI_VENDOR_ID_INTEL;
> +
> +    dc->desc = "CXL Host Bridge";
> +    device_class_set_props(dc, pxb_dev_properties);
> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> +
> +    /* Host bridges aren't hotpluggable. FIXME: spec reference */
> +    dc->hotpluggable = false;
> +}
> +
> +static const TypeInfo pxb_cxl_dev_info = {
> +    .name          = TYPE_PXB_CXL_DEVICE,
> +    .parent        = TYPE_PCI_DEVICE,
> +    .instance_size = sizeof(PXBDev),
> +    .class_init    = pxb_cxl_dev_class_init,
> +    .interfaces =
> +        (InterfaceInfo[]){
> +            { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> +            {},
> +        },
> +};
> +
>  static void pxb_register_types(void)
>  {
>      type_register_static(&pxb_bus_info);
>      type_register_static(&pxb_pcie_bus_info);
> +    type_register_static(&pxb_cxl_bus_info);
>      type_register_static(&pxb_host_info);
>      type_register_static(&pxb_dev_info);
>      type_register_static(&pxb_pcie_dev_info);
> +    type_register_static(&pxb_cxl_dev_info);
>  }
>  
>  type_init(pxb_register_types)
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index db88788c4b..67eed889a4 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -220,6 +220,12 @@ static const TypeInfo pcie_bus_info = {
>      .class_init = pcie_bus_class_init,
>  };
>  
> +static const TypeInfo cxl_bus_info = {
> +    .name       = TYPE_CXL_BUS,
> +    .parent     = TYPE_PCIE_BUS,
> +    .class_init = pcie_bus_class_init,
> +};
> +
>  static PCIBus *pci_find_bus_nr(PCIBus *bus, int bus_num);
>  static void pci_update_mappings(PCIDevice *d);
>  static void pci_irq_handler(void *opaque, int irq_num, int level);
> @@ -2847,6 +2853,7 @@ static void pci_register_types(void)
>  {
>      type_register_static(&pci_bus_info);
>      type_register_static(&pcie_bus_info);
> +    type_register_static(&cxl_bus_info);
>      type_register_static(&conventional_pci_interface_info);
>      type_register_static(&cxl_interface_info);
>      type_register_static(&pcie_interface_info);
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 4e6fd59fdd..52267ff69e 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -405,6 +405,7 @@ typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, 
> int pin);
>  #define TYPE_PCI_BUS "PCI"
>  OBJECT_DECLARE_TYPE(PCIBus, PCIBusClass, PCI_BUS)
>  #define TYPE_PCIE_BUS "PCIE"
> +#define TYPE_CXL_BUS "CXL"
>  
>  bool pci_bus_is_express(PCIBus *bus);
>  
> @@ -753,6 +754,11 @@ static inline void pci_irq_pulse(PCIDevice *pci_dev)
>      pci_irq_deassert(pci_dev);
>  }
>  
> +static inline int pci_is_cxl(const PCIDevice *d)
> +{
> +    return d->cap_present & QEMU_PCIE_CAP_CXL;
> +}
> +
>  static inline int pci_is_express(const PCIDevice *d)
>  {
>      return d->cap_present & QEMU_PCI_CAP_EXPRESS;