[PATCH v2 1/2] hw/pci-host/pam.c: Fully support RE^WE semantics of i440FX PAM

[PATCH v2 1/2] hw/pci-host/pam.c: Fully support RE^WE semantics of i440FX PAM

[Lev Kujawski]

The Programmable Attribute Registers (PAM) of QEMU's emulated i440FX
chipset now fully support the exclusive Read Enable (RE) and Write
Enable (WE) modes by forwarding reads of the applicable PAM region to
RAM and writes to the bus or vice versa, respectively.

The prior behavior for the RE case was to setup a RAM alias and mark
it read-only, but no attempt was made to forward writes to the bus,
and read-only aliases of RAM do not prevent writes. Now, pam.c creates
a ROMD region (with read-only memory backing) coupled with a memory
operation that forwards writes to the bus.

For the WE case, a RAM alias was created, but with no attempt to
forward reads to the bus. Now, pam.c creates a MMIO region that writes
directly to RAM (bypassing the PAM region) and forwards reads to the
bus.

Additional changes:
- Change the type of pam_update parameter idx to type uint8_t,
  eliminating an assert check.
- Remove the fourth PAM alias, for normal RAM-based reads and writes
  of PAM regions, saving memory and clutter in mtree output.

Tested with SeaBIOS and AMIBIOS.

Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>
---
(v2) Write to an AddressSpace mapped over ram_memory instead of using
     a pointer, as it suprisingly may not be backed by RAM on, e.g.,
     NUMA configurations.

 hw/pci-host/pam.c         | 136 +++++++++++++++++++++++++++++++-------
 include/hw/pci-host/pam.h |   7 +-
 2 files changed, 118 insertions(+), 25 deletions(-)

diff --git a/hw/pci-host/pam.c b/hw/pci-host/pam.c
index 454dd120db..787428c7d8 100644
--- a/hw/pci-host/pam.c
+++ b/hw/pci-host/pam.c
@@ -28,43 +28,133 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "hw/pci-host/pam.h"
 
+static void
+pam_rmem_write(void *opaque, hwaddr addr, uint64_t val, unsigned int size)
+{
+    PAMMemoryRegion * const pam = (PAMMemoryRegion *)opaque;
+
+    (void)memory_region_dispatch_write(pam->pci_mr, pam->offset + addr, val,
+                                       size_memop(size), MEMTXATTRS_UNSPECIFIED);
+}
+
+static uint64_t
+pam_wmem_read(void *opaque, hwaddr addr, unsigned int size)
+{
+    PAMMemoryRegion * const pam = (PAMMemoryRegion *)opaque;
+    uint64_t val = (uint64_t)~0;
+
+    (void)memory_region_dispatch_read(pam->pci_mr, pam->offset + addr, &val,
+                                      size_memop(size), MEMTXATTRS_UNSPECIFIED);
+
+    return val;
+}
+
+static void
+pam_wmem_write(void *opaque, hwaddr addr, uint64_t val, unsigned int size)
+{
+    PAMMemoryRegion * const pam = (PAMMemoryRegion *)opaque;
+
+    switch (size) {
+    case 1:
+        stb_phys(&pam->ram_as, pam->offset + addr, val);
+        break;
+    case 2:
+        stw_le_phys(&pam->ram_as, pam->offset + addr, val);
+        break;
+    case 4:
+        stl_le_phys(&pam->ram_as, pam->offset + addr, val);
+        break;
+    case 8:
+        stq_le_phys(&pam->ram_as, pam->offset + addr, val);
+        break;
+    default:
+        g_assert_not_reached();
+    }
+}
+
+static const MemoryRegionOps pam_rmem_ops = {
+    .write = pam_rmem_write,
+};
+
+static const MemoryRegionOps pam_wmem_ops = {
+    .read = pam_wmem_read,
+    .write = pam_wmem_write,
+    .valid = {
+        .min_access_size = 1,
+        .max_access_size = 8,
+        .unaligned = true,
+    },
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 8,
+        .unaligned = true,
+    },
+};
+
 void init_pam(DeviceState *dev, MemoryRegion *ram_memory,
-              MemoryRegion *system_memory, MemoryRegion *pci_address_space,
-              PAMMemoryRegion *mem, uint32_t start, uint32_t size)
+              MemoryRegion *system, MemoryRegion *pci,
+              PAMMemoryRegion *pam, uint32_t start, uint32_t size)
 {
+    char name[12] = "pam-splitr";
     int i;
 
-    /* RAM */
-    memory_region_init_alias(&mem->alias[3], OBJECT(dev), "pam-ram", ram_memory,
-                             start, size);
-    /* ROM (XXX: not quite correct) */
-    memory_region_init_alias(&mem->alias[1], OBJECT(dev), "pam-rom", ram_memory,
-                             start, size);
-    memory_region_set_readonly(&mem->alias[1], true);
+    name[10] = (start >> 14) + 17;
+    name[11] = '\0';
+
+    /* Forward all memory accesses to the bus.  */
+    memory_region_init_alias(&pam->alias[0], OBJECT(dev), "pam-pci",
+                             pci, start, size);
 
-    /* XXX: should distinguish read/write cases */
-    memory_region_init_alias(&mem->alias[0], OBJECT(dev), "pam-pci", pci_address_space,
-                             start, size);
-    memory_region_init_alias(&mem->alias[2], OBJECT(dev), "pam-pci", ram_memory,
-                             start, size);
+    /* Split modes */
+    /* Forward reads to RAM, writes to the bus.  */
+    memory_region_init_rom_device(&pam->alias[1], OBJECT(dev),
+                                  &pam_rmem_ops, pam, name, size,
+                                  &error_fatal);
+
+    /* Forward writes to RAM, reads to the bus.  */
+    name[9] = 'w';
+    memory_region_init_io(&pam->alias[2], OBJECT(dev), &pam_wmem_ops,
+                          pam, name, size);
 
     memory_region_transaction_begin();
-    for (i = 0; i < 4; ++i) {
-        memory_region_set_enabled(&mem->alias[i], false);
-        memory_region_add_subregion_overlap(system_memory, start,
-                                            &mem->alias[i], 1);
+    for (i = 0; i < 3; ++i) {
+        /* The caller is responsible for the initial state.  */
+        memory_region_set_enabled(&pam->alias[i], false);
+        memory_region_add_subregion_overlap(system, start,
+                                            &pam->alias[i], 1);
     }
     memory_region_transaction_commit();
-    mem->current = 0;
+    address_space_init(&pam->ram_as, ram_memory, "PAMRAM");
+    pam->current = 0;
+    pam->pci_mr = pci;
+    pam->offset = start;
 }
 
-void pam_update(PAMMemoryRegion *pam, int idx, uint8_t val)
+void pam_update(PAMMemoryRegion *pam, uint8_t idx, uint8_t val)
 {
-    assert(0 <= idx && idx < PAM_REGIONS_COUNT);
+    uint8_t ai;
+    assert(idx < PAM_REGIONS_COUNT);
+
+    ai = (val >> ((!(idx & 1)) * 4)) & PAM_ATTR_MASK;
 
+    /* The caller is responsible for setting up a transaction.  */
     memory_region_set_enabled(&pam->alias[pam->current], false);
-    pam->current = (val >> ((!(idx & 1)) * 4)) & PAM_ATTR_MASK;
-    memory_region_set_enabled(&pam->alias[pam->current], true);
+    switch (ai) {
+    case 1: {
+        const hwaddr pamsize = memory_region_size(&pam->alias[ai]);
+
+        address_space_read(&pam->ram_as, pam->offset,
+                           MEMTXATTRS_UNSPECIFIED,
+                           memory_region_get_ram_ptr(&pam->alias[ai]),
+                           pamsize);
+    }
+    /* FALLTHROUGH */
+    case 0:
+    case 2:
+        memory_region_set_enabled(&pam->alias[ai], true);
+        pam->current = ai;
+    }
 }
diff --git a/include/hw/pci-host/pam.h b/include/hw/pci-host/pam.h
index c1fd06ba2a..6f12210498 100644
--- a/include/hw/pci-host/pam.h
+++ b/include/hw/pci-host/pam.h
@@ -83,12 +83,15 @@
 #define PAM_REGIONS_COUNT       13
 
 typedef struct PAMMemoryRegion {
-    MemoryRegion alias[4];  /* index = PAM value */
+    AddressSpace ram_as;
+    MemoryRegion alias[3];  /* index = PAM value */
     unsigned current;
+    ram_addr_t offset;
+    MemoryRegion *pci_mr;
 } PAMMemoryRegion;
 
 void init_pam(DeviceState *dev, MemoryRegion *ram, MemoryRegion *system,
               MemoryRegion *pci, PAMMemoryRegion *mem, uint32_t start, uint32_t size);
-void pam_update(PAMMemoryRegion *mem, int idx, uint8_t val);
+void pam_update(PAMMemoryRegion *mem, uint8_t idx, uint8_t val);
 
 #endif /* QEMU_PAM_H */
-- 
2.34.1

[PATCH v2 2/2] tests/qtest/i440fx-test.c: Enable full test of i440FX PAM operation

[Lev Kujawski]

With the prior patch in this series adding support for RE^WE PAM
semantics, the '#ifndef BROKEN' segments of test_i440fx_pam can now be
enabled.

Additionally:
- Verify that changing attributes does not affect the initial contents
  of the PAM region;
- Verify that that the first new mask is written before switching
  attributes;
- Switch back to PAM_RE after PAM_WE to read original contents;
- Tighten logic of the !WE write test because we know what the
  original contents were; and
- Write the last mask before testing for it.

Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>
---
(v2) No change.

 tests/qtest/i440fx-test.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/tests/qtest/i440fx-test.c b/tests/qtest/i440fx-test.c
index 6d7d4d8d8f..073a16bbed 100644
--- a/tests/qtest/i440fx-test.c
+++ b/tests/qtest/i440fx-test.c
@@ -236,33 +236,34 @@ static void test_i440fx_pam(gconstpointer opaque)
 
         /* Switch to WE for the area */
         pam_set(dev, i, PAM_RE | PAM_WE);
+        /* Verify the RAM is still all zeros */
+        g_assert(verify_area(pam_area[i].start, pam_area[i].end, 0));
         /* Write out a non-zero mask to the full area */
         write_area(pam_area[i].start, pam_area[i].end, 0x42);
-
-#ifndef BROKEN
-        /* QEMU only supports a limited form of PAM */
+        /* Verify the area contains the new mask */
+        g_assert(verify_area(pam_area[i].start, pam_area[i].end, 0x42));
 
         /* Switch to !RE for the area */
         pam_set(dev, i, PAM_WE);
         /* Verify the area is not our mask */
         g_assert(!verify_area(pam_area[i].start, pam_area[i].end, 0x42));
-#endif
 
-        /* Verify the area is our new mask */
+        /* Switch to !WE for the area */
+        pam_set(dev, i, PAM_RE);
+        /* Verify the area is once again our mask */
         g_assert(verify_area(pam_area[i].start, pam_area[i].end, 0x42));
 
         /* Write out a new mask */
         write_area(pam_area[i].start, pam_area[i].end, 0x82);
 
-#ifndef BROKEN
-        /* QEMU only supports a limited form of PAM */
-
-        /* Verify the area is not our mask */
-        g_assert(!verify_area(pam_area[i].start, pam_area[i].end, 0x82));
+        /* Verify the area is not the new mask */
+        g_assert(verify_area(pam_area[i].start, pam_area[i].end, 0x42));
 
         /* Switch to RE for the area */
         pam_set(dev, i, PAM_RE | PAM_WE);
-#endif
+        /* Write out a new mask again */
+        write_area(pam_area[i].start, pam_area[i].end, 0x82);
+
         /* Verify the area is our new mask */
         g_assert(verify_area(pam_area[i].start, pam_area[i].end, 0x82));
 
-- 
2.34.1

Re: [PATCH v2 2/2] tests/qtest/i440fx-test.c: Enable full test of i440FX PAM operation

[Thomas Huth]

On 16/06/2022 12.05, Lev Kujawski wrote:
> With the prior patch in this series adding support for RE^WE PAM
> semantics, the '#ifndef BROKEN' segments of test_i440fx_pam can now be
> enabled.
> 
> Additionally:
> - Verify that changing attributes does not affect the initial contents
>    of the PAM region;
> - Verify that that the first new mask is written before switching
>    attributes;
> - Switch back to PAM_RE after PAM_WE to read original contents;
> - Tighten logic of the !WE write test because we know what the
>    original contents were; and
> - Write the last mask before testing for it.
> 
> Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>
> ---
> (v2) No change.
> 
>   tests/qtest/i440fx-test.c | 23 ++++++++++++-----------
>   1 file changed, 12 insertions(+), 11 deletions(-)

Acked-by: Thomas Huth <thuth@redhat.com>