On Thu, 15 Oct 2026 at 19:11, Marcelo Manzo <[email protected]> wrote:
>
> Add the BCM2838 PCIe host bridge model: config space, MMIO window,
> and register access for the root port.
>
> Per Peter Maydell's review of a later follow-up series, fold in what
> was originally a separate bugfix here, since this device isn't in
> upstream git yet and the bug was in code this patch itself introduces.
> Four defects fixed, all on the enumeration path (none of this was
> exercised until a later patch actually attaches a PCI device):
>
> 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.
>
> Signed-off-by: Marcelo Manzo <[email protected]>
> +static uint64_t bcm2838_pcie_host_read(void *opaque, hwaddr offset,
> + unsigned size) {
> + hwaddr mmcfg_addr;
> + uint64_t value = ~0;
> + BCM2838PcieHostState *s = opaque;
> + PCIExpressHost *pcie_hb = PCIE_HOST_BRIDGE(s);
> + uint8_t *root_regs = s->root_port.regs;
> + uint32_t *cfg_idx = (uint32_t *)(root_regs + BCM2838_PCIE_EXT_CFG_INDEX
> + - PCIE_CONFIG_SPACE_SIZE);
I think these read and write functions have endianness issues.
In a read/write function the values are host-order, but we are
updating the regs[] array by simple memcpy(), which means that
on a big-endian host the guest will see the wrong thing if it
writes as 32-bits and then reads back as 8-bits.
You probably want to define that the regs[] array is bytes
to be treated as little-endian order, and then use
ldn_le_p()/stn_le_p() to write an arbitrary sized value into
it, or lduw_le_p()/stw_le_p() for fixed-sized accesses, etc.
(pci.h wraps the latter into pci_{set,get}_{word,long,...}
accessors for this kind of "access config space" purpose, but
has no equivalent wrapper of the ldn/stn fns for "N byte access".)
> +
> + 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:
> + mmcfg_addr = *cfg_idx
> + | PCIE_MMCFG_CONFOFFSET(offset - BCM2838_PCIE_EXT_CFG_DATA);
> + value = pcie_hb->mmio.ops->read(opaque, mmcfg_addr, size);
> + break;
> + default:
> + memcpy(&value, root_regs + offset - PCIE_CONFIG_SPACE_SIZE,
> size);
> + }
> + } else {
> + qemu_log_mask(
> + LOG_GUEST_ERROR,
> + "%s: out-of-range access, %u bytes @ offset 0x%04" PRIx64 "\n",
> + __func__, size, offset);
> + }
> +
> + trace_bcm2838_pcie_host_read(size, offset, value);
> + return value;
> +}
> +static PCIINTxRoute bcm2838_pcie_host_route_intx_pin_to_irq(void *opaque,
> + int pin)
> +{
> + PCIINTxRoute route;
> + BCM2838PcieHostState *s = opaque;
> +
> + route.irq = s->irq_num[pin];
> + route.mode = route.irq < 0 ? PCI_INTX_DISABLED : PCI_INTX_ENABLED;
> +
> + return route;
> +}
It's not clear to me if we need this -- most PCI controllers don't
seem to call pci_bus_set_route_irq_fn(). I have a feeling this is
only relevant for PCI passthrough. Eric, do you know ?
thanks
-- PMM
