Re: [RFC PATCH 1/2] hw/arm/sbsa-ref: Enable CXL Host Bridge by pxb-cxl

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Fri, 30 Aug 2024 12:15:56 +0800
Yuquan Wang <wangyuquan1236@phytium.com.cn> wrote:

> The memory layout places 1M space for 16 host bridge register regions
> in the sbsa-ref memmap. In addition, this creates a default pxb-cxl
> (bus_nr=0xfe) bridge with one cxl-rp on sbsa-ref.

If you are only supporting 1 host bridge you could reduce the space to just
fit that?

From the command line example and code here it seems the pxb instances are hard
coded so you don't need to support the flexibility I needed for virt.

Otherwise, just superficial code comments inline.
Fixed setups are definitely easier to support :)

Jonathan


> 
> Signed-off-by: Yuquan Wang <wangyuquan1236@phytium.com.cn>
> ---
>  hw/arm/sbsa-ref.c | 54 ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index ae37a92301..dc924d181e 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -41,7 +41,10 @@
>  #include "hw/intc/arm_gicv3_common.h"
>  #include "hw/intc/arm_gicv3_its_common.h"
>  #include "hw/loader.h"
> +#include "hw/pci/pci_bridge.h"
> +#include "hw/pci/pci_bus.h"
>  #include "hw/pci-host/gpex.h"
> +#include "hw/pci-bridge/pci_expander_bridge.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/usb.h"
>  #include "hw/usb/xhci.h"
> @@ -52,6 +55,8 @@
>  #include "qom/object.h"
>  #include "target/arm/cpu-qom.h"
>  #include "target/arm/gtimer.h"
> +#include "hw/cxl/cxl.h"
> +#include "hw/cxl/cxl_host.h"

Headers look to be alphabetical order.

>  
>  #define RAMLIMIT_GB 8192
>  #define RAMLIMIT_BYTES (RAMLIMIT_GB * GiB)
> @@ -94,6 +99,7 @@ enum {
>      SBSA_SECURE_MEM,
>      SBSA_AHCI,
>      SBSA_XHCI,
> +    SBSA_CXL_HOST,
>  };
>  
>  struct SBSAMachineState {
> @@ -105,6 +111,9 @@ struct SBSAMachineState {
>      int psci_conduit;
>      DeviceState *gic;
>      PFlashCFI01 *flash[2];
> +    CXLState cxl_devices_state;
> +    PCIBus *bus;

Lot of buses in a system, I'd call the pcibus
or similar. However, see below - I don't think
it is necessary.


> +    PCIBus *cxlbus;
>  };
>  
>  #define TYPE_SBSA_MACHINE   MACHINE_TYPE_NAME("sbsa-ref")
> @@ -132,6 +141,8 @@ static const MemMapEntry sbsa_ref_memmap[] = {
>      /* Space here reserved for more SMMUs */
>      [SBSA_AHCI] =               { 0x60100000, 0x00010000 },
>      [SBSA_XHCI] =               { 0x60110000, 0x00010000 },
> +    /* 1M CXL Host Bridge Registers space (64KiB * 16) */
> +    [SBSA_CXL_HOST] =           { 0x60120000, 0x00100000 },

As below, can just leave space for however many you are creating.

>      /* Space here reserved for other devices */
>      [SBSA_PCIE_PIO] =           { 0x7fff0000, 0x00010000 },
>      /* 32-bit address PCIE MMIO space */
> @@ -631,6 +642,26 @@ static void create_smmu(const SBSAMachineState *sms, PCIBus *bus)
>      }
>  }
>  
> +static void create_pxb_cxl(SBSAMachineState *sms)
> +{
> +    CXLHost *host;
> +    PCIHostState *cxl;
> +
> +    sms->cxl_devices_state.is_enabled = true;
> +
> +    DeviceState *qdev;

I think qemu still sticks mostly to declarations at the top
of functions. So move this up.

> +    qdev = qdev_new(TYPE_PXB_CXL_DEV);
> +    qdev_prop_set_uint32(qdev, "bus_nr", 0xfe);

Ouch. That's not a lot of space in bus numbers.
Move it down a bit so there is room for switches etc
and not just a single root port.
Up to SBSA ref maintainers, but I'd use 0xc0 or something
like that.

> +
> +    PCIDevice *dev = PCI_DEVICE(qdev);

Declarations at the top I think.

> +    pci_realize_and_unref(dev, sms->bus, &error_fatal);
> +
> +    host = PXB_CXL_DEV(dev)->cxl_host_bridge;
> +    cxl = PCI_HOST_BRIDGE(host);
> +    sms->cxlbus = cxl->bus;
> +    pci_create_simple(sms->cxlbus, -1, "cxl-rp");

You want to enable at least some interleaving the HB so I'd
add at least 2 root ports.

> +}
> +
>  static void create_pcie(SBSAMachineState *sms)
>  {
>      hwaddr base_ecam = sbsa_ref_memmap[SBSA_PCIE_ECAM].base;
> @@ -682,12 +713,25 @@ static void create_pcie(SBSAMachineState *sms)
>      }
>  
>      pci = PCI_HOST_BRIDGE(dev);
> +    sms->bus = pci->bus;

I'd carry on using pci->bus for this code to minimize changes
needed and set sms->bus at the end of the function, not the
start (see below - you can get rid of need to store pci->bus
I think).

> +
> +    pci_init_nic_devices(sms->bus, mc->default_nic);
>  
> -    pci_init_nic_devices(pci->bus, mc->default_nic);
> +    pci_create_simple(sms->bus, -1, "bochs-display");
>  
> -    pci_create_simple(pci->bus, -1, "bochs-display");
> +    create_smmu(sms, sms->bus);
>  
> -    create_smmu(sms, pci->bus);
> +    create_pxb_cxl(sms);

Keep this similar to the others and pass in pci->bus even
though you are going to stash pci->bus

> +}


>  
>  static void *sbsa_ref_dtb(const struct arm_boot_info *binfo, int *fdt_size)
> @@ -823,6 +867,10 @@ static void sbsa_ref_init(MachineState *machine)
>  
>      create_pcie(sms);
>  
> +    create_cxl_host_reg_region(sms);
> +
> +    cxl_hook_up_pxb_registers(sms->bus, &sms->cxl_devices_state, &error_fatal);
> +

Fixed pxb instances certainly make this less of a dance than we need for pc / virt
as the creation order is constrained in a way it isn't for those generic machines.

Given you know you only have one PXB-cxl and have it in sms->cxlbus
you could just call
pxb_cxl_hook_up_registers(&sms->cxl_devices_state, pci->cxlbus, &error_fatal)
I think and get same result without needed to add sms->bus to get hold of the
pci bus.


>      create_secure_ec(secure_sysmem);
>  
>      sms->bootinfo.ram_size = machine->ram_size;