On 17 Oct 2026, Peter Maydell wrote: > > I think these read and write functions have endianness issues. > [...] > 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
Agreed, thanks -- I'll do that in v4 (regs[] as little-endian bytes, accessed with ldn_le_p()/stn_le_p(), and ldl_le_p() for the EXT_CFG_INDEX value instead of the uint32_t * overlay). > 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 ? I had a look, and I think you're right. The only users of pci_device_route_intx_to_irq() are VFIO, the x86 PIIX/Xen code and the multi-process proxy. VFIO only calls it when KVM is active with irqfds and resamplefds enabled; there is a comment in vfio_intx_enable() saying it is conditional "to avoid generating error messages on platforms where we won't actually use the result anyway". raspi4b runs under TCG only, so none of those can apply. I copied this from the original series (and gpex, which has it for VFIO under KVM) without needing it. I also tried it: with the hook, the irq_num[] storage and bcm2838_pcie_host_set_irq_num() removed, a qemu-xhci with a usb-kbd on the root port still enumerates and takes its INTx interrupts exactly as before. So I'll drop it in v4. Eric, shout if you know of a reason to keep it. thanks -- Marcelo On Sat, Oct 17, 2026 09:07 AM, Peter Maydell <[email protected]> wrote: > 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 >
