| Message ID | 20240830041557.600607-2-wangyuquan1236@phytium.com.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Sbsa-ref CXL Enablement | expand |
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;
On 30.08.2024 06:15, Yuquan Wang 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. With this patchset applied I no longer can add pcie devices to sbsa-ref. -device nvme,serial=deadbeef,bus=root_port_for_nvme1,drive=hdd -drive file=disks/full-debian.hddimg,format=raw,id=hdd,if=none Normally this adds NVME as pcie device but now it probably ends on pxb-cxl bus instead. Also please bump platform_version.minor and document adding CXL in docs/system/arm/sbsa.rst file.
Hi，Marcin I am updating this patches into v2 with Separate MMIO address space for CXL, however, I'm not confident about the addresss design on sbsa-ref. Below are some questions about that. 1) With the pxb-cxl-host, any cxl root ports and cxl endpoint devices would occupy the BDF number of the original pcie domain. Hence, the max available pcie devices on sbsa-ref would decrease, which seems to bring a series of trouble. Do you have any suggestions? 2) In the situation described above, is it necessary to add a separate ecam space for cxl host? -------------- Many thanks Yuquan Wang >On 30.08.2024 06:15, Yuquan Wang 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. > >With this patchset applied I no longer can add pcie devices to sbsa-ref. > >-device nvme,serial=deadbeef,bus=root_port_for_nvme1,drive=hdd >-drive file=disks/full-debian.hddimg,format=raw,id=hdd,if=none > >Normally this adds NVME as pcie device but now it probably ends on >pxb-cxl bus instead. > >Also please bump platform_version.minor and document adding CXL in >docs/system/arm/sbsa.rst file.
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" #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; + 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 }, /* 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; + qdev = qdev_new(TYPE_PXB_CXL_DEV); + qdev_prop_set_uint32(qdev, "bus_nr", 0xfe); + + PCIDevice *dev = PCI_DEVICE(qdev); + 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"); +} + 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; + + 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); +} + +static void create_cxl_host_reg_region(SBSAMachineState *sms) +{ + MemoryRegion *sysmem = get_system_memory(); + MemoryRegion *mr = &sms->cxl_devices_state.host_mr; + + memory_region_init(mr, OBJECT(sms), "cxl_host_reg", + sbsa_ref_memmap[SBSA_CXL_HOST].size); + memory_region_add_subregion(sysmem, sbsa_ref_memmap[SBSA_CXL_HOST].base, mr); } 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); + create_secure_ec(secure_sysmem); sms->bootinfo.ram_size = machine->ram_size;
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. Signed-off-by: Yuquan Wang <wangyuquan1236@phytium.com.cn> --- hw/arm/sbsa-ref.c | 54 ++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 51 insertions(+), 3 deletions(-)