Re: [RFC PATCH v3 2/2] pci-host/cxl: Support creation of a new CXL Host Bridge

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

On Tue, 17 Jun 2025 12:06:49 +0800
wangyuquan <wangyuquan1236@phytium.com.cn> wrote:

> From: Yuquan Wang <wangyuquan1236@phytium.com.cn>
> 
> Define a new CXL host bridge type (TYPE_CXL_HOST). This is an
> independent CXL host bridge which combined GPEX features (ECAM, MMIO
> windows and irq) and CXL Host Bridge Component Registers (CHBCR).
> 
> The root bus path of CXL_HOST is "0001:00", that would not affect the
> original PCIe host topology on some platforms. In the previous, the
> pxb-cxl-host with any CXL root ports and CXL endpoint devices would
> share the resources (like BDF, MMIO space) of the original pcie
> domain, but it would cause some platforms like sbsa-ref are unable to
> support the original number of PCIe devices. The new type provides a
> solution to resolve the problem.
> 
> Signed-off-by: Yuquan Wang <wangyuquan1236@phytium.com.cn>
A few trivial things inline.  To me this looks fine but it needs more
eyes, particularly form PCI folk on whether anything is missing for the
host bridge.

> diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig
> index 35c0415242..05c772bcf4 100644
> --- a/hw/pci-host/Kconfig
> +++ b/hw/pci-host/Kconfig
> @@ -74,6 +74,10 @@ config PCI_POWERNV
>      select MSI_NONBROKEN
>      select PCIE_PORT
>  
> +config CXL_HOST_BRIDGE
> +    bool
> +    select PCI_EXPRESS
> +
>  config REMOTE_PCIHOST
>      bool
>  
> diff --git a/hw/pci-host/cxl.c b/hw/pci-host/cxl.c
> new file mode 100644
> index 0000000000..74c8c83314
> --- /dev/null
> +++ b/hw/pci-host/cxl.c
> @@ -0,0 +1,152 @@
> +/*
> + * QEMU CXL Host Bridge Emulation
> + *
> + * Copyright (C) 2025, Phytium Technology Co, Ltd. All rights reserved.
> + *
> + * Based on gpex.c
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/pci/pci_bus.h"
> +#include "hw/pci-host/cxl_host_bridge.h"
> +
> +static void cxl_host_set_irq(void *opaque, int irq_num, int level)
> +{
> +    CXLHostBridge *host = opaque;
> +
> +    qemu_set_irq(host->irq[irq_num], level);
> +}
> +
> +int cxl_host_set_irq_num(CXLHostBridge *host, int index, int gsi)
> +{
> +    if (index >= PCI_NUM_PINS) {
> +        return -EINVAL;
> +    }
> +
> +    host->irq_num[index] = gsi;
> +    return 0;
> +}
> +
> +static PCIINTxRoute cxl_host_route_intx_pin_to_irq(void *opaque, int pin)
> +{
> +    PCIINTxRoute route;
> +    CXLHostBridge *host = opaque;
> +    int gsi = host->irq_num[pin];
> +
> +    route.irq = gsi;
> +    if (gsi < 0) {
> +        route.mode = PCI_INTX_DISABLED;
> +    } else {
> +        route.mode = PCI_INTX_ENABLED;
> +    }
> +
> +    return route;
    CXLHostBridge *host = opaque;
    int gsi = host->irq_num[pin];
    PCIINTxRoute route = {
       .irq = gsi,
       .mode = gsi < 0 ? PCI_INTX_DISABLED : PCI_INTX_ENABLED,
    }

    return route;

perhaps?  Or maybe not worth bothering.

    
> +}

> +static void cxl_host_realize(DeviceState *dev, Error **errp)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    CXLHostBridge *host = CXL_HOST(dev);
> +    CXLComponentState *cxl_cstate = &host->cxl_cstate;
> +    struct MemoryRegion *mr = &cxl_cstate->crb.component_registers;
> +    PCIHostState *pci = PCI_HOST_BRIDGE(dev);
> +    PCIExpressHost *pex = PCIE_HOST_BRIDGE(dev);
> +    PCIBus *cxlbus;
> +    int i;
> +
> +    cxl_host_reset(host);
> +    cxl_component_register_block_init(OBJECT(dev), cxl_cstate, TYPE_CXL_HOST);
> +    sysbus_init_mmio(sbd, mr);
> +
> +    pcie_host_mmcfg_init(pex, PCIE_MMCFG_SIZE_MAX);
> +    sysbus_init_mmio(sbd, &pex->mmio);
> +
> +    memory_region_init(&host->io_mmio, OBJECT(host), "cxl_host_mmio",
> +                        UINT64_MAX);
> +
> +    memory_region_init_io(&host->io_mmio_window, OBJECT(host),
> +                              &unassigned_io_ops, OBJECT(host),

Odd code alignment.  The & should be under the &
Check for other instances of this.

> +                              "cxl_host_mmio_window", UINT64_MAX);
> +
> +    memory_region_add_subregion(&host->io_mmio_window, 0, &host->io_mmio);
> +    sysbus_init_mmio(sbd, &host->io_mmio_window);
> +
> +    /* ioport window init, 64K is the legacy size in x86 */
> +    memory_region_init(&host->io_ioport, OBJECT(host), "cxl_host_ioport",
> +                        64 * 1024);
> +
> +    memory_region_init_io(&host->io_ioport_window, OBJECT(host),
> +                              &unassigned_io_ops, OBJECT(host),
> +                              "cxl_host_ioport_window", 64 * 1024);
> +
> +    memory_region_add_subregion(&host->io_ioport_window, 0, &host->io_ioport);
> +    sysbus_init_mmio(sbd, &host->io_ioport_window);
> +
> +    /* PCIe host bridge use 4 legacy IRQ lines */
> +    for (i = 0; i < PCI_NUM_PINS; i++) {
> +        sysbus_init_irq(sbd, &host->irq[i]);
> +        host->irq_num[i] = -1;
> +    }
> +
> +    pci->bus = pci_register_root_bus(dev, "cxlhost.0", cxl_host_set_irq,
> +                                 pci_swizzle_map_irq_fn, host, &host->io_mmio,
> +                                 &host->io_ioport, 0, 4, TYPE_CXL_BUS);
> +    cxlbus = pci->bus;
> +    cxlbus->flags |= PCI_BUS_CXL;
As cxlbus is only used for flags, why not simply
    pci->bus->flags |= PCI_BUS_CXL;

and drop the local variable. Or... 

> +
> +    pci_bus_set_route_irq_fn(pci->bus, cxl_host_route_intx_pin_to_irq);

Use cxlbus here instead of pci->bus giving a second user.

> +}


> diff --git a/include/hw/pci-host/cxl_host_bridge.h b/include/hw/pci-host/cxl_host_bridge.h
> new file mode 100644
> index 0000000000..833e460f01
> --- /dev/null
> +++ b/include/hw/pci-host/cxl_host_bridge.h
> @@ -0,0 +1,23 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "hw/cxl/cxl.h"
> +#include "hw/irq.h"
> +#include "hw/pci/pcie_host.h"

We should follow approx include what you use principles.
So need the header for MemoryRegion as well here.
"system/memory.h" seems to be most likely choice for that.


> +
> +typedef struct CXLHostBridge {
> +    PCIExpressHost parent_obj;
> +
> +    CXLComponentState cxl_cstate;
> +
> +    MemoryRegion io_ioport;

I'm not sure the io_ prefix is adding much given this is all in
an CXLHostBridge object?

> +    MemoryRegion io_mmio;
> +    MemoryRegion io_ioport_window;
> +    MemoryRegion io_mmio_window;
> +    qemu_irq irq[PCI_NUM_PINS];
> +    int irq_num[PCI_NUM_PINS];
> +} CXLHostBridge;
> +
> +int cxl_host_set_irq_num(CXLHostBridge *host, int index, int gsi);
> +void cxl_host_hook_up_registers(CXLState *cxl_state, CXLHostBridge *host);