Re: [PATCH 1/4] hw/riscv/virt: Add CXL support to the RISC-V virt machine

[Jonathan Cameron <jic23@kernel.org>]

On Tue,  2 Jun 2026 15:41:24 +0800
Chen Pei <cp0613@linux.alibaba.com> wrote:

> Enable CXL support on the RISC-V virt machine following the same
> approach used by the ARM virt machine:
> - Add PXB and ACPI_CXL Kconfig selections
> - Add CXLState and PCIBus pointer to RISCVVirtState
> - Register CXL machine properties via cxl_machine_init()
> - Create CXL host register region above the PCIe high MMIO region
> - Call cxl_hook_up_pxb_registers() and cxl_fmws_link_targets() at
>   machine_done time
> - Map Fixed Memory Windows above the CXL host register region
> - Add ACPI0017 device in DSDT and build CEDT table in virt-acpi-build.c
> 
> Signed-off-by: Chen Pei <cp0613@linux.alibaba.com>
Just really minor stuff inline. Been a while since we added an architecture
but all looks fine to me.

Reviewed-by: Jonathan Cameron <jic23@kernel.org>

> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index ce64eaaef7..899f632de7 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -55,6 +55,8 @@
>  #include "hw/pci/pci.h"
>  #include "hw/pci-host/gpex.h"
>  #include "hw/display/ramfb.h"
> +#include "hw/cxl/cxl.h"
> +#include "hw/cxl/cxl_host.h"
>  #include "hw/acpi/aml-build.h"
>  #include "qapi/qapi-visit-common.h"
>  #include "hw/virtio/virtio-iommu.h"
> @@ -1259,9 +1261,27 @@ static inline DeviceState *gpex_pcie_init(MemoryRegion *sys_mem,
>      }
>  
>      GPEX_HOST(dev)->gpex_cfg.bus = PCI_HOST_BRIDGE(dev)->bus;
> +    s->bus = PCI_HOST_BRIDGE(dev)->bus;
>      return dev;
>  }

>  static FWCfgState *create_fw_cfg(const MachineState *ms, hwaddr base)
>  {
>      FWCfgState *fw_cfg;
> @@ -1426,6 +1446,15 @@ static void virt_machine_done(Notifier *notifier, void *data)
>                                       machine_done);
>      MachineState *machine = MACHINE(s);
>      hwaddr start_addr = s->memmap[VIRT_DRAM].base;
> +
> +    if (s->bus) {

There is a guard against !s->bus inside the function so you should be
able to do this unconditionally.

> +        cxl_hook_up_pxb_registers(s->bus, &s->cxl_devices_state,
> +                                  &error_fatal);
> +    }
> +
> +    if (s->cxl_devices_state.is_enabled) {
> +        cxl_fmws_link_targets(&error_fatal);
> +    }
>      hwaddr firmware_end_addr;
>      vaddr kernel_start_addr;
>      const char *firmware_name = riscv_default_firmware_name(&s->soc[0]);
> @@ -1663,6 +1692,17 @@ static void virt_machine_init(MachineState *machine)
>              ROUND_UP(virt_high_pcie_memmap.base, virt_high_pcie_memmap.size);
>      }
>  
> +    create_cxl_host_reg_region(s);
> +
> +    if (s->cxl_devices_state.is_enabled) {
> +        hwaddr cxl_base = virt_high_pcie_memmap.base +
> +                          virt_high_pcie_memmap.size;
> +        cxl_base += memory_region_size(&s->cxl_devices_state.host_mr);
> +        cxl_base = ROUND_UP(cxl_base, 256 * MiB);
> +        cxl_fmws_set_memmap(cxl_base, UINT64_MAX);
> +        cxl_fmws_update_mmio();
> +    }
> +
>      /* register system main memory (actual RAM) */
>      memory_region_add_subregion(system_memory, s->memmap[VIRT_DRAM].base,
>                                  machine->ram);
> @@ -1769,6 +1809,8 @@ static void virt_machine_instance_init(Object *obj)
>      s->oem_table_id = g_strndup(ACPI_BUILD_APPNAME8, 8);
>      s->acpi = ON_OFF_AUTO_AUTO;
>      s->iommu_sys = ON_OFF_AUTO_AUTO;
> +
> +    cxl_machine_init(obj, &s->cxl_devices_state);
>  }
>  
>  static char *virt_get_aia_guests(Object *obj, Error **errp)
> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> index 18a2a323a3..c3201588bb 100644
> --- a/include/hw/riscv/virt.h
> +++ b/include/hw/riscv/virt.h
> @@ -24,6 +24,7 @@
>  #include "hw/core/sysbus.h"
>  #include "hw/block/flash.h"
>  #include "hw/intc/riscv_imsic.h"
> +#include "hw/cxl/cxl.h"
>  
>  #define VIRT_CPUS_MAX_BITS             9
>  #define VIRT_CPUS_MAX                  (1 << VIRT_CPUS_MAX_BITS)
> @@ -64,6 +65,8 @@ struct RISCVVirtState {
>      struct GPEXHost *gpex_host;
>      OnOffAuto iommu_sys;
>      uint16_t pci_iommu_bdf;
> +    CXLState cxl_devices_state;
> +    PCIBus *bus;

That's a very vague bit of naming.  I'd make it explicit what PCIBus this is.

>  };
>  
>  enum {