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 <gs...@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;
      }
  }