From 1a8f3b92944843c0acc6c46ca1e49b0111f0686e Mon Sep 17 00:00:00 2001 From: Otis Chung Date: Wed, 11 Jun 2025 19:29:44 +0800 Subject: [PATCH 1/2] Replace BAR flags with unified `layout` parameter This change updates pci_set_bar() to accept a single `layout` bitmask instead of separate boolean flags, simplifying BAR configuration. The new `layout` argument encodes I/O vs. memory space, 32-/64-bit decoding, and prefetchable settings via standard PCI_BASE_ADDRESS_* macros. Existing callers can now pass any combination of: - PCI_BASE_ADDRESS_SPACE_IO or PCI_BASE_ADDRESS_SPACE_MEMORY - PCI_BASE_ADDRESS_MEM_TYPE_32 or PCI_BASE_ADDRESS_MEM_TYPE_64 - PCI_BASE_ADDRESS_MEM_PREFETCH The function writes the full layout to the BAR, derives `bar_is_io_space` from bit[0], and initializes the region with the provided callback. Docstrings updated with Doxygen examples illustrating MMIO and port I/O. BREAKING CHANGE: pci_set_bar() signature changed; call sites must be updated to pass the new `layout` parameter rather than separate flags. --- src/pci.c | 7 +++---- src/pci.h | 31 ++++++++++++++++++++++++++++++- src/virtio-pci.c | 4 +++- 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/src/pci.c b/src/pci.c index 2b59ded..aa4dccf 100644 --- a/src/pci.c +++ b/src/pci.c @@ -145,14 +145,13 @@ static void pci_mmio_io(void *owner, void pci_set_bar(struct pci_dev *dev, uint8_t bar, uint32_t bar_size, - bool is_io_space, + uint32_t layout, dev_io_fn do_io) { - /* TODO: mem type, prefetch */ /* FIXME: bar_size must be power of 2 */ - PCI_HDR_WRITE(dev->hdr, PCI_BAR_OFFSET(bar), is_io_space, 32); + PCI_HDR_WRITE(dev->hdr, PCI_BAR_OFFSET(bar), layout, 32); dev->bar_size[bar] = bar_size; - dev->bar_is_io_space[bar] = is_io_space; + dev->bar_is_io_space[bar] = layout & 0x1U; // Get the bit[0] of layout dev_init(&dev->space_dev[bar], 0, bar_size, dev, do_io); } diff --git a/src/pci.h b/src/pci.h index ff37be7..ca9d322 100644 --- a/src/pci.h +++ b/src/pci.h @@ -46,10 +46,39 @@ struct pci { struct dev pci_mmio_dev; }; +/** + * @brief Configure and initialize a PCI Base Address Register (BAR). + * + * This writes the caller-provided layout bitmask into the BAR register + * in the PCI configuration header, records the region size and I/O type, + * and sets up the address space (MMIO or port I/O) with the specified + * callback. + * + * @param dev Pointer to the pci_dev representing the device. + * @param bar BAR index to program (0–5 in a standard PCI header). + * @param bar_size Size of the BAR region in bytes (must be a power of two). + * @param layout Bitmask of PCI_BASE_ADDRESS_* flags defined in + * `/usr/include/linux/pci_regs.h`: + * - Bit 0: I/O space (1) vs. memory space (0) + * (`PCI_BASE_ADDRESS_SPACE_IO` or + * `PCI_BASE_ADDRESS_SPACE_MEMORY`) + * - Bits [2:1]: Memory decoding type + * (`PCI_BASE_ADDRESS_MEM_TYPE_32` or + * `PCI_BASE_ADDRESS_MEM_TYPE_64`) + * - Bit 3: Prefetchable flag for memory + * (`PCI_BASE_ADDRESS_MEM_PREFETCH`) + * @param do_io Callback (dev_io_fn) invoked on accesses within + * the BAR region. + * + * @note bar_size must be a power of two for correct decoding by the + * PCI framework. + * @note For 64-bit memory BARs, callers must reserve the next BAR index + * (n+1) for the high 32 bits if required by the platform. + */ void pci_set_bar(struct pci_dev *dev, uint8_t bar, uint32_t bar_size, - bool is_io_space, + uint32_t is_io_space, dev_io_fn do_io); void pci_set_status(struct pci_dev *dev, uint16_t status); void pci_dev_register(struct pci_dev *dev); diff --git a/src/virtio-pci.c b/src/virtio-pci.c index 7dede78..c3b6682 100644 --- a/src/virtio-pci.c +++ b/src/virtio-pci.c @@ -268,7 +268,9 @@ void virtio_pci_init(struct virtio_pci_dev *dev, PCI_HDR_WRITE(dev->pci_dev.hdr, PCI_HEADER_TYPE, PCI_HEADER_TYPE_NORMAL, 8); PCI_HDR_WRITE(dev->pci_dev.hdr, PCI_INTERRUPT_PIN, 1, 8); pci_set_status(&dev->pci_dev, PCI_STATUS_CAP_LIST | PCI_STATUS_INTERRUPT); - pci_set_bar(&dev->pci_dev, 0, 0x100, PCI_BASE_ADDRESS_SPACE_MEMORY, + pci_set_bar(&dev->pci_dev, 0, 0x100, + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32 + /* | PCI_BASE_ADDRESS_MEM_PREFETCH */, virtio_pci_space_io); virtio_pci_set_cap(dev, cap_list); dev->device_feature |= From 833b3d0321b07cebfb5cef19c240115b278d872c Mon Sep 17 00:00:00 2001 From: Otis Chung Date: Thu, 17 Jul 2025 17:46:10 +0800 Subject: [PATCH 2/2] Preserve full BAR layout across guest sizing This replaces bar_is_io_space[6] with bar_layout[6] so the full PCI_BASE_ADDRESS_* bitmask written by pci_set_bar() survives a guest BAR write. Previously only bit 0 (space) was retained; MEM_TYPE and PREFETCH bits were silently dropped on the first config-space probe, which meant pci_set_bar()'s new `layout` parameter was a partial lie. pci_config_bar() now restores all non-address bits via (bar_layout[bar] & ~mask) and assigns space_dev[bar].base to the masked address only, fixing a latent I/O BAR routing bug where the SPACE_IO bit in 'base' made bus_find_dev() miss the first port. Header parameter renamed is_io_space -> layout to match the implementation, and the magic 0x1U mask in pci_set_bar() now uses the standard PCI_BASE_ADDRESS_SPACE_IO constant. 64-bit BARs still need a paired adjacent slot for the upper dword; that limitation is now documented in pci_set_bar(). Co-authored-by: Jim Huang --- src/pci.c | 17 +++++++++++------ src/pci.h | 36 ++++++------------------------------ src/virtio-pci.c | 3 +-- 3 files changed, 18 insertions(+), 38 deletions(-) diff --git a/src/pci.c b/src/pci.c index aa4dccf..0ee38be 100644 --- a/src/pci.c +++ b/src/pci.c @@ -52,8 +52,9 @@ static void pci_command_bar(struct pci_dev *dev) bool enable_mem = PCI_HDR_READ(dev->hdr, PCI_COMMAND, 16) & PCI_COMMAND_MEMORY; for (int i = 0; i < PCI_STD_NUM_BARS; i++) { - struct bus *bus = dev->bar_is_io_space[i] ? dev->io_bus : dev->mmio_bus; - bool enable = dev->bar_is_io_space[i] ? enable_io : enable_mem; + bool is_io = dev->bar_layout[i] & PCI_BASE_ADDRESS_SPACE_IO; + struct bus *bus = is_io ? dev->io_bus : dev->mmio_bus; + bool enable = is_io ? enable_io : enable_mem; if (enable) pci_activate_bar(dev, i, bus); @@ -71,9 +72,9 @@ static void pci_config_bar(struct pci_dev *dev, uint8_t bar) { uint32_t mask = ~(dev->bar_size[bar] - 1); uint32_t old_bar = PCI_HDR_READ(dev->hdr, PCI_BAR_OFFSET(bar), 32); - uint32_t new_bar = (old_bar & mask) | dev->bar_is_io_space[bar]; + uint32_t new_bar = (old_bar & mask) | (dev->bar_layout[bar] & ~mask); PCI_HDR_WRITE(dev->hdr, PCI_BAR_OFFSET(bar), new_bar, 32); - dev->space_dev[bar].base = new_bar; + dev->space_dev[bar].base = new_bar & mask; } static void pci_config_write(struct pci_dev *dev, @@ -148,10 +149,14 @@ void pci_set_bar(struct pci_dev *dev, uint32_t layout, dev_io_fn do_io) { - /* FIXME: bar_size must be power of 2 */ + /* + * FIXME: bar_size must be a power of two. + * TODO: 64-bit BARs need a second adjacent slot for the upper dword; + * only 32-bit memory and I/O BARs are wired up here. + */ PCI_HDR_WRITE(dev->hdr, PCI_BAR_OFFSET(bar), layout, 32); dev->bar_size[bar] = bar_size; - dev->bar_is_io_space[bar] = layout & 0x1U; // Get the bit[0] of layout + dev->bar_layout[bar] = layout; dev_init(&dev->space_dev[bar], 0, bar_size, dev, do_io); } diff --git a/src/pci.h b/src/pci.h index ca9d322..17d73bc 100644 --- a/src/pci.h +++ b/src/pci.h @@ -30,7 +30,7 @@ struct pci_dev { void *hdr; uint32_t bar_size[6]; bool bar_active[6]; - bool bar_is_io_space[6]; + uint32_t bar_layout[6]; struct dev space_dev[6]; struct dev config_dev; struct bus *io_bus; @@ -46,39 +46,15 @@ struct pci { struct dev pci_mmio_dev; }; -/** - * @brief Configure and initialize a PCI Base Address Register (BAR). - * - * This writes the caller-provided layout bitmask into the BAR register - * in the PCI configuration header, records the region size and I/O type, - * and sets up the address space (MMIO or port I/O) with the specified - * callback. - * - * @param dev Pointer to the pci_dev representing the device. - * @param bar BAR index to program (0–5 in a standard PCI header). - * @param bar_size Size of the BAR region in bytes (must be a power of two). - * @param layout Bitmask of PCI_BASE_ADDRESS_* flags defined in - * `/usr/include/linux/pci_regs.h`: - * - Bit 0: I/O space (1) vs. memory space (0) - * (`PCI_BASE_ADDRESS_SPACE_IO` or - * `PCI_BASE_ADDRESS_SPACE_MEMORY`) - * - Bits [2:1]: Memory decoding type - * (`PCI_BASE_ADDRESS_MEM_TYPE_32` or - * `PCI_BASE_ADDRESS_MEM_TYPE_64`) - * - Bit 3: Prefetchable flag for memory - * (`PCI_BASE_ADDRESS_MEM_PREFETCH`) - * @param do_io Callback (dev_io_fn) invoked on accesses within - * the BAR region. - * - * @note bar_size must be a power of two for correct decoding by the - * PCI framework. - * @note For 64-bit memory BARs, callers must reserve the next BAR index - * (n+1) for the high 32 bits if required by the platform. +/* + * Configure a PCI BAR. @layout is a bitmask of PCI_BASE_ADDRESS_* flags + * from (space, mem type, prefetch). The non-address + * bits are preserved across guest BAR sizing probes. */ void pci_set_bar(struct pci_dev *dev, uint8_t bar, uint32_t bar_size, - uint32_t is_io_space, + uint32_t layout, dev_io_fn do_io); void pci_set_status(struct pci_dev *dev, uint16_t status); void pci_dev_register(struct pci_dev *dev); diff --git a/src/virtio-pci.c b/src/virtio-pci.c index c3b6682..ea25027 100644 --- a/src/virtio-pci.c +++ b/src/virtio-pci.c @@ -269,8 +269,7 @@ void virtio_pci_init(struct virtio_pci_dev *dev, PCI_HDR_WRITE(dev->pci_dev.hdr, PCI_INTERRUPT_PIN, 1, 8); pci_set_status(&dev->pci_dev, PCI_STATUS_CAP_LIST | PCI_STATUS_INTERRUPT); pci_set_bar(&dev->pci_dev, 0, 0x100, - PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32 - /* | PCI_BASE_ADDRESS_MEM_PREFETCH */, + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32, virtio_pci_space_io); virtio_pci_set_cap(dev, cap_list); dev->device_feature |=