Re: [PATCH 2/3] hw/arm/virt: Honor highmem setting when computing highest_gpa

[Peter Maydell <peter.maydell@linaro.org>]

On Sun, 22 Aug 2021 at 15:45, Marc Zyngier <maz@kernel.org> wrote:
>
> Even when the VM is configured with highmem=off, the highest_gpa
> field includes devices that are above the 4GiB limit, which is
> what highmem=off is supposed to enforce. This leads to failures
> in virt_kvm_type() on systems that have a crippled IPA range,
> as the reported IPA space is larger than what it should be.
>
> Instead, honor the user-specified limit to only use the devices
> at the lowest end of the spectrum.
>
> Note that this doesn't affect memory, which is still allowed to
> go beyond 4GiB with highmem=on configurations.
>
> Cc: Andrew Jones <drjones@redhat.com>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  hw/arm/virt.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 81eda46b0b..bc189e30b8 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1598,7 +1598,7 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx)
>  static void virt_set_memmap(VirtMachineState *vms)
>  {
>      MachineState *ms = MACHINE(vms);
> -    hwaddr base, device_memory_base, device_memory_size;
> +    hwaddr base, device_memory_base, device_memory_size, ceiling;
>      int i;
>
>      vms->memmap = extended_memmap;
> @@ -1625,7 +1625,7 @@ static void virt_set_memmap(VirtMachineState *vms)
>      device_memory_size = ms->maxram_size - ms->ram_size + ms->ram_slots * GiB;
>
>      /* Base address of the high IO region */
> -    base = device_memory_base + ROUND_UP(device_memory_size, GiB);
> +    ceiling = base = device_memory_base + ROUND_UP(device_memory_size, GiB);
>      if (base < device_memory_base) {
>          error_report("maxmem/slots too huge");
>          exit(EXIT_FAILURE);
> @@ -1642,7 +1642,11 @@ static void virt_set_memmap(VirtMachineState *vms)
>          vms->memmap[i].size = size;
>          base += size;
>      }
> -    vms->highest_gpa = base - 1;
> +    if (vms->highmem) {
> +           /* If we have highmem, move the IPA limit to the top */
> +           ceiling = base;
> +    }
> +    vms->highest_gpa = ceiling - 1;

This doesn't look right to me. If highmem is false and the
high IO region would be above the 4GB mark then we should not
create the high IO region at all, surely? This code looks like
it goes ahead and puts the high IO region above 4GB and then
lies in the highest_gpa value about what the highest used GPA is.

-- PMM