Re: [SPAM] [PATCH v2 03/14] hw/pci-host/aspeed: Add AST2600 PCIe config space and host bridge

["Cédric Le Goater" <clg@kaod.org>]

On 9/17/25 11:05, Jamin Lin wrote:
> Hi Cédric
> 
>> Subject: RE: [SPAM] [PATCH v2 03/14] hw/pci-host/aspeed: Add AST2600 PCIe
>> config space and host bridge
>>
>> Hi Cédric
>>
>>> Subject: Re: [SPAM] [PATCH v2 03/14] hw/pci-host/aspeed: Add AST2600
>>> PCIe config space and host bridge
>>>
>>> On 9/11/25 09:24, Jamin Lin wrote:
>>>> Introduce PCIe config and host bridge model for the AST2600 platform.
>>>>
>>>> This patch adds support for the H2X (AHB to PCIe Bus Bridge)
>>>> controller with a 0x100 byte register space. The register layout is
>>>> shared between two root complexes: 0x00–0x7f is common, 0x80–0xbf
>>>> for RC_L, and 0xc0–0xff for RC_H. Only RC_H is modeled in this
>>> implementation.
>>>>
>>>> The RC_H bus uses bus numbers in the 0x80–0xff range instead of the
>>>> standard root bus 0x00. To allow the PCI subsystem to discover
>>>> devices, the host bridge logic remaps the root bus number back to
>>>> 0x00 whenever the configured bus number matches the "bus-nr" property.
>>>>
>>>> New MMIO callbacks are added for the H2X config space:
>>>> - aspeed_pcie_cfg_read() and aspeed_pcie_cfg_write() handle register
>>>>     accesses.
>>>> - aspeed_pcie_cfg_readwrite() provides configuration read/write support.
>>>> - aspeed_pcie_cfg_translate_write() handles PCIe byte-enable semantics
>> for
>>>>     write operations.
>>>>
>>>> The reset handler initializes the H2X register block with default
>>>> values as defined in the AST2600 datasheet.
>>>>
>>>> Additional changes:
>>>> - Implement ASPEED PCIe root complex (TYPE_ASPEED_PCIE_RC).
>>>> - Wire up interrupt propagation via aspeed_pcie_rc_set_irq().
>>>> - Add tracepoints for config read/write and INTx handling.
>>>>
>>>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
>>>> ---
>>>>    include/hw/pci-host/aspeed_pcie.h |  58 ++++
>>>>    hw/pci-host/aspeed_pcie.c         | 422
>>> ++++++++++++++++++++++++++++++
>>>>    hw/pci-host/trace-events          |   4 +
>>>>    3 files changed, 484 insertions(+)
>>>>
>>>> diff --git a/include/hw/pci-host/aspeed_pcie.h
>>>> b/include/hw/pci-host/aspeed_pcie.h
>>>> index faf87073ab..e2c5dc6f62 100644
>>>> --- a/include/hw/pci-host/aspeed_pcie.h
>>>> +++ b/include/hw/pci-host/aspeed_pcie.h
>>>> @@ -24,6 +24,64 @@
>>>>    #include "hw/pci/pcie_host.h"
>>>>    #include "qom/object.h"
>>>>
>>>> +typedef struct AspeedPCIECfgTxDesc {
>>>> +    uint32_t desc0;
>>>> +    uint32_t desc1;
>>>> +    uint32_t desc2;
>>>> +    uint32_t desc3;
>>>> +    uint32_t wdata;
>>>> +    uint32_t rdata_reg;
>>>> +} AspeedPCIECfgTxDesc;
>>>> +
>>>> +typedef struct AspeedPCIERcRegs {
>>>> +    uint32_t int_en_reg;
>>>> +    uint32_t int_sts_reg;
>>>> +} AspeedPCIERcRegs;
>>>> +
>>>> +typedef struct AspeedPCIERegMap {
>>>> +    AspeedPCIERcRegs rc;
>>>> +} AspeedPCIERegMap;
>>>> +
>>>> +#define TYPE_ASPEED_PCIE_RC "aspeed.pcie-rc"
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(AspeedPCIERcState, ASPEED_PCIE_RC);
>>>> +
>>>> +struct AspeedPCIERcState {
>>>> +    PCIExpressHost parent_obj;
>>>> +
>>>> +    MemoryRegion mmio_window;
>>>> +    MemoryRegion io_window;
>>>> +    MemoryRegion mmio;
>>>> +    MemoryRegion io;
>>>> +
>>>> +    uint32_t bus_nr;
>>>> +    char name[16];
>>>> +    qemu_irq irq;
>>>> +};
>>>> +
>>>> +/* Bridge between AHB bus and PCIe RC. */ #define
>>>> +TYPE_ASPEED_PCIE_CFG "aspeed.pcie-cfg"
>>>> +OBJECT_DECLARE_TYPE(AspeedPCIECfgState, AspeedPCIECfgClass,
>>>> +ASPEED_PCIE_CFG);
>>>> +
>>>> +struct AspeedPCIECfgState {
>>>> +    SysBusDevice parent_obj;
>>>> +
>>>> +    MemoryRegion mmio;
>>>> +    uint32_t *regs;
>>>> +    uint32_t id;
>>>> +
>>>> +    AspeedPCIERcState rc;
>>>> +};
>>>> +
>>>> +struct AspeedPCIECfgClass {
>>>> +    SysBusDeviceClass parent_class;
>>>> +
>>>> +    const AspeedPCIERegMap *reg_map;
>>>> +    const MemoryRegionOps *reg_ops;
>>>> +
>>>> +    uint64_t rc_bus_nr;
>>>> +    uint64_t nr_regs;
>>>> +};
>>>> +
>>>>    #define TYPE_ASPEED_PCIE_PHY "aspeed.pcie-phy"
>>>>    OBJECT_DECLARE_TYPE(AspeedPCIEPhyState, AspeedPCIEPhyClass,
>>>> ASPEED_PCIE_PHY);
>>>>
>>>> diff --git a/hw/pci-host/aspeed_pcie.c b/hw/pci-host/aspeed_pcie.c
>>>> index 5584449b17..9fb7c1ef67 100644
>>>> --- a/hw/pci-host/aspeed_pcie.c
>>>> +++ b/hw/pci-host/aspeed_pcie.c
>>>> @@ -27,6 +27,426 @@
>>>>    #include "hw/pci/msi.h"
>>>>    #include "trace.h"
>>>>
>>>> +/*
>>>> + * PCIe Root Complex (RC)
>>>> + */
>>>> +
>>>> +static void aspeed_pcie_rc_set_irq(void *opaque, int irq, int
>>>> +level) {
>>>> +    AspeedPCIERcState *rc = (AspeedPCIERcState *) opaque;
>>>> +    AspeedPCIECfgState *cfg =
>>>> +        container_of(rc, AspeedPCIECfgState, rc);
>>>> +    AspeedPCIECfgClass *apc = ASPEED_PCIE_CFG_GET_CLASS(cfg);
>>>> +    const AspeedPCIERcRegs *rc_regs;
>>>
>>> I suggest you cache &apc->reg_map->rc under AspeedPCIECfgState as an
>>> attribute (at realize time). This will ease reading the code and
>>> improve performance.
>>>
>> Thanks for your review and suggestion.
>> Will do.
>>
>>>> +    bool intx;
>>>> +
>>>> +    assert(irq < PCI_NUM_PINS);
>>>> +
>>>> +    rc_regs = &apc->reg_map->rc;
>>>> +
>>>> +    if (level) {
>>>> +        cfg->regs[rc_regs->int_sts_reg] |= BIT(irq);
>>>> +    } else {
>>>> +        cfg->regs[rc_regs->int_sts_reg] &= ~BIT(irq);
>>>> +    }
>>>> +
>>>> +    intx = !!(cfg->regs[rc_regs->int_sts_reg] &
>>> cfg->regs[rc_regs->int_en_reg]);
>>>> +    trace_aspeed_pcie_rc_intx_set_irq(cfg->id, irq, intx);
>>>> +    qemu_set_irq(rc->irq, intx);
>>>> +}
>>>> +
>>>> +static int aspeed_pcie_rc_map_irq(PCIDevice *pci_dev, int irq_num) {
>>>> +    return irq_num % PCI_NUM_PINS;
>>>> +}
>>>> +
>>>> +static void aspeed_pcie_rc_realize(DeviceState *dev, Error **errp) {
>>>> +    PCIExpressHost *pex = PCIE_HOST_BRIDGE(dev);
>>>> +    AspeedPCIERcState *rc = ASPEED_PCIE_RC(dev);
>>>> +    AspeedPCIECfgState *cfg =
>>>> +           container_of(rc, AspeedPCIECfgState, rc);
>>>> +    PCIHostState *pci = PCI_HOST_BRIDGE(dev);
>>>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>>> +    g_autofree char *name;
>>>> +
>>>> +    /* PCI configuration space */
>>>> +    pcie_host_mmcfg_init(pex, PCIE_MMCFG_SIZE_MAX);
>>>> +    sysbus_init_mmio(sbd, &pex->mmio);
>>>> +
>>>> +    /* MMIO and IO region */
>>>> +    memory_region_init(&rc->mmio, OBJECT(rc), "mmio",
>> UINT64_MAX);
>>>> +    memory_region_init(&rc->io, OBJECT(rc), "io", 0x10000);
>>>> +
>>>> +    name = g_strdup_printf("pcie.%d.mmio_window", cfg->id);
>>>> +    memory_region_init_io(&rc->mmio_window, OBJECT(rc),
>>> &unassigned_io_ops,
>>>> +                          OBJECT(rc), name, UINT64_MAX);
>>>> +    name = g_strdup_printf("pcie.%d.ioport_window", cfg->id);
>>>> +    memory_region_init_io(&rc->io_window, OBJECT(rc),
>>> &unassigned_io_ops,
>>>> +                          OBJECT(rc), name, 0x10000);
>>>> +
>>>> +    memory_region_add_subregion(&rc->mmio_window, 0,
>> &rc->mmio);
>>>> +    memory_region_add_subregion(&rc->io_window, 0, &rc->io);
>>>> +    sysbus_init_mmio(sbd, &rc->mmio_window);
>>>> +    sysbus_init_mmio(sbd, &rc->io_window);
>>>> +
>>>> +    sysbus_init_irq(sbd, &rc->irq);
>>>> +    name = g_strdup_printf("pcie.rc%d", cfg->id);
>>>> +    pci->bus = pci_register_root_bus(dev, name, aspeed_pcie_rc_set_irq,
>>>> +                                     aspeed_pcie_rc_map_irq,
>> rc,
>>> &rc->mmio,
>>>> +                                     &rc->io, 0, 4,
>>> TYPE_PCIE_BUS);
>>>> +    pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE; }
>>>> +
>>>> +static const char *aspeed_pcie_rc_root_bus_path(PCIHostState
>>> *host_bridge,
>>>> +                                                PCIBus
>> *rootbus)
>>> {
>>>> +    AspeedPCIERcState *rc = ASPEED_PCIE_RC(host_bridge);
>>>> +    AspeedPCIECfgState *cfg =
>>>> +           container_of(rc, AspeedPCIECfgState, rc);
>>>> +
>>>> +    snprintf(rc->name, sizeof(rc->name), "%04x:%02x", cfg->id,
>>>> + rc->bus_nr);
>>>> +
>>>> +    return rc->name;
>>>> +}
>>>> +
>>>> +static const Property aspeed_pcie_rc_props[] = {
>>>> +    DEFINE_PROP_UINT32("bus-nr", AspeedPCIERcState, bus_nr, 0), };
>>>> +
>>>> +static void aspeed_pcie_rc_class_init(ObjectClass *klass, const
>>>> +void
>>>> +*data) {
>>>> +    PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
>>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>>> +
>>>> +    dc->desc = "ASPEED PCIe RC";
>>>> +    dc->realize = aspeed_pcie_rc_realize;
>>>> +    dc->fw_name = "pci";
>>>> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>>>> +
>>>> +    hc->root_bus_path = aspeed_pcie_rc_root_bus_path;
>>>> +    device_class_set_props(dc, aspeed_pcie_rc_props);
>>>> +
>>>> +    msi_nonbroken = true;
>>>> +}
>>>> +
>>>> +static const TypeInfo aspeed_pcie_rc_info = {
>>>> +    .name = TYPE_ASPEED_PCIE_RC,
>>>> +    .parent = TYPE_PCIE_HOST_BRIDGE,
>>>> +    .instance_size = sizeof(AspeedPCIERcState),
>>>> +    .class_init = aspeed_pcie_rc_class_init, };
>>>> +
>>>> +/*
>>>> + * PCIe Config
>>>> + *
>>>> + * AHB to PCIe Bus Bridge (H2X)
>>>> + *
>>>> + * On the AST2600:
>>>> + * NOTE: rc_l is not supported by this model.
>>>> + * - Registers 0x00 - 0x7F are shared by both PCIe0 (rc_l) and PCIe1 (rc_h).
>>>> + * - Registers 0x80 - 0xBF are specific to PCIe0.
>>>> + * - Registers 0xC0 - 0xFF are specific to PCIe1.
>>>> + */
>>>> +
>>>> +/* AST2600 */
>>>> +REG32(H2X_CTRL,             0x00)
>>>> +    FIELD(H2X_CTRL, CLEAR_RX, 4, 1)
>>>> +REG32(H2X_TX_CLEAR,         0x08)
>>>> +    FIELD(H2X_TX_CLEAR, IDLE, 0, 1)
>>>> +REG32(H2X_RDATA,            0x0C)
>>>> +REG32(H2X_TX_DESC0,         0x10)
>>>> +REG32(H2X_TX_DESC1,         0x14)
>>>> +REG32(H2X_TX_DESC2,         0x18)
>>>> +REG32(H2X_TX_DESC3,         0x1C)
>>>> +REG32(H2X_TX_DATA,          0x20)
>>>> +REG32(H2X_TX_STS,           0x24)
>>>> +    FIELD(H2X_TX_STS, IDLE, 31, 1)
>>>> +    FIELD(H2X_TX_STS, RC_L_TX_COMP, 24, 1)
>>>> +    FIELD(H2X_TX_STS, RC_H_TX_COMP, 25, 1)
>>>> +    FIELD(H2X_TX_STS, TRIG, 0, 1)
>>>> +REG32(H2X_RC_H_CTRL,        0xC0)
>>>> +REG32(H2X_RC_H_INT_EN,      0xC4)
>>>> +REG32(H2X_RC_H_INT_STS,     0xC8)
>>>> +    SHARED_FIELD(H2X_RC_INT_INTDONE, 4, 1)
>>>> +    SHARED_FIELD(H2X_RC_INT_INTX, 0, 4)
>>>> +REG32(H2X_RC_H_RDATA,       0xCC)
>>>> +
>>>> +#define TLP_FMTTYPE_CFGRD0  0x04 /* Configuration Read  Type 0 */
>>>> +#define TLP_FMTTYPE_CFGWR0  0x44 /* Configuration Write Type 0 */
>>>> +#define TLP_FMTTYPE_CFGRD1  0x05 /* Configuration Read  Type 1 */
>>>> +#define TLP_FMTTYPE_CFGWR1  0x45 /* Configuration Write Type 1 */
>>>> +
>>>> +#define PCIE_CFG_FMTTYPE_MASK(x) (((x) >> 24) & 0xff) #define
>>>> +PCIE_CFG_BYTE_EN(x) ((x) & 0xf)
>>>> +
>>>> +static const AspeedPCIERegMap aspeed_regmap = {
>>>> +    .rc = {
>>>> +        .int_en_reg     = R_H2X_RC_H_INT_EN,
>>>> +        .int_sts_reg    = R_H2X_RC_H_INT_STS,
>>>> +    },
>>>> +};
>>>> +
>>>> +static uint64_t aspeed_pcie_cfg_read(void *opaque, hwaddr addr,
>>>> +                                     unsigned int size) {
>>>> +    AspeedPCIECfgState *s = ASPEED_PCIE_CFG(opaque);
>>>> +    uint32_t reg = addr >> 2;
>>>> +    uint32_t value = 0;
>>>> +
>>>> +    value = s->regs[reg];
>>>> +
>>>> +    trace_aspeed_pcie_cfg_read(s->id, addr, value);
>>>> +
>>>> +    return value;
>>>> +}
>>>> +
>>>> +static void aspeed_pcie_cfg_translate_write(uint8_t byte_en,
>>>> +uint32_t
>>> *addr,
>>>> +                                            uint64_t *val, int
>>> *len)
>>>> +{
>>>> +    uint64_t packed_val = 0;
>>>> +    int first_bit = -1;
>>>> +    int index = 0;
>>>> +    int i;
>>>> +
>>>> +    *len = ctpop8(byte_en);
>>>> +
>>>> +    if (*len == 0 || *len > 4) {
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    /* Special case: full 4-byte write must be 4-byte aligned */
>>>> +    if (byte_en == 0x0f) {
>>>> +        if (*addr % 4 != 0) {
>>>
>>> This is an aligment issue to be reported as such and not with "invalid
>>> byte enable"
>>>
>>> I think you should remove the "goto err" and generate a
>>> LOG_GUEST_ERROR instead.
>>>
>> Will fix it.
>>>> +            goto err;
>>>> +        }
>>>> +        *val = *val & 0xffffffff;
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    for (i = 0; i < 4; i++) {
>>>> +        if (byte_en & (1 << i)) {
>>>> +            if (first_bit < 0) {
>>>> +                first_bit = i;
>>>> +            }
>>>> +            packed_val |= ((*val >> (i * 8)) & 0xff) << (index * 8);
>>>> +            index++;> +        }
>>>> +    }
>>>> +
>>>> +    *addr += first_bit;
>>>> +    *val = packed_val;
>>>> +
>>>> +    return;
>>>> +
>>>> +err:
>>>> +    qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid byte enable:
>>> 0x%x\n",
>>>> +                  __func__, byte_en); }
>>>> +
>>>> +static void aspeed_pcie_cfg_readwrite(AspeedPCIECfgState *s,
>>>> +                                      const
>> AspeedPCIECfgTxDesc
>>>> +*desc) {
>>>> +    AspeedPCIERcState *rc = &s->rc;
>>>> +    PCIHostState *pci;
>>>> +    uint32_t cfg_addr;
>>>> +    PCIDevice *pdev;
>>>> +    uint32_t offset;
>>>> +    uint8_t byte_en;
>>>> +    bool is_write;
>>>> +    uint8_t devfn;
>>>> +    uint64_t val;
>>>> +    uint8_t bus;
>>>> +    int len;
>>>> +
>>>> +    val = ~0;
>>>> +    is_write = !!(desc->desc0 & BIT(30));
>>>> +    cfg_addr = desc->desc2;
>>>
>>>
>>> hmm, what about endianess ?
>>>
> 
> We keep .endianness = DEVICE_LITTLE_ENDIAN on the MMIO region, so the guest wire-LE bytes are already converted at the MMIO boundary.
> Inside the device (s->regs[], FIFOs, and desc.*) we consistently use CPU-endian integers and apply bitfield operations on those.
> 
> I tried adding explicit le32_to_cpu()/cpu_to_le32() on s->regs[] to “convert” them again to CPU-endian,
> but that caused a "double swap" on big-endian hosts (e.g. MIPS64). The result was broken config TLP decoding and incorrect byte-enable packing.
> 
> I experimented with the patch below—does this look incorrect given the MMIO endianness model?
> case R_H2X_TX_STS:
>      if (data & R_H2X_TX_STS_TRIG_MASK) {
> -       desc.desc0 = s->regs[R_H2X_TX_DESC0];
> -       desc.desc1 = s->regs[R_H2X_TX_DESC1];
> -       desc.desc2 = s->regs[R_H2X_TX_DESC2];
> -       desc.desc3 = s->regs[R_H2X_TX_DESC3];
> -       desc.wdata = s->regs[R_H2X_TX_DATA];
> +       desc.desc0 = le32_to_cpu(s->regs[R_H2X_TX_DESC0]);
> +       desc.desc1 = le32_to_cpu(s->regs[R_H2X_TX_DESC1]);
> +       desc.desc2 = le32_to_cpu(s->regs[R_H2X_TX_DESC2]);
> +       desc.desc3 = le32_to_cpu(s->regs[R_H2X_TX_DESC3]);
> +       desc.wdata = le32_to_cpu(s->regs[R_H2X_TX_DATA]);
>          desc.rdata_reg = R_H2X_RC_H_RDATA;
>          aspeed_pcie_cfg_readwrite(s, &desc);
>          ...
> 
> On MIPS64 (BE) this change made the device non-functional, which aligns with the double-swap hypothesis.
> My understanding is we should not add these conversions and instead keep s->regs[]/desc.* strictly CPU-endian, relying on the MMIO layer for wire-LE ↔ CPU conversions.
> 
> I tested the v2 patch series and it passes on a big-endian host machine (qemumips64)
> Does anything above look incorrect to you?

My bad. Conversions are no needed since the descriptors come from
register values, and not RAM :/

A e1000e device works find on a BE host :

10: eth4: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast qlen 1000
     link/ether 52:54:00:12:34:59 brd ff:ff:ff:ff:ff:ff
     inet 10.0.2.15/24 brd 10.0.2.255 scope global dynamic eth4
        valid_lft 86279sec preferred_lft 86279sec
     inet6 fe80::5054:ff:fe12:3459/64 scope link
        valid_lft forever preferred_lft forever
root@ast2600-default:~# grep  eth4 /proc/interrupts
  82:        155          0  PCIe MSI 67633152 Edge      eth4-rx-0
  83:        139          0  PCIe MSI 67633153 Edge      eth4-tx-0
  84:          4          0  PCIe MSI 67633154 Edge      eth4
root@ast2600-default:~# dmesg  | grep e1000e
[    1.349634] e1000e: Intel(R) PRO/1000 Network Driver
[    1.350111] e1000e: Copyright(c) 1999 - 2015 Intel Corporation.
[   12.940541] e1000e 0000:81:00.0: Interrupt Throttling Rate (ints/sec) set to dynamic conservative mode
[   13.008532] e1000e 0000:81:00.0 0000:81:00.0 (uninitialized): registered PHC clock
[   13.079534] e1000e 0000:81:00.0 eth4: (PCI Express:2.5GT/s:Width x1) 52:54:00:12:34:59
[   13.080149] e1000e 0000:81:00.0 eth4: Intel(R) PRO/1000 Network Connection
[   13.080612] e1000e 0000:81:00.0 eth4: MAC: 3, PHY: 8, PBA No: 000000-000
[   38.428521] e1000e 0000:81:00.0 eth4: NIC Link is Up 1000 Mbps Full Duplex, Flow Control: Rx/Tx

Thanks for looking.

C.