Re: [PATCH 2/2] hw/pxb-cxl: Support passthrough HDM Decoders unless overridden

[Fan Ni <fan.ni@samsung.com>]

On Wed, Jan 25, 2023 at 03:27:03PM +0000, Jonathan Cameron wrote:

> The CXL r3.0 specification allows for there to be no HDM decoders on CXL
> Host Bridges if they have only a single root port. Instead, all accesses
> directed to the host bridge (as specified in CXL Fixed Memory Windows)
> are assumed to be routed to the single root port.
> 
> Linux currently assumes this implementation choice. So to simplify testing,
> make QEMU emulation also default to no HDM decoders under these particular
> circumstances, but provide a hdm_for_passthrough boolean option to have
> HDM decoders as previously.
> 
> Technically this is breaking backwards compatibility, but given the only
> known software stack used with the QEMU emulation is the Linux kernel
> and this configuration did not work before this change, there are
> unlikely to be any complaints that it now works. The option is retained
> to allow testing of software that does allow for these HDM decoders to exist,
> once someone writes it.
> 
> Reported-by: Fan Ni <fan.ni@samsung.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  hw/cxl/cxl-host.c                   | 31 ++++++++++++--------
>  hw/pci-bridge/pci_expander_bridge.c | 44 +++++++++++++++++++++++++----
>  include/hw/cxl/cxl.h                |  1 +
>  include/hw/cxl/cxl_component.h      |  1 +
>  include/hw/pci/pci_bridge.h         |  1 +
>  5 files changed, 61 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
> index 3c1ec8732a..6e923ceeaf 100644
> --- a/hw/cxl/cxl-host.c
> +++ b/hw/cxl/cxl-host.c
> @@ -146,21 +146,28 @@ static PCIDevice *cxl_cfmws_find_device(CXLFixedWindow *fw, hwaddr addr)
>          return NULL;
>      }
>  
> -    hb_cstate = cxl_get_hb_cstate(hb);
> -    if (!hb_cstate) {
> -        return NULL;
> -    }
> +    if (cxl_get_hb_passthrough(hb)) {
> +        rp = pcie_find_port_first(hb->bus);
> +        if (!rp) {
> +            return NULL;
> +        }
> +    } else {
> +        hb_cstate = cxl_get_hb_cstate(hb);
> +        if (!hb_cstate) {
> +            return NULL;
> +        }
>  
> -    cache_mem = hb_cstate->crb.cache_mem_registers;
> +        cache_mem = hb_cstate->crb.cache_mem_registers;
>  
> -    target_found = cxl_hdm_find_target(cache_mem, addr, &target);
> -    if (!target_found) {
> -        return NULL;
> -    }
> +        target_found = cxl_hdm_find_target(cache_mem, addr, &target);
> +        if (!target_found) {
> +            return NULL;
> +        }
>  
> -    rp = pcie_find_port_by_pn(hb->bus, target);
> -    if (!rp) {
> -        return NULL;
> +        rp = pcie_find_port_by_pn(hb->bus, target);
> +        if (!rp) {
> +            return NULL;
> +        }
>      }
>  
>      d = pci_bridge_get_sec_bus(PCI_BRIDGE(rp))->devices[0];
> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> index e752a21292..ead33f0c05 100644
> --- a/hw/pci-bridge/pci_expander_bridge.c
> +++ b/hw/pci-bridge/pci_expander_bridge.c
> @@ -15,6 +15,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/pci/pci_bus.h"
>  #include "hw/pci/pci_host.h"
> +#include "hw/pci/pcie_port.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/pci/pci_bridge.h"
>  #include "hw/pci-bridge/pci_expander_bridge.h"
> @@ -79,6 +80,13 @@ CXLComponentState *cxl_get_hb_cstate(PCIHostState *hb)
>      return &host->cxl_cstate;
>  }
>  
> +bool cxl_get_hb_passthrough(PCIHostState *hb)
> +{
> +    CXLHost *host = PXB_CXL_HOST(hb);
> +
> +    return host->passthrough;
> +}
> +
>  static int pxb_bus_num(PCIBus *bus)
>  {
>      PXBDev *pxb = convert_to_pxb(bus->parent_dev);
> @@ -289,15 +297,32 @@ static int pxb_map_irq_fn(PCIDevice *pci_dev, int pin)
>      return pin - PCI_SLOT(pxb->devfn);
>  }
>  
> -static void pxb_dev_reset(DeviceState *dev)
> +static void pxb_cxl_dev_reset(DeviceState *dev)
>  {
>      CXLHost *cxl = PXB_CXL_DEV(dev)->cxl.cxl_host_bridge;
>      CXLComponentState *cxl_cstate = &cxl->cxl_cstate;
> +    PCIHostState *hb = PCI_HOST_BRIDGE(cxl);
>      uint32_t *reg_state = cxl_cstate->crb.cache_mem_registers;
>      uint32_t *write_msk = cxl_cstate->crb.cache_mem_regs_write_mask;
> +    int dsp_count = 0;
>  
>      cxl_component_register_init_common(reg_state, write_msk, CXL2_ROOT_PORT);
> -    ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, TARGET_COUNT, 8);
> +    /*
> +     * The CXL specification allows for host bridges with no HDM decoders
> +     * if they only have a single root port.
> +     */
> +    if (!PXB_DEV(dev)->hdm_for_passthrough) {
> +        dsp_count = pcie_count_ds_ports(hb->bus);
> +    }
> +    /* Initial reset will have 0 dsp so wait until > 0 */
> +    if (dsp_count == 1) {
> +        cxl->passthrough = true;
> +        /* Set Capability ID in header to NONE */
> +        ARRAY_FIELD_DP32(reg_state, CXL_HDM_CAPABILITY_HEADER, ID, 0);
> +    } else {
> +        ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, TARGET_COUNT,
> +                         8);
> +    }
>  }
>  
>  static gint pxb_compare(gconstpointer a, gconstpointer b)
> @@ -481,9 +506,18 @@ static void pxb_cxl_dev_realize(PCIDevice *dev, Error **errp)
>      }
>  
>      pxb_dev_realize_common(dev, CXL, errp);
> -    pxb_dev_reset(DEVICE(dev));
> +    pxb_cxl_dev_reset(DEVICE(dev));
>  }
>  
> +static Property pxb_cxl_dev_properties[] = {
> +    /* Note: 0 is not a legal PXB bus number. */
> +    DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0),
> +    DEFINE_PROP_UINT16("numa_node", PXBDev, numa_node, NUMA_NODE_UNASSIGNED),
> +    DEFINE_PROP_BOOL("bypass_iommu", PXBDev, bypass_iommu, false),
> +    DEFINE_PROP_BOOL("hdm_for_passthrough", PXBDev, hdm_for_passthrough, false),
when setting hdm_for_passthrough to true at the qemu command line, we
will see the segfault issue as before. I think this is expected as it
is the logic in cxl_cfmws_find_device. Wondering if there will be
following fixes to handle the case when hdm_for_passthrough is true.
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void pxb_cxl_dev_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc   = DEVICE_CLASS(klass);
> @@ -497,12 +531,12 @@ static void pxb_cxl_dev_class_init(ObjectClass *klass, void *data)
>       */
>  
>      dc->desc = "CXL Host Bridge";
> -    device_class_set_props(dc, pxb_dev_properties);
> +    device_class_set_props(dc, pxb_cxl_dev_properties);
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>  
>      /* Host bridges aren't hotpluggable. FIXME: spec reference */
>      dc->hotpluggable = false;
> -    dc->reset = pxb_dev_reset;
> +    dc->reset = pxb_cxl_dev_reset;
>  }
>  
>  static const TypeInfo pxb_cxl_dev_info = {
> diff --git a/include/hw/cxl/cxl.h b/include/hw/cxl/cxl.h
> index b161be59b7..b2cffbb364 100644
> --- a/include/hw/cxl/cxl.h
> +++ b/include/hw/cxl/cxl.h
> @@ -49,6 +49,7 @@ struct CXLHost {
>      PCIHostState parent_obj;
>  
>      CXLComponentState cxl_cstate;
> +    bool passthrough;
>  };
>  
>  #define TYPE_PXB_CXL_HOST "pxb-cxl-host"
> diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
> index 8752171f70..b4104b78b5 100644
> --- a/include/hw/cxl/cxl_component.h
> +++ b/include/hw/cxl/cxl_component.h
> @@ -249,6 +249,7 @@ static inline hwaddr cxl_decode_ig(int ig)
>  }
>  
>  CXLComponentState *cxl_get_hb_cstate(PCIHostState *hb);
> +bool cxl_get_hb_passthrough(PCIHostState *hb);
>  
>  void cxl_doe_cdat_init(CXLComponentState *cxl_cstate, Error **errp);
>  void cxl_doe_cdat_release(CXLComponentState *cxl_cstate);
> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
> index 63a7521567..81a058bb2c 100644
> --- a/include/hw/pci/pci_bridge.h
> +++ b/include/hw/pci/pci_bridge.h
> @@ -92,6 +92,7 @@ struct PXBDev {
>      uint8_t bus_nr;
>      uint16_t numa_node;
>      bool bypass_iommu;
> +    bool hdm_for_passthrough;
>      struct cxl_dev {
>          CXLHost *cxl_host_bridge; /* Pointer to a CXLHost */
>      } cxl;
> -- 
> 2.37.2
> 
>