[PATCH v2 0/7] hw/misc: Add memory_region_add_subregion_aliased() helper [pflash part]

[PATCH v2 0/7] hw/misc: Add memory_region_add_subregion_aliased() helper [pflash part]

[Philippe Mathieu-Daudé]

Hi,

This series introduce the memory_region_add_subregion_aliased()
helper which basically create a device which maps a subregion
multiple times.

Since v1:
- Split series in 2, keeping the I/O regions (showed with the q800
  machine) part for 2nd part
- Added R-b tags

Examples are easier, so having a subregion aliased every @span_size
then mapped onto a container at an offset, you get something like:

          ^-----------^
          |           |
          |           |
          | +-------+ |                 +---------+          <--+
          |           |                 +---------+             |
          |           |                 |         |             |
          |           |   +-----------> | alias#3 |             |
          |           |   |             |         |             |
          |           |   |             +---------+             |
          |           |   |             +---------+             |
          |           |   |             |         |             |
          |           |   |   +-------> | alias#2 |             |
          |           |   |   |         |         |             |region
          | container |   |   |         +---------+             | size
          |           |   |   |         +---------+             |
          |           |   |   |         |         |             |
          |           |   |   |  +----> | alias#1 |             |
          |           |   |   |  |      |         |             |
          |           |   |   |  |      +---------+  <--+       |
          |           | +-+---+--+--+   +---------+     |       |
          |           | |           |   |         |     |span   |
          |           | | subregion +-> | alias#0 |     |size   |
   offset |           | |           |   |         |     |       |
   +----> | +-------+ | +-----------+   +---------+  <--+    <--+
   |      |           |
   |      |           |
   |      |           |
   |      |           |
   |      |           |
   |      ^-----------^

I know it need more documentation and tests, but I prefer to send
as draft RFC for early review before spending more time on it.

Supersedes: <20210326002728.1069834-1-f4bug@amsat.org>
Based-on: <20210325120921.858993-1-f4bug@amsat.org>
https://www.mail-archive.com/qemu-devel@nongnu.org/msg795218.html

Philippe Mathieu-Daudé (7):
  hw/misc: Add device to help managing aliased memory regions
  hw/arm/musicpal: Open-code pflash_cfi02_register() call
  hw/arm/musicpal: Map flash using memory_region_add_subregion_aliased()
  hw/arm/digic: Open-code pflash_cfi02_register() call
  hw/arm/digic: Map flash using memory_region_add_subregion_aliased()
  hw/block/pflash_cfi02: Remove pflash_setup_mappings()
  hw/block/pflash_cfi02: Simplify pflash_cfi02_register() prototype

 include/hw/block/flash.h         |   1 -
 include/hw/misc/aliased_region.h |  87 ++++++++++++++++++++
 hw/arm/digic_boards.c            |  28 +++++--
 hw/arm/musicpal.c                |  29 +++++--
 hw/arm/xilinx_zynq.c             |   2 +-
 hw/block/pflash_cfi02.c          |  36 +--------
 hw/lm32/lm32_boards.c            |   4 +-
 hw/misc/aliased_region.c         | 132 +++++++++++++++++++++++++++++++
 hw/ppc/ppc405_boards.c           |   6 +-
 hw/sh4/r2d.c                     |   2 +-
 MAINTAINERS                      |   6 ++
 hw/arm/Kconfig                   |   2 +
 hw/misc/Kconfig                  |   3 +
 hw/misc/meson.build              |   1 +
 14 files changed, 285 insertions(+), 54 deletions(-)
 create mode 100644 include/hw/misc/aliased_region.h
 create mode 100644 hw/misc/aliased_region.c

-- 
2.26.3

[RFC PATCH v2 1/7] hw/misc: Add device to help managing aliased memory regions

[Philippe Mathieu-Daudé]

// TODO explain here how buses work? when some address lines are
// not bound we get memory aliasing, high addresses are masked.
// etc...

Add a helper to manage this use case easily.

For example a having @span_size = @region_size / 4 we get such mapping:

          ^-----------^
          |           |
          |           |
          | +-------+ |                 +---------+          <--+
          |           |                 +---------+             |
          |           |                 |         |             |
          |           |   +-----------> | alias#3 |             |
          |           |   |             |         |             |
          |           |   |             +---------+             |
          |           |   |             +---------+             |
          |           |   |             |         |             |
          |           |   |   +-------> | alias#2 |             |
          |           |   |   |         |         |             |region
          | container |   |   |         +---------+             | size
          |           |   |   |         +---------+             |
          |           |   |   |         |         |             |
          |           |   |   |  +----> | alias#1 |             |
          |           |   |   |  |      |         |             |
          |           |   |   |  |      +---------+  <--+       |
          |           | +-+---+--+--+   +---------+     |       |
          |           | |           |   |         |     |span   |
          |           | | subregion +-> | alias#0 |     |size   |
   offset |           | |           |   |         |     |       |
   +----> | +-------+ | +-----------+   +---------+  <--+    <--+
   |      |           |
   |      |           |
   |      |           |
   |      |           |
   |      |           |
   |      ^-----------^

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Not really RFC, simply that I'v to add the technical description,
but I'd like to know if there could be a possibility to not accept
this device (because I missed something) before keeping working on
it. So far it is only used in hobbyist boards.

Cc: Peter Xu <peterx@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 include/hw/misc/aliased_region.h |  87 ++++++++++++++++++++
 hw/misc/aliased_region.c         | 132 +++++++++++++++++++++++++++++++
 MAINTAINERS                      |   6 ++
 hw/misc/Kconfig                  |   3 +
 hw/misc/meson.build              |   1 +
 5 files changed, 229 insertions(+)
 create mode 100644 include/hw/misc/aliased_region.h
 create mode 100644 hw/misc/aliased_region.c

diff --git a/include/hw/misc/aliased_region.h b/include/hw/misc/aliased_region.h
new file mode 100644
index 00000000000..0ce0d5d1cef
--- /dev/null
+++ b/include/hw/misc/aliased_region.h
@@ -0,0 +1,87 @@
+/*
+ * Aliased memory regions
+ *
+ * Copyright (c) 2018  Philippe Mathieu-Daudé <f4bug@amsat.org>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef HW_MISC_ALIASED_REGION_H
+#define HW_MISC_ALIASED_REGION_H
+
+#include "exec/memory.h"
+#include "hw/sysbus.h"
+
+#define TYPE_ALIASED_REGION "aliased-memory-region"
+OBJECT_DECLARE_SIMPLE_TYPE(AliasedRegionState, ALIASED_REGION)
+
+struct AliasedRegionState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+
+    /*< public >*/
+    MemoryRegion container;
+    uint64_t region_size;
+    uint64_t span_size;
+    MemoryRegion *mr;
+
+    struct {
+        size_t count;
+        MemoryRegion *alias;
+    } mem;
+};
+
+/**
+ * memory_region_add_subregion_aliased:
+ * @container: the #MemoryRegion to contain the aliased subregions.
+ * @offset: the offset relative to @container where the aliased subregion
+ *          are added.
+ * @region_size: size of the region containing the aliased subregions.
+ * @subregion: the subregion to be aliased.
+ * @span_size: size between each aliased subregion
+ *
+ * This utility function creates and maps an instance of aliased-memory-region,
+ * which is a dummy device of a single region which simply contains multiple
+ * aliases of the provided @subregion, spanned over the @region_size every
+ * @span_size. The device is mapped at @offset within @container.
+ *
+ * For example a having @span_size = @region_size / 4 we get such mapping:
+ *
+ *               +-----------+
+ *               |           |
+ *               |           |
+ *               | +-------+ |                 +---------+          <--+
+ *               |           |                 +---------+             |
+ *               |           |                 |         |             |
+ *               |           |   +-----------> | alias#3 |             |
+ *               |           |   |             |         |             |
+ *               |           |   |             +---------+             |
+ *               |           |   |             +---------+             |
+ *               |           |   |             |         |             |
+ *               |           |   |   +-------> | alias#2 |             |
+ *               |           |   |   |         |         |             |region
+ *               | container |   |   |         +---------+             | size
+ *               |           |   |   |         +---------+             |
+ *               |           |   |   |         |         |             |
+ *               |           |   |   |  +----> | alias#1 |             |
+ *               |           |   |   |  |      |         |             |
+ *               |           |   |   |  |      +---------+  <--+       |
+ *               |           | +-+---+--+--+   +---------+     |       |
+ *               |           | |           |   |         |     |span   |
+ *               |           | | subregion +-> | alias#0 |     |size   |
+ *        offset |           | |           |   |         |     |       |
+ *        +----> | +-------+ | +-----------+   +---------+  <--+    <--+
+ *        |      |           |
+ *        |      |           |
+ *        |      |           |
+ *        |      |           |
+ *        |      |           |
+ *        +      +-----------+
+ */
+void memory_region_add_subregion_aliased(MemoryRegion *container,
+                                         hwaddr offset,
+                                         uint64_t region_size,
+                                         MemoryRegion *subregion,
+                                         uint64_t span_size);
+
+#endif
diff --git a/hw/misc/aliased_region.c b/hw/misc/aliased_region.c
new file mode 100644
index 00000000000..3132276af29
--- /dev/null
+++ b/hw/misc/aliased_region.c
@@ -0,0 +1,132 @@
+/*
+ * Aliased memory regions
+ *
+ * Copyright (c) 2018  Philippe Mathieu-Daudé <f4bug@amsat.org>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/cutils.h"
+#include "qapi/error.h"
+#include "hw/sysbus.h"
+#include "hw/misc/aliased_region.h"
+#include "hw/qdev-properties.h"
+
+static void aliased_mem_realize(AliasedRegionState *s, const char *mr_name)
+{
+    uint64_t subregion_size;
+    int subregion_bits;
+
+    memory_region_init(&s->container, OBJECT(s), mr_name, s->region_size);
+
+    subregion_bits = 64 - clz64(s->span_size - 1);
+    s->mem.count = s->region_size >> subregion_bits;
+    assert(s->mem.count > 1);
+    subregion_size = 1ULL << subregion_bits;
+
+    s->mem.alias = g_new(MemoryRegion, s->mem.count);
+    for (size_t i = 0; i < s->mem.count; i++) {
+        g_autofree char *name = g_strdup_printf("%s [#%zu/%zu]",
+                                                memory_region_name(s->mr),
+                                                i, s->mem.count);
+        memory_region_init_alias(&s->mem.alias[i], OBJECT(s), name,
+                                 s->mr, 0, s->span_size);
+        memory_region_add_subregion(&s->container, i * subregion_size,
+                                    &s->mem.alias[i]);
+    }
+}
+
+static void aliased_mr_realize(DeviceState *dev, Error **errp)
+{
+    AliasedRegionState *s = ALIASED_REGION(dev);
+    g_autofree char *name = NULL, *span = NULL;
+
+    if (s->region_size == 0) {
+        error_setg(errp, "property 'region-size' not specified or zero");
+        return;
+    }
+
+    if (s->mr == NULL) {
+        error_setg(errp, "property 'iomem' not specified");
+        return;
+    }
+
+    if (!s->span_size) {
+        s->span_size = pow2ceil(memory_region_size(s->mr));
+    } else if (!is_power_of_2(s->span_size)) {
+        error_setg(errp, "property 'span-size' must be a power of 2");
+        return;
+    }
+
+    span = size_to_str(s->span_size);
+    name = g_strdup_printf("masked %s [span of %s]",
+                           memory_region_name(s->mr), span);
+    aliased_mem_realize(s, name);
+    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->container);
+}
+
+static void aliased_mr_unrealize(DeviceState *dev)
+{
+    AliasedRegionState *s = ALIASED_REGION(dev);
+
+    g_free(s->mem.alias);
+}
+
+static Property aliased_mr_properties[] = {
+    DEFINE_PROP_UINT64("region-size", AliasedRegionState, region_size, 0),
+    DEFINE_PROP_UINT64("span-size", AliasedRegionState, span_size, 0),
+    DEFINE_PROP_LINK("iomem", AliasedRegionState, mr,
+                     TYPE_MEMORY_REGION, MemoryRegion *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void aliased_mr_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = aliased_mr_realize;
+    dc->unrealize = aliased_mr_unrealize;
+    /* Reason: needs to be wired up to work */
+    dc->user_creatable = false;
+    device_class_set_props(dc, aliased_mr_properties);
+}
+
+static const TypeInfo aliased_mr_info = {
+    .name = TYPE_ALIASED_REGION,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(AliasedRegionState),
+    .class_init = aliased_mr_class_init,
+};
+
+static void aliased_mr_register_types(void)
+{
+    type_register_static(&aliased_mr_info);
+}
+
+type_init(aliased_mr_register_types)
+
+void memory_region_add_subregion_aliased(MemoryRegion *container,
+                                         hwaddr offset,
+                                         uint64_t region_size,
+                                         MemoryRegion *subregion,
+                                         uint64_t span_size)
+{
+    DeviceState *dev;
+
+    if (!region_size) {
+        region_size = pow2ceil(memory_region_size(container));
+    } else {
+        assert(region_size <= memory_region_size(container));
+    }
+
+    dev = qdev_new(TYPE_ALIASED_REGION);
+    qdev_prop_set_uint64(dev, "region-size", region_size);
+    qdev_prop_set_uint64(dev, "span-size", span_size);
+    object_property_set_link(OBJECT(dev), "iomem", OBJECT(subregion),
+                             &error_abort);
+    sysbus_realize(SYS_BUS_DEVICE(dev), &error_abort);
+
+    memory_region_add_subregion(container, offset,
+                                sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0));
+}
diff --git a/MAINTAINERS b/MAINTAINERS
index 36055f14c59..151c342e338 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2095,6 +2095,12 @@ S: Maintained
 F: include/hw/misc/empty_slot.h
 F: hw/misc/empty_slot.c
 
+Aliased memory region
+M: Philippe Mathieu-Daudé <f4bug@amsat.org>
+S: Maintained
+F: include/hw/misc/aliased_region.h
+F: hw/misc/aliased_region.c
+
 Standard VGA
 M: Gerd Hoffmann <kraxel@redhat.com>
 S: Maintained
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index c71ed258204..ca51b99989e 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -151,6 +151,9 @@ config AUX
 config UNIMP
     bool
 
+config ALIASED_REGION
+    bool
+
 config LED
     bool
 
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 21034dc60a8..e65541b835f 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -12,6 +12,7 @@
 softmmu_ss.add(when: 'CONFIG_EMC141X', if_true: files('emc141x.c'))
 softmmu_ss.add(when: 'CONFIG_UNIMP', if_true: files('unimp.c'))
 softmmu_ss.add(when: 'CONFIG_EMPTY_SLOT', if_true: files('empty_slot.c'))
+softmmu_ss.add(when: 'CONFIG_ALIASED_REGION', if_true: files('aliased_region.c'))
 softmmu_ss.add(when: 'CONFIG_LED', if_true: files('led.c'))
 softmmu_ss.add(when: 'CONFIG_PVPANIC_COMMON', if_true: files('pvpanic.c'))
 
-- 
2.26.3

[PATCH v2 2/7] hw/arm/musicpal: Open-code pflash_cfi02_register() call

[Philippe Mathieu-Daudé]

To be able to manually map the flash region on the main memory
(in the next commit), first expand the pflash_cfi02_register
in place.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/musicpal.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index 9cebece2de0..8b58b66f263 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -10,6 +10,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/units.h"
 #include "qapi/error.h"
 #include "cpu.h"
 #include "hw/sysbus.h"
@@ -1640,6 +1641,7 @@ static void musicpal_init(MachineState *machine)
     /* Register flash */
     dinfo = drive_get(IF_PFLASH, 0, 0);
     if (dinfo) {
+        static const size_t sector_size = 64 * KiB;
         BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
 
         flash_size = blk_getlength(blk);
@@ -1649,17 +1651,30 @@ static void musicpal_init(MachineState *machine)
             exit(1);
         }
 
+        dev = qdev_new(TYPE_PFLASH_CFI02);
+        qdev_prop_set_drive(dev, "drive", blk);
+        qdev_prop_set_uint32(dev, "num-blocks", flash_size / sector_size);
+        qdev_prop_set_uint32(dev, "sector-length", sector_size);
+        qdev_prop_set_uint8(dev, "width", 2); /* 16-bit */
+        qdev_prop_set_uint8(dev, "mappings", MP_FLASH_SIZE_MAX / flash_size);
+        qdev_prop_set_uint8(dev, "big-endian", 0);
+        qdev_prop_set_uint16(dev, "id0", 0x00bf);
+        qdev_prop_set_uint16(dev, "id1", 0x236d);
+        qdev_prop_set_uint16(dev, "id2", 0x0000);
+        qdev_prop_set_uint16(dev, "id3", 0x0000);
+        qdev_prop_set_uint16(dev, "unlock-addr0", 0x5555);
+        qdev_prop_set_uint16(dev, "unlock-addr1", 0x2aaa);
+        qdev_prop_set_string(dev, "name", "musicpal.flash");
+        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+
+        sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0,
+                        0x100000000ULL - MP_FLASH_SIZE_MAX);
+
         /*
          * The original U-Boot accesses the flash at 0xFE000000 instead of
          * 0xFF800000 (if there is 8 MB flash). So remap flash access if the
          * image is smaller than 32 MB.
          */
-        pflash_cfi02_register(0x100000000ULL - MP_FLASH_SIZE_MAX,
-                              "musicpal.flash", flash_size,
-                              blk, 0x10000,
-                              MP_FLASH_SIZE_MAX / flash_size,
-                              2, 0x00BF, 0x236D, 0x0000, 0x0000,
-                              0x5555, 0x2AAA, 0);
     }
     sysbus_create_simple(TYPE_MV88W8618_FLASHCFG, MP_FLASHCFG_BASE, NULL);
 
-- 
2.26.3

[PATCH v2 3/7] hw/arm/musicpal: Map flash using memory_region_add_subregion_aliased()

[Philippe Mathieu-Daudé]

Instead of using a device specific feature for mapping the
flash memory multiple times over a wider region, use the
generic memory_region_add_subregion_aliased() helper.

There is no change in the memory layout:

- before:

  (qemu) info mtree
  00000000fe000000-00000000ffffffff (prio 0, i/o): pflash
    00000000fe000000-00000000fe7fffff (prio 0, romd): alias pflash-alias @musicpal.flash 0000000000000000-00000000007fffff
    00000000fe800000-00000000feffffff (prio 0, romd): alias pflash-alias @musicpal.flash 0000000000000000-00000000007fffff
    00000000ff000000-00000000ff7fffff (prio 0, romd): alias pflash-alias @musicpal.flash 0000000000000000-00000000007fffff
    00000000ff800000-00000000ffffffff (prio 0, romd): alias pflash-alias @musicpal.flash 0000000000000000-00000000007fffff

- after:

  00000000fe000000-00000000ffffffff (prio 0, i/o): masked musicpal.flash [span of 8 MiB]
    00000000fe000000-00000000fe7fffff (prio 0, romd): alias musicpal.flash [#0/4] @musicpal.flash 0000000000000000-00000000007fffff
    00000000fe800000-00000000feffffff (prio 0, romd): alias musicpal.flash [#1/4] @musicpal.flash 0000000000000000-00000000007fffff
    00000000ff000000-00000000ff7fffff (prio 0, romd): alias musicpal.flash [#2/4] @musicpal.flash 0000000000000000-00000000007fffff
    00000000ff800000-00000000ffffffff (prio 0, romd): alias musicpal.flash [#3/4] @musicpal.flash 0000000000000000-00000000007fffff

Flatview is the same:

  (qemu) info mtree -f
  FlatView #0
   AS "memory", root: system
   AS "cpu-memory-0", root: system
   AS "emac-dma", root: system
   Root memory region: system
    00000000fe000000-00000000fe7fffff (prio 0, romd): musicpal.flash
    00000000fe800000-00000000feffffff (prio 0, romd): musicpal.flash
    00000000ff000000-00000000ff7fffff (prio 0, romd): musicpal.flash
    00000000ff800000-00000000ffffffff (prio 0, romd): musicpal.flash

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/musicpal.c | 11 +++++++----
 hw/arm/Kconfig    |  1 +
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index 8b58b66f263..7d1f2f3fb3f 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -30,6 +30,7 @@
 #include "hw/irq.h"
 #include "hw/or-irq.h"
 #include "hw/audio/wm8750.h"
+#include "hw/misc/aliased_region.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/runstate.h"
 #include "sysemu/dma.h"
@@ -1656,7 +1657,7 @@ static void musicpal_init(MachineState *machine)
         qdev_prop_set_uint32(dev, "num-blocks", flash_size / sector_size);
         qdev_prop_set_uint32(dev, "sector-length", sector_size);
         qdev_prop_set_uint8(dev, "width", 2); /* 16-bit */
-        qdev_prop_set_uint8(dev, "mappings", MP_FLASH_SIZE_MAX / flash_size);
+        qdev_prop_set_uint8(dev, "mappings", 0);
         qdev_prop_set_uint8(dev, "big-endian", 0);
         qdev_prop_set_uint16(dev, "id0", 0x00bf);
         qdev_prop_set_uint16(dev, "id1", 0x236d);
@@ -1667,14 +1668,16 @@ static void musicpal_init(MachineState *machine)
         qdev_prop_set_string(dev, "name", "musicpal.flash");
         sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
-        sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0,
-                        0x100000000ULL - MP_FLASH_SIZE_MAX);
-
         /*
          * The original U-Boot accesses the flash at 0xFE000000 instead of
          * 0xFF800000 (if there is 8 MB flash). So remap flash access if the
          * image is smaller than 32 MB.
          */
+        memory_region_add_subregion_aliased(get_system_memory(),
+                                0x100000000ULL - MP_FLASH_SIZE_MAX,
+                                MP_FLASH_SIZE_MAX,
+                                sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0),
+                                flash_size);
     }
     sysbus_create_simple(TYPE_MV88W8618_FLASHCFG, MP_FLASHCFG_BASE, NULL);
 
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 8c37cf00da7..aa8553b3cd3 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -101,6 +101,7 @@ config MUSICPAL
     select MARVELL_88W8618
     select PTIMER
     select PFLASH_CFI02
+    select ALIASED_REGION
     select SERIAL
     select WM8750
 
-- 
2.26.3

[PATCH v2 4/7] hw/arm/digic: Open-code pflash_cfi02_register() call

[Philippe Mathieu-Daudé]

To be able to manually map the flash region on the main memory
(in the next commit), first expand the pflash_cfi02_register
in place.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/digic_boards.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/hw/arm/digic_boards.c b/hw/arm/digic_boards.c
index 6cdc1d83fca..fc4a671b2e1 100644
--- a/hw/arm/digic_boards.c
+++ b/hw/arm/digic_boards.c
@@ -31,6 +31,8 @@
 #include "hw/boards.h"
 #include "exec/address-spaces.h"
 #include "qemu/error-report.h"
+#include "hw/qdev-properties.h"
+#include "hw/misc/aliased_region.h"
 #include "hw/arm/digic.h"
 #include "hw/block/flash.h"
 #include "hw/loader.h"
@@ -120,12 +122,25 @@ static void digic4_add_k8p3215uqb_rom(DigicState *s, hwaddr addr,
 #define FLASH_K8P3215UQB_SIZE (4 * 1024 * 1024)
 #define FLASH_K8P3215UQB_SECTOR_SIZE (64 * 1024)
 
-    pflash_cfi02_register(addr, "pflash", FLASH_K8P3215UQB_SIZE,
-                          NULL, FLASH_K8P3215UQB_SECTOR_SIZE,
-                          DIGIC4_ROM_MAX_SIZE / FLASH_K8P3215UQB_SIZE,
-                          4,
-                          0x00EC, 0x007E, 0x0003, 0x0001,
-                          0x0555, 0x2aa, 0);
+    DeviceState *dev = qdev_new(TYPE_PFLASH_CFI02);
+
+    qdev_prop_set_uint32(dev, "num-blocks",
+                         FLASH_K8P3215UQB_SIZE / FLASH_K8P3215UQB_SECTOR_SIZE);
+    qdev_prop_set_uint32(dev, "sector-length", FLASH_K8P3215UQB_SECTOR_SIZE);
+    qdev_prop_set_uint8(dev, "width", 4); /* 32-bit */
+    qdev_prop_set_uint8(dev, "mappings",
+                        DIGIC4_ROM_MAX_SIZE / FLASH_K8P3215UQB_SIZE);
+    qdev_prop_set_uint8(dev, "big-endian", 0);
+    qdev_prop_set_uint16(dev, "id0", 0x00ec);
+    qdev_prop_set_uint16(dev, "id1", 0x007e);
+    qdev_prop_set_uint16(dev, "id2", 0x0003);
+    qdev_prop_set_uint16(dev, "id3", 0x0001);
+    qdev_prop_set_uint16(dev, "unlock-addr0", 0x0555);
+    qdev_prop_set_uint16(dev, "unlock-addr1", 0x2aa);
+    qdev_prop_set_string(dev, "name", "pflash");
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, addr);
 
     digic_load_rom(s, addr, FLASH_K8P3215UQB_SIZE, filename);
 }
-- 
2.26.3

[PATCH v2 5/7] hw/arm/digic: Map flash using memory_region_add_subregion_aliased()

[Philippe Mathieu-Daudé]

Instead of using a device specific feature for mapping the
flash memory multiple times over a wider region, use the
generic memory_region_add_subregion_aliased() helper.

There is no change in the memory layout.

* before:

  $ qemu-system-arm -M canon-a1100 -S -monitor stdio
  QEMU 5.2.90 monitor - type 'help' for more information
  (qemu) info mtree
  address-space: memory
    0000000000000000-ffffffffffffffff (prio 0, i/o): system
      0000000000000000-0000000003ffffff (prio 0, ram): ram
      00000000c0210000-00000000c02100ff (prio 0, i/o): digic-timer
      00000000c0210100-00000000c02101ff (prio 0, i/o): digic-timer
      00000000c0210200-00000000c02102ff (prio 0, i/o): digic-timer
      00000000c0800000-00000000c0800017 (prio 0, i/o): digic-uart
      00000000f8000000-00000000ffffffff (prio 0, i/o): pflash
        00000000f8000000-00000000f83fffff (prio 0, romd): alias pflash-alias @pflash 0000000000000000-00000000003fffff
        00000000f8400000-00000000f87fffff (prio 0, romd): alias pflash-alias @pflash 0000000000000000-00000000003fffff
        00000000f8800000-00000000f8bfffff (prio 0, romd): alias pflash-alias @pflash 0000000000000000-00000000003fffff
        ...
        00000000ff400000-00000000ff7fffff (prio 0, romd): alias pflash-alias @pflash 0000000000000000-00000000003fffff
        00000000ff800000-00000000ffbfffff (prio 0, romd): alias pflash-alias @pflash 0000000000000000-00000000003fffff
        00000000ffc00000-00000000ffffffff (prio 0, romd): alias pflash-alias @pflash 0000000000000000-00000000003fffff

* after:

  (qemu) info mtree
  address-space: memory
    0000000000000000-ffffffffffffffff (prio 0, i/o): system
      0000000000000000-0000000003ffffff (prio 0, ram): ram
      00000000c0210000-00000000c02100ff (prio 0, i/o): digic-timer
      00000000c0210100-00000000c02101ff (prio 0, i/o): digic-timer
      00000000c0210200-00000000c02102ff (prio 0, i/o): digic-timer
      00000000c0800000-00000000c0800017 (prio 0, i/o): digic-uart
      00000000f8000000-00000000ffffffff (prio 0, i/o): masked pflash [span of 4 MiB]
        00000000f8000000-00000000f83fffff (prio 0, romd): alias pflash [#0/32] @pflash 0000000000000000-00000000003fffff
        00000000f8400000-00000000f87fffff (prio 0, romd): alias pflash [#1/32] @pflash 0000000000000000-00000000003fffff
        00000000f8800000-00000000f8bfffff (prio 0, romd): alias pflash [#2/32] @pflash 0000000000000000-00000000003fffff
        ...
        00000000ff400000-00000000ff7fffff (prio 0, romd): alias pflash [#29/32] @pflash 0000000000000000-00000000003fffff
        00000000ff800000-00000000ffbfffff (prio 0, romd): alias pflash [#30/32] @pflash 0000000000000000-00000000003fffff
        00000000ffc00000-00000000ffffffff (prio 0, romd): alias pflash [#31/32] @pflash 0000000000000000-00000000003fffff

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/digic_boards.c | 8 +++++---
 hw/arm/Kconfig        | 1 +
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/arm/digic_boards.c b/hw/arm/digic_boards.c
index fc4a671b2e1..293402b1240 100644
--- a/hw/arm/digic_boards.c
+++ b/hw/arm/digic_boards.c
@@ -128,8 +128,7 @@ static void digic4_add_k8p3215uqb_rom(DigicState *s, hwaddr addr,
                          FLASH_K8P3215UQB_SIZE / FLASH_K8P3215UQB_SECTOR_SIZE);
     qdev_prop_set_uint32(dev, "sector-length", FLASH_K8P3215UQB_SECTOR_SIZE);
     qdev_prop_set_uint8(dev, "width", 4); /* 32-bit */
-    qdev_prop_set_uint8(dev, "mappings",
-                        DIGIC4_ROM_MAX_SIZE / FLASH_K8P3215UQB_SIZE);
+    qdev_prop_set_uint8(dev, "mappings", 0);
     qdev_prop_set_uint8(dev, "big-endian", 0);
     qdev_prop_set_uint16(dev, "id0", 0x00ec);
     qdev_prop_set_uint16(dev, "id1", 0x007e);
@@ -140,7 +139,10 @@ static void digic4_add_k8p3215uqb_rom(DigicState *s, hwaddr addr,
     qdev_prop_set_string(dev, "name", "pflash");
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
-    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, addr);
+    memory_region_add_subregion_aliased(get_system_memory(),
+                            addr, DIGIC4_ROM_MAX_SIZE,
+                            sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0),
+                            FLASH_K8P3215UQB_SIZE);
 
     digic_load_rom(s, addr, FLASH_K8P3215UQB_SIZE, filename);
 }
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index aa8553b3cd3..1a7b9724d6c 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -42,6 +42,7 @@ config DIGIC
     bool
     select PTIMER
     select PFLASH_CFI02
+    select ALIASED_REGION
 
 config EXYNOS4
     bool
-- 
2.26.3

[PATCH v2 6/7] hw/block/pflash_cfi02: Remove pflash_setup_mappings()

[Philippe Mathieu-Daudé]

All boards calling pflash_cfi02_register() use nb_mappings=1,
which does not do any mapping:

  $ git grep -wl pflash_cfi02_register hw/
  hw/arm/xilinx_zynq.c
  hw/block/pflash_cfi02.c
  hw/lm32/lm32_boards.c
  hw/ppc/ppc405_boards.c
  hw/sh4/r2d.c

We can remove this now unneeded code.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/block/pflash_cfi02.c | 35 ++---------------------------------
 1 file changed, 2 insertions(+), 33 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 02c514fb6e0..6f4b3e3c3fe 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -75,7 +75,6 @@ struct PFlashCFI02 {
     uint32_t nb_blocs[PFLASH_MAX_ERASE_REGIONS];
     uint32_t sector_len[PFLASH_MAX_ERASE_REGIONS];
     uint32_t chip_len;
-    uint8_t mappings;
     uint8_t width;
     uint8_t be;
     int wcycle; /* if 0, the flash is read normally */
@@ -92,13 +91,6 @@ struct PFlashCFI02 {
     uint16_t unlock_addr1;
     uint8_t cfi_table[0x4d];
     QEMUTimer timer;
-    /*
-     * The device replicates the flash memory across its memory space.  Emulate
-     * that by having a container (.mem) filled with an array of aliases
-     * (.mem_mappings) pointing to the flash memory (.orig_mem).
-     */
-    MemoryRegion mem;
-    MemoryRegion *mem_mappings;    /* array; one per mapping */
     MemoryRegion orig_mem;
     bool rom_mode;
     int read_counter; /* used for lazy switch-back to rom mode */
@@ -158,23 +150,6 @@ static inline void toggle_dq2(PFlashCFI02 *pfl)
     pfl->status ^= 0x04;
 }
 
-/*
- * Set up replicated mappings of the same region.
- */
-static void pflash_setup_mappings(PFlashCFI02 *pfl)
-{
-    unsigned i;
-    hwaddr size = memory_region_size(&pfl->orig_mem);
-
-    memory_region_init(&pfl->mem, OBJECT(pfl), "pflash", pfl->mappings * size);
-    pfl->mem_mappings = g_new(MemoryRegion, pfl->mappings);
-    for (i = 0; i < pfl->mappings; ++i) {
-        memory_region_init_alias(&pfl->mem_mappings[i], OBJECT(pfl),
-                                 "pflash-alias", &pfl->orig_mem, 0, size);
-        memory_region_add_subregion(&pfl->mem, i * size, &pfl->mem_mappings[i]);
-    }
-}
-
 static void pflash_reset_state_machine(PFlashCFI02 *pfl)
 {
     trace_pflash_reset(pfl->name);
@@ -917,12 +892,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
     pfl->sector_erase_map = bitmap_new(pfl->total_sectors);
 
     pfl->rom_mode = true;
-    if (pfl->mappings > 1) {
-        pflash_setup_mappings(pfl);
-        sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
-    } else {
-        sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->orig_mem);
-    }
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->orig_mem);
 
     timer_init_ns(&pfl->timer, QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
     pfl->status = 0;
@@ -950,7 +920,6 @@ static Property pflash_cfi02_properties[] = {
     DEFINE_PROP_UINT32("num-blocks3", PFlashCFI02, nb_blocs[3], 0),
     DEFINE_PROP_UINT32("sector-length3", PFlashCFI02, sector_len[3], 0),
     DEFINE_PROP_UINT8("width", PFlashCFI02, width, 0),
-    DEFINE_PROP_UINT8("mappings", PFlashCFI02, mappings, 0),
     DEFINE_PROP_UINT8("big-endian", PFlashCFI02, be, 0),
     DEFINE_PROP_UINT16("id0", PFlashCFI02, ident0, 0),
     DEFINE_PROP_UINT16("id1", PFlashCFI02, ident1, 0),
@@ -1008,6 +977,7 @@ PFlashCFI02 *pflash_cfi02_register(hwaddr base,
 {
     DeviceState *dev = qdev_new(TYPE_PFLASH_CFI02);
 
+    assert(nb_mappings <= 1);
     if (blk) {
         qdev_prop_set_drive(dev, "drive", blk);
     }
@@ -1015,7 +985,6 @@ PFlashCFI02 *pflash_cfi02_register(hwaddr base,
     qdev_prop_set_uint32(dev, "num-blocks", size / sector_len);
     qdev_prop_set_uint32(dev, "sector-length", sector_len);
     qdev_prop_set_uint8(dev, "width", width);
-    qdev_prop_set_uint8(dev, "mappings", nb_mappings);
     qdev_prop_set_uint8(dev, "big-endian", !!be);
     qdev_prop_set_uint16(dev, "id0", id0);
     qdev_prop_set_uint16(dev, "id1", id1);
-- 
2.26.3

[PATCH v2 7/7] hw/block/pflash_cfi02: Simplify pflash_cfi02_register() prototype

[Philippe Mathieu-Daudé]

The previous commit removed the mapping code from TYPE_PFLASH_CFI02.
pflash_cfi02_register() doesn't use the 'nb_mappings' argument
anymore. Simply remove it to simplify.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/block/flash.h | 1 -
 hw/arm/digic_boards.c    | 1 -
 hw/arm/musicpal.c        | 1 -
 hw/arm/xilinx_zynq.c     | 2 +-
 hw/block/pflash_cfi02.c  | 3 +--
 hw/lm32/lm32_boards.c    | 4 ++--
 hw/ppc/ppc405_boards.c   | 6 +++---
 hw/sh4/r2d.c             | 2 +-
 8 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 7dde0adcee7..0e5dd818a9d 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -36,7 +36,6 @@ PFlashCFI02 *pflash_cfi02_register(hwaddr base,
                                    hwaddr size,
                                    BlockBackend *blk,
                                    uint32_t sector_len,
-                                   int nb_mappings,
                                    int width,
                                    uint16_t id0, uint16_t id1,
                                    uint16_t id2, uint16_t id3,
diff --git a/hw/arm/digic_boards.c b/hw/arm/digic_boards.c
index 293402b1240..eb694c70d4c 100644
--- a/hw/arm/digic_boards.c
+++ b/hw/arm/digic_boards.c
@@ -128,7 +128,6 @@ static void digic4_add_k8p3215uqb_rom(DigicState *s, hwaddr addr,
                          FLASH_K8P3215UQB_SIZE / FLASH_K8P3215UQB_SECTOR_SIZE);
     qdev_prop_set_uint32(dev, "sector-length", FLASH_K8P3215UQB_SECTOR_SIZE);
     qdev_prop_set_uint8(dev, "width", 4); /* 32-bit */
-    qdev_prop_set_uint8(dev, "mappings", 0);
     qdev_prop_set_uint8(dev, "big-endian", 0);
     qdev_prop_set_uint16(dev, "id0", 0x00ec);
     qdev_prop_set_uint16(dev, "id1", 0x007e);
diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index 7d1f2f3fb3f..e882e11df36 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -1657,7 +1657,6 @@ static void musicpal_init(MachineState *machine)
         qdev_prop_set_uint32(dev, "num-blocks", flash_size / sector_size);
         qdev_prop_set_uint32(dev, "sector-length", sector_size);
         qdev_prop_set_uint8(dev, "width", 2); /* 16-bit */
-        qdev_prop_set_uint8(dev, "mappings", 0);
         qdev_prop_set_uint8(dev, "big-endian", 0);
         qdev_prop_set_uint16(dev, "id0", 0x00bf);
         qdev_prop_set_uint16(dev, "id1", 0x236d);
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 8db6cfd47f5..d12b00e7648 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -220,7 +220,7 @@ static void zynq_init(MachineState *machine)
     pflash_cfi02_register(0xe2000000, "zynq.pflash", FLASH_SIZE,
                           dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
                           FLASH_SECTOR_SIZE, 1,
-                          1, 0x0066, 0x0022, 0x0000, 0x0000, 0x0555, 0x2aa,
+                          0x0066, 0x0022, 0x0000, 0x0000, 0x0555, 0x2aa,
                           0);
 
     /* Create the main clock source, and feed slcr with it */
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 6f4b3e3c3fe..2b412402fac 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -968,7 +968,7 @@ PFlashCFI02 *pflash_cfi02_register(hwaddr base,
                                    hwaddr size,
                                    BlockBackend *blk,
                                    uint32_t sector_len,
-                                   int nb_mappings, int width,
+                                   int width,
                                    uint16_t id0, uint16_t id1,
                                    uint16_t id2, uint16_t id3,
                                    uint16_t unlock_addr0,
@@ -977,7 +977,6 @@ PFlashCFI02 *pflash_cfi02_register(hwaddr base,
 {
     DeviceState *dev = qdev_new(TYPE_PFLASH_CFI02);
 
-    assert(nb_mappings <= 1);
     if (blk) {
         qdev_prop_set_drive(dev, "drive", blk);
     }
diff --git a/hw/lm32/lm32_boards.c b/hw/lm32/lm32_boards.c
index b5d97dd53ed..96877ba7cfb 100644
--- a/hw/lm32/lm32_boards.c
+++ b/hw/lm32/lm32_boards.c
@@ -121,7 +121,7 @@ static void lm32_evr_init(MachineState *machine)
     pflash_cfi02_register(flash_base, "lm32_evr.flash", flash_size,
                           dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
                           flash_sector_size,
-                          1, 2, 0x01, 0x7e, 0x43, 0x00, 0x555, 0x2aa, 1);
+                          2, 0x01, 0x7e, 0x43, 0x00, 0x555, 0x2aa, 1);
 
     /* create irq lines */
     env->pic_state = lm32_pic_init(qemu_allocate_irq(cpu_irq_handler, cpu, 0));
@@ -218,7 +218,7 @@ static void lm32_uclinux_init(MachineState *machine)
     pflash_cfi02_register(flash_base, "lm32_uclinux.flash", flash_size,
                           dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
                           flash_sector_size,
-                          1, 2, 0x01, 0x7e, 0x43, 0x00, 0x555, 0x2aa, 1);
+                          2, 0x01, 0x7e, 0x43, 0x00, 0x555, 0x2aa, 1);
 
     /* create irq lines */
     env->pic_state = lm32_pic_init(qemu_allocate_irq(cpu_irq_handler, env, 0));
diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
index 8f77887fb18..2503e033497 100644
--- a/hw/ppc/ppc405_boards.c
+++ b/hw/ppc/ppc405_boards.c
@@ -198,7 +198,7 @@ static void ref405ep_init(MachineState *machine)
         pflash_cfi02_register((uint32_t)(-bios_size),
                               "ef405ep.bios", bios_size,
                               blk_by_legacy_dinfo(dinfo),
-                              64 * KiB, 1,
+                              64 * KiB,
                               2, 0x0001, 0x22DA, 0x0000, 0x0000, 0x555, 0x2AA,
                               1);
     } else
@@ -469,7 +469,7 @@ static void taihu_405ep_init(MachineState *machine)
         pflash_cfi02_register(0xFFE00000,
                               "taihu_405ep.bios", bios_size,
                               blk_by_legacy_dinfo(dinfo),
-                              64 * KiB, 1,
+                              64 * KiB,
                               4, 0x0001, 0x22DA, 0x0000, 0x0000, 0x555, 0x2AA,
                               1);
         fl_idx++;
@@ -502,7 +502,7 @@ static void taihu_405ep_init(MachineState *machine)
         bios_size = 32 * MiB;
         pflash_cfi02_register(0xfc000000, "taihu_405ep.flash", bios_size,
                               blk_by_legacy_dinfo(dinfo),
-                              64 * KiB, 1,
+                              64 * KiB,
                               4, 0x0001, 0x22DA, 0x0000, 0x0000, 0x555, 0x2AA,
                               1);
         fl_idx++;
diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
index 443820901d4..b7288dcba80 100644
--- a/hw/sh4/r2d.c
+++ b/hw/sh4/r2d.c
@@ -301,7 +301,7 @@ static void r2d_init(MachineState *machine)
     dinfo = drive_get(IF_PFLASH, 0, 0);
     pflash_cfi02_register(0x0, "r2d.flash", FLASH_SIZE,
                           dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                          64 * KiB, 1, 2, 0x0001, 0x227e, 0x2220, 0x2200,
+                          64 * KiB, 2, 0x0001, 0x227e, 0x2220, 0x2200,
                           0x555, 0x2aa, 0);
 
     /* NIC: rtl8139 on-board, and 2 slots. */
-- 
2.26.3

Re: [RFC PATCH v2 1/7] hw/misc: Add device to help managing aliased memory regions

[Richard Henderson]

On 4/19/21 2:43 AM, Philippe Mathieu-Daudé wrote:
> Not really RFC, simply that I'v to add the technical description,
> but I'd like to know if there could be a possibility to not accept
> this device (because I missed something) before keeping working on
> it. So far it is only used in hobbyist boards.
> 
> Cc: Peter Xu<peterx@redhat.com>
> Cc: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   include/hw/misc/aliased_region.h |  87 ++++++++++++++++++++
>   hw/misc/aliased_region.c         | 132 +++++++++++++++++++++++++++++++
>   MAINTAINERS                      |   6 ++
>   hw/misc/Kconfig                  |   3 +
>   hw/misc/meson.build              |   1 +
>   5 files changed, 229 insertions(+)
>   create mode 100644 include/hw/misc/aliased_region.h
>   create mode 100644 hw/misc/aliased_region.c

Looks reasonable to me.


> +    subregion_bits = 64 - clz64(s->span_size - 1);
> +    s->mem.count = s->region_size >> subregion_bits;
> +    assert(s->mem.count > 1);
> +    subregion_size = 1ULL << subregion_bits;

So... subregion_size = pow2floor(s->span_size)?

Why use a floor-ish computation here and pow2ceil later in aliased_mr_realize? 
  Why use either floor or ceil as opposed to asserting that the size is in fact 
a power of 2?  Why must the region be a power of 2, as opposed to e.g. a 
multiple of page_size?  Or just the most basic requirement that region_size be 
a multiple of span_size?


r~

Re: [PATCH v2 2/7] hw/arm/musicpal: Open-code pflash_cfi02_register() call

[Richard Henderson]

On 4/19/21 2:43 AM, Philippe Mathieu-Daudé wrote:
> To be able to manually map the flash region on the main memory
> (in the next commit), first expand the pflash_cfi02_register
> in place.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/arm/musicpal.c | 27 +++++++++++++++++++++------
>   1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
> index 9cebece2de0..8b58b66f263 100644
> --- a/hw/arm/musicpal.c
> +++ b/hw/arm/musicpal.c
> @@ -10,6 +10,7 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qemu/units.h"
>   #include "qapi/error.h"
>   #include "cpu.h"
>   #include "hw/sysbus.h"
> @@ -1640,6 +1641,7 @@ static void musicpal_init(MachineState *machine)
>       /* Register flash */
>       dinfo = drive_get(IF_PFLASH, 0, 0);
>       if (dinfo) {
> +        static const size_t sector_size = 64 * KiB;

Drop the static.  We do not need permanent storage for this.
Otherwise,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 3/7] hw/arm/musicpal: Map flash using memory_region_add_subregion_aliased()

[Richard Henderson]

On 4/19/21 2:43 AM, Philippe Mathieu-Daudé wrote:
> Instead of using a device specific feature for mapping the
> flash memory multiple times over a wider region, use the
> generic memory_region_add_subregion_aliased() helper.
> 
> There is no change in the memory layout:
> 
> - before:
> 
>    (qemu) info mtree
>    00000000fe000000-00000000ffffffff (prio 0, i/o): pflash
>      00000000fe000000-00000000fe7fffff (prio 0, romd): alias pflash-alias @musicpal.flash 0000000000000000-00000000007fffff
>      00000000fe800000-00000000feffffff (prio 0, romd): alias pflash-alias @musicpal.flash 0000000000000000-00000000007fffff
>      00000000ff000000-00000000ff7fffff (prio 0, romd): alias pflash-alias @musicpal.flash 0000000000000000-00000000007fffff
>      00000000ff800000-00000000ffffffff (prio 0, romd): alias pflash-alias @musicpal.flash 0000000000000000-00000000007fffff
> 
> - after:
> 
>    00000000fe000000-00000000ffffffff (prio 0, i/o): masked musicpal.flash [span of 8 MiB]
>      00000000fe000000-00000000fe7fffff (prio 0, romd): alias musicpal.flash [#0/4] @musicpal.flash 0000000000000000-00000000007fffff
>      00000000fe800000-00000000feffffff (prio 0, romd): alias musicpal.flash [#1/4] @musicpal.flash 0000000000000000-00000000007fffff
>      00000000ff000000-00000000ff7fffff (prio 0, romd): alias musicpal.flash [#2/4] @musicpal.flash 0000000000000000-00000000007fffff
>      00000000ff800000-00000000ffffffff (prio 0, romd): alias musicpal.flash [#3/4] @musicpal.flash 0000000000000000-00000000007fffff
> 
> Flatview is the same:
> 
>    (qemu) info mtree -f
>    FlatView #0
>     AS "memory", root: system
>     AS "cpu-memory-0", root: system
>     AS "emac-dma", root: system
>     Root memory region: system
>      00000000fe000000-00000000fe7fffff (prio 0, romd): musicpal.flash
>      00000000fe800000-00000000feffffff (prio 0, romd): musicpal.flash
>      00000000ff000000-00000000ff7fffff (prio 0, romd): musicpal.flash
>      00000000ff800000-00000000ffffffff (prio 0, romd): musicpal.flash
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 4/7] hw/arm/digic: Open-code pflash_cfi02_register() call

[Richard Henderson]

On 4/19/21 2:43 AM, Philippe Mathieu-Daudé wrote:
> To be able to manually map the flash region on the main memory
> (in the next commit), first expand the pflash_cfi02_register
> in place.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/arm/digic_boards.c | 27 +++++++++++++++++++++------
>   1 file changed, 21 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 5/7] hw/arm/digic: Map flash using memory_region_add_subregion_aliased()

[Richard Henderson]

On 4/19/21 2:43 AM, Philippe Mathieu-Daudé wrote:
> Instead of using a device specific feature for mapping the
> flash memory multiple times over a wider region, use the
> generic memory_region_add_subregion_aliased() helper.
> 
> There is no change in the memory layout.
> 
> * before:
> 
>    $ qemu-system-arm -M canon-a1100 -S -monitor stdio
>    QEMU 5.2.90 monitor - type 'help' for more information
>    (qemu) info mtree
>    address-space: memory
>      0000000000000000-ffffffffffffffff (prio 0, i/o): system
>        0000000000000000-0000000003ffffff (prio 0, ram): ram
>        00000000c0210000-00000000c02100ff (prio 0, i/o): digic-timer
>        00000000c0210100-00000000c02101ff (prio 0, i/o): digic-timer
>        00000000c0210200-00000000c02102ff (prio 0, i/o): digic-timer
>        00000000c0800000-00000000c0800017 (prio 0, i/o): digic-uart
>        00000000f8000000-00000000ffffffff (prio 0, i/o): pflash
>          00000000f8000000-00000000f83fffff (prio 0, romd): alias pflash-alias @pflash 0000000000000000-00000000003fffff
>          00000000f8400000-00000000f87fffff (prio 0, romd): alias pflash-alias @pflash 0000000000000000-00000000003fffff
>          00000000f8800000-00000000f8bfffff (prio 0, romd): alias pflash-alias @pflash 0000000000000000-00000000003fffff
>          ...
>          00000000ff400000-00000000ff7fffff (prio 0, romd): alias pflash-alias @pflash 0000000000000000-00000000003fffff
>          00000000ff800000-00000000ffbfffff (prio 0, romd): alias pflash-alias @pflash 0000000000000000-00000000003fffff
>          00000000ffc00000-00000000ffffffff (prio 0, romd): alias pflash-alias @pflash 0000000000000000-00000000003fffff
> 
> * after:
> 
>    (qemu) info mtree
>    address-space: memory
>      0000000000000000-ffffffffffffffff (prio 0, i/o): system
>        0000000000000000-0000000003ffffff (prio 0, ram): ram
>        00000000c0210000-00000000c02100ff (prio 0, i/o): digic-timer
>        00000000c0210100-00000000c02101ff (prio 0, i/o): digic-timer
>        00000000c0210200-00000000c02102ff (prio 0, i/o): digic-timer
>        00000000c0800000-00000000c0800017 (prio 0, i/o): digic-uart
>        00000000f8000000-00000000ffffffff (prio 0, i/o): masked pflash [span of 4 MiB]
>          00000000f8000000-00000000f83fffff (prio 0, romd): alias pflash [#0/32] @pflash 0000000000000000-00000000003fffff
>          00000000f8400000-00000000f87fffff (prio 0, romd): alias pflash [#1/32] @pflash 0000000000000000-00000000003fffff
>          00000000f8800000-00000000f8bfffff (prio 0, romd): alias pflash [#2/32] @pflash 0000000000000000-00000000003fffff
>          ...
>          00000000ff400000-00000000ff7fffff (prio 0, romd): alias pflash [#29/32] @pflash 0000000000000000-00000000003fffff
>          00000000ff800000-00000000ffbfffff (prio 0, romd): alias pflash [#30/32] @pflash 0000000000000000-00000000003fffff
>          00000000ffc00000-00000000ffffffff (prio 0, romd): alias pflash [#31/32] @pflash 0000000000000000-00000000003fffff
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/arm/digic_boards.c | 8 +++++---
>   hw/arm/Kconfig        | 1 +
>   2 files changed, 6 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 6/7] hw/block/pflash_cfi02: Remove pflash_setup_mappings()

[Richard Henderson]

On 4/19/21 2:43 AM, Philippe Mathieu-Daudé wrote:
> All boards calling pflash_cfi02_register() use nb_mappings=1,
> which does not do any mapping:
> 
>    $ git grep -wl pflash_cfi02_register hw/
>    hw/arm/xilinx_zynq.c
>    hw/block/pflash_cfi02.c
>    hw/lm32/lm32_boards.c
>    hw/ppc/ppc405_boards.c
>    hw/sh4/r2d.c
> 
> We can remove this now unneeded code.
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 7/7] hw/block/pflash_cfi02: Simplify pflash_cfi02_register() prototype

[Richard Henderson]

On 4/19/21 2:43 AM, Philippe Mathieu-Daudé wrote:
> The previous commit removed the mapping code from TYPE_PFLASH_CFI02.
> pflash_cfi02_register() doesn't use the 'nb_mappings' argument
> anymore. Simply remove it to simplify.
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC PATCH v2 1/7] hw/misc: Add device to help managing aliased memory regions

[Philippe Mathieu-Daudé]

On 4/22/21 3:33 AM, Richard Henderson wrote:
> On 4/19/21 2:43 AM, Philippe Mathieu-Daudé wrote:
>> Not really RFC, simply that I'v to add the technical description,
>> but I'd like to know if there could be a possibility to not accept
>> this device (because I missed something) before keeping working on
>> it. So far it is only used in hobbyist boards.
>>
>> Cc: Peter Xu<peterx@redhat.com>
>> Cc: Paolo Bonzini<pbonzini@redhat.com>
>> ---
>>   include/hw/misc/aliased_region.h |  87 ++++++++++++++++++++
>>   hw/misc/aliased_region.c         | 132 +++++++++++++++++++++++++++++++
>>   MAINTAINERS                      |   6 ++
>>   hw/misc/Kconfig                  |   3 +
>>   hw/misc/meson.build              |   1 +
>>   5 files changed, 229 insertions(+)
>>   create mode 100644 include/hw/misc/aliased_region.h
>>   create mode 100644 hw/misc/aliased_region.c
> 
> Looks reasonable to me.
> 
> 
>> +    subregion_bits = 64 - clz64(s->span_size - 1);
>> +    s->mem.count = s->region_size >> subregion_bits;
>> +    assert(s->mem.count > 1);
>> +    subregion_size = 1ULL << subregion_bits;
> 
> So... subregion_size = pow2floor(s->span_size)?

No... should be subregion_size = pow2ceil(s->span_size).

> Why use a floor-ish computation here and pow2ceil later in
> aliased_mr_realize?

I missed it :)

> Why use either floor or ceil as opposed to
> asserting that the size is in fact a power of 2?

Unfortunately not all memory regions are power of 2 :(

See for example the armv7m_systick device (size 0xe0).

> Why must the region be
> a power of 2, as opposed to e.g. a multiple of page_size?

I/O regions don't have to be multiple of page_size... See
AVR devices for example:

(qemu) info mtree
address-space: memory
  0000000000000000-ffffffffffffffff (prio 0, i/o): system
    0000000000000000-0000000000007fff (prio 0, rom): flash
    0000000000800000-00000000008000ff (prio -1234, i/o): I/O
    0000000000800023-0000000000800025 (prio -1000, i/o): atmega-gpio-b
    0000000000800026-0000000000800028 (prio -1000, i/o): atmega-gpio-c
    0000000000800029-000000000080002b (prio -1000, i/o): atmega-gpio-d
    0000000000800035-0000000000800035 (prio -1000, i/o): avr-timer8-intflag
    0000000000800036-0000000000800036 (prio 0, i/o): avr-timer16-intflag
    0000000000800037-0000000000800037 (prio -1000, i/o): avr-timer8-intflag
    000000000080003f-0000000000800041 (prio -1000, i/o): avr-eeprom
    0000000000800044-0000000000800048 (prio -1000, i/o): avr-timer8
    000000000080004c-000000000080004e (prio -1000, i/o): avr-spi
    0000000000800060-0000000000800060 (prio -1000, i/o): avr-watchdog
    0000000000800064-0000000000800064 (prio 0, i/o): avr-power
    000000000080006e-000000000080006e (prio -1000, i/o): avr-timer8-intmask
    000000000080006f-000000000080006f (prio 0, i/o): avr-timer16-intmask
    0000000000800070-0000000000800070 (prio -1000, i/o): avr-timer8-intmask
    0000000000800074-0000000000800075 (prio -1000, i/o): avr-ext-mem-ctrl
    0000000000800078-000000000080007f (prio -1000, i/o): avr-adc
    0000000000800080-000000000080008d (prio 0, i/o): avr-timer16
    00000000008000b0-00000000008000b4 (prio -1000, i/o): avr-timer8
    00000000008000b8-00000000008000bd (prio -1000, i/o): avr-twi
    00000000008000c0-00000000008000c6 (prio 0, i/o): avr-usart
    0000000000800100-00000000008008ff (prio 0, ram): sram

> Or just the
> most basic requirement that region_size be a multiple of span_size?

OK.

Thanks for the review! Now I need to fill the documentation part :/

Phil.