[PATCH 0/2] hw/arm/bcm2838_pcie: make PCI device enumeration work

[PATCH 0/2] hw/arm/bcm2838_pcie: make PCI device enumeration work

[Marcelo Manzo]

Follow-up to "hw/arm/raspi4b: working PCIe and GENET (real
networking)" [1]. That series got the PCIe Root Complex link up and
GENET (a plain sysbus device, not PCI) working, but nothing in it
actually required a guest to enumerate a device *on* the PCIe bus --
so a real gap went unnoticed until now: attach anything to
raspi4b's PCIe port and "lspci"/"/sys/bus/pci/devices/" come back
completely empty, even though the link itself is up.

Root causes, all on the same host-bridge config-space path in
hw/arm/bcm2838_pcie.c:

 1. bcm2838_pcie_host_read/write computed
    "offset - PCIE_CONFIG_SPACE_SIZE" on an unsigned hwaddr, which
    underflows for every offset below 4KB -- i.e. the root port's own
    config space, including vendor/device ID at offset 0. Every such
    access fell into the "out of range" branch and read back as
    all-ones ("no device present"), so the guest's PCI core gave up
    immediately.

 2. pcie_host_mmcfg_init() was never called, even though both
    accessors dereference the MemoryRegion it sets up to service
    EXT_CFG_DATA. Fixing (1) let the guest reach that path for the
    first time, which then dereferenced an uninitialised region.

 3. The root port overrode PCIDeviceClass::config_read/config_write
    with plain pci_default_*_config(), bypassing
    pci_bridge_write_config(). Bridge memory windows were therefore
    never actually programmed, so BARs behind the root port stayed
    unreachable however the guest wrote them.

 4. PCIE_MMIO_OFFSET (0xc0000000) was defined in
    hw/arm/bcm2838_peripherals.c for exactly this purpose and never
    used -- grep count of 1, the definition itself. The PCI MMIO
    window was mapped straight onto the ARM address with no
    translation, even though the DTB declares CPU 0x600000000 as
    mapping to PCI 0xc0000000. The guest ended up reading PCI address
    0 where a device's BAR should have been.

Patch 1 fixes all four. I want to flag why they're one patch rather
than four: I tried splitting them during development, and every
subset short of all four either changes nothing (dormant bug) or
produces a kernel panic that doesn't happen on unfixed master (wrong
BAR content, alignment fault in a driver's MMIO access) -- worse
than the current silent "no devices enumerated". None of the four
are a coherent, independently-safe change on their own.

With all four, a PCIe device is enumerated and its driver binds:

  xhci_hcd 0000:01:00.0: xHCI Host Controller
  xhci_hcd 0000:01:00.0: new USB bus registered, assigned bus number 1
  xhci_hcd 0000:01:00.0: Host supports USB 3.0 SuperSpeed

This is not a complete PCIe implementation. The BCM2711's own MSI
controller is not implemented -- the guest driver programs it
correctly (visible as BRCM STB PCIe MSI vectors in
/proc/interrupts, permanently at 0 count) but QEMU never delivers
anything, so a device that only signals via MSI/MSI-X never actually
gets its interrupt. INTx works. Patch 1 documents this in
docs/system/arm/raspi.rst; patch 2's functional test attaches its
xHCI controller with "msi=off,msix=off" for the same reason. I
plan to look at the MSI controller separately; wanted this
enumeration fix to stand on its own rather than block on it, since it
already turns "no PCI device is usable at all" into "PCI devices
using INTx work".

Tested against real hardware, not just the emulated xHCI: with a USB
serial device passed through to the guest via QEMU's usb-serial
device pointed at the real /dev/cu.usbmodem* port, a real serial
peripheral connected to my Mac was reachable from inside the guest,
byte-for-byte identical to reading it directly on the host.

checkpatch.pl is clean (0 errors, 0 warnings) across both patches.
All 4 tests in tests/functional/aarch64/test_raspi4.py pass,
including the 3 pre-existing ones (no regression) and the new one
added here.

[1] https://patchwork.kernel.org/project/qemu-devel/cover/20260725004219.66222-1-marcelomanzo@gmail.com/

Marcelo Manzo (2):
  hw/arm/bcm2838_pcie: make PCI device enumeration actually work
  tests/functional/aarch64: add raspi4b PCIe enumeration test

 docs/system/arm/raspi.rst               |  2 ++
 hw/arm/bcm2838_pcie.c                   | 45 +++++++++++++++----------
 hw/arm/bcm2838_peripherals.c            | 16 +++++++--
 include/hw/arm/bcm2838_peripherals.h    |  1 +
 tests/functional/aarch64/test_raspi4.py | 37 ++++++++++++++++++++
 5 files changed, 82 insertions(+), 19 deletions(-)

-- 
2.47.1

[PATCH 1/2] hw/arm/bcm2838_pcie: make PCI device enumeration actually work

[Marcelo Manzo]

raspi4b's PCIe root complex brought the link up but no device was ever
enumerated: "lspci" was empty and /sys/bus/pci/devices/ had no entries.
Nothing exercised this before, since GENET is a sysbus device rather
than a PCI one.

Four separate defects, all on the same path:

1. bcm2838_pcie_host_read/write computed
   "offset - PCIE_CONFIG_SPACE_SIZE" on an unsigned hwaddr, which
   underflows for every offset below 4KB. The root port's own config
   space -- including vendor/device ID at offset 0 -- therefore always
   fell into the out-of-range branch and read back as all-ones, i.e.
   "no device present", so the guest stopped scanning immediately.

2. pcie_host_mmcfg_init() was never called, yet both accessors
   dereference pcie_hb->mmio.ops to service EXT_CFG_DATA. Once (1) was
   fixed and the guest actually reached that path, it touched an
   uninitialised MemoryRegion.

3. The root port overrode PCIDeviceClass::config_read/config_write with
   plain pci_default_*_config(). That bypasses rp_write_config() ->
   pci_bridge_write_config(), so programming the bridge's memory window
   never updated the bridge's address space and BARs behind the root
   port stayed unreachable. Drop the overrides and dispatch through
   pci_host_config_{read,write}_common() so the device's real handlers
   run.

4. The PCI MMIO window was mapped at the ARM base address with no
   address translation, even though PCIE_MMIO_OFFSET was defined for
   exactly that purpose (and otherwise unused). The DTB declares CPU
   0x600000000 as mapping to PCI 0xc0000000, so the guest was reading
   PCI address 0 where it expected a device BAR. Map an alias at the
   correct PCI offset instead.

With these, a PCIe device is enumerated and its driver binds:

  xhci_hcd 0000:01:00.0: xHCI Host Controller
  xhci_hcd 0000:01:00.0: new USB bus registered, assigned bus number 1
  xhci_hcd 0000:01:00.0: Host supports USB 3.0 SuperSpeed

Note this is not yet complete: devices attached behind the xHCI are
still not enumerated by the guest, which looks like a separate
interrupt-delivery problem rather than an enumeration one.

Signed-off-by: Marcelo Manzo <marcelomanzo@gmail.com>
---
 docs/system/arm/raspi.rst            |  2 ++
 hw/arm/bcm2838_pcie.c                | 45 +++++++++++++++++-----------
 hw/arm/bcm2838_peripherals.c         | 16 ++++++++--
 include/hw/arm/bcm2838_peripherals.h |  1 +
 4 files changed, 45 insertions(+), 19 deletions(-)

diff --git a/docs/system/arm/raspi.rst b/docs/system/arm/raspi.rst
index a4f83d0a..a8fc37e9 100644
--- a/docs/system/arm/raspi.rst
+++ b/docs/system/arm/raspi.rst
@@ -43,3 +43,5 @@ Missing devices
 ---------------
 
  * Pulse Width Modulation (PWM)
+ * PCIe MSI/MSI-X interrupt delivery (raspi4b) -- INTx works; a PCIe
+   device that only signals via MSI/MSI-X needs ``msi=off,msix=off``
diff --git a/hw/arm/bcm2838_pcie.c b/hw/arm/bcm2838_pcie.c
index 1596145b..88166d1f 100644
--- a/hw/arm/bcm2838_pcie.c
+++ b/hw/arm/bcm2838_pcie.c
@@ -11,24 +11,13 @@
 #include "qapi/error.h"
 #include "hw/core/irq.h"
 #include "hw/pci-host/gpex.h"
+#include "hw/pci/pci_host.h"
 #include "hw/core/qdev-properties.h"
 #include "migration/vmstate.h"
 #include "qemu/module.h"
 #include "hw/arm/bcm2838_pcie.h"
 #include "trace.h"
 
-static uint32_t bcm2838_pcie_config_read(PCIDevice *d,
-                                         uint32_t address, int len)
-{
-    return pci_default_read_config(d, address, len);
-}
-
-static void bcm2838_pcie_config_write(PCIDevice *d, uint32_t addr, uint32_t val,
-                                      int len)
-{
-    return pci_default_write_config(d, addr, val, len);
-}
-
 static uint64_t bcm2838_pcie_host_read(void *opaque, hwaddr offset,
                                        unsigned size) {
     hwaddr mmcfg_addr;
@@ -39,7 +28,18 @@ static uint64_t bcm2838_pcie_host_read(void *opaque, hwaddr offset,
     uint32_t *cfg_idx = (uint32_t *)(root_regs + BCM2838_PCIE_EXT_CFG_INDEX
                                      - PCIE_CONFIG_SPACE_SIZE);
 
-    if (offset - PCIE_CONFIG_SPACE_SIZE + size <= sizeof(s->root_port.regs)) {
+    if (offset < PCIE_CONFIG_SPACE_SIZE) {
+        /*
+         * The first 4KB of the window is the root port's own PCI config
+         * space (vendor/device ID, BARs, capabilities). Serve it from the
+         * real PCIDevice, not from the raw regs[] shadow buffer, which is
+         * only backing store for the controller registers above 4KB.
+         */
+        value = pci_host_config_read_common(PCI_DEVICE(&s->root_port),
+                                            offset, PCIE_CONFIG_SPACE_SIZE,
+                                            size);
+    } else if (offset - PCIE_CONFIG_SPACE_SIZE + size
+               <= sizeof(s->root_port.regs)) {
         switch (offset) {
         case BCM2838_PCIE_EXT_CFG_DATA
             ... BCM2838_PCIE_EXT_CFG_DATA + PCIE_CONFIG_SPACE_SIZE - 1:
@@ -72,7 +72,12 @@ static void bcm2838_pcie_host_write(void *opaque, hwaddr offset,
 
     trace_bcm2838_pcie_host_write(size, offset, value);
 
-    if (offset - PCIE_CONFIG_SPACE_SIZE + size <= sizeof(s->root_port.regs)) {
+    if (offset < PCIE_CONFIG_SPACE_SIZE) {
+        /* Root port's own PCI config space -- see read path above */
+        pci_host_config_write_common(PCI_DEVICE(&s->root_port), offset,
+                                     PCIE_CONFIG_SPACE_SIZE, value, size);
+    } else if (offset - PCIE_CONFIG_SPACE_SIZE + size
+               <= sizeof(s->root_port.regs)) {
         switch (offset) {
         case BCM2838_PCIE_EXT_CFG_DATA
             ... BCM2838_PCIE_EXT_CFG_DATA + PCIE_CONFIG_SPACE_SIZE - 1:
@@ -137,9 +142,18 @@ static void bcm2838_pcie_host_realize(DeviceState *dev, Error **errp)
     PCIHostState *pci = PCI_HOST_BRIDGE(dev);
     BCM2838PcieHostState *s = BCM2838_PCIE_HOST(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    PCIExpressHost *pex = PCIE_HOST_BRIDGE(dev);
 
     int i;
 
+    /*
+     * Initialise the ECAM config-space window. The BCM2838 does not expose
+     * it to the guest directly; config accesses are made indirectly through
+     * the EXT_CFG_INDEX/EXT_CFG_DATA register pair, which dispatch into
+     * pex->mmio. Without this the ops pointer is never set up.
+     */
+    pcie_host_mmcfg_init(pex, PCIE_MMCFG_SIZE_MAX);
+
     memory_region_init_io(&s->cfg_regs, OBJECT(s), &bcm2838_pcie_host_ops, s,
                           "bcm2838_pcie_cfg_regs", BCM2838_PCIE_REGS_SIZE);
     sysbus_init_mmio(sbd, &s->cfg_regs);
@@ -273,9 +287,6 @@ static void bcm2838_pcie_root_class_init(ObjectClass *class, const void *data)
     k->device_id = BCM2838_PCIE_DEVICE_ID;
     k->revision = BCM2838_PCIE_REVISION;
 
-    k->config_read = bcm2838_pcie_config_read;
-    k->config_write = bcm2838_pcie_config_write;
-
     rpc->exp_offset = BCM2838_PCIE_EXP_CAP_OFFSET;
     rpc->aer_offset = BCM2838_PCIE_AER_CAP_OFFSET;
 }
diff --git a/hw/arm/bcm2838_peripherals.c b/hw/arm/bcm2838_peripherals.c
index de49f48b..012a4567 100644
--- a/hw/arm/bcm2838_peripherals.c
+++ b/hw/arm/bcm2838_peripherals.c
@@ -204,10 +204,22 @@ static void bcm2838_peripherals_realize(DeviceState *dev, Error **errp)
     /* RC registers region */
     regs_mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->pcie_host), 0);
     memory_region_add_subregion(&s->peri_low_mr, PCIE_RC_OFFSET, regs_mr);
-    /* MMIO region */
+    /*
+     * MMIO region.
+     *
+     * The BCM2711 PCIe controller translates addresses between the ARM and
+     * PCI address spaces: the DTB declares CPU 0x600000000 as mapping to PCI
+     * 0xc0000000. Map an alias of the PCI window starting at that PCI offset
+     * so accesses land on the right addresses; mapping the window directly
+     * would expose PCI address 0 at the ARM base instead, and every BAR
+     * behind the root port would be read at the wrong address.
+     */
     mmio_mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->pcie_host), 1);
+    memory_region_init_alias(&s->pcie_mmio_alias, OBJECT(s),
+                             "bcm2838_pcie_mmio_alias", mmio_mr,
+                             PCIE_MMIO_OFFSET, PCIE_MMIO_SIZE);
     memory_region_add_subregion(get_system_memory(), PCIE_MMIO_ARM_OFFSET,
-                                mmio_mr);
+                                &s->pcie_mmio_alias);
 
     /* Gigabit Ethernet */
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->genet), errp)) {
diff --git a/include/hw/arm/bcm2838_peripherals.h b/include/hw/arm/bcm2838_peripherals.h
index 5da340f2..c05709f4 100644
--- a/include/hw/arm/bcm2838_peripherals.h
+++ b/include/hw/arm/bcm2838_peripherals.h
@@ -70,6 +70,7 @@ struct BCM2838PeripheralState {
 
     SDHCIState emmc2;
     BCM2838PcieHostState pcie_host;
+    MemoryRegion pcie_mmio_alias;
     BCM2838GenetState genet;
     BCM2838GpioState gpio;
 
-- 
2.47.1

[PATCH 2/2] tests/functional/aarch64: add raspi4b PCIe enumeration test

[Marcelo Manzo]

Add test_arm_raspi4_pcie, exercising the fix in the previous patch:
attach a qemu-xhci controller to the PCIe root port, confirm the
guest's driver binds it, and confirm both the root port (00:00.0)
and the xHCI controller (01:00.0) show up in
/sys/bus/pci/devices/. Before the previous patch this would find no
devices at all.

Attaches with msi=off,msix=off: the BCM2838 PCIe root complex's own
MSI controller is not implemented, so a device relying on MSI/MSI-X
for interrupts never signals its driver. INTx works and is what this
test (and any real device behind this root port, for now) relies on.

Signed-off-by: Marcelo Manzo <marcelomanzo@gmail.com>
---
 tests/functional/aarch64/test_raspi4.py | 37 +++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/tests/functional/aarch64/test_raspi4.py b/tests/functional/aarch64/test_raspi4.py
index 0e254a20..32542b84 100755
--- a/tests/functional/aarch64/test_raspi4.py
+++ b/tests/functional/aarch64/test_raspi4.py
@@ -122,5 +122,42 @@ def test_arm_raspi4_genet(self):
         exec_command_and_wait_for_pattern(self, 'halt', 'reboot: System halted')
 
 
+    def test_arm_raspi4_pcie(self):
+        kernel_path = self.archive_extract(self.ASSET_KERNEL_20190215,
+                                           member='boot/kernel8.img')
+        dtb_path = self.archive_extract(self.ASSET_KERNEL_20190215,
+                                        member='boot/bcm2711-rpi-4-b.dtb')
+        initrd_path = self.uncompress(self.ASSET_INITRD)
+
+        self.set_machine('raspi4b')
+        self.vm.set_console()
+        kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
+                               'earlycon=pl011,mmio32,0xfe201000 ' +
+                               'console=ttyAMA0,115200 ' +
+                               'panic=-1 noreboot ' +
+                               'dwc_otg.fiq_fsm_enable=0')
+        self.vm.add_args('-kernel', kernel_path,
+                         '-dtb', dtb_path,
+                         '-initrd', initrd_path,
+                         '-append', kernel_command_line,
+                         '-no-reboot',
+                         # msi=off,msix=off: the BCM2838 PCIe root complex's
+                         # MSI controller is not implemented, so a device
+                         # relying on MSI/MSI-X for interrupts never signals
+                         # and its driver never probes. INTx works.
+                         '-device',
+                         'qemu-xhci,bus=pcie.1,id=xhci,msi=off,msix=off')
+        self.vm.launch()
+        self.wait_for_console_pattern(
+            'xhci_hcd 0000:01:00.0: xHCI Host Controller')
+        self.wait_for_console_pattern('Boot successful.')
+
+        exec_command_and_wait_for_pattern(self, 'ls /sys/bus/pci/devices/',
+                                                '0000:00:00.0')
+        exec_command_and_wait_for_pattern(self, 'ls /sys/bus/pci/devices/',
+                                                '0000:01:00.0')
+        exec_command_and_wait_for_pattern(self, 'halt', 'reboot: System halted')
+
+
 if __name__ == '__main__':
     LinuxKernelTest.main()
-- 
2.47.1