Currently, all viewport memory regions are created upfront in the realize phase.
This has several drawbacks: First, two MemoryRegion members are needed per
viewport while maximum only one is ever used at a time. Second, for inbound
viewports, the `cfg` member is never used nor initialized. Third, adding
support for other types of memory such as I/O space would require adding even
more MemoryRegion members, one for each type. Fix these limiations by having
just one MemoryRegion member which gets configured on demand.
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
include/hw/pci-host/designware.h | 3 +-
hw/pci-host/designware.c | 181 ++++++++++++++-----------------
2 files changed, 81 insertions(+), 103 deletions(-)
diff --git a/include/hw/pci-host/designware.h b/include/hw/pci-host/designware.h
index a35a3bd06c..7dc8af049d 100644
--- a/include/hw/pci-host/designware.h
+++ b/include/hw/pci-host/designware.h
@@ -40,8 +40,7 @@ struct DesignwarePCIERootBus {
typedef struct DesignwarePCIEViewport {
DesignwarePCIERoot *root;
-
- MemoryRegion cfg;
+ const char *name;
MemoryRegion mem;
uint64_t base;
diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index 3a10dc9f99..7d47d8228f 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -272,33 +272,54 @@ static const MemoryRegionOps designware_pci_host_conf_ops = {
static void designware_pcie_update_viewport(DesignwarePCIERoot *root,
DesignwarePCIEViewport *viewport)
{
+ DesignwarePCIEHost *host = designware_pcie_root_to_host(root);
const uint64_t target = viewport->target;
const uint64_t base = viewport->base;
const uint64_t size = (uint64_t)viewport->limit - base + 1;
const bool enabled = viewport->cr[1] & DESIGNWARE_PCIE_ATU_ENABLE;
- MemoryRegion *current, *other;
-
- if (viewport->cr[0] == DESIGNWARE_PCIE_ATU_TYPE_MEM) {
- current = &viewport->mem;
- other = &viewport->cfg;
- memory_region_set_alias_offset(current, target);
- } else {
- current = &viewport->cfg;
- other = &viewport->mem;
+ if (memory_region_is_mapped(&viewport->mem)) {
+ memory_region_del_subregion(viewport->mem.container, &viewport->mem);
}
+ object_unparent(OBJECT(&viewport->mem));
- /*
- * An outbound viewport can be reconfigure from being MEM to CFG,
- * to account for that we disable the "other" memory region that
- * becomes unused due to that fact.
- */
- memory_region_set_enabled(other, false);
if (enabled) {
- memory_region_set_size(current, size);
- memory_region_set_address(current, base);
+ if (viewport->cr[0] == DESIGNWARE_PCIE_ATU_TYPE_MEM) {
+ if (viewport->inbound) {
+ /*
+ * Configure MemoryRegion implementing PCI -> CPU memory
+ * access
+ */
+ memory_region_init_alias(&viewport->mem, OBJECT(root),
+ viewport->name, get_system_memory(),
+ target, size);
+ memory_region_add_subregion_overlap(&host->pci.address_space_root,
+ base, &viewport->mem, -1);
+ } else {
+ /*
+ * Configure MemoryRegion implementing CPU -> PCI memory
+ * access
+ */
+ memory_region_init_alias(&viewport->mem, OBJECT(root),
+ viewport->name, &host->pci.memory,
+ target, size);
+ memory_region_add_subregion(get_system_memory(), base,
+ &viewport->mem);
+ }
+ } else {
+ if (!viewport->inbound) {
+ /*
+ * Configure MemoryRegion implementing access to configuration
+ * space
+ */
+ memory_region_init_io(&viewport->mem, OBJECT(root),
+ &designware_pci_host_conf_ops,
+ viewport, viewport->name, size);
+ memory_region_add_subregion(get_system_memory(), base,
+ &viewport->mem);
+ }
+ }
}
- memory_region_set_enabled(current, enabled);
}
static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
@@ -378,27 +399,16 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
}
}
-static char *designware_pcie_viewport_name(const char *direction,
- unsigned int i,
- const char *type)
-{
- return g_strdup_printf("PCI %s Viewport %u [%s]",
- direction, i, type);
-}
-
static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
{
DesignwarePCIERoot *root = DESIGNWARE_PCIE_ROOT(dev);
DesignwarePCIEHost *host = designware_pcie_root_to_host(root);
PCIBridge *br = PCI_BRIDGE(dev);
- DesignwarePCIEViewport *viewport;
/*
* Dummy values used for initial configuration of MemoryRegions
* that belong to a given viewport
*/
const hwaddr dummy_offset = 0;
- const uint64_t dummy_size = 4;
- size_t i;
br->bus_name = "dw-pcie";
@@ -416,80 +426,49 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
msi_nonbroken = true;
msi_init(dev, 0x50, 32, true, true, &error_fatal);
- for (i = 0; i < DESIGNWARE_PCIE_NUM_VIEWPORTS; i++) {
- const char *direction;
- char *name;
-
- viewport = &root->viewports[DESIGNWARE_PCIE_VIEWPORT_INBOUND][i];
- viewport->inbound = true;
- viewport->base = 0x0000000000000000ULL;
- viewport->target = 0x0000000000000000ULL;
- viewport->limit = UINT32_MAX;
- viewport->cr[0] = DESIGNWARE_PCIE_ATU_TYPE_MEM;
-
- direction = "Inbound";
-
- /*
- * Configure MemoryRegion implementing PCI -> CPU memory
- * access
- */
- name = designware_pcie_viewport_name(direction, i, "MEM");
- memory_region_init_alias(&viewport->mem, OBJECT(root), name,
- get_system_memory(), dummy_offset, dummy_size);
- memory_region_add_subregion_overlap(&host->pci.address_space_root,
- dummy_offset, &viewport->mem, -1);
- memory_region_set_enabled(&viewport->mem, false);
- g_free(name);
-
- viewport = &root->viewports[DESIGNWARE_PCIE_VIEWPORT_OUTBOUND][i];
- viewport->root = root;
- viewport->inbound = false;
- viewport->base = 0x0000000000000000ULL;
- viewport->target = 0x0000000000000000ULL;
- viewport->limit = UINT32_MAX;
- viewport->cr[0] = DESIGNWARE_PCIE_ATU_TYPE_MEM;
-
- direction = "Outbound";
-
- /*
- * Configure MemoryRegion implementing CPU -> PCI memory
- * access
- */
- name = designware_pcie_viewport_name(direction, i, "MEM");
- memory_region_init_alias(&viewport->mem, OBJECT(root), name,
- &host->pci.memory, dummy_offset, dummy_size);
- memory_region_add_subregion(get_system_memory(), dummy_offset,
- &viewport->mem);
- memory_region_set_enabled(&viewport->mem, false);
- g_free(name);
-
- /*
- * Configure MemoryRegion implementing access to configuration
- * space
- */
- name = designware_pcie_viewport_name(direction, i, "CFG");
- memory_region_init_io(&viewport->cfg, OBJECT(root),
- &designware_pci_host_conf_ops,
- viewport, name, dummy_size);
- memory_region_add_subregion(get_system_memory(), dummy_offset,
- &viewport->cfg);
- memory_region_set_enabled(&viewport->cfg, false);
- g_free(name);
+ for (size_t i = 0; i < ARRAY_SIZE(root->viewports); i++) {
+ static const char *names[][DESIGNWARE_PCIE_NUM_VIEWPORTS] = {
+ {
+ "PCI Outbound Viewport 0",
+ "PCI Outbound Viewport 1",
+ "PCI Outbound Viewport 2",
+ "PCI Outbound Viewport 3"
+ },
+ {
+ "PCI Inbound Viewport 0",
+ "PCI Inbound Viewport 1",
+ "PCI Inbound Viewport 2",
+ "PCI Inbound Viewport 3"
+ },
+ };
+
+ for (size_t j = 0; j < DESIGNWARE_PCIE_NUM_VIEWPORTS; j++) {
+ DesignwarePCIEViewport *viewport = &root->viewports[i][j];
+ viewport->root = root;
+ viewport->name = names[i][j];
+ viewport->inbound = i == DESIGNWARE_PCIE_VIEWPORT_INBOUND;
+ viewport->base = 0x0000000000000000ULL;
+ viewport->target = 0x0000000000000000ULL;
+ viewport->limit = UINT32_MAX;
+ viewport->cr[0] = DESIGNWARE_PCIE_ATU_TYPE_MEM;
+
+ /*
+ * If no inbound iATU windows are configured, HW defaults to
+ * letting inbound TLPs to pass in. We emulate that by explicitly
+ * configuring first inbound window to cover all of target's
+ * address space.
+ *
+ * NOTE: This will not work correctly for the case when first
+ * configured inbound window is window 0
+ */
+ viewport->cr[1] = (viewport->inbound && j == 0)
+ ? DESIGNWARE_PCIE_ATU_ENABLE
+ : 0;
+
+ designware_pcie_update_viewport(root, viewport);
+ }
}
- /*
- * If no inbound iATU windows are configured, HW defaults to
- * letting inbound TLPs to pass in. We emulate that by explicitly
- * configuring first inbound window to cover all of target's
- * address space.
- *
- * NOTE: This will not work correctly for the case when first
- * configured inbound window is window 0
- */
- viewport = &root->viewports[DESIGNWARE_PCIE_VIEWPORT_INBOUND][0];
- viewport->cr[1] = DESIGNWARE_PCIE_ATU_ENABLE;
- designware_pcie_update_viewport(root, viewport);
-
memory_region_init_io(&root->msi.iomem, OBJECT(root),
&designware_pci_host_msi_ops,
root, "pcie-msi", 0x4);
--
2.50.1On Wed, 20 Aug 2025 at 22:19, Bernhard Beschow <shentey@gmail.com> wrote: > > Currently, all viewport memory regions are created upfront in the realize phase. > This has several drawbacks: First, two MemoryRegion members are needed per > viewport while maximum only one is ever used at a time. Second, for inbound > viewports, the `cfg` member is never used nor initialized. Third, adding > support for other types of memory such as I/O space would require adding even > more MemoryRegion members, one for each type. Fix these limiations by having > just one MemoryRegion member which gets configured on demand. On the other hand, docs/devel/memory.rst says: > as a general rule do not create or destroy memory regions dynamically > during a device's lifetime with a flat ban on doing this for MRs that aren't container or alias MRs, and even though it makes an exception for the container/alias case it says "Exploiting this exception is rarely necessary, and therefore it is discouraged". (I also find it harder to reason about, because it means I have to think about MR lifetimes and references and so on, whereas "create MRs at realize and they exist as long as the device does" is easy because it's the same way every other device works.) thanks -- PMM
© 2016 - 2025 Red Hat, Inc.