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

[PATCH v2 0/4] 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] and PATCH[2] are cleanup and preparatory works.
PATCH[3] improves address assignment for these high memory regions
PATCH[4] moves the address assignment logic into standalone helper

History
=======
v1: https://lists.nongnu.org/archive/html/qemu-arm/2022-08/msg00013.html

Changelog
=========
v2:
  * Split the patches for easier review                        (Gavin)
  * Improved changelog                                         (Marc)
  * Use 'bool fits' in virt_set_high_memmap()                  (Eric)


Gavin Shan (4):
  hw/arm/virt: Rename variable size to region_size in virt_set_memmap()
  hw/arm/virt: Introduce variable region_base in virt_set_memmap()
  hw/arm/virt: Improve address assignment for high memory regions
  virt/hw/virt: Add virt_set_high_memmap() helper

 hw/arm/virt.c | 84 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 50 insertions(+), 34 deletions(-)

-- 
2.23.0

[PATCH v2 1/4] hw/arm/virt: Rename variable size to region_size in virt_set_memmap()

[Gavin Shan]

This renames variable 'size' to 'region_size' in virt_set_memmap().
It's counterpart to 'region_base', which will be introducded in
next patch.

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 9633f822f3..f8e9f3e205 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1744,12 +1744,12 @@ static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
     vms->highest_gpa = memtop - 1;
 
     for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
-        hwaddr size = extended_memmap[i].size;
+        hwaddr region_size = extended_memmap[i].size;
         bool fits;
 
-        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,
@@ -1757,9 +1757,9 @@ static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
          *
          * 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) {
@@ -1774,7 +1774,7 @@ static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
             break;
         }
 
-        base += size;
+        base += region_size;
     }
 
     if (device_memory_size > 0) {
-- 
2.23.0

[PATCH v2 2/4] hw/arm/virt: Introduce variable region_base in virt_set_memmap()

[Gavin Shan]

This introduces variable 'region_base' for the base address of
the specific high memory region. It's the preparatory to improve
the address assignment for high memory region in next patch.

No functional change intended.

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

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f8e9f3e205..582a8960fc 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1744,11 +1744,11 @@ static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
     vms->highest_gpa = memtop - 1;
 
     for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
+        hwaddr region_base = ROUND_UP(base, extended_memmap[i].size);
         hwaddr region_size = extended_memmap[i].size;
         bool fits;
 
-        base = ROUND_UP(base, region_size);
-        vms->memmap[i].base = base;
+        vms->memmap[i].base = region_base;
         vms->memmap[i].size = region_size;
 
         /*
@@ -1757,9 +1757,9 @@ static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
          *
          * 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) {
@@ -1774,7 +1774,7 @@ static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
             break;
         }
 
-        base += region_size;
+        base = region_base + region_size;
     }
 
     if (device_memory_size > 0) {
-- 
2.23.0

[PATCH v2 3/4] 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.

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 | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 582a8960fc..e38b6919c9 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1746,10 +1746,26 @@ static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
     for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
         hwaddr region_base = ROUND_UP(base, extended_memmap[i].size);
         hwaddr region_size = extended_memmap[i].size;
-        bool fits;
+        bool *region_enabled, fits;
 
-        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 unknwon or disabled regions */
+        if (!region_enabled || !*region_enabled) {
+            continue;
+        }
 
         /*
          * Check each device to see if they fit in the PA space,
@@ -1759,22 +1775,14 @@ static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
          */
         fits = (region_base + region_size) <= BIT_ULL(pa_bits);
         if (fits) {
-            vms->highest_gpa = region_base + region_size - 1;
-        }
+            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;
+            base = region_base + region_size;
+            vms->highest_gpa = region_base + region_size - 1;
+        } else {
+            *region_enabled = false;
         }
-
-        base = region_base + region_size;
     }
 
     if (device_memory_size > 0) {
-- 
2.23.0

[PATCH v2 4/4] virt/hw/virt: Add virt_set_high_memmap() helper

[Gavin Shan]

The logic to assign high memory region's address in virt_set_memmap()
is independent. Lets move the logic to virt_set_high_memmap() helper.
"each device" is replaced by "each region" in the comments.

No functional change intended.

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

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index e38b6919c9..4dde08a924 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1688,6 +1688,55 @@ 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)
+{
+    hwaddr region_base, region_size;
+    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;
+
+        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 unknwon or disabled regions */
+        if (!region_enabled || !*region_enabled) {
+            continue;
+        }
+
+        /*
+         * Check each region 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 = (region_base + region_size) <= BIT_ULL(pa_bits);
+        if (fits) {
+            vms->memmap[i].base = region_base;
+            vms->memmap[i].size = region_size;
+
+            base = region_base + region_size;
+            vms->highest_gpa = region_base + region_size - 1;
+        } else {
+            *region_enabled = false;
+        }
+    }
+}
+
 static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
 {
     MachineState *ms = MACHINE(vms);
@@ -1742,48 +1791,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 region_base = ROUND_UP(base, extended_memmap[i].size);
-        hwaddr region_size = extended_memmap[i].size;
-        bool *region_enabled, fits;
-
-        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 unknwon or disabled regions */
-        if (!region_enabled || !*region_enabled) {
-            continue;
-        }
-
-        /*
-         * 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 = (region_base + region_size) <= BIT_ULL(pa_bits);
-        if (fits) {
-            vms->memmap[i].base = region_base;
-            vms->memmap[i].size = region_size;
-
-            base = region_base + region_size;
-            vms->highest_gpa = region_base + region_size - 1;
-        } else {
-            *region_enabled = false;
-        }
-    }
+    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

Re: [PATCH v2 4/4] virt/hw/virt: Add virt_set_high_memmap() helper

[Zhenyu Zhang]

commit 49e00c1fe2ab24b73ac16908f3c05ebe88b9186d (HEAD -> master)
Author: Gavin Shan <gshan@redhat.com>
Date:   Mon Aug 15 14:29:58 2022 +0800

    virt/hw/virt: Add virt_set_high_memmap() helper

    The logic to assign high memory region's address in virt_set_memmap()
    is independent. Lets move the logic to virt_set_high_memmap() helper.
    "each device" is replaced by "each region" in the comments.

    No functional change intended.

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

The patchs works well on my Fujitsu host.

[root@hpe-apollo80-02-n00 qemu]# /home/qemu/build/qemu-system-aarch64 -version
QEMU emulator version 7.0.92 (v7.1.0-rc2-12-gd102b8162a)
[root@hpe-apollo80-02-n00 qemu]# /home/qemu/build/qemu-system-aarch64
-accel kvm -m 4096,maxmem=1023G -machine virt-2.12 -cpu host

[root@hpe-apollo80-02-n00 qemu]# /home/qemu/build/qemu-system-aarch64
-accel kvm -m 4096,maxmem=1024G -machine virt-2.12 -cpu host
qemu-system-aarch64: -accel kvm: Addressing limited to 40 bits, but
memory exceeds it by 1073741824 bytes

[root@hpe-apollo80-02-n00 qemu]# /home/qemu/build/qemu-system-aarch64
-accel kvm -m 4096,maxmem=1023G -machine virt -cpu host

[root@hpe-apollo80-02-n00 qemu]# /home/qemu/build/qemu-system-aarch64
-accel kvm -m 4096,maxmem=1024G -machine virt -cpu host
qemu-system-aarch64: -accel kvm: Addressing limited to 40 bits, but
memory exceeds it by 1073741824 bytes

Tested-by:zhenyzha@redhat.com


On Mon, Aug 15, 2022 at 2:30 PM Gavin Shan <gshan@redhat.com> wrote:
>
> The logic to assign high memory region's address in virt_set_memmap()
> is independent. Lets move the logic to virt_set_high_memmap() helper.
> "each device" is replaced by "each region" in the comments.
>
> No functional change intended.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  hw/arm/virt.c | 92 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 50 insertions(+), 42 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index e38b6919c9..4dde08a924 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1688,6 +1688,55 @@ 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)
> +{
> +    hwaddr region_base, region_size;
> +    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;
> +
> +        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 unknwon or disabled regions */
> +        if (!region_enabled || !*region_enabled) {
> +            continue;
> +        }
> +
> +        /*
> +         * Check each region 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 = (region_base + region_size) <= BIT_ULL(pa_bits);
> +        if (fits) {
> +            vms->memmap[i].base = region_base;
> +            vms->memmap[i].size = region_size;
> +
> +            base = region_base + region_size;
> +            vms->highest_gpa = region_base + region_size - 1;
> +        } else {
> +            *region_enabled = false;
> +        }
> +    }
> +}
> +
>  static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
>  {
>      MachineState *ms = MACHINE(vms);
> @@ -1742,48 +1791,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 region_base = ROUND_UP(base, extended_memmap[i].size);
> -        hwaddr region_size = extended_memmap[i].size;
> -        bool *region_enabled, fits;
> -
> -        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 unknwon or disabled regions */
> -        if (!region_enabled || !*region_enabled) {
> -            continue;
> -        }
> -
> -        /*
> -         * 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 = (region_base + region_size) <= BIT_ULL(pa_bits);
> -        if (fits) {
> -            vms->memmap[i].base = region_base;
> -            vms->memmap[i].size = region_size;
> -
> -            base = region_base + region_size;
> -            vms->highest_gpa = region_base + region_size - 1;
> -        } else {
> -            *region_enabled = false;
> -        }
> -    }
> +    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
>


-- 
I respect your work/life balance, no need to reply to this email if it
is outside of your normal working hours.

Zhenyu Zhang

Senior Quality Engineer, Products And Technologies

Red Hat Software (Beijing) Co., R&D Branch

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

[Gavin Shan]

Hi Marc,

On 8/15/22 4:29 PM, 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.
> 
> The series intends to improve the address assignment for these
> high memory regions:
> 
> PATCH[1] and PATCH[2] are cleanup and preparatory works.
> PATCH[3] improves address assignment for these high memory regions
> PATCH[4] moves the address assignment logic into standalone helper
> 
> History
> =======
> v1: https://lists.nongnu.org/archive/html/qemu-arm/2022-08/msg00013.html
> 
> Changelog
> =========
> v2:
>    * Split the patches for easier review                        (Gavin)
>    * Improved changelog                                         (Marc)
>    * Use 'bool fits' in virt_set_high_memmap()                  (Eric)
> 

Could you help to review when you have free cycles? It's just a kindly
ping :)

Thanks,
Gavin

> 
> Gavin Shan (4):
>    hw/arm/virt: Rename variable size to region_size in virt_set_memmap()
>    hw/arm/virt: Introduce variable region_base in virt_set_memmap()
>    hw/arm/virt: Improve address assignment for high memory regions
>    virt/hw/virt: Add virt_set_high_memmap() helper
> 
>   hw/arm/virt.c | 84 ++++++++++++++++++++++++++++++---------------------
>   1 file changed, 50 insertions(+), 34 deletions(-)
> 

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

[Eric Auger]

Hi Gavin,

On 8/24/22 05:29, Gavin Shan wrote:
> Hi Marc,
>
> On 8/15/22 4:29 PM, 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.
>>
>> The series intends to improve the address assignment for these
>> high memory regions:
>>
>> PATCH[1] and PATCH[2] are cleanup and preparatory works.
>> PATCH[3] improves address assignment for these high memory regions
>> PATCH[4] moves the address assignment logic into standalone helper
>>
>> History
>> =======
>> v1: https://lists.nongnu.org/archive/html/qemu-arm/2022-08/msg00013.html
>>
>> Changelog
>> =========
>> v2:
>>    * Split the patches for easier review                        (Gavin)
>>    * Improved changelog                                         (Marc)
>>    * Use 'bool fits' in virt_set_high_memmap()                  (Eric)
>>
You did not really convince me about migration compat wrt the high MMIO
region. Aren't the PCI BARs saved/restored meaning the device driver is
expecting to find data at the same GPA. But what if your high MMIO
region was relocated in the dest QEMU with a possibly smaller VM IPA?
Don't you have MMIO regions now allocated outside of the dest MMIO
region? How does the PCI host bridge route accesses to those regions?
What do I miss?

Thanks

Eric
>
> Could you help to review when you have free cycles? It's just a kindly
> ping :)
>
> Thanks,
> Gavin
>
>>
>> Gavin Shan (4):
>>    hw/arm/virt: Rename variable size to region_size in virt_set_memmap()
>>    hw/arm/virt: Introduce variable region_base in virt_set_memmap()
>>    hw/arm/virt: Improve address assignment for high memory regions
>>    virt/hw/virt: Add virt_set_high_memmap() helper
>>
>>   hw/arm/virt.c | 84 ++++++++++++++++++++++++++++++---------------------
>>   1 file changed, 50 insertions(+), 34 deletions(-)
>>
>

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

[Gavin Shan]

Hi Eric,

On 8/24/22 6:06 PM, Eric Auger wrote:
> On 8/24/22 05:29, Gavin Shan wrote:
>> On 8/15/22 4:29 PM, 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.
>>>
>>> The series intends to improve the address assignment for these
>>> high memory regions:
>>>
>>> PATCH[1] and PATCH[2] are cleanup and preparatory works.
>>> PATCH[3] improves address assignment for these high memory regions
>>> PATCH[4] moves the address assignment logic into standalone helper
>>>
>>> History
>>> =======
>>> v1: https://lists.nongnu.org/archive/html/qemu-arm/2022-08/msg00013.html
>>>
>>> Changelog
>>> =========
>>> v2:
>>>     * Split the patches for easier review                        (Gavin)
>>>     * Improved changelog                                         (Marc)
>>>     * Use 'bool fits' in virt_set_high_memmap()                  (Eric)
>>>
> You did not really convince me about migration compat wrt the high MMIO
> region. Aren't the PCI BARs saved/restored meaning the device driver is
> expecting to find data at the same GPA. But what if your high MMIO
> region was relocated in the dest QEMU with a possibly smaller VM IPA?
> Don't you have MMIO regions now allocated outside of the dest MMIO
> region? How does the PCI host bridge route accesses to those regions?
> What do I miss?
> 

I'm currently looking into virtio-pci-net migration, but need time to
investigate how the device is migrated. I will get back to you once
I have something. Thanks for your comments :)

Thanks,
Gavin

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

[Gavin Shan]

Hi Eric,

On 8/24/22 6:06 PM, Eric Auger wrote:
> On 8/24/22 05:29, Gavin Shan wrote:
>> On 8/15/22 4:29 PM, 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.
>>>
>>> The series intends to improve the address assignment for these
>>> high memory regions:
>>>
>>> PATCH[1] and PATCH[2] are cleanup and preparatory works.
>>> PATCH[3] improves address assignment for these high memory regions
>>> PATCH[4] moves the address assignment logic into standalone helper
>>>
>>> History
>>> =======
>>> v1: https://lists.nongnu.org/archive/html/qemu-arm/2022-08/msg00013.html
>>>
>>> Changelog
>>> =========
>>> v2:
>>>     * Split the patches for easier review                        (Gavin)
>>>     * Improved changelog                                         (Marc)
>>>     * Use 'bool fits' in virt_set_high_memmap()                  (Eric)
>>>
> You did not really convince me about migration compat wrt the high MMIO
> region. Aren't the PCI BARs saved/restored meaning the device driver is
> expecting to find data at the same GPA. But what if your high MMIO
> region was relocated in the dest QEMU with a possibly smaller VM IPA?
> Don't you have MMIO regions now allocated outside of the dest MMIO
> region? How does the PCI host bridge route accesses to those regions?
> What do I miss?
> 

[...]

Sorry for the delay because I was offline last week. I was intending
to explain the migration on virtio-net device and spent some time to
go through the code. I found it's still complicated for an example
because PCI and virtio device models are involved. So lets still use
e1000e.c as an example here.

There are lots of registers residing in MMIO region, including MSIx
table. The MSIx table is backed by PCIDevice::msix_table, which is
a buffer. The access to MSIx table is read from or write to the buffer.
The corresponding handler is hw/msix.c::msix_table_mmio_ops. msix_init()
is called by e1000e.c to setup the MSIx table, which is associated with
memory BAR#3. As the registers in MSIx table is totally emulated by
QEMU, the BAR's base address isn't a concern.

   struct PCIDevice {
      :
      uint8_t *msix_table;
      :
      MemoryRegion msix_table_mmio;
      :
   };

   /* @table_bar is registered as memory BAR#3 in e1000e_pci_realize() */
   int msix_init(struct PCIDevice *dev, unsigned short nentries,
                 MemoryRegion *table_bar, uint8_t table_bar_nr,
                 unsigned table_offset, MemoryRegion *pba_bar,
                 uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos,
                 Error **errp)
   {
        :
     memory_region_init_io(&dev->msix_table_mmio, OBJECT(dev), &msix_table_mmio_ops, dev,
                           "msix-table", table_size);
     memory_region_add_subregion(table_bar, table_offset, &dev->msix_table_mmio);
        :
   }

As we concerned, the BAR's base addresses for MSIx tables are different on source
and destination VMs. It's still not a problem because the registers in MSIx table
are migrated, saved on source VM and restored on destination VM one by one. It's
to say, not the whole buffer (PCIDevice::msix_table) is saved and restored at once.
Further more, the unique ID string, instead the corresponding BAR's base address,
is used to identify the MSIx table. For this particular case, the ID string is
something like "e1000e_dev_id/pci-0000:05:00.0/msix state". With this ID string
is received on the destination VM, the object and PCI device is located and the
forth-coming data is saved to PCIDevice::msix_table.

   static const VMStateDescription e1000e_vmstate = {
     .name = "e1000e",
     .version_id = 1,
     .minimum_version_id = 1,
     .pre_save = e1000e_pre_save,
     .post_load = e1000e_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_PCI_DEVICE(parent_obj, E1000EState),
         VMSTATE_MSIX(parent_obj, E1000EState),
         :
     }
   };

   #define VMSTATE_MSIX_TEST(_field, _state, _test) {                 \
     .name         = (stringify(_field)),                             \
     .size         = sizeof(PCIDevice),                               \
     .vmsd         = &vmstate_msix,                                   \
     .flags        = VMS_STRUCT,                                      \
     .offset       = vmstate_offset_value(_state, _field, PCIDevice), \
     .field_exists = (_test)                                          \
   }

   #define VMSTATE_MSIX(_f, _s)                                       \
       VMSTATE_MSIX_TEST(_f, _s, NULL)


   /* On source VM, PCIDevice::msix_table is transferred to destination VM */
   void msix_save(PCIDevice *dev, QEMUFile *f)
   {
     :
     qemu_put_buffer(f, dev->msix_table, n * PCI_MSIX_ENTRY_SIZE);
     qemu_put_buffer(f, dev->msix_pba, DIV_ROUND_UP(n, 8));
   }

   /* On destination VM, the received data is write to PCIDevice::msix_table */
   void msix_load(PCIDevice *dev, QEMUFile *f)
   {
     :
     qemu_get_buffer(f, dev->msix_table, n * PCI_MSIX_ENTRY_SIZE);
     qemu_get_buffer(f, dev->msix_pba, DIV_ROUND_UP(n, 8));
     :
   }

   static int put_msix_state(QEMUFile *f, void *pv, size_t size,
                             const VMStateField *field, JSONWriter *vmdesc)
   {
     msix_save(pv, f);

     return 0;
   }

   static int get_msix_state(QEMUFile *f, void *pv, size_t size,
                             const VMStateField *field)
   {
     msix_load(pv, f);
     return 0;
   }

   static VMStateInfo vmstate_info_msix = {
     .name = "msix state",
     .get  = get_msix_state,
     .put  = put_msix_state,
   };

   const VMStateDescription vmstate_msix = {
     .name = "msix",
     .fields = (VMStateField[]) {
         {
             .name         = "msix",
             .version_id   = 0,
             .field_exists = NULL,
             .size         = 0,   /* ouch */
             .info         = &vmstate_info_msix,
             .flags        = VMS_SINGLE,
             .offset       = 0,
         },
     }
  };

Thanks,
Gavin