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