Re: [PATCH 1/2] hw/arm/virt: Improve address assignment for highmem IO regions

[Eric Auger]

Hi Gavin,

On 8/2/22 08:45, Gavin Shan wrote:
> There are 3 highmem IO regions as below. They can be disabled in
> two situations: (a) The specific region is disabled by user. (b)
> The specific region doesn't fit in the PA space. However, the base
> address and highest_gpa are still updated no matter if the region
> is enabled or disabled. It's incorrectly incurring waste in the PA
> space.
If I am not wrong highmem_redists and highmem_mmio are not user selectable

Only highmem ecam depends on machine type & ACPI setup. But I would say
that in server use case it is always set. So is that optimization really
needed?
>
> Improve address assignment for highmem IO regions to avoid the waste
> in the PA space by putting the logic into virt_memmap_fits().
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  hw/arm/virt.c | 54 +++++++++++++++++++++++++++++----------------------
>  1 file changed, 31 insertions(+), 23 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 9633f822f3..bc0cd218f9 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1688,6 +1688,34 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState 
> *vms, int idx)
>      return arm_cpu_mp_affinity(idx, clustersz);
>  }
>  
> +static void virt_memmap_fits(VirtMachineState *vms, int index,
> +                             bool *enabled, hwaddr *base, int pa_bits)
> +{
> +    hwaddr size = extended_memmap[index].size;
> +
> +    /* The region will be disabled if its size isn't given */
> +    if (!*enabled || !size) {
In which case do you have !size?
> +        *enabled = false;
> +        vms->memmap[index].base = 0;
> +        vms->memmap[index].size = 0;
It looks dangerous to me to reset the region's base and size like that.
for instance fdt_add_gic_node() will add dummy data in the dt.
> +        return;
> +    }
> +
> +    /*
> +     * Check if the memory region fits in the PA space. The memory map
> +     * and highest_gpa are updated if it fits. Otherwise, it's disabled.
> +     */
> +    *enabled = (ROUND_UP(*base, size) + size <= BIT_ULL(pa_bits));
using a 'fits' local variable would make the code more obvious I think
> +    if (*enabled) {
> +        *base = ROUND_UP(*base, size);
> +        vms->memmap[index].base = *base;
> +        vms->memmap[index].size = size;
> +        vms->highest_gpa = *base + size - 1;
> +
> +     *base = *base + size;
> +    }
> +}
> +
>  static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
>  {
>      MachineState *ms = MACHINE(vms);
> @@ -1744,37 +1772,17 @@ 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;
> -        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;
> -        }
> -

we could avoid running the code below in case highmem is not set. We would need 
to reset that flags though.

>          switch (i) {
>          case VIRT_HIGH_GIC_REDIST2:
> -            vms->highmem_redists &= fits;
> +            virt_memmap_fits(vms, i, &vms->highmem_redists, &base, pa_bits);
>              break;
>          case VIRT_HIGH_PCIE_ECAM:
> -            vms->highmem_ecam &= fits;
> +            virt_memmap_fits(vms, i, &vms->highmem_ecam, &base, pa_bits);
>              break;
>          case VIRT_HIGH_PCIE_MMIO:
> -            vms->highmem_mmio &= fits;
> +            virt_memmap_fits(vms, i, &vms->highmem_mmio, &base, pa_bits);
>              break;
>          }
> -
> -        base += size;
>      }
>  
>      if (device_memory_size > 0) {
Thanks

Eric