From: Titus Rwantare <titusr@google.com>

 This switches to a using a fully sized PCI memory region that's
 separate from system memory. Accesses to this PCI memory region are
 gated by the AXI to PCIe windows whose size and offsets are validated.

 - PCIe config space is not necessarily aliased with PCIe mmio space.
   Ignore translation addresses for config space windows.
 - Make window configuration register writes order independent.

 Tested with pci-testdev.

Signed-off-by: Titus Rwantare <titusr@google.com>
---
 hw/arm/npcm8xx.c                  |   1 -
 hw/pci-host/npcm_pcierc.c         | 156 ++++++++++++++++++++++++++------------
 include/hw/pci-host/npcm_pcierc.h |   9 ++-
 3 files changed, 115 insertions(+), 51 deletions(-)

diff --git a/hw/arm/npcm8xx.c b/hw/arm/npcm8xx.c
index f7a5ae2d121ffec99c519b484503e71dc8a43695..504874c99e7d12afa92268786239ca946e8e2129 100644
--- a/hw/arm/npcm8xx.c
+++ b/hw/arm/npcm8xx.c
@@ -773,7 +773,6 @@ static void npcm8xx_realize(DeviceState *dev, Error **errp)
     /* PCIe RC */
     sysbus_realize(SYS_BUS_DEVICE(&s->pcierc), &error_abort);
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->pcierc), 0, NPCM8XX_PCIERC_BA);
-    sysbus_mmio_map(SYS_BUS_DEVICE(&s->pcierc), 1, NPCM8XX_PCIE_ROOT_BA);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->pcierc), 0,
                        npcm8xx_irq(s, NPCM8XX_PCIE_RC_IRQ));
 
diff --git a/hw/pci-host/npcm_pcierc.c b/hw/pci-host/npcm_pcierc.c
index 0af76d1067a78bdbb169af3e3d5c4a2514cd0ff5..3aab7d401a7be8c1b14a476ed934f521b8dfdaa7 100644
--- a/hw/pci-host/npcm_pcierc.c
+++ b/hw/pci-host/npcm_pcierc.c
@@ -17,63 +17,123 @@
 #include "qom/object.h"
 #include "trace.h"
 
+
+#define NPCM_SAL            BIT(0)
+#define NPCM_SAH            BIT(1)
+#define NPCM_TAL            BIT(2)
+#define NPCM_TAH            BIT(3)
+#define NPCM_PARAMS         BIT(4)
+#define NPCM_BITFIELDS_ALL  0x1f
+
+
+static bool npcm_pcierc_valid_window_addr(hwaddr addr, uint32_t size)
+{
+    if ((addr + size) > NPCM_PCIE_HOLE_END) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: window mapping @0x%lx, size: %d is invalid.\n",
+                      __func__, addr, size);
+        return false;
+    } else if (addr < NPCM_PCIE_HOLE) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: window mapping @0x%lx, is invalid.\n",
+                      __func__, addr);
+        return false;
+    } else {
+        return true;
+    }
+};
+
+static bool npcm_pcierc_valid_window_size(hwaddr src, hwaddr dst, uint32_t size)
+{
+    if (size > 2 * GiB || size < 4 * KiB) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Invalid PCI window size %d bytes\n",
+                      __func__, size);
+        return false;
+    }
+
+    return true;
+}
+
 /* Map enabled windows to a memory subregion */
 static void npcm_pcierc_map_enabled(NPCMPCIERCState *s, NPCMPCIEWindow *w)
 {
     MemoryRegion *system = get_system_memory();
     uint32_t size = NPCM_PCIERC_SAL_SIZE(w->sal);
-    hwaddr bar = ((uint64_t)w->sah) << 32 | (w->sal & 0xFFFFF000);
+    hwaddr src_ba = ((uint64_t)w->sah) << 32 | (w->sal & 0xFFFFF000);
+    hwaddr dest_ba = ((uint64_t)w->tah) << 32 | w->tal;
     char name[26];
 
-    /* check if window is enabled */
-    if (!(w->sal & NPCM_PCIERC_SAL_EN)) {
+    if (!(w->sal & NPCM_PCIERC_SAL_EN) || /* ignore disabled windows */
+        !npcm_pcierc_valid_window_size(src_ba, dest_ba, size) ||
+        memory_region_is_mapped(&w->mem) /* ignore existing windows */) {
         return;
     }
 
-    if (size > 2 * GiB || size < 4 * KiB) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "%s: Invalid PCI window size %d bytes\n",
-                      __func__, size);
+    /* bitfield for all 5 registers required to create a PCIe window */
+    if (w->set_fields != NPCM_BITFIELDS_ALL) {
         return;
     }
+    w->set_fields = 0;
 
+    /*
+     * This implementation of the Nuvoton root complex uses memory region
+     * aliasing to emulate the behaviour of the windowing system on hardware.
+     * AXI to PCIe windows in QEMU are system_memory subregions aliased to PCI
+     * memory at the respective source and translation addresses
+     * PCIe to AXI windows are done as PCI memory subregions aliased to system
+     * memory. PCIe to AXI windows have no address restrictions.
+     */
     if (w->type == AXI2PCIE) {
+        if (!npcm_pcierc_valid_window_addr(src_ba, size)) {
+            return;
+        };
         snprintf(name, sizeof(name), "npcm-axi2pcie-window-%d", w->id);
+        if (w->params &
+            (NPCM_PCIERC_TRSF_PARAM_CONFIG | NPCM_PCIERC_TRSL_ID_CONFIG)) {
+            memory_region_init_alias(&w->mem, OBJECT(s), name,
+                                     &s->rp_config, 0, size);
+        } else {
+            memory_region_init_alias(&w->mem, OBJECT(s), name,
+                                     &s->pcie_memory, dest_ba, size);
+        }
+        memory_region_add_subregion(system, src_ba, &w->mem);
     } else if (w->type == PCIE2AXI) {
         snprintf(name, sizeof(name), "npcm-pcie2axi-window-%d", w->id);
+        memory_region_init_alias(&w->mem, OBJECT(s), name,
+                                 system, src_ba, size);
+        memory_region_add_subregion(&s->pcie_memory, dest_ba, &w->mem);
     } else {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: unable to map uninitialized PCIe window",
                       __func__);
         return;
     }
-
-    /* TODO: set subregion to target translation address */
-    /* add subregion starting at the window source address */
-    if (!memory_region_is_mapped(&w->mem)) {
-        memory_region_init(&w->mem, OBJECT(s), name, size);
-        memory_region_add_subregion(system, bar, &w->mem);
-    }
 }
 
 /* unmap windows marked as disabled */
-static void npcm_pcierc_unmap_disabled(NPCMPCIEWindow *w)
+static void npcm_pcierc_unmap_disabled(NPCMPCIERCState *s, NPCMPCIEWindow *w)
 {
     MemoryRegion *system = get_system_memory();
+
     /* Bit 0 in the Source address enables the window */
     if (memory_region_is_mapped(&w->mem) && !(w->sal & NPCM_PCIERC_SAL_EN)) {
-        memory_region_del_subregion(system, &w->mem);
+        if (w->type == AXI2PCIE) {
+            memory_region_del_subregion(system, &w->mem);
+        } else {
+            memory_region_del_subregion(&s->pcie_memory, &w->mem);
+        }
     }
 }
 
 static void npcm_pcie_update_window_maps(NPCMPCIERCState *s)
 {
     for (int i = 0; i < NPCM_PCIERC_NUM_PA_WINDOWS; i++) {
-        npcm_pcierc_unmap_disabled(&s->pcie2axi[i]);
+        npcm_pcierc_unmap_disabled(s, &s->pcie2axi[i]);
     }
 
     for (int i = 0; i < NPCM_PCIERC_NUM_AP_WINDOWS; i++) {
-        npcm_pcierc_unmap_disabled(&s->axi2pcie[i]);
+        npcm_pcierc_unmap_disabled(s, &s->axi2pcie[i]);
     }
 
     for (int i = 0; i < NPCM_PCIERC_NUM_AP_WINDOWS; i++) {
@@ -177,22 +237,27 @@ static void npcm_pcierc_write_window(NPCMPCIERCState *s, hwaddr addr,
     switch (offset) {
     case NPCM_PCIERC_SAL_OFFSET:
         window->sal = data;
+        window->set_fields |= NPCM_SAL;
         break;
 
     case NPCM_PCIERC_SAH_OFFSET:
         window->sah = data;
+        window->set_fields |= NPCM_SAH;
         break;
 
     case NPCM_PCIERC_TAL_OFFSET:
         window->tal = data;
+        window->set_fields |= NPCM_TAL;
         break;
 
     case NPCM_PCIERC_TAH_OFFSET:
         window->tah = data;
+        window->set_fields |= NPCM_TAH;
         break;
 
     case NPCM_PCIERC_PARAM_OFFSET:
         window->params = data;
+        window->set_fields |= NPCM_PARAMS;
         break;
 
     default:
@@ -305,7 +370,7 @@ static uint64_t npcm_pcie_host_config_read(void *opaque, hwaddr addr,
     PCIDevice *pcid = pci_find_device(pcih->bus, bus, devfn);
 
     if (pcid) {
-        return pci_host_config_read_common(pcid, addr,
+        return pci_host_config_read_common(pcid, (addr & 0x7FF),
                                            pci_config_size(pcid),
                                            size);
     }
@@ -323,7 +388,7 @@ static void npcm_pcie_host_config_write(void *opaque, hwaddr addr,
     PCIDevice *pcid = pci_find_device(pcih->bus, bus, devfn);
 
     if (pcid) {
-        pci_host_config_write_common(pcid, addr,
+        pci_host_config_write_common(pcid, (addr & 0x7FF),
                                      pci_config_size(pcid),
                                      data,
                                      size);
@@ -413,40 +478,43 @@ static void npcm_pcie_set_irq(void *opaque, int irq_num, int level)
 static void npcm_pcierc_realize(DeviceState *dev, Error **errp)
 {
     NPCMPCIERCState *s = NPCM_PCIERC(dev);
+    PCIHostState *phs = PCI_HOST_BRIDGE(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
-    PCIHostState *pci = PCI_HOST_BRIDGE(dev);
     PCIDevice *root = pci_new(PCI_DEVFN(0, 0), TYPE_NPCM_PCIE_ROOT_PORT);
 
-    memory_region_init_io(&s->mmio, OBJECT(s), &npcm_pcierc_cfg_ops,
-                          s, TYPE_NPCM_PCIERC, 4 * KiB);
-    sysbus_init_mmio(sbd, &s->mmio);
-    sysbus_init_irq(sbd, &s->irq);
+    /* init the underlying memory region for all PCI address space */
+    memory_region_init(&s->pcie_memory, OBJECT(s), "npcm-pcie-mem", UINT64_MAX);
 
-    /* IO memory region is needed to create a PCI bus, but is unused on ARM */
+    /* I/O memory region is needed to create a PCI bus, but is unused on ARM */
     memory_region_init(&s->pcie_io, OBJECT(s), "npcm-pcie-io", 16);
 
-    /*
-     * pcie_root is a 128 MiB memory region in the BMC physical address space
-     * in which all PCIe windows must have their programmable source or
-     * destination address
-     */
-    memory_region_init_io(&s->pcie_root, OBJECT(s), &npcm_pcie_cfg_space_ops,
-                          s, "npcm-pcie-config", 128 * MiB);
-    sysbus_init_mmio(sbd, &s->pcie_root);
-
-    pci->bus = pci_register_root_bus(dev, "pcie",
+    phs->bus = pci_register_root_bus(dev, "pcie",
                                      npcm_pcie_set_irq,
                                      pci_swizzle_map_irq_fn,
-                                     s, &s->pcie_root, &s->pcie_io,
+                                     s, &s->pcie_memory, &s->pcie_io,
                                      0, 4, TYPE_PCIE_BUS);
 
-    address_space_init(&s->pcie_space, &s->pcie_root, "pcie-address-space");
-    pci_realize_and_unref(root, pci->bus, &error_fatal);
-    pci_setup_iommu(pci->bus, &npcm_pcierc_iommu_ops, s);
+    address_space_init(&s->pcie_space, &s->pcie_memory, "pcie-address-space");
+    pci_setup_iommu(phs->bus, &npcm_pcierc_iommu_ops, s);
+    /* init region for root complex registers (not config space) */
+    memory_region_init_io(&s->rc_regs, OBJECT(s), &npcm_pcierc_cfg_ops,
+                          s, TYPE_NPCM_PCIERC, 4 * KiB);
+    sysbus_init_mmio(sbd, &s->rc_regs);
+    sysbus_init_irq(sbd, &s->irq);
+
+    /* create and add region for the root port in config space */
+    memory_region_init_io(&s->rp_config, OBJECT(s),
+                          &npcm_pcie_cfg_space_ops, s, "npcm-pcie-config",
+                          4 * KiB);
 
+    /* realize the root port */
+    pci_realize_and_unref(root, phs->bus, &error_fatal);
+    /* enable MSI (non-X) in root port config space */
     msi_nonbroken = true;
     msi_init(root, NPCM_PCIERC_MSI_OFFSET, NPCM_PCIERC_MSI_NR,
              true, true, errp);
+
+    npcm_pcierc_reset_pcie_windows(s);
 }
 
 static void npcm_pcie_root_port_realize(DeviceState *dev, Error **errp)
@@ -461,13 +529,6 @@ static void npcm_pcie_root_port_realize(DeviceState *dev, Error **errp)
     }
 }
 
-static void npcm_pcierc_instance_init(Object *obj)
-{
-    NPCMPCIERCState *s = NPCM_PCIERC(obj);
-
-    npcm_pcierc_reset_pcie_windows(s);
-}
-
 static void npcm_pcierc_class_init(ObjectClass *klass, const void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -507,7 +568,6 @@ static const TypeInfo npcm_pcierc_type_info = {
     .name = TYPE_NPCM_PCIERC,
     .parent = TYPE_PCIE_HOST_BRIDGE,
     .instance_size = sizeof(NPCMPCIERCState),
-    .instance_init = npcm_pcierc_instance_init,
     .class_init = npcm_pcierc_class_init,
 };
 
diff --git a/include/hw/pci-host/npcm_pcierc.h b/include/hw/pci-host/npcm_pcierc.h
index 7d18177510f60d49f7fae7908dd1e3bfbe9ae12b..a986e7666abadd8c0bb97ac5e10853339f0fe815 100644
--- a/include/hw/pci-host/npcm_pcierc.h
+++ b/include/hw/pci-host/npcm_pcierc.h
@@ -96,6 +96,9 @@
 #define TYPE_NPCM_PCIERC "npcm-pcie-root-complex"
 OBJECT_DECLARE_SIMPLE_TYPE(NPCMPCIERCState, NPCM_PCIERC)
 
+#define NPCM_PCIE_HOLE       (0xe8000000)
+#define NPCM_PCIE_HOLE_END   (0xe8000000 + (128 * MiB))
+
 typedef enum {
     AXI2PCIE = 1,
     PCIE2AXI
@@ -111,6 +114,7 @@ typedef struct NPCMPCIEWindow {
 
     MemoryRegion mem;        /* QEMU memory subregion per window */
     NPCMPCIEWindowType type; /* translation direction */
+    uint8_t set_fields;
     uint8_t id;
 } NPCMPCIEWindow;
 
@@ -127,7 +131,7 @@ struct NPCMPCIERCState {
     qemu_irq irq;
 
     /* PCIe RC registers */
-    MemoryRegion mmio;
+    MemoryRegion rc_regs;
     uint32_t rccfgnum;
     uint32_t rcinten;
     uint32_t rcintstat;
@@ -137,8 +141,9 @@ struct NPCMPCIERCState {
 
     /* Address translation state */
     AddressSpace pcie_space;
-    MemoryRegion pcie_root;
+    MemoryRegion pcie_memory;
     MemoryRegion pcie_io; /* unused - but required for IO space PCI */
+    MemoryRegion rp_config;
     NPCMPCIERootPort port;
     /* PCIe to AXI Windows */
     NPCMPCIEWindow pcie2axi[NPCM_PCIERC_NUM_PA_WINDOWS];

-- 
2.51.0.384.g4c02a37b29-goog

On Tue, 9 Sept 2025 at 23:11, Yubin Zou <yubinz@google.com> wrote:
>
> From: Titus Rwantare <titusr@google.com>
>
>  This switches to a using a fully sized PCI memory region that's
>  separate from system memory. Accesses to this PCI memory region are
>  gated by the AXI to PCIe windows whose size and offsets are validated.
>
>  - PCIe config space is not necessarily aliased with PCIe mmio space.
>    Ignore translation addresses for config space windows.
>  - Make window configuration register writes order independent.
>
>  Tested with pci-testdev.

I'm in general not a fan of introducing something in
one patch and then "reworking" it in a later patch in
the same series. It's usually easier to understand and
review if you implement it the right way the first time.

thanks
-- PMM

Loop in more Nuvoton folks.

-----Original Message-----
From: Peter Maydell <peter.maydell@linaro.org>
Sent: Friday, September 26, 2025 12:40 AM
To: Yubin Zou <yubinz@google.com>
Cc: qemu-devel@nongnu.org; Paolo Bonzini <pbonzini@redhat.com>; CS20 KFTing <KFTING@nuvoton.com>; Hao Wu <wuhaotsh@google.com>; qemu-arm@nongnu.org; Titus Rwantare <titusr@google.com>
Subject: Re: [PATCH 6/7] hw/pci-host: rework Nuvoton PCIe windowing and memory regions

CAUTION - External Email: Do not click links or open attachments unless you acknowledge the sender and content.


On Tue, 9 Sept 2025 at 23:11, Yubin Zou <yubinz@google.com> wrote:
>
> From: Titus Rwantare <titusr@google.com>
>
>  This switches to a using a fully sized PCI memory region that's
> separate from system memory. Accesses to this PCI memory region are
> gated by the AXI to PCIe windows whose size and offsets are validated.
>
>  - PCIe config space is not necessarily aliased with PCIe mmio space.
>    Ignore translation addresses for config space windows.
>  - Make window configuration register writes order independent.
>
>  Tested with pci-testdev.

I'm in general not a fan of introducing something in one patch and then "reworking" it in a later patch in the same series. It's usually easier to understand and review if you implement it the right way the first time.

thanks
-- PMM
________________________________
________________________________
 The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Nuvoton is strictly prohibited; and any information in this email irrelevant to the official business of Nuvoton shall be deemed as neither given nor endorsed by Nuvoton.