[PATCH v3 0/5] hw/arm/virt: Improve address assignment for high memory regions

[PATCH v3 0/5] hw/arm/virt: Improve address assignment for high memory regions

[Gavin Shan]

There are three high memory regions, which are VIRT_HIGH_REDIST2,
VIRT_HIGH_PCIE_ECAM and VIRT_HIGH_PCIE_MMIO. Their base addresses
are floating on highest RAM address. However, they can be disabled
in several cases.

(1) One specific high memory region is disabled by developer by
    toggling vms->highmem_{redists, ecam, mmio}.

(2) VIRT_HIGH_PCIE_ECAM region is disabled on machine, which is
    'virt-2.12' or ealier than it.

(3) VIRT_HIGH_PCIE_ECAM region is disabled when firmware is loaded
    on 32-bits system.

(4) One specific high memory region is disabled when it breaks the
    PA space limit.

The current implementation of virt_set_memmap() isn't comprehensive
because the space for one specific high memory region is always
reserved from the PA space for case (1), (2) and (3). In the code,
'base' and 'vms->highest_gpa' are always increased for those three
cases. It's unnecessary since the assigned space of the disabled
high memory region won't be used afterwards.

The series intends to improve the address assignment for these
high memory regions.

PATCH[1-3] preparatory work for the improvment
PATCH[4]   improve high memory region address assignment
PATCH[5]   adds 'highmem-compact' to enable or disable the optimization

History
=======
v2: https://lore.kernel.org/all/20220815062958.100366-1-gshan@redhat.com/T/
v1: https://lists.nongnu.org/archive/html/qemu-arm/2022-08/msg00013.html

Changelog
==========
v3:
  * Reorder the patches                                        (Gavin)
  * Add 'highmem-compact' property for backwards compatibility (Eric)
v2:
  * Split the patches for easier review                        (Gavin)
  * Improved changelog                                         (Marc)
  * Use 'bool fits' in virt_set_high_memmap()                  (Eric)

Gavin Shan (5):
  hw/arm/virt: Introduce virt_set_high_memmap() helper
  hw/arm/virt: Rename variable size to region_size in
    virt_set_high_memmap()
  hw/arm/virt: Introduce variable region_base in virt_set_high_memmap()
  hw/arm/virt: Improve high memory region address assignment
  hw/arm/virt: Add 'highmem-compact' property

 docs/system/arm/virt.rst |   4 ++
 hw/arm/virt.c            | 116 ++++++++++++++++++++++++++++-----------
 include/hw/arm/virt.h    |   2 +
 3 files changed, 89 insertions(+), 33 deletions(-)

-- 
2.23.0

[PATCH v3 1/5] hw/arm/virt: Introduce virt_set_high_memmap() helper

[Gavin Shan]

This introduces virt_set_high_memmap() helper. The logic of high
memory region address assignment is moved to the helper. The intention
is to make the subsequent optimization for high memory region address
assignment easier.

No functional change intended.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 hw/arm/virt.c | 74 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 41 insertions(+), 33 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 0961e053e5..4dab528b82 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1689,6 +1689,46 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx)
     return arm_cpu_mp_affinity(idx, clustersz);
 }
 
+static void virt_set_high_memmap(VirtMachineState *vms,
+                                 hwaddr base, int pa_bits)
+{
+    int i;
+
+    for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
+        hwaddr size = extended_memmap[i].size;
+        bool fits;
+
+        base = ROUND_UP(base, size);
+        vms->memmap[i].base = base;
+        vms->memmap[i].size = size;
+
+        /*
+         * Check each device to see if they fit in the PA space,
+         * moving highest_gpa as we go.
+         *
+         * For each device that doesn't fit, disable it.
+         */
+        fits = (base + size) <= BIT_ULL(pa_bits);
+        if (fits) {
+            vms->highest_gpa = base + size - 1;
+        }
+
+        switch (i) {
+        case VIRT_HIGH_GIC_REDIST2:
+            vms->highmem_redists &= fits;
+            break;
+        case VIRT_HIGH_PCIE_ECAM:
+            vms->highmem_ecam &= fits;
+            break;
+        case VIRT_HIGH_PCIE_MMIO:
+            vms->highmem_mmio &= fits;
+            break;
+        }
+
+        base += size;
+    }
+}
+
 static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
 {
     MachineState *ms = MACHINE(vms);
@@ -1744,39 +1784,7 @@ static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
     /* We know for sure that at least the memory fits in the PA space */
     vms->highest_gpa = memtop - 1;
 
-    for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
-        hwaddr size = extended_memmap[i].size;
-        bool fits;
-
-        base = ROUND_UP(base, size);
-        vms->memmap[i].base = base;
-        vms->memmap[i].size = size;
-
-        /*
-         * Check each device to see if they fit in the PA space,
-         * moving highest_gpa as we go.
-         *
-         * For each device that doesn't fit, disable it.
-         */
-        fits = (base + size) <= BIT_ULL(pa_bits);
-        if (fits) {
-            vms->highest_gpa = base + size - 1;
-        }
-
-        switch (i) {
-        case VIRT_HIGH_GIC_REDIST2:
-            vms->highmem_redists &= fits;
-            break;
-        case VIRT_HIGH_PCIE_ECAM:
-            vms->highmem_ecam &= fits;
-            break;
-        case VIRT_HIGH_PCIE_MMIO:
-            vms->highmem_mmio &= fits;
-            break;
-        }
-
-        base += size;
-    }
+    virt_set_high_memmap(vms, base, pa_bits);
 
     if (device_memory_size > 0) {
         ms->device_memory = g_malloc0(sizeof(*ms->device_memory));
-- 
2.23.0

[PATCH v3 2/5] hw/arm/virt: Rename variable size to region_size in virt_set_high_memmap()

[Gavin Shan]

This renames variable 'size' to 'region_size' in virt_set_high_memmap().
Its counterpart ('region_base') will be introduced in next patch.

No functional change intended.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 hw/arm/virt.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 4dab528b82..187b3ee0e2 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1692,15 +1692,16 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx)
 static void virt_set_high_memmap(VirtMachineState *vms,
                                  hwaddr base, int pa_bits)
 {
+    hwaddr region_size;
+    bool fits;
     int i;
 
     for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
-        hwaddr size = extended_memmap[i].size;
-        bool fits;
+        region_size = extended_memmap[i].size;
 
-        base = ROUND_UP(base, size);
+        base = ROUND_UP(base, region_size);
         vms->memmap[i].base = base;
-        vms->memmap[i].size = size;
+        vms->memmap[i].size = region_size;
 
         /*
          * Check each device to see if they fit in the PA space,
@@ -1708,9 +1709,9 @@ static void virt_set_high_memmap(VirtMachineState *vms,
          *
          * For each device that doesn't fit, disable it.
          */
-        fits = (base + size) <= BIT_ULL(pa_bits);
+        fits = (base + region_size) <= BIT_ULL(pa_bits);
         if (fits) {
-            vms->highest_gpa = base + size - 1;
+            vms->highest_gpa = base + region_size - 1;
         }
 
         switch (i) {
@@ -1725,7 +1726,7 @@ static void virt_set_high_memmap(VirtMachineState *vms,
             break;
         }
 
-        base += size;
+        base += region_size;
     }
 }
 
-- 
2.23.0

[PATCH v3 3/5] hw/arm/virt: Introduce variable region_base in virt_set_high_memmap()

[Gavin Shan]

This introduces variable 'region_base' for the base address of the
specific high memory region. It's the preparatory work to optimize
high memory region address assignment.

No functional change intended.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 hw/arm/virt.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 187b3ee0e2..b0b679d1f4 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1692,15 +1692,15 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx)
 static void virt_set_high_memmap(VirtMachineState *vms,
                                  hwaddr base, int pa_bits)
 {
-    hwaddr region_size;
+    hwaddr region_base, region_size;
     bool fits;
     int i;
 
     for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
+        region_base = ROUND_UP(base, extended_memmap[i].size);
         region_size = extended_memmap[i].size;
 
-        base = ROUND_UP(base, region_size);
-        vms->memmap[i].base = base;
+        vms->memmap[i].base = region_base;
         vms->memmap[i].size = region_size;
 
         /*
@@ -1709,9 +1709,9 @@ static void virt_set_high_memmap(VirtMachineState *vms,
          *
          * For each device that doesn't fit, disable it.
          */
-        fits = (base + region_size) <= BIT_ULL(pa_bits);
+        fits = (region_base + region_size) <= BIT_ULL(pa_bits);
         if (fits) {
-            vms->highest_gpa = base + region_size - 1;
+            vms->highest_gpa = region_base + region_size - 1;
         }
 
         switch (i) {
@@ -1726,7 +1726,7 @@ static void virt_set_high_memmap(VirtMachineState *vms,
             break;
         }
 
-        base += region_size;
+        base = region_base + region_size;
     }
 }
 
-- 
2.23.0

[PATCH v3 4/5] hw/arm/virt: Improve high memory region address assignment

[Gavin Shan]

There are three high memory regions, which are VIRT_HIGH_REDIST2,
VIRT_HIGH_PCIE_ECAM and VIRT_HIGH_PCIE_MMIO. Their base addresses
are floating on highest RAM address. However, they can be disabled
in several cases.

(1) One specific high memory region is disabled by developer by
    toggling vms->highmem_{redists, ecam, mmio}.

(2) VIRT_HIGH_PCIE_ECAM region is disabled on machine, which is
    'virt-2.12' or ealier than it.

(3) VIRT_HIGH_PCIE_ECAM region is disabled when firmware is loaded
    on 32-bits system.

(4) One specific high memory region is disabled when it breaks the
    PA space limit.

The current implementation of virt_set_memmap() isn't comprehensive
because the space for one specific high memory region is always
reserved from the PA space for case (1), (2) and (3). In the code,
'base' and 'vms->highest_gpa' are always increased for those three
cases. It's unnecessary since the assigned space of the disabled
high memory region won't be used afterwards.

This improves the address assignment for those three high memory
region by skipping the address assignment for one specific high
memory region if it has been disabled in case (1), (2) and (3).

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 hw/arm/virt.c | 44 ++++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index b0b679d1f4..b702f8f2b5 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1693,15 +1693,31 @@ static void virt_set_high_memmap(VirtMachineState *vms,
                                  hwaddr base, int pa_bits)
 {
     hwaddr region_base, region_size;
-    bool fits;
+    bool *region_enabled, fits;
     int i;
 
     for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
         region_base = ROUND_UP(base, extended_memmap[i].size);
         region_size = extended_memmap[i].size;
 
-        vms->memmap[i].base = region_base;
-        vms->memmap[i].size = region_size;
+        switch (i) {
+        case VIRT_HIGH_GIC_REDIST2:
+            region_enabled = &vms->highmem_redists;
+            break;
+        case VIRT_HIGH_PCIE_ECAM:
+            region_enabled = &vms->highmem_ecam;
+            break;
+        case VIRT_HIGH_PCIE_MMIO:
+            region_enabled = &vms->highmem_mmio;
+            break;
+        default:
+            region_enabled = NULL;
+        }
+
+        /* Skip unknown region */
+        if (!region_enabled) {
+            continue;
+        }
 
         /*
          * Check each device to see if they fit in the PA space,
@@ -1710,23 +1726,15 @@ static void virt_set_high_memmap(VirtMachineState *vms,
          * For each device that doesn't fit, disable it.
          */
         fits = (region_base + region_size) <= BIT_ULL(pa_bits);
-        if (fits) {
-            vms->highest_gpa = region_base + region_size - 1;
-        }
+        if (*region_enabled && fits) {
+            vms->memmap[i].base = region_base;
+            vms->memmap[i].size = region_size;
 
-        switch (i) {
-        case VIRT_HIGH_GIC_REDIST2:
-            vms->highmem_redists &= fits;
-            break;
-        case VIRT_HIGH_PCIE_ECAM:
-            vms->highmem_ecam &= fits;
-            break;
-        case VIRT_HIGH_PCIE_MMIO:
-            vms->highmem_mmio &= fits;
-            break;
+            vms->highest_gpa = region_base + region_size - 1;
+            base = region_base + region_size;
+        } else {
+            *region_enabled = false;
         }
-
-        base = region_base + region_size;
     }
 }
 
-- 
2.23.0

[PATCH v3 5/5] hw/arm/virt: Add 'highmem-compact' property

[Gavin Shan]

After the improvement to high memory region address assignment is
applied, the memory layout is changed. For example, VIRT_HIGH_PCIE_MMIO
memory region is enabled when the improvement is applied, but it's
disabled if the improvement isn't applied.

    pa_bits              = 40;
    vms->highmem_redists = false;
    vms->highmem_ecam    = false;
    vms->highmem_mmio    = true;

    # qemu-system-aarch64 -accel kvm -cpu host \
      -machine virt-7.2 -m 4G,maxmem=511G      \
      -monitor stdio

In order to keep backwords compatibility, we need to disable the
optimization on machines, which is virt-7.1 or ealier than it. It
means the optimization is enabled by default from virt-7.2. Besides,
'highmem-compact' property is added so that the optimization can be
explicitly enabled or disabled on all machine types by users.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 docs/system/arm/virt.rst |  4 ++++
 hw/arm/virt.c            | 33 +++++++++++++++++++++++++++++++++
 include/hw/arm/virt.h    |  2 ++
 3 files changed, 39 insertions(+)

diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
index 20442ea2c1..f05ec2253b 100644
--- a/docs/system/arm/virt.rst
+++ b/docs/system/arm/virt.rst
@@ -94,6 +94,10 @@ highmem
   address space above 32 bits. The default is ``on`` for machine types
   later than ``virt-2.12``.
 
+highmem-compact
+  Set ``on``/``off`` to enable/disable compact space for high memory regions.
+  The default is ``on`` for machine types later than ``virt-7.2``
+
 gic-version
   Specify the version of the Generic Interrupt Controller (GIC) to provide.
   Valid values are:
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index b702f8f2b5..a4fbdaef91 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1734,6 +1734,13 @@ static void virt_set_high_memmap(VirtMachineState *vms,
             base = region_base + region_size;
         } else {
             *region_enabled = false;
+
+            if (!vms->highmem_compact) {
+                base = region_base + region_size;
+                if (fits) {
+                    vms->highest_gpa = region_base + region_size - 1;
+                }
+            }
         }
     }
 }
@@ -2348,6 +2355,20 @@ static void virt_set_highmem(Object *obj, bool value, Error **errp)
     vms->highmem = value;
 }
 
+static bool virt_get_highmem_compact(Object *obj, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+
+    return vms->highmem_compact;
+}
+
+static void virt_set_highmem_compact(Object *obj, bool value, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+
+    vms->highmem_compact = value;
+}
+
 static bool virt_get_its(Object *obj, Error **errp)
 {
     VirtMachineState *vms = VIRT_MACHINE(obj);
@@ -2966,6 +2987,13 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
                                           "Set on/off to enable/disable using "
                                           "physical address space above 32 bits");
 
+    object_class_property_add_bool(oc, "highmem-compact",
+                                   virt_get_highmem_compact,
+                                   virt_set_highmem_compact);
+    object_class_property_set_description(oc, "highmem-compact",
+                                          "Set on/off to enable/disable compact "
+                                          "space for high memory regions");
+
     object_class_property_add_str(oc, "gic-version", virt_get_gic_version,
                                   virt_set_gic_version);
     object_class_property_set_description(oc, "gic-version",
@@ -3050,6 +3078,7 @@ static void virt_instance_init(Object *obj)
 
     /* High memory is enabled by default */
     vms->highmem = true;
+    vms->highmem_compact = !vmc->no_highmem_compact;
     vms->gic_version = VIRT_GIC_VERSION_NOSEL;
 
     vms->highmem_ecam = !vmc->no_highmem_ecam;
@@ -3119,8 +3148,12 @@ DEFINE_VIRT_MACHINE_AS_LATEST(7, 2)
 
 static void virt_machine_7_1_options(MachineClass *mc)
 {
+    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
+
     virt_machine_7_2_options(mc);
     compat_props_add(mc->compat_props, hw_compat_7_1, hw_compat_7_1_len);
+    /* Compact space for high memory regions was introduced with 7.2 */
+    vmc->no_highmem_compact = true;
 }
 DEFINE_VIRT_MACHINE(7, 1)
 
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 6ec479ca2b..c7dd59d7f1 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -125,6 +125,7 @@ struct VirtMachineClass {
     bool no_pmu;
     bool claim_edge_triggered_timers;
     bool smbios_old_sys_ver;
+    bool no_highmem_compact;
     bool no_highmem_ecam;
     bool no_ged;   /* Machines < 4.2 have no support for ACPI GED device */
     bool kvm_no_adjvtime;
@@ -144,6 +145,7 @@ struct VirtMachineState {
     PFlashCFI01 *flash[2];
     bool secure;
     bool highmem;
+    bool highmem_compact;
     bool highmem_ecam;
     bool highmem_mmio;
     bool highmem_redists;
-- 
2.23.0

Re: [PATCH v3 0/5] hw/arm/virt: Improve address assignment for high memory regions

[Zhenyu Zhang]

[PATCH v3 0/5] hw/arm/virt: Improve address assignment for high memory regions
Author: Gavin Shan <gshan@redhat.com>
Date:   Thu Sep 22 07:13:45 2022 +0800

    PATCH[1-3] preparatory work for the improvment
    PATCH[4]   improve high memory region address assignment
    PATCH[5]   adds 'highmem-compact' to enable or disable the optimization

    Signed-off-by: Gavin Shan <gshan@redhat.com>

The patchs works well on "IPA Size Limit: 40" FUJITSU machine.
I added some debug code to show high memory region address.
The test results are as expected.

1. virt-7.2 default
# /home/qemu/build/qemu-system-aarch64 -accel kvm -cpu host -machine
virt-7.2 -m 4G,maxmem=511G -monitor stdio
=====> virt_set_high_memmap: pa_bits=40, base=0x8000000000
[HIGH_GIC_REDIST2]: disabled, highest_gpa=0x7fffffffff    [no] [yes]
[HIGH_PCIE_ECAM]: disabled, highest_gpa=0x7fffffffff    [no] [yes]
[HIGH_PCIE_MMIO] enabled, highest_gpa=0xffffffffff

2. When virt-7.2,highmem-compact=off
# /home/qemu/build/qemu-system-aarch64 -accel kvm -cpu host -machine
virt-7.2,highmem-compact=off -m 4G,maxmem=511G -monitor stdio
=====> virt_set_high_memmap: pa_bits=40, base=0x8000000000
[HIGH_GIC_REDIST2]: disabled, highest_gpa=0x8003ffffff    [no] [yes]
[HIGH_PCIE_ECAM]: disabled, highest_gpa=0x801fffffff    [no] [yes]
[HIGH_PCIE_MMIO]: disabled, highest_gpa=0x801fffffff    [no] [no]

3. virt-7.1 default
# /home/qemu/build/qemu-system-aarch64 -accel kvm -cpu host -machine
virt-7.1 -m 4G,maxmem=511G -monitor stdio
=====> virt_set_high_memmap: pa_bits=40, base=0x8000000000
[HIGH_GIC_REDIST2]: disabled, highest_gpa=0x8003ffffff    [no] [yes]
[HIGH_PCIE_ECAM]: disabled, highest_gpa=0x801fffffff    [no] [yes]
[HIGH_PCIE_MMIO]: disabled, highest_gpa=0x801fffffff    [no] [no]

2. When virt-7.1,highmem-compact=on
# /home/qemu/build/qemu-system-aarch64 -accel kvm -cpu host -machine
virt-7.1,highmem-compact=on -m 4G,maxmem=511G -monitor stdio
=====> virt_set_high_memmap: pa_bits=40, base=0x8000000000
[HIGH_GIC_REDIST2]: disabled, highest_gpa=0x7fffffffff    [no] [yes]
[HIGH_PCIE_ECAM]: disabled, highest_gpa=0x7fffffffff    [no] [yes]
[HIGH_PCIE_MMIO] enabled, highest_gpa=0xffffffffff


Tested-by: Zhenyu Zhang<zhenyzha@redhat.com>

On Thu, Sep 22, 2022 at 7:13 AM Gavin Shan <gshan@redhat.com> wrote:
>
> There are three high memory regions, which are VIRT_HIGH_REDIST2,
> VIRT_HIGH_PCIE_ECAM and VIRT_HIGH_PCIE_MMIO. Their base addresses
> are floating on highest RAM address. However, they can be disabled
> in several cases.
>
> (1) One specific high memory region is disabled by developer by
>     toggling vms->highmem_{redists, ecam, mmio}.
>
> (2) VIRT_HIGH_PCIE_ECAM region is disabled on machine, which is
>     'virt-2.12' or ealier than it.
>
> (3) VIRT_HIGH_PCIE_ECAM region is disabled when firmware is loaded
>     on 32-bits system.
>
> (4) One specific high memory region is disabled when it breaks the
>     PA space limit.
>
> The current implementation of virt_set_memmap() isn't comprehensive
> because the space for one specific high memory region is always
> reserved from the PA space for case (1), (2) and (3). In the code,
> 'base' and 'vms->highest_gpa' are always increased for those three
> cases. It's unnecessary since the assigned space of the disabled
> high memory region won't be used afterwards.
>
> The series intends to improve the address assignment for these
> high memory regions.
>
> PATCH[1-3] preparatory work for the improvment
> PATCH[4]   improve high memory region address assignment
> PATCH[5]   adds 'highmem-compact' to enable or disable the optimization
>
> History
> =======
> v2: https://lore.kernel.org/all/20220815062958.100366-1-gshan@redhat.com/T/
> v1: https://lists.nongnu.org/archive/html/qemu-arm/2022-08/msg00013.html
>
> Changelog
> ==========
> v3:
>   * Reorder the patches                                        (Gavin)
>   * Add 'highmem-compact' property for backwards compatibility (Eric)
> v2:
>   * Split the patches for easier review                        (Gavin)
>   * Improved changelog                                         (Marc)
>   * Use 'bool fits' in virt_set_high_memmap()                  (Eric)
>
> Gavin Shan (5):
>   hw/arm/virt: Introduce virt_set_high_memmap() helper
>   hw/arm/virt: Rename variable size to region_size in
>     virt_set_high_memmap()
>   hw/arm/virt: Introduce variable region_base in virt_set_high_memmap()
>   hw/arm/virt: Improve high memory region address assignment
>   hw/arm/virt: Add 'highmem-compact' property
>
>  docs/system/arm/virt.rst |   4 ++
>  hw/arm/virt.c            | 116 ++++++++++++++++++++++++++++-----------
>  include/hw/arm/virt.h    |   2 +
>  3 files changed, 89 insertions(+), 33 deletions(-)
>
> --
> 2.23.0
>

Re: [PATCH v3 1/5] hw/arm/virt: Introduce virt_set_high_memmap() helper

[Eric Auger]

Hi Gavin,

On 9/22/22 01:13, Gavin Shan wrote:
> This introduces virt_set_high_memmap() helper. The logic of high
> memory region address assignment is moved to the helper. The intention
> is to make the subsequent optimization for high memory region address
> assignment easier.
>
> No functional change intended.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
>  hw/arm/virt.c | 74 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 41 insertions(+), 33 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 0961e053e5..4dab528b82 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1689,6 +1689,46 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx)
>      return arm_cpu_mp_affinity(idx, clustersz);
>  }
>  
> +static void virt_set_high_memmap(VirtMachineState *vms,
> +                                 hwaddr base, int pa_bits)
> +{
> +    int i;
> +
> +    for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
> +        hwaddr size = extended_memmap[i].size;
> +        bool fits;
> +
> +        base = ROUND_UP(base, size);
> +        vms->memmap[i].base = base;
> +        vms->memmap[i].size = size;
> +
> +        /*
> +         * Check each device to see if they fit in the PA space,
> +         * moving highest_gpa as we go.
> +         *
> +         * For each device that doesn't fit, disable it.
> +         */
> +        fits = (base + size) <= BIT_ULL(pa_bits);
> +        if (fits) {
> +            vms->highest_gpa = base + size - 1;
> +        }
> +
> +        switch (i) {
> +        case VIRT_HIGH_GIC_REDIST2:
> +            vms->highmem_redists &= fits;
> +            break;
> +        case VIRT_HIGH_PCIE_ECAM:
> +            vms->highmem_ecam &= fits;
> +            break;
> +        case VIRT_HIGH_PCIE_MMIO:
> +            vms->highmem_mmio &= fits;
> +            break;
> +        }
> +
> +        base += size;
> +    }
> +}
> +
>  static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
>  {
>      MachineState *ms = MACHINE(vms);
> @@ -1744,39 +1784,7 @@ static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
>      /* We know for sure that at least the memory fits in the PA space */
>      vms->highest_gpa = memtop - 1;
>  
> -    for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
> -        hwaddr size = extended_memmap[i].size;
> -        bool fits;
> -
> -        base = ROUND_UP(base, size);
> -        vms->memmap[i].base = base;
> -        vms->memmap[i].size = size;
> -
> -        /*
> -         * Check each device to see if they fit in the PA space,
> -         * moving highest_gpa as we go.
> -         *
> -         * For each device that doesn't fit, disable it.
> -         */
> -        fits = (base + size) <= BIT_ULL(pa_bits);
> -        if (fits) {
> -            vms->highest_gpa = base + size - 1;
> -        }
> -
> -        switch (i) {
> -        case VIRT_HIGH_GIC_REDIST2:
> -            vms->highmem_redists &= fits;
> -            break;
> -        case VIRT_HIGH_PCIE_ECAM:
> -            vms->highmem_ecam &= fits;
> -            break;
> -        case VIRT_HIGH_PCIE_MMIO:
> -            vms->highmem_mmio &= fits;
> -            break;
> -        }
> -
> -        base += size;
> -    }
> +    virt_set_high_memmap(vms, base, pa_bits);
>  
>      if (device_memory_size > 0) {
>          ms->device_memory = g_malloc0(sizeof(*ms->device_memory));

Re: [PATCH v3 3/5] hw/arm/virt: Introduce variable region_base in virt_set_high_memmap()

[Eric Auger]

Hi Gavin,

On 9/22/22 01:13, Gavin Shan wrote:
> This introduces variable 'region_base' for the base address of the
> specific high memory region. It's the preparatory work to optimize
> high memory region address assignment.
Why is it a preparatory work (same comment for previous patch, ie [2/5]
). Are those changes really needed? why?

Eric
>
> No functional change intended.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  hw/arm/virt.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 187b3ee0e2..b0b679d1f4 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1692,15 +1692,15 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx)
>  static void virt_set_high_memmap(VirtMachineState *vms,
>                                   hwaddr base, int pa_bits)
>  {
> -    hwaddr region_size;
> +    hwaddr region_base, region_size;
>      bool fits;
>      int i;
>  
>      for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
> +        region_base = ROUND_UP(base, extended_memmap[i].size);
>          region_size = extended_memmap[i].size;
>  
> -        base = ROUND_UP(base, region_size);
> -        vms->memmap[i].base = base;
> +        vms->memmap[i].base = region_base;
>          vms->memmap[i].size = region_size;
>  
>          /*
> @@ -1709,9 +1709,9 @@ static void virt_set_high_memmap(VirtMachineState *vms,
>           *
>           * For each device that doesn't fit, disable it.
>           */
> -        fits = (base + region_size) <= BIT_ULL(pa_bits);
> +        fits = (region_base + region_size) <= BIT_ULL(pa_bits);
>          if (fits) {
> -            vms->highest_gpa = base + region_size - 1;
> +            vms->highest_gpa = region_base + region_size - 1;
>          }
>  
>          switch (i) {
> @@ -1726,7 +1726,7 @@ static void virt_set_high_memmap(VirtMachineState *vms,
>              break;
>          }
>  
> -        base += region_size;
> +        base = region_base + region_size;
>      }
>  }
>  

Re: [PATCH v3 5/5] hw/arm/virt: Add 'highmem-compact' property

[Eric Auger]

Hi Gavin,
On 9/22/22 01:13, Gavin Shan wrote:
> After the improvement to high memory region address assignment is
> applied, the memory layout is changed. For example, VIRT_HIGH_PCIE_MMIO
s/the memory layout is changed./the memory layout is changed,
introducing possible migration breakage.
> memory region is enabled when the improvement is applied, but it's
> disabled if the improvement isn't applied.
>
>     pa_bits              = 40;
>     vms->highmem_redists = false;
>     vms->highmem_ecam    = false;
>     vms->highmem_mmio    = true;
>
>     # qemu-system-aarch64 -accel kvm -cpu host \
>       -machine virt-7.2 -m 4G,maxmem=511G      \
>       -monitor stdio
>
> In order to keep backwords compatibility, we need to disable the
> optimization on machines, which is virt-7.1 or ealier than it. It
> means the optimization is enabled by default from virt-7.2. Besides,
> 'highmem-compact' property is added so that the optimization can be
I would rather rename the property into compact-highmem even if the vms
field is name highmem_compact to align with other highmem fields
> explicitly enabled or disabled on all machine types by users.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  docs/system/arm/virt.rst |  4 ++++
>  hw/arm/virt.c            | 33 +++++++++++++++++++++++++++++++++
>  include/hw/arm/virt.h    |  2 ++
>  3 files changed, 39 insertions(+)
>
> diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
> index 20442ea2c1..f05ec2253b 100644
> --- a/docs/system/arm/virt.rst
> +++ b/docs/system/arm/virt.rst
> @@ -94,6 +94,10 @@ highmem
>    address space above 32 bits. The default is ``on`` for machine types
>    later than ``virt-2.12``.
>  
> +highmem-compact
> +  Set ``on``/``off`` to enable/disable compact space for high memory regions.
> +  The default is ``on`` for machine types later than ``virt-7.2``
I think you should document what is compact layout versus legacy one,
both in the commit msg and maybe as a comment in a code along with the
comment in hw/arm/virt.c starting with 'Highmem IO Regions: '
> +
>  gic-version
>    Specify the version of the Generic Interrupt Controller (GIC) to provide.
>    Valid values are:
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index b702f8f2b5..a4fbdaef91 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1734,6 +1734,13 @@ static void virt_set_high_memmap(VirtMachineState *vms,
>              base = region_base + region_size;
>          } else {
>              *region_enabled = false;
> +
> +            if (!vms->highmem_compact) {
this snippet should be already present in previous patch otherwise this
will break bisectability.

> +                base = region_base + region_size;
> +                if (fits) {
> +                    vms->highest_gpa = region_base + region_size - 1;
> +                }
> +            }
>          }
>      }
>  }
> @@ -2348,6 +2355,20 @@ static void virt_set_highmem(Object *obj, bool value, Error **errp)
>      vms->highmem = value;
>  }
>  
> +static bool virt_get_highmem_compact(Object *obj, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    return vms->highmem_compact;
> +}
> +
> +static void virt_set_highmem_compact(Object *obj, bool value, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    vms->highmem_compact = value;
> +}
> +
>  static bool virt_get_its(Object *obj, Error **errp)
>  {
>      VirtMachineState *vms = VIRT_MACHINE(obj);
> @@ -2966,6 +2987,13 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>                                            "Set on/off to enable/disable using "
>                                            "physical address space above 32 bits");
>  
> +    object_class_property_add_bool(oc, "highmem-compact",
> +                                   virt_get_highmem_compact,
> +                                   virt_set_highmem_compact);
> +    object_class_property_set_description(oc, "highmem-compact",
> +                                          "Set on/off to enable/disable compact "
> +                                          "space for high memory regions");
> +
>      object_class_property_add_str(oc, "gic-version", virt_get_gic_version,
>                                    virt_set_gic_version);
>      object_class_property_set_description(oc, "gic-version",
> @@ -3050,6 +3078,7 @@ static void virt_instance_init(Object *obj)
>  
>      /* High memory is enabled by default */
>      vms->highmem = true;
> +    vms->highmem_compact = !vmc->no_highmem_compact;
>      vms->gic_version = VIRT_GIC_VERSION_NOSEL;
>  
>      vms->highmem_ecam = !vmc->no_highmem_ecam;
> @@ -3119,8 +3148,12 @@ DEFINE_VIRT_MACHINE_AS_LATEST(7, 2)
>  
>  static void virt_machine_7_1_options(MachineClass *mc)
>  {
> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> +
>      virt_machine_7_2_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_7_1, hw_compat_7_1_len);
> +    /* Compact space for high memory regions was introduced with 7.2 */
> +    vmc->no_highmem_compact = true;
>  }
>  DEFINE_VIRT_MACHINE(7, 1)
>  
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 6ec479ca2b..c7dd59d7f1 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -125,6 +125,7 @@ struct VirtMachineClass {
>      bool no_pmu;
>      bool claim_edge_triggered_timers;
>      bool smbios_old_sys_ver;
> +    bool no_highmem_compact;
>      bool no_highmem_ecam;
>      bool no_ged;   /* Machines < 4.2 have no support for ACPI GED device */
>      bool kvm_no_adjvtime;
> @@ -144,6 +145,7 @@ struct VirtMachineState {
>      PFlashCFI01 *flash[2];
>      bool secure;
>      bool highmem;
> +    bool highmem_compact;
>      bool highmem_ecam;
>      bool highmem_mmio;
>      bool highmem_redists;

Thanks

Eric

Re: [PATCH v3 4/5] hw/arm/virt: Improve high memory region address assignment

[Eric Auger]

Hi Gavin,

On 9/22/22 01:13, Gavin Shan wrote:
> There are three high memory regions, which are VIRT_HIGH_REDIST2,
> VIRT_HIGH_PCIE_ECAM and VIRT_HIGH_PCIE_MMIO. Their base addresses
> are floating on highest RAM address. However, they can be disabled
> in several cases.
>
> (1) One specific high memory region is disabled by developer by
>     toggling vms->highmem_{redists, ecam, mmio}.
>
> (2) VIRT_HIGH_PCIE_ECAM region is disabled on machine, which is
>     'virt-2.12' or ealier than it.
>
> (3) VIRT_HIGH_PCIE_ECAM region is disabled when firmware is loaded
>     on 32-bits system.
>
> (4) One specific high memory region is disabled when it breaks the
>     PA space limit.
>
> The current implementation of virt_set_memmap() isn't comprehensive
> because the space for one specific high memory region is always
> reserved from the PA space for case (1), (2) and (3). In the code,
> 'base' and 'vms->highest_gpa' are always increased for those three
> cases. It's unnecessary since the assigned space of the disabled
> high memory region won't be used afterwards.
>
> This improves the address assignment for those three high memory
> region by skipping the address assignment for one specific high
> memory region if it has been disabled in case (1), (2) and (3).
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  hw/arm/virt.c | 44 ++++++++++++++++++++++++++------------------
>  1 file changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index b0b679d1f4..b702f8f2b5 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1693,15 +1693,31 @@ static void virt_set_high_memmap(VirtMachineState *vms,
>                                   hwaddr base, int pa_bits)
>  {
>      hwaddr region_base, region_size;
> -    bool fits;
> +    bool *region_enabled, fits;
IDo you really need a pointer? If the region is unknown this is a bug in
virt code.
>      int i;
>  
>      for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
>          region_base = ROUND_UP(base, extended_memmap[i].size);
>          region_size = extended_memmap[i].size;
>  
> -        vms->memmap[i].base = region_base;
> -        vms->memmap[i].size = region_size;
> +        switch (i) {
> +        case VIRT_HIGH_GIC_REDIST2:
> +            region_enabled = &vms->highmem_redists;
> +            break;
> +        case VIRT_HIGH_PCIE_ECAM:
> +            region_enabled = &vms->highmem_ecam;
> +            break;
> +        case VIRT_HIGH_PCIE_MMIO:
> +            region_enabled = &vms->highmem_mmio;
> +            break;
While we are at it I would change the vms fields dealing with those
highmem regions and turn those fields into an array of bool indexed
using i - VIRT_LOWMEMMAP_LAST (using a macro or something alike). We
would not be obliged to have this switch, now duplicated.
> +        default:
> +            region_enabled = NULL;
> +        }
> +
> +        /* Skip unknown region */
> +        if (!region_enabled) {
> +            continue;
> +        }
>  
>          /*
>           * Check each device to see if they fit in the PA space,
> @@ -1710,23 +1726,15 @@ static void virt_set_high_memmap(VirtMachineState *vms,
>           * For each device that doesn't fit, disable it.
>           */
>          fits = (region_base + region_size) <= BIT_ULL(pa_bits);
> -        if (fits) {
> -            vms->highest_gpa = region_base + region_size - 1;
> -        }
> +        if (*region_enabled && fits) {
> +            vms->memmap[i].base = region_base;
> +            vms->memmap[i].size = region_size;
>  
> -        switch (i) {
> -        case VIRT_HIGH_GIC_REDIST2:
> -            vms->highmem_redists &= fits;
> -            break;
> -        case VIRT_HIGH_PCIE_ECAM:
> -            vms->highmem_ecam &= fits;
> -            break;
> -        case VIRT_HIGH_PCIE_MMIO:
> -            vms->highmem_mmio &= fits;
> -            break;
> +            vms->highest_gpa = region_base + region_size - 1;
> +            base = region_base + region_size;
> +        } else {
> +            *region_enabled = false;
>          }
> -
> -        base = region_base + region_size;
>      }
>  }
>  
Thanks

Eric

Re: [PATCH v3 3/5] hw/arm/virt: Introduce variable region_base in virt_set_high_memmap()

[Gavin Shan]

Hi Eric,

On 9/28/22 10:10 PM, Eric Auger wrote:
> On 9/22/22 01:13, Gavin Shan wrote:
>> This introduces variable 'region_base' for the base address of the
>> specific high memory region. It's the preparatory work to optimize
>> high memory region address assignment.
> Why is it a preparatory work (same comment for previous patch, ie [2/5]
> ). Are those changes really needed? why?
> 

In PATCH[4/5], @base argument is added to virt_set_high_memmap(), to
represent current global base address. With the optimization applied
in PATCH[4/5], @base isn't unconditionally updated to the top of the
iterated high memory region. So we need @region_base here (PATCH[3/5])
to track the aligned base address for the iterated high memory region,
which may or may be not updated to @base.

Since we have @region_base in PATCH[3/5], it'd better to have @region_size
in PATCH[2/5].

Actually, PATCH[1-3/5] are all preparatory patches for PATCH[4/5]. My
intention was to organize the patches in a way to keep the logical
change part simple enough, for easier review.

Thanks,
Gavin

>>
>> No functional change intended.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   hw/arm/virt.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 187b3ee0e2..b0b679d1f4 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1692,15 +1692,15 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx)
>>   static void virt_set_high_memmap(VirtMachineState *vms,
>>                                    hwaddr base, int pa_bits)
>>   {
>> -    hwaddr region_size;
>> +    hwaddr region_base, region_size;
>>       bool fits;
>>       int i;
>>   
>>       for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
>> +        region_base = ROUND_UP(base, extended_memmap[i].size);
>>           region_size = extended_memmap[i].size;
>>   
>> -        base = ROUND_UP(base, region_size);
>> -        vms->memmap[i].base = base;
>> +        vms->memmap[i].base = region_base;
>>           vms->memmap[i].size = region_size;
>>   
>>           /*
>> @@ -1709,9 +1709,9 @@ static void virt_set_high_memmap(VirtMachineState *vms,
>>            *
>>            * For each device that doesn't fit, disable it.
>>            */
>> -        fits = (base + region_size) <= BIT_ULL(pa_bits);
>> +        fits = (region_base + region_size) <= BIT_ULL(pa_bits);
>>           if (fits) {
>> -            vms->highest_gpa = base + region_size - 1;
>> +            vms->highest_gpa = region_base + region_size - 1;
>>           }
>>   
>>           switch (i) {
>> @@ -1726,7 +1726,7 @@ static void virt_set_high_memmap(VirtMachineState *vms,
>>               break;
>>           }
>>   
>> -        base += region_size;
>> +        base = region_base + region_size;
>>       }
>>   }
>>   
> 

Re: [PATCH v3 4/5] hw/arm/virt: Improve high memory region address assignment

[Gavin Shan]

Hi Eric,

On 9/28/22 10:51 PM, Eric Auger wrote:
> On 9/22/22 01:13, Gavin Shan wrote:
>> There are three high memory regions, which are VIRT_HIGH_REDIST2,
>> VIRT_HIGH_PCIE_ECAM and VIRT_HIGH_PCIE_MMIO. Their base addresses
>> are floating on highest RAM address. However, they can be disabled
>> in several cases.
>>
>> (1) One specific high memory region is disabled by developer by
>>      toggling vms->highmem_{redists, ecam, mmio}.
>>
>> (2) VIRT_HIGH_PCIE_ECAM region is disabled on machine, which is
>>      'virt-2.12' or ealier than it.
>>
>> (3) VIRT_HIGH_PCIE_ECAM region is disabled when firmware is loaded
>>      on 32-bits system.
>>
>> (4) One specific high memory region is disabled when it breaks the
>>      PA space limit.
>>
>> The current implementation of virt_set_memmap() isn't comprehensive
>> because the space for one specific high memory region is always
>> reserved from the PA space for case (1), (2) and (3). In the code,
>> 'base' and 'vms->highest_gpa' are always increased for those three
>> cases. It's unnecessary since the assigned space of the disabled
>> high memory region won't be used afterwards.
>>
>> This improves the address assignment for those three high memory
>> region by skipping the address assignment for one specific high
>> memory region if it has been disabled in case (1), (2) and (3).
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   hw/arm/virt.c | 44 ++++++++++++++++++++++++++------------------
>>   1 file changed, 26 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index b0b679d1f4..b702f8f2b5 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1693,15 +1693,31 @@ static void virt_set_high_memmap(VirtMachineState *vms,
>>                                    hwaddr base, int pa_bits)
>>   {
>>       hwaddr region_base, region_size;
>> -    bool fits;
>> +    bool *region_enabled, fits;
> IDo you really need a pointer? If the region is unknown this is a bug in
> virt code.

The pointer is needed so that we can disable the region by setting 'false'
to it at later point. Yeah, I think you're correct that 'unknown region'
is a bug and we need to do assert(region_enabled), or something like below.

>>       int i;
>>   
>>       for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
>>           region_base = ROUND_UP(base, extended_memmap[i].size);
>>           region_size = extended_memmap[i].size;
>>   
>> -        vms->memmap[i].base = region_base;
>> -        vms->memmap[i].size = region_size;
>> +        switch (i) {
>> +        case VIRT_HIGH_GIC_REDIST2:
>> +            region_enabled = &vms->highmem_redists;
>> +            break;
>> +        case VIRT_HIGH_PCIE_ECAM:
>> +            region_enabled = &vms->highmem_ecam;
>> +            break;
>> +        case VIRT_HIGH_PCIE_MMIO:
>> +            region_enabled = &vms->highmem_mmio;
>> +            break;
> While we are at it I would change the vms fields dealing with those
> highmem regions and turn those fields into an array of bool indexed
> using i - VIRT_LOWMEMMAP_LAST (using a macro or something alike). We
> would not be obliged to have this switch, now duplicated.

It makes sense to me. How about to have something like below in v4?

static inline bool *virt_get_high_memmap_enabled(VirtMachineState *vms, int index)
{
     bool *enabled_array[] = {
           &vms->highmem_redists,
           &vms->highmem_ecam,
           &vms->highmem_mmio,
     };

     assert(index - VIRT_LOWMEMMAP_LAST < ARRAY_SIZE(enabled_array));

     return enabled_array[index - VIRT_LOWMEMMAP_LAST];
}

>> +        default:
>> +            region_enabled = NULL;
>> +        }
>> +
>> +        /* Skip unknown region */
>> +        if (!region_enabled) {
>> +            continue;
>> +        }
>>   
>>           /*
>>            * Check each device to see if they fit in the PA space,
>> @@ -1710,23 +1726,15 @@ static void virt_set_high_memmap(VirtMachineState *vms,
>>            * For each device that doesn't fit, disable it.
>>            */
>>           fits = (region_base + region_size) <= BIT_ULL(pa_bits);
>> -        if (fits) {
>> -            vms->highest_gpa = region_base + region_size - 1;
>> -        }
>> +        if (*region_enabled && fits) {
>> +            vms->memmap[i].base = region_base;
>> +            vms->memmap[i].size = region_size;
>>   
>> -        switch (i) {
>> -        case VIRT_HIGH_GIC_REDIST2:
>> -            vms->highmem_redists &= fits;
>> -            break;
>> -        case VIRT_HIGH_PCIE_ECAM:
>> -            vms->highmem_ecam &= fits;
>> -            break;
>> -        case VIRT_HIGH_PCIE_MMIO:
>> -            vms->highmem_mmio &= fits;
>> -            break;
>> +            vms->highest_gpa = region_base + region_size - 1;
>> +            base = region_base + region_size;
>> +        } else {
>> +            *region_enabled = false;
>>           }
>> -
>> -        base = region_base + region_size;
>>       }
>>   }
>>   

Thanks,
Gavin

Re: [PATCH v3 5/5] hw/arm/virt: Add 'highmem-compact' property

[Gavin Shan]

Hi Eric,

On 9/28/22 10:22 PM, Eric Auger wrote:
> On 9/22/22 01:13, Gavin Shan wrote:
>> After the improvement to high memory region address assignment is
>> applied, the memory layout is changed. For example, VIRT_HIGH_PCIE_MMIO
> s/the memory layout is changed./the memory layout is changed,
> introducing possible migration breakage.

Ok, much clearer.

>> memory region is enabled when the improvement is applied, but it's
>> disabled if the improvement isn't applied.
>>
>>      pa_bits              = 40;
>>      vms->highmem_redists = false;
>>      vms->highmem_ecam    = false;
>>      vms->highmem_mmio    = true;
>>
>>      # qemu-system-aarch64 -accel kvm -cpu host \
>>        -machine virt-7.2 -m 4G,maxmem=511G      \
>>        -monitor stdio
>>
>> In order to keep backwords compatibility, we need to disable the
>> optimization on machines, which is virt-7.1 or ealier than it. It
>> means the optimization is enabled by default from virt-7.2. Besides,
>> 'highmem-compact' property is added so that the optimization can be
> I would rather rename the property into compact-highmem even if the vms
> field is name highmem_compact to align with other highmem fields

Ok, but I would love to know why. Note that we already have
'highmem=on|off'. 'highmem_compact=on|off' seems consistent
to me.

>> explicitly enabled or disabled on all machine types by users.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   docs/system/arm/virt.rst |  4 ++++
>>   hw/arm/virt.c            | 33 +++++++++++++++++++++++++++++++++
>>   include/hw/arm/virt.h    |  2 ++
>>   3 files changed, 39 insertions(+)
>>
>> diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
>> index 20442ea2c1..f05ec2253b 100644
>> --- a/docs/system/arm/virt.rst
>> +++ b/docs/system/arm/virt.rst
>> @@ -94,6 +94,10 @@ highmem
>>     address space above 32 bits. The default is ``on`` for machine types
>>     later than ``virt-2.12``.
>>   
>> +highmem-compact
>> +  Set ``on``/``off`` to enable/disable compact space for high memory regions.
>> +  The default is ``on`` for machine types later than ``virt-7.2``
> I think you should document what is compact layout versus legacy one,
> both in the commit msg and maybe as a comment in a code along with the
> comment in hw/arm/virt.c starting with 'Highmem IO Regions: '

Ok, I will add this into the commit log in v4. I don't think it's necessary
to add duplicate comment in the code. People can check the commit log for
details if needed.

>> +
>>   gic-version
>>     Specify the version of the Generic Interrupt Controller (GIC) to provide.
>>     Valid values are:
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index b702f8f2b5..a4fbdaef91 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1734,6 +1734,13 @@ static void virt_set_high_memmap(VirtMachineState *vms,
>>               base = region_base + region_size;
>>           } else {
>>               *region_enabled = false;
>> +
>> +            if (!vms->highmem_compact) {
> this snippet should be already present in previous patch otherwise this
> will break bisectability.
> 

Hmm, nice catch! I think I need to swap PATCH[4] and PATCH[5] in next
revision. In that order, 'compact-highmem' is introduced in PATCH[4],
but not used yet. PATCH[5] has the optimization and 'compact-highmem'
is used.

>> +                base = region_base + region_size;
>> +                if (fits) {
>> +                    vms->highest_gpa = region_base + region_size - 1;
>> +                }
>> +            }
>>           }
>>       }
>>   }
>> @@ -2348,6 +2355,20 @@ static void virt_set_highmem(Object *obj, bool value, Error **errp)
>>       vms->highmem = value;
>>   }
>>   
>> +static bool virt_get_highmem_compact(Object *obj, Error **errp)
>> +{
>> +    VirtMachineState *vms = VIRT_MACHINE(obj);
>> +
>> +    return vms->highmem_compact;
>> +}
>> +
>> +static void virt_set_highmem_compact(Object *obj, bool value, Error **errp)
>> +{
>> +    VirtMachineState *vms = VIRT_MACHINE(obj);
>> +
>> +    vms->highmem_compact = value;
>> +}
>> +
>>   static bool virt_get_its(Object *obj, Error **errp)
>>   {
>>       VirtMachineState *vms = VIRT_MACHINE(obj);
>> @@ -2966,6 +2987,13 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>>                                             "Set on/off to enable/disable using "
>>                                             "physical address space above 32 bits");
>>   
>> +    object_class_property_add_bool(oc, "highmem-compact",
>> +                                   virt_get_highmem_compact,
>> +                                   virt_set_highmem_compact);
>> +    object_class_property_set_description(oc, "highmem-compact",
>> +                                          "Set on/off to enable/disable compact "
>> +                                          "space for high memory regions");
>> +
>>       object_class_property_add_str(oc, "gic-version", virt_get_gic_version,
>>                                     virt_set_gic_version);
>>       object_class_property_set_description(oc, "gic-version",
>> @@ -3050,6 +3078,7 @@ static void virt_instance_init(Object *obj)
>>   
>>       /* High memory is enabled by default */
>>       vms->highmem = true;
>> +    vms->highmem_compact = !vmc->no_highmem_compact;
>>       vms->gic_version = VIRT_GIC_VERSION_NOSEL;
>>   
>>       vms->highmem_ecam = !vmc->no_highmem_ecam;
>> @@ -3119,8 +3148,12 @@ DEFINE_VIRT_MACHINE_AS_LATEST(7, 2)
>>   
>>   static void virt_machine_7_1_options(MachineClass *mc)
>>   {
>> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
>> +
>>       virt_machine_7_2_options(mc);
>>       compat_props_add(mc->compat_props, hw_compat_7_1, hw_compat_7_1_len);
>> +    /* Compact space for high memory regions was introduced with 7.2 */
>> +    vmc->no_highmem_compact = true;
>>   }
>>   DEFINE_VIRT_MACHINE(7, 1)
>>   
>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>> index 6ec479ca2b..c7dd59d7f1 100644
>> --- a/include/hw/arm/virt.h
>> +++ b/include/hw/arm/virt.h
>> @@ -125,6 +125,7 @@ struct VirtMachineClass {
>>       bool no_pmu;
>>       bool claim_edge_triggered_timers;
>>       bool smbios_old_sys_ver;
>> +    bool no_highmem_compact;
>>       bool no_highmem_ecam;
>>       bool no_ged;   /* Machines < 4.2 have no support for ACPI GED device */
>>       bool kvm_no_adjvtime;
>> @@ -144,6 +145,7 @@ struct VirtMachineState {
>>       PFlashCFI01 *flash[2];
>>       bool secure;
>>       bool highmem;
>> +    bool highmem_compact;
>>       bool highmem_ecam;
>>       bool highmem_mmio;
>>       bool highmem_redists;
> 

Thanks,
Gavin

Re: [PATCH v3 5/5] hw/arm/virt: Add 'highmem-compact' property

[Cornelia Huck]

On Thu, Sep 29 2022, Gavin Shan <gshan@redhat.com> wrote:

> Hi Eric,
>
> On 9/28/22 10:22 PM, Eric Auger wrote:
>> On 9/22/22 01:13, Gavin Shan wrote:
>>> After the improvement to high memory region address assignment is
>>> applied, the memory layout is changed. For example, VIRT_HIGH_PCIE_MMIO
>> s/the memory layout is changed./the memory layout is changed,
>> introducing possible migration breakage.
>
> Ok, much clearer.
>
>>> memory region is enabled when the improvement is applied, but it's
>>> disabled if the improvement isn't applied.
>>>
>>>      pa_bits              = 40;
>>>      vms->highmem_redists = false;
>>>      vms->highmem_ecam    = false;
>>>      vms->highmem_mmio    = true;
>>>
>>>      # qemu-system-aarch64 -accel kvm -cpu host \
>>>        -machine virt-7.2 -m 4G,maxmem=511G      \
>>>        -monitor stdio
>>>
>>> In order to keep backwords compatibility, we need to disable the
>>> optimization on machines, which is virt-7.1 or ealier than it. It
>>> means the optimization is enabled by default from virt-7.2. Besides,
>>> 'highmem-compact' property is added so that the optimization can be
>> I would rather rename the property into compact-highmem even if the vms
>> field is name highmem_compact to align with other highmem fields
>
> Ok, but I would love to know why. Note that we already have
> 'highmem=on|off'. 'highmem_compact=on|off' seems consistent
> to me.

FWIW, I initially misread 'highmem_compact' as 'highmem_compat' (and had
to re-read because I got confused). At least to me, 'compact_highmem'
has less chance of being parsed incorrectly :) (although that is
probably a personal thing.)

>
>>> explicitly enabled or disabled on all machine types by users.
>>>
>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>> ---
>>>   docs/system/arm/virt.rst |  4 ++++
>>>   hw/arm/virt.c            | 33 +++++++++++++++++++++++++++++++++
>>>   include/hw/arm/virt.h    |  2 ++
>>>   3 files changed, 39 insertions(+)
>>>
>>> diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
>>> index 20442ea2c1..f05ec2253b 100644
>>> --- a/docs/system/arm/virt.rst
>>> +++ b/docs/system/arm/virt.rst
>>> @@ -94,6 +94,10 @@ highmem
>>>     address space above 32 bits. The default is ``on`` for machine types
>>>     later than ``virt-2.12``.
>>>   
>>> +highmem-compact
>>> +  Set ``on``/``off`` to enable/disable compact space for high memory regions.
>>> +  The default is ``on`` for machine types later than ``virt-7.2``
>> I think you should document what is compact layout versus legacy one,
>> both in the commit msg and maybe as a comment in a code along with the
>> comment in hw/arm/virt.c starting with 'Highmem IO Regions: '
>
> Ok, I will add this into the commit log in v4. I don't think it's necessary
> to add duplicate comment in the code. People can check the commit log for
> details if needed.

Rather explain it in this file here, maybe? I'd prefer to be able to
find out what 'compact' means without digging through the commit log.

Re: [PATCH v3 5/5] hw/arm/virt: Add 'highmem-compact' property

[Gavin Shan]

Hi Cornelia,

On 9/29/22 8:27 PM, Cornelia Huck wrote:
> On Thu, Sep 29 2022, Gavin Shan <gshan@redhat.com> wrote:
>> On 9/28/22 10:22 PM, Eric Auger wrote:
>>> On 9/22/22 01:13, Gavin Shan wrote:
>>>> After the improvement to high memory region address assignment is
>>>> applied, the memory layout is changed. For example, VIRT_HIGH_PCIE_MMIO
>>> s/the memory layout is changed./the memory layout is changed,
>>> introducing possible migration breakage.
>>
>> Ok, much clearer.
>>
>>>> memory region is enabled when the improvement is applied, but it's
>>>> disabled if the improvement isn't applied.
>>>>
>>>>       pa_bits              = 40;
>>>>       vms->highmem_redists = false;
>>>>       vms->highmem_ecam    = false;
>>>>       vms->highmem_mmio    = true;
>>>>
>>>>       # qemu-system-aarch64 -accel kvm -cpu host \
>>>>         -machine virt-7.2 -m 4G,maxmem=511G      \
>>>>         -monitor stdio
>>>>
>>>> In order to keep backwords compatibility, we need to disable the
>>>> optimization on machines, which is virt-7.1 or ealier than it. It
>>>> means the optimization is enabled by default from virt-7.2. Besides,
>>>> 'highmem-compact' property is added so that the optimization can be
>>> I would rather rename the property into compact-highmem even if the vms
>>> field is name highmem_compact to align with other highmem fields
>>
>> Ok, but I would love to know why. Note that we already have
>> 'highmem=on|off'. 'highmem_compact=on|off' seems consistent
>> to me.
> 
> FWIW, I initially misread 'highmem_compact' as 'highmem_compat' (and had
> to re-read because I got confused). At least to me, 'compact_highmem'
> has less chance of being parsed incorrectly :) (although that is
> probably a personal thing.)
> 

Ok. 'compact-highmem' is also fine to me. I'm really bad at naming :)

>>
>>>> explicitly enabled or disabled on all machine types by users.
>>>>
>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>> ---
>>>>    docs/system/arm/virt.rst |  4 ++++
>>>>    hw/arm/virt.c            | 33 +++++++++++++++++++++++++++++++++
>>>>    include/hw/arm/virt.h    |  2 ++
>>>>    3 files changed, 39 insertions(+)
>>>>
>>>> diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
>>>> index 20442ea2c1..f05ec2253b 100644
>>>> --- a/docs/system/arm/virt.rst
>>>> +++ b/docs/system/arm/virt.rst
>>>> @@ -94,6 +94,10 @@ highmem
>>>>      address space above 32 bits. The default is ``on`` for machine types
>>>>      later than ``virt-2.12``.
>>>>    
>>>> +highmem-compact
>>>> +  Set ``on``/``off`` to enable/disable compact space for high memory regions.
>>>> +  The default is ``on`` for machine types later than ``virt-7.2``
>>> I think you should document what is compact layout versus legacy one,
>>> both in the commit msg and maybe as a comment in a code along with the
>>> comment in hw/arm/virt.c starting with 'Highmem IO Regions: '
>>
>> Ok, I will add this into the commit log in v4. I don't think it's necessary
>> to add duplicate comment in the code. People can check the commit log for
>> details if needed.
> 
> Rather explain it in this file here, maybe? I'd prefer to be able to
> find out what 'compact' means without digging through the commit log.
> 

Ok, lets do as Eric suggested. There are existing comments about
@extended_memmap[] in hw/arm/virt.c. We need to explain the legacy/modern
laoyout and 'compact-highmem' property there.

Thanks,
Gavin

Re: [PATCH v3 1/5] hw/arm/virt: Introduce virt_set_high_memmap() helper

[Cornelia Huck]

On Thu, Sep 22 2022, Gavin Shan <gshan@redhat.com> wrote:

> This introduces virt_set_high_memmap() helper. The logic of high
> memory region address assignment is moved to the helper. The intention
> is to make the subsequent optimization for high memory region address
> assignment easier.
>
> No functional change intended.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  hw/arm/virt.c | 74 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 41 insertions(+), 33 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v3 3/5] hw/arm/virt: Introduce variable region_base in virt_set_high_memmap()

[Eric Auger]

Hi Gavin,

On 9/29/22 01:15, Gavin Shan wrote:
> Hi Eric,
>
> On 9/28/22 10:10 PM, Eric Auger wrote:
>> On 9/22/22 01:13, Gavin Shan wrote:
>>> This introduces variable 'region_base' for the base address of the
>>> specific high memory region. It's the preparatory work to optimize
>>> high memory region address assignment.
>> Why is it a preparatory work (same comment for previous patch, ie [2/5]
>> ). Are those changes really needed? why?
>>
>
> In PATCH[4/5], @base argument is added to virt_set_high_memmap(), to
> represent current global base address. With the optimization applied
> in PATCH[4/5], @base isn't unconditionally updated to the top of the
> iterated high memory region. So we need @region_base here (PATCH[3/5])
> to track the aligned base address for the iterated high memory region,
> which may or may be not updated to @base.
>
> Since we have @region_base in PATCH[3/5], it'd better to have
> @region_size
> in PATCH[2/5].
>
> Actually, PATCH[1-3/5] are all preparatory patches for PATCH[4/5]. My
> intention was to organize the patches in a way to keep the logical
> change part simple enough, for easier review.
OK fair enough

Eric
>
> Thanks,
> Gavin
>
>>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>> ---
>>>   hw/arm/virt.c | 12 ++++++------
>>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index 187b3ee0e2..b0b679d1f4 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -1692,15 +1692,15 @@ static uint64_t
>>> virt_cpu_mp_affinity(VirtMachineState *vms, int idx)
>>>   static void virt_set_high_memmap(VirtMachineState *vms,
>>>                                    hwaddr base, int pa_bits)
>>>   {
>>> -    hwaddr region_size;
>>> +    hwaddr region_base, region_size;
>>>       bool fits;
>>>       int i;
>>>         for (i = VIRT_LOWMEMMAP_LAST; i <
>>> ARRAY_SIZE(extended_memmap); i++) {
>>> +        region_base = ROUND_UP(base, extended_memmap[i].size);
>>>           region_size = extended_memmap[i].size;
>>>   -        base = ROUND_UP(base, region_size);
>>> -        vms->memmap[i].base = base;
>>> +        vms->memmap[i].base = region_base;
>>>           vms->memmap[i].size = region_size;
>>>             /*
>>> @@ -1709,9 +1709,9 @@ static void
>>> virt_set_high_memmap(VirtMachineState *vms,
>>>            *
>>>            * For each device that doesn't fit, disable it.
>>>            */
>>> -        fits = (base + region_size) <= BIT_ULL(pa_bits);
>>> +        fits = (region_base + region_size) <= BIT_ULL(pa_bits);
>>>           if (fits) {
>>> -            vms->highest_gpa = base + region_size - 1;
>>> +            vms->highest_gpa = region_base + region_size - 1;
>>>           }
>>>             switch (i) {
>>> @@ -1726,7 +1726,7 @@ static void
>>> virt_set_high_memmap(VirtMachineState *vms,
>>>               break;
>>>           }
>>>   -        base += region_size;
>>> +        base = region_base + region_size;
>>>       }
>>>   }
>>>   
>>
>

Re: [PATCH v3 2/5] hw/arm/virt: Rename variable size to region_size in virt_set_high_memmap()

[Eric Auger]

On 9/22/22 01:13, Gavin Shan wrote:
> This renames variable 'size' to 'region_size' in virt_set_high_memmap().
> Its counterpart ('region_base') will be introduced in next patch.
>
> No functional change intended.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  hw/arm/virt.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 4dab528b82..187b3ee0e2 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1692,15 +1692,16 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx)
>  static void virt_set_high_memmap(VirtMachineState *vms,
>                                   hwaddr base, int pa_bits)
>  {
> +    hwaddr region_size;
> +    bool fits;
>      int i;
>  
>      for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
> -        hwaddr size = extended_memmap[i].size;
> -        bool fits;
> +        region_size = extended_memmap[i].size;
>  
> -        base = ROUND_UP(base, size);
> +        base = ROUND_UP(base, region_size);
>          vms->memmap[i].base = base;
> -        vms->memmap[i].size = size;
> +        vms->memmap[i].size = region_size;
>  
>          /*
>           * Check each device to see if they fit in the PA space,
> @@ -1708,9 +1709,9 @@ static void virt_set_high_memmap(VirtMachineState *vms,
>           *
>           * For each device that doesn't fit, disable it.
>           */
> -        fits = (base + size) <= BIT_ULL(pa_bits);
> +        fits = (base + region_size) <= BIT_ULL(pa_bits);
>          if (fits) {
> -            vms->highest_gpa = base + size - 1;
> +            vms->highest_gpa = base + region_size - 1;
>          }
>  
>          switch (i) {
> @@ -1725,7 +1726,7 @@ static void virt_set_high_memmap(VirtMachineState *vms,
>              break;
>          }
>  
> -        base += size;
> +        base += region_size;
>      }
>  }
>  

Re: [PATCH v3 3/5] hw/arm/virt: Introduce variable region_base in virt_set_high_memmap()

[Eric Auger]

On 9/22/22 01:13, Gavin Shan wrote:
> This introduces variable 'region_base' for the base address of the
> specific high memory region. It's the preparatory work to optimize
> high memory region address assignment.
>
> No functional change intended.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  hw/arm/virt.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 187b3ee0e2..b0b679d1f4 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1692,15 +1692,15 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx)
>  static void virt_set_high_memmap(VirtMachineState *vms,
>                                   hwaddr base, int pa_bits)
>  {
> -    hwaddr region_size;
> +    hwaddr region_base, region_size;
>      bool fits;
>      int i;
>  
>      for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
> +        region_base = ROUND_UP(base, extended_memmap[i].size);
>          region_size = extended_memmap[i].size;
>  
> -        base = ROUND_UP(base, region_size);
> -        vms->memmap[i].base = base;
> +        vms->memmap[i].base = region_base;
>          vms->memmap[i].size = region_size;
>  
>          /*
> @@ -1709,9 +1709,9 @@ static void virt_set_high_memmap(VirtMachineState *vms,
>           *
>           * For each device that doesn't fit, disable it.
>           */
> -        fits = (base + region_size) <= BIT_ULL(pa_bits);
> +        fits = (region_base + region_size) <= BIT_ULL(pa_bits);
>          if (fits) {
> -            vms->highest_gpa = base + region_size - 1;
> +            vms->highest_gpa = region_base + region_size - 1;
>          }
>  
>          switch (i) {
> @@ -1726,7 +1726,7 @@ static void virt_set_high_memmap(VirtMachineState *vms,
>              break;
>          }
>  
> -        base += region_size;
> +        base = region_base + region_size;
>      }
>  }
>  

Re: [PATCH v3 4/5] hw/arm/virt: Improve high memory region address assignment

[Eric Auger]

Hi Gavin,

On 9/29/22 01:37, Gavin Shan wrote:
> Hi Eric,
>
> On 9/28/22 10:51 PM, Eric Auger wrote:
>> On 9/22/22 01:13, Gavin Shan wrote:
>>> There are three high memory regions, which are VIRT_HIGH_REDIST2,
>>> VIRT_HIGH_PCIE_ECAM and VIRT_HIGH_PCIE_MMIO. Their base addresses
>>> are floating on highest RAM address. However, they can be disabled
>>> in several cases.
>>>
>>> (1) One specific high memory region is disabled by developer by
>>>      toggling vms->highmem_{redists, ecam, mmio}.
>>>
>>> (2) VIRT_HIGH_PCIE_ECAM region is disabled on machine, which is
>>>      'virt-2.12' or ealier than it.
>>>
>>> (3) VIRT_HIGH_PCIE_ECAM region is disabled when firmware is loaded
>>>      on 32-bits system.
>>>
>>> (4) One specific high memory region is disabled when it breaks the
>>>      PA space limit.
>>>
>>> The current implementation of virt_set_memmap() isn't comprehensive
>>> because the space for one specific high memory region is always
>>> reserved from the PA space for case (1), (2) and (3). In the code,
>>> 'base' and 'vms->highest_gpa' are always increased for those three
>>> cases. It's unnecessary since the assigned space of the disabled
>>> high memory region won't be used afterwards.
>>>
>>> This improves the address assignment for those three high memory
>>> region by skipping the address assignment for one specific high
>>> memory region if it has been disabled in case (1), (2) and (3).
>>>
>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>> ---
>>>   hw/arm/virt.c | 44 ++++++++++++++++++++++++++------------------
>>>   1 file changed, 26 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index b0b679d1f4..b702f8f2b5 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -1693,15 +1693,31 @@ static void
>>> virt_set_high_memmap(VirtMachineState *vms,
>>>                                    hwaddr base, int pa_bits)
>>>   {
>>>       hwaddr region_base, region_size;
>>> -    bool fits;
>>> +    bool *region_enabled, fits;
>> IDo you really need a pointer? If the region is unknown this is a bug in
>> virt code.
>
> The pointer is needed so that we can disable the region by setting
> 'false'
> to it at later point. Yeah, I think you're correct that 'unknown region'
> is a bug and we need to do assert(region_enabled), or something like
> below.
Yeah I don't think using a pointer here is useful.
>
>>>       int i;
>>>         for (i = VIRT_LOWMEMMAP_LAST; i <
>>> ARRAY_SIZE(extended_memmap); i++) {
>>>           region_base = ROUND_UP(base, extended_memmap[i].size);
>>>           region_size = extended_memmap[i].size;
>>>   -        vms->memmap[i].base = region_base;
>>> -        vms->memmap[i].size = region_size;
>>> +        switch (i) {
>>> +        case VIRT_HIGH_GIC_REDIST2:
>>> +            region_enabled = &vms->highmem_redists;
>>> +            break;
>>> +        case VIRT_HIGH_PCIE_ECAM:
>>> +            region_enabled = &vms->highmem_ecam;
>>> +            break;
>>> +        case VIRT_HIGH_PCIE_MMIO:
>>> +            region_enabled = &vms->highmem_mmio;
>>> +            break;
>> While we are at it I would change the vms fields dealing with those
>> highmem regions and turn those fields into an array of bool indexed
>> using i - VIRT_LOWMEMMAP_LAST (using a macro or something alike). We
>> would not be obliged to have this switch, now duplicated.
>
> It makes sense to me. How about to have something like below in v4?
>
> static inline bool *virt_get_high_memmap_enabled(VirtMachineState
> *vms, int index)
> {
>     bool *enabled_array[] = {
>           &vms->highmem_redists,
>           &vms->highmem_ecam,
>           &vms->highmem_mmio,
>     };
>
>     assert(index - VIRT_LOWMEMMAP_LAST < ARRAY_SIZE(enabled_array));
>
>     return enabled_array[index - VIRT_LOWMEMMAP_LAST];
> } 
I was rather thinking as directly using a vms->highmem_flags[] but your
proposal may work as well.
>
>>> +        default:
>>> +            region_enabled = NULL;
>>> +        }
>>> +
>>> +        /* Skip unknown region */
>>> +        if (!region_enabled) {
>>> +            continue;
>>> +        }
>>>             /*
>>>            * Check each device to see if they fit in the PA space,
>>> @@ -1710,23 +1726,15 @@ static void
>>> virt_set_high_memmap(VirtMachineState *vms,
>>>            * For each device that doesn't fit, disable it.
>>>            */
>>>           fits = (region_base + region_size) <= BIT_ULL(pa_bits);
>>> -        if (fits) {
>>> -            vms->highest_gpa = region_base + region_size - 1;
>>> -        }
>>> +        if (*region_enabled && fits) {
>>> +            vms->memmap[i].base = region_base;
>>> +            vms->memmap[i].size = region_size;
>>>   -        switch (i) {
>>> -        case VIRT_HIGH_GIC_REDIST2:
>>> -            vms->highmem_redists &= fits;
>>> -            break;
>>> -        case VIRT_HIGH_PCIE_ECAM:
>>> -            vms->highmem_ecam &= fits;
>>> -            break;
>>> -        case VIRT_HIGH_PCIE_MMIO:
>>> -            vms->highmem_mmio &= fits;
>>> -            break;
>>> +            vms->highest_gpa = region_base + region_size - 1;
>>> +            base = region_base + region_size;
>>> +        } else {
>>> +            *region_enabled = false;
what's the purpose to update the region_enabled here? Is it used anywhere?

The fact you do not update vms->highmem_* flags may introduce
regressions I think as the resulting flag may be used in some places
such as:
virt_gicv3_redist_region_count().

>>> -
>>> -        base = region_base + region_size;
>>>       }
>>>   }
>>>   
>
> Thanks,
> Gavin
>
Thanks

Eric

Re: [PATCH v3 5/5] hw/arm/virt: Add 'highmem-compact' property

[Eric Auger]

Hi Gavin,

On 9/29/22 01:49, Gavin Shan wrote:
> Hi Eric,
>
> On 9/28/22 10:22 PM, Eric Auger wrote:
>> On 9/22/22 01:13, Gavin Shan wrote:
>>> After the improvement to high memory region address assignment is
>>> applied, the memory layout is changed. For example, VIRT_HIGH_PCIE_MMIO
>> s/the memory layout is changed./the memory layout is changed,
>> introducing possible migration breakage.
>
> Ok, much clearer.
>
>>> memory region is enabled when the improvement is applied, but it's
>>> disabled if the improvement isn't applied.
>>>
>>>      pa_bits              = 40;
>>>      vms->highmem_redists = false;
>>>      vms->highmem_ecam    = false;
>>>      vms->highmem_mmio    = true;
>>>
>>>      # qemu-system-aarch64 -accel kvm -cpu host \
>>>        -machine virt-7.2 -m 4G,maxmem=511G      \
>>>        -monitor stdio
>>>
>>> In order to keep backwords compatibility, we need to disable the
>>> optimization on machines, which is virt-7.1 or ealier than it. It
>>> means the optimization is enabled by default from virt-7.2. Besides,
>>> 'highmem-compact' property is added so that the optimization can be
>> I would rather rename the property into compact-highmem even if the vms
>> field is name highmem_compact to align with other highmem fields
>
> Ok, but I would love to know why. Note that we already have
> 'highmem=on|off'. 'highmem_compact=on|off' seems consistent
> to me.
To me the property name should rather sound 'english' with the adjective
before the name 'high memory"' but I am not a native english speaker
either.
>
>>> explicitly enabled or disabled on all machine types by users.
>>>
>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>> ---
>>>   docs/system/arm/virt.rst |  4 ++++
>>>   hw/arm/virt.c            | 33 +++++++++++++++++++++++++++++++++
>>>   include/hw/arm/virt.h    |  2 ++
>>>   3 files changed, 39 insertions(+)
>>>
>>> diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
>>> index 20442ea2c1..f05ec2253b 100644
>>> --- a/docs/system/arm/virt.rst
>>> +++ b/docs/system/arm/virt.rst
>>> @@ -94,6 +94,10 @@ highmem
>>>     address space above 32 bits. The default is ``on`` for machine
>>> types
>>>     later than ``virt-2.12``.
>>>   +highmem-compact
>>> +  Set ``on``/``off`` to enable/disable compact space for high
>>> memory regions.
>>> +  The default is ``on`` for machine types later than ``virt-7.2``
>> I think you should document what is compact layout versus legacy one,
>> both in the commit msg and maybe as a comment in a code along with the
>> comment in hw/arm/virt.c starting with 'Highmem IO Regions: '
>
> Ok, I will add this into the commit log in v4. I don't think it's
> necessary
> to add duplicate comment in the code. People can check the commit log for
> details if needed.
>
>>> +
>>>   gic-version
>>>     Specify the version of the Generic Interrupt Controller (GIC) to
>>> provide.
>>>     Valid values are:
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index b702f8f2b5..a4fbdaef91 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -1734,6 +1734,13 @@ static void
>>> virt_set_high_memmap(VirtMachineState *vms,
>>>               base = region_base + region_size;
>>>           } else {
>>>               *region_enabled = false;
>>> +
>>> +            if (!vms->highmem_compact) {
>> this snippet should be already present in previous patch otherwise this
>> will break bisectability.
>>
>
> Hmm, nice catch! I think I need to swap PATCH[4] and PATCH[5] in next
> revision. In that order, 'compact-highmem' is introduced in PATCH[4],
> but not used yet. PATCH[5] has the optimization and 'compact-highmem'
> is used.
No in general you introduce the property at the very end with the code
guarded with an unset vms->highmem_compact in the previous patch.

Thanks

Eric
>
>>> +                base = region_base + region_size;
>>> +                if (fits) {
>>> +                    vms->highest_gpa = region_base + region_size - 1;
>>> +                }
>>> +            }
>>>           }
>>>       }
>>>   }
>>> @@ -2348,6 +2355,20 @@ static void virt_set_highmem(Object *obj,
>>> bool value, Error **errp)
>>>       vms->highmem = value;
>>>   }
>>>   +static bool virt_get_highmem_compact(Object *obj, Error **errp)
>>> +{
>>> +    VirtMachineState *vms = VIRT_MACHINE(obj);
>>> +
>>> +    return vms->highmem_compact;
>>> +}
>>> +
>>> +static void virt_set_highmem_compact(Object *obj, bool value, Error
>>> **errp)
>>> +{
>>> +    VirtMachineState *vms = VIRT_MACHINE(obj);
>>> +
>>> +    vms->highmem_compact = value;
>>> +}
>>> +
>>>   static bool virt_get_its(Object *obj, Error **errp)
>>>   {
>>>       VirtMachineState *vms = VIRT_MACHINE(obj);
>>> @@ -2966,6 +2987,13 @@ static void
>>> virt_machine_class_init(ObjectClass *oc, void *data)
>>>                                             "Set on/off to
>>> enable/disable using "
>>>                                             "physical address space
>>> above 32 bits");
>>>   +    object_class_property_add_bool(oc, "highmem-compact",
>>> +                                   virt_get_highmem_compact,
>>> +                                   virt_set_highmem_compact);
>>> +    object_class_property_set_description(oc, "highmem-compact",
>>> +                                          "Set on/off to
>>> enable/disable compact "
>>> +                                          "space for high memory
>>> regions");
>>> +
>>>       object_class_property_add_str(oc, "gic-version",
>>> virt_get_gic_version,
>>>                                     virt_set_gic_version);
>>>       object_class_property_set_description(oc, "gic-version",
>>> @@ -3050,6 +3078,7 @@ static void virt_instance_init(Object *obj)
>>>         /* High memory is enabled by default */
>>>       vms->highmem = true;
>>> +    vms->highmem_compact = !vmc->no_highmem_compact;
>>>       vms->gic_version = VIRT_GIC_VERSION_NOSEL;
>>>         vms->highmem_ecam = !vmc->no_highmem_ecam;
>>> @@ -3119,8 +3148,12 @@ DEFINE_VIRT_MACHINE_AS_LATEST(7, 2)
>>>     static void virt_machine_7_1_options(MachineClass *mc)
>>>   {
>>> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
>>> +
>>>       virt_machine_7_2_options(mc);
>>>       compat_props_add(mc->compat_props, hw_compat_7_1,
>>> hw_compat_7_1_len);
>>> +    /* Compact space for high memory regions was introduced with
>>> 7.2 */
>>> +    vmc->no_highmem_compact = true;
>>>   }
>>>   DEFINE_VIRT_MACHINE(7, 1)
>>>   diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>>> index 6ec479ca2b..c7dd59d7f1 100644
>>> --- a/include/hw/arm/virt.h
>>> +++ b/include/hw/arm/virt.h
>>> @@ -125,6 +125,7 @@ struct VirtMachineClass {
>>>       bool no_pmu;
>>>       bool claim_edge_triggered_timers;
>>>       bool smbios_old_sys_ver;
>>> +    bool no_highmem_compact;
>>>       bool no_highmem_ecam;
>>>       bool no_ged;   /* Machines < 4.2 have no support for ACPI GED
>>> device */
>>>       bool kvm_no_adjvtime;
>>> @@ -144,6 +145,7 @@ struct VirtMachineState {
>>>       PFlashCFI01 *flash[2];
>>>       bool secure;
>>>       bool highmem;
>>> +    bool highmem_compact;
>>>       bool highmem_ecam;
>>>       bool highmem_mmio;
>>>       bool highmem_redists;
>>
>
> Thanks,
> Gavin
>

Re: [PATCH v3 4/5] hw/arm/virt: Improve high memory region address assignment

[Gavin Shan]

Hi Eric,

On 10/3/22 4:44 PM, Eric Auger wrote:
> On 9/29/22 01:37, Gavin Shan wrote:
>> On 9/28/22 10:51 PM, Eric Auger wrote:
>>> On 9/22/22 01:13, Gavin Shan wrote:
>>>> There are three high memory regions, which are VIRT_HIGH_REDIST2,
>>>> VIRT_HIGH_PCIE_ECAM and VIRT_HIGH_PCIE_MMIO. Their base addresses
>>>> are floating on highest RAM address. However, they can be disabled
>>>> in several cases.
>>>>
>>>> (1) One specific high memory region is disabled by developer by
>>>>       toggling vms->highmem_{redists, ecam, mmio}.
>>>>
>>>> (2) VIRT_HIGH_PCIE_ECAM region is disabled on machine, which is
>>>>       'virt-2.12' or ealier than it.
>>>>
>>>> (3) VIRT_HIGH_PCIE_ECAM region is disabled when firmware is loaded
>>>>       on 32-bits system.
>>>>
>>>> (4) One specific high memory region is disabled when it breaks the
>>>>       PA space limit.
>>>>
>>>> The current implementation of virt_set_memmap() isn't comprehensive
>>>> because the space for one specific high memory region is always
>>>> reserved from the PA space for case (1), (2) and (3). In the code,
>>>> 'base' and 'vms->highest_gpa' are always increased for those three
>>>> cases. It's unnecessary since the assigned space of the disabled
>>>> high memory region won't be used afterwards.
>>>>
>>>> This improves the address assignment for those three high memory
>>>> region by skipping the address assignment for one specific high
>>>> memory region if it has been disabled in case (1), (2) and (3).
>>>>
>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>> ---
>>>>    hw/arm/virt.c | 44 ++++++++++++++++++++++++++------------------
>>>>    1 file changed, 26 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>>> index b0b679d1f4..b702f8f2b5 100644
>>>> --- a/hw/arm/virt.c
>>>> +++ b/hw/arm/virt.c
>>>> @@ -1693,15 +1693,31 @@ static void
>>>> virt_set_high_memmap(VirtMachineState *vms,
>>>>                                     hwaddr base, int pa_bits)
>>>>    {
>>>>        hwaddr region_base, region_size;
>>>> -    bool fits;
>>>> +    bool *region_enabled, fits;
>>> IDo you really need a pointer? If the region is unknown this is a bug in
>>> virt code.
>>
>> The pointer is needed so that we can disable the region by setting
>> 'false'
>> to it at later point. Yeah, I think you're correct that 'unknown region'
>> is a bug and we need to do assert(region_enabled), or something like
>> below.
> Yeah I don't think using a pointer here is useful.

When the high memory region can't fit into the PA space, it is disabled
by toggling the corresponding flag (vms->highmem_{redists, ecam, mmio})
to false. It's part of the original implementation, as below. We either
need a 'switch ... case' or a pointer. A pointer is more convenient since
we need check and possibly update to the value.

        switch (i) {
         case VIRT_HIGH_GIC_REDIST2:
             vms->highmem_redists &= fits;
             break;
         case VIRT_HIGH_PCIE_ECAM:
             vms->highmem_ecam &= fits;
             break;
         case VIRT_HIGH_PCIE_MMIO:
             vms->highmem_mmio &= fits;
             break;
         }

>>
>>>>        int i;
>>>>          for (i = VIRT_LOWMEMMAP_LAST; i <
>>>> ARRAY_SIZE(extended_memmap); i++) {
>>>>            region_base = ROUND_UP(base, extended_memmap[i].size);
>>>>            region_size = extended_memmap[i].size;
>>>>    -        vms->memmap[i].base = region_base;
>>>> -        vms->memmap[i].size = region_size;
>>>> +        switch (i) {
>>>> +        case VIRT_HIGH_GIC_REDIST2:
>>>> +            region_enabled = &vms->highmem_redists;
>>>> +            break;
>>>> +        case VIRT_HIGH_PCIE_ECAM:
>>>> +            region_enabled = &vms->highmem_ecam;
>>>> +            break;
>>>> +        case VIRT_HIGH_PCIE_MMIO:
>>>> +            region_enabled = &vms->highmem_mmio;
>>>> +            break;
>>> While we are at it I would change the vms fields dealing with those
>>> highmem regions and turn those fields into an array of bool indexed
>>> using i - VIRT_LOWMEMMAP_LAST (using a macro or something alike). We
>>> would not be obliged to have this switch, now duplicated.
>>
>> It makes sense to me. How about to have something like below in v4?
>>
>> static inline bool *virt_get_high_memmap_enabled(VirtMachineState
>> *vms, int index)
>> {
>>      bool *enabled_array[] = {
>>            &vms->highmem_redists,
>>            &vms->highmem_ecam,
>>            &vms->highmem_mmio,
>>      };
>>
>>      assert(index - VIRT_LOWMEMMAP_LAST < ARRAY_SIZE(enabled_array));
>>
>>      return enabled_array[index - VIRT_LOWMEMMAP_LAST];
>> }
> I was rather thinking as directly using a vms->highmem_flags[] but your
> proposal may work as well.

Ok. I will use my proposed change in next revision.

>>
>>>> +        default:
>>>> +            region_enabled = NULL;
>>>> +        }
>>>> +
>>>> +        /* Skip unknown region */
>>>> +        if (!region_enabled) {
>>>> +            continue;
>>>> +        }
>>>>              /*
>>>>             * Check each device to see if they fit in the PA space,
>>>> @@ -1710,23 +1726,15 @@ static void
>>>> virt_set_high_memmap(VirtMachineState *vms,
>>>>             * For each device that doesn't fit, disable it.
>>>>             */
>>>>            fits = (region_base + region_size) <= BIT_ULL(pa_bits);
>>>> -        if (fits) {
>>>> -            vms->highest_gpa = region_base + region_size - 1;
>>>> -        }
>>>> +        if (*region_enabled && fits) {
>>>> +            vms->memmap[i].base = region_base;
>>>> +            vms->memmap[i].size = region_size;
>>>>    -        switch (i) {
>>>> -        case VIRT_HIGH_GIC_REDIST2:
>>>> -            vms->highmem_redists &= fits;
>>>> -            break;
>>>> -        case VIRT_HIGH_PCIE_ECAM:
>>>> -            vms->highmem_ecam &= fits;
>>>> -            break;
>>>> -        case VIRT_HIGH_PCIE_MMIO:
>>>> -            vms->highmem_mmio &= fits;
>>>> -            break;
>>>> +            vms->highest_gpa = region_base + region_size - 1;
>>>> +            base = region_base + region_size;
>>>> +        } else {
>>>> +            *region_enabled = false;
> what's the purpose to update the region_enabled here? Is it used anywhere?
> 
> The fact you do not update vms->highmem_* flags may introduce
> regressions I think as the resulting flag may be used in some places
> such as:
> virt_gicv3_redist_region_count().
> 

'region_enabled' points to 'vms->highmem_{redist2, ecam, mmio}'. They
are same thing.

>>>> -
>>>> -        base = region_base + region_size;
>>>>        }
>>>>    }
>>>>    

Thanks,
Gavin

Re: [PATCH v3 5/5] hw/arm/virt: Add 'highmem-compact' property

[Gavin Shan]

Hi Eric,

On 10/3/22 4:49 PM, Eric Auger wrote:
> On 9/29/22 01:49, Gavin Shan wrote:
>> On 9/28/22 10:22 PM, Eric Auger wrote:
>>> On 9/22/22 01:13, Gavin Shan wrote:
>>>> After the improvement to high memory region address assignment is
>>>> applied, the memory layout is changed. For example, VIRT_HIGH_PCIE_MMIO
>>> s/the memory layout is changed./the memory layout is changed,
>>> introducing possible migration breakage.
>>
>> Ok, much clearer.
>>
>>>> memory region is enabled when the improvement is applied, but it's
>>>> disabled if the improvement isn't applied.
>>>>
>>>>       pa_bits              = 40;
>>>>       vms->highmem_redists = false;
>>>>       vms->highmem_ecam    = false;
>>>>       vms->highmem_mmio    = true;
>>>>
>>>>       # qemu-system-aarch64 -accel kvm -cpu host \
>>>>         -machine virt-7.2 -m 4G,maxmem=511G      \
>>>>         -monitor stdio
>>>>
>>>> In order to keep backwords compatibility, we need to disable the
>>>> optimization on machines, which is virt-7.1 or ealier than it. It
>>>> means the optimization is enabled by default from virt-7.2. Besides,
>>>> 'highmem-compact' property is added so that the optimization can be
>>> I would rather rename the property into compact-highmem even if the vms
>>> field is name highmem_compact to align with other highmem fields
>>
>> Ok, but I would love to know why. Note that we already have
>> 'highmem=on|off'. 'highmem_compact=on|off' seems consistent
>> to me.
> To me the property name should rather sound 'english' with the adjective
> before the name 'high memory"' but I am not a native english speaker
> either.

Ok. I agree 'compact-highmem' is better. The backup variable name will
be still 'highmem_compact', which is consistent with the existing ones.

>>
>>>> explicitly enabled or disabled on all machine types by users.
>>>>
>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>> ---
>>>>    docs/system/arm/virt.rst |  4 ++++
>>>>    hw/arm/virt.c            | 33 +++++++++++++++++++++++++++++++++
>>>>    include/hw/arm/virt.h    |  2 ++
>>>>    3 files changed, 39 insertions(+)
>>>>
>>>> diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
>>>> index 20442ea2c1..f05ec2253b 100644
>>>> --- a/docs/system/arm/virt.rst
>>>> +++ b/docs/system/arm/virt.rst
>>>> @@ -94,6 +94,10 @@ highmem
>>>>      address space above 32 bits. The default is ``on`` for machine
>>>> types
>>>>      later than ``virt-2.12``.
>>>>    +highmem-compact
>>>> +  Set ``on``/``off`` to enable/disable compact space for high
>>>> memory regions.
>>>> +  The default is ``on`` for machine types later than ``virt-7.2``
>>> I think you should document what is compact layout versus legacy one,
>>> both in the commit msg and maybe as a comment in a code along with the
>>> comment in hw/arm/virt.c starting with 'Highmem IO Regions: '
>>
>> Ok, I will add this into the commit log in v4. I don't think it's
>> necessary
>> to add duplicate comment in the code. People can check the commit log for
>> details if needed.
>>
>>>> +
>>>>    gic-version
>>>>      Specify the version of the Generic Interrupt Controller (GIC) to
>>>> provide.
>>>>      Valid values are:
>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>>> index b702f8f2b5..a4fbdaef91 100644
>>>> --- a/hw/arm/virt.c
>>>> +++ b/hw/arm/virt.c
>>>> @@ -1734,6 +1734,13 @@ static void
>>>> virt_set_high_memmap(VirtMachineState *vms,
>>>>                base = region_base + region_size;
>>>>            } else {
>>>>                *region_enabled = false;
>>>> +
>>>> +            if (!vms->highmem_compact) {
>>> this snippet should be already present in previous patch otherwise this
>>> will break bisectability.
>>>
>>
>> Hmm, nice catch! I think I need to swap PATCH[4] and PATCH[5] in next
>> revision. In that order, 'compact-highmem' is introduced in PATCH[4],
>> but not used yet. PATCH[5] has the optimization and 'compact-highmem'
>> is used.
> No in general you introduce the property at the very end with the code
> guarded with an unset vms->highmem_compact in the previous patch.
> 

Yeah, what I need is define 'vms->highmem_compact' in PATCH[v3 4/5],
whose value is false. I also need to update @base and @vms->highest_gpa
on !vms->highmem_compact' in PATCH[v3 4/5].

>>
>>>> +                base = region_base + region_size;
>>>> +                if (fits) {
>>>> +                    vms->highest_gpa = region_base + region_size - 1;
>>>> +                }
>>>> +            }
>>>>            }
>>>>        }
>>>>    }
>>>> @@ -2348,6 +2355,20 @@ static void virt_set_highmem(Object *obj,
>>>> bool value, Error **errp)
>>>>        vms->highmem = value;
>>>>    }
>>>>    +static bool virt_get_highmem_compact(Object *obj, Error **errp)
>>>> +{
>>>> +    VirtMachineState *vms = VIRT_MACHINE(obj);
>>>> +
>>>> +    return vms->highmem_compact;
>>>> +}
>>>> +
>>>> +static void virt_set_highmem_compact(Object *obj, bool value, Error
>>>> **errp)
>>>> +{
>>>> +    VirtMachineState *vms = VIRT_MACHINE(obj);
>>>> +
>>>> +    vms->highmem_compact = value;
>>>> +}
>>>> +
>>>>    static bool virt_get_its(Object *obj, Error **errp)
>>>>    {
>>>>        VirtMachineState *vms = VIRT_MACHINE(obj);
>>>> @@ -2966,6 +2987,13 @@ static void
>>>> virt_machine_class_init(ObjectClass *oc, void *data)
>>>>                                              "Set on/off to
>>>> enable/disable using "
>>>>                                              "physical address space
>>>> above 32 bits");
>>>>    +    object_class_property_add_bool(oc, "highmem-compact",
>>>> +                                   virt_get_highmem_compact,
>>>> +                                   virt_set_highmem_compact);
>>>> +    object_class_property_set_description(oc, "highmem-compact",
>>>> +                                          "Set on/off to
>>>> enable/disable compact "
>>>> +                                          "space for high memory
>>>> regions");
>>>> +
>>>>        object_class_property_add_str(oc, "gic-version",
>>>> virt_get_gic_version,
>>>>                                      virt_set_gic_version);
>>>>        object_class_property_set_description(oc, "gic-version",
>>>> @@ -3050,6 +3078,7 @@ static void virt_instance_init(Object *obj)
>>>>          /* High memory is enabled by default */
>>>>        vms->highmem = true;
>>>> +    vms->highmem_compact = !vmc->no_highmem_compact;
>>>>        vms->gic_version = VIRT_GIC_VERSION_NOSEL;
>>>>          vms->highmem_ecam = !vmc->no_highmem_ecam;
>>>> @@ -3119,8 +3148,12 @@ DEFINE_VIRT_MACHINE_AS_LATEST(7, 2)
>>>>      static void virt_machine_7_1_options(MachineClass *mc)
>>>>    {
>>>> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
>>>> +
>>>>        virt_machine_7_2_options(mc);
>>>>        compat_props_add(mc->compat_props, hw_compat_7_1,
>>>> hw_compat_7_1_len);
>>>> +    /* Compact space for high memory regions was introduced with
>>>> 7.2 */
>>>> +    vmc->no_highmem_compact = true;
>>>>    }
>>>>    DEFINE_VIRT_MACHINE(7, 1)
>>>>    diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>>>> index 6ec479ca2b..c7dd59d7f1 100644
>>>> --- a/include/hw/arm/virt.h
>>>> +++ b/include/hw/arm/virt.h
>>>> @@ -125,6 +125,7 @@ struct VirtMachineClass {
>>>>        bool no_pmu;
>>>>        bool claim_edge_triggered_timers;
>>>>        bool smbios_old_sys_ver;
>>>> +    bool no_highmem_compact;
>>>>        bool no_highmem_ecam;
>>>>        bool no_ged;   /* Machines < 4.2 have no support for ACPI GED
>>>> device */
>>>>        bool kvm_no_adjvtime;
>>>> @@ -144,6 +145,7 @@ struct VirtMachineState {
>>>>        PFlashCFI01 *flash[2];
>>>>        bool secure;
>>>>        bool highmem;
>>>> +    bool highmem_compact;
>>>>        bool highmem_ecam;
>>>>        bool highmem_mmio;
>>>>        bool highmem_redists;
>>>

Thanks,
Gavin

Re: [PATCH v3 4/5] hw/arm/virt: Improve high memory region address assignment

[Eric Auger]

On 10/4/22 00:17, Gavin Shan wrote:
> Hi Eric,
>
> On 10/3/22 4:44 PM, Eric Auger wrote:
>> On 9/29/22 01:37, Gavin Shan wrote:
>>> On 9/28/22 10:51 PM, Eric Auger wrote:
>>>> On 9/22/22 01:13, Gavin Shan wrote:
>>>>> There are three high memory regions, which are VIRT_HIGH_REDIST2,
>>>>> VIRT_HIGH_PCIE_ECAM and VIRT_HIGH_PCIE_MMIO. Their base addresses
>>>>> are floating on highest RAM address. However, they can be disabled
>>>>> in several cases.
>>>>>
>>>>> (1) One specific high memory region is disabled by developer by
>>>>>       toggling vms->highmem_{redists, ecam, mmio}.
>>>>>
>>>>> (2) VIRT_HIGH_PCIE_ECAM region is disabled on machine, which is
>>>>>       'virt-2.12' or ealier than it.
>>>>>
>>>>> (3) VIRT_HIGH_PCIE_ECAM region is disabled when firmware is loaded
>>>>>       on 32-bits system.
>>>>>
>>>>> (4) One specific high memory region is disabled when it breaks the
>>>>>       PA space limit.
>>>>>
>>>>> The current implementation of virt_set_memmap() isn't comprehensive
>>>>> because the space for one specific high memory region is always
>>>>> reserved from the PA space for case (1), (2) and (3). In the code,
>>>>> 'base' and 'vms->highest_gpa' are always increased for those three
>>>>> cases. It's unnecessary since the assigned space of the disabled
>>>>> high memory region won't be used afterwards.
>>>>>
>>>>> This improves the address assignment for those three high memory
>>>>> region by skipping the address assignment for one specific high
>>>>> memory region if it has been disabled in case (1), (2) and (3).
>>>>>
>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>>> ---
>>>>>    hw/arm/virt.c | 44 ++++++++++++++++++++++++++------------------
>>>>>    1 file changed, 26 insertions(+), 18 deletions(-)
>>>>>
>>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>>>> index b0b679d1f4..b702f8f2b5 100644
>>>>> --- a/hw/arm/virt.c
>>>>> +++ b/hw/arm/virt.c
>>>>> @@ -1693,15 +1693,31 @@ static void
>>>>> virt_set_high_memmap(VirtMachineState *vms,
>>>>>                                     hwaddr base, int pa_bits)
>>>>>    {
>>>>>        hwaddr region_base, region_size;
>>>>> -    bool fits;
>>>>> +    bool *region_enabled, fits;
>>>> IDo you really need a pointer? If the region is unknown this is a
>>>> bug in
>>>> virt code.
>>>
>>> The pointer is needed so that we can disable the region by setting
>>> 'false'
>>> to it at later point. Yeah, I think you're correct that 'unknown
>>> region'
>>> is a bug and we need to do assert(region_enabled), or something like
>>> below.
>> Yeah I don't think using a pointer here is useful.
>
> When the high memory region can't fit into the PA space, it is disabled
> by toggling the corresponding flag (vms->highmem_{redists, ecam, mmio})
> to false. It's part of the original implementation, as below. We either
> need a 'switch ... case' or a pointer. A pointer is more convenient since
> we need check and possibly update to the value.
>
>        switch (i) {
>         case VIRT_HIGH_GIC_REDIST2:
>             vms->highmem_redists &= fits;
>             break;
>         case VIRT_HIGH_PCIE_ECAM:
>             vms->highmem_ecam &= fits;
>             break;
>         case VIRT_HIGH_PCIE_MMIO:
>             vms->highmem_mmio &= fits;
>             break;
>         }
>
>>>
>>>>>        int i;
>>>>>          for (i = VIRT_LOWMEMMAP_LAST; i <
>>>>> ARRAY_SIZE(extended_memmap); i++) {
>>>>>            region_base = ROUND_UP(base, extended_memmap[i].size);
>>>>>            region_size = extended_memmap[i].size;
>>>>>    -        vms->memmap[i].base = region_base;
>>>>> -        vms->memmap[i].size = region_size;
>>>>> +        switch (i) {
>>>>> +        case VIRT_HIGH_GIC_REDIST2:
>>>>> +            region_enabled = &vms->highmem_redists;
>>>>> +            break;
>>>>> +        case VIRT_HIGH_PCIE_ECAM:
>>>>> +            region_enabled = &vms->highmem_ecam;
>>>>> +            break;
>>>>> +        case VIRT_HIGH_PCIE_MMIO:
>>>>> +            region_enabled = &vms->highmem_mmio;
>>>>> +            break;
>>>> While we are at it I would change the vms fields dealing with those
>>>> highmem regions and turn those fields into an array of bool indexed
>>>> using i - VIRT_LOWMEMMAP_LAST (using a macro or something alike). We
>>>> would not be obliged to have this switch, now duplicated.
>>>
>>> It makes sense to me. How about to have something like below in v4?
>>>
>>> static inline bool *virt_get_high_memmap_enabled(VirtMachineState
>>> *vms, int index)
>>> {
>>>      bool *enabled_array[] = {
>>>            &vms->highmem_redists,
>>>            &vms->highmem_ecam,
>>>            &vms->highmem_mmio,
>>>      };
>>>
>>>      assert(index - VIRT_LOWMEMMAP_LAST < ARRAY_SIZE(enabled_array));
>>>
>>>      return enabled_array[index - VIRT_LOWMEMMAP_LAST];
>>> }
>> I was rather thinking as directly using a vms->highmem_flags[] but your
>> proposal may work as well.
>
> Ok. I will use my proposed change in next revision.
>
>>>
>>>>> +        default:
>>>>> +            region_enabled = NULL;
>>>>> +        }
>>>>> +
>>>>> +        /* Skip unknown region */
>>>>> +        if (!region_enabled) {
>>>>> +            continue;
>>>>> +        }
>>>>>              /*
>>>>>             * Check each device to see if they fit in the PA space,
>>>>> @@ -1710,23 +1726,15 @@ static void
>>>>> virt_set_high_memmap(VirtMachineState *vms,
>>>>>             * For each device that doesn't fit, disable it.
>>>>>             */
>>>>>            fits = (region_base + region_size) <= BIT_ULL(pa_bits);
>>>>> -        if (fits) {
>>>>> -            vms->highest_gpa = region_base + region_size - 1;
>>>>> -        }
>>>>> +        if (*region_enabled && fits) {
>>>>> +            vms->memmap[i].base = region_base;
>>>>> +            vms->memmap[i].size = region_size;
>>>>>    -        switch (i) {
>>>>> -        case VIRT_HIGH_GIC_REDIST2:
>>>>> -            vms->highmem_redists &= fits;
>>>>> -            break;
>>>>> -        case VIRT_HIGH_PCIE_ECAM:
>>>>> -            vms->highmem_ecam &= fits;
>>>>> -            break;
>>>>> -        case VIRT_HIGH_PCIE_MMIO:
>>>>> -            vms->highmem_mmio &= fits;
>>>>> -            break;
>>>>> +            vms->highest_gpa = region_base + region_size - 1;
>>>>> +            base = region_base + region_size;
>>>>> +        } else {
>>>>> +            *region_enabled = false;
>> what's the purpose to update the region_enabled here? Is it used
>> anywhere?
>>
>> The fact you do not update vms->highmem_* flags may introduce
>> regressions I think as the resulting flag may be used in some places
>> such as:
>> virt_gicv3_redist_region_count().
>>
>
> 'region_enabled' points to 'vms->highmem_{redist2, ecam, mmio}'. They
> are same thing.
Oh OK. Sorry for the noise.

Eric
>
>>>>> -
>>>>> -        base = region_base + region_size;
>>>>>        }
>>>>>    }
>>>>>    
>
> Thanks,
> Gavin
>