Re: [PATCH v2 2/2] hw/arm/virt: KVM: The IPA lower bound is 32

[Auger Eric]

Hi Drew,

On 3/10/21 2:52 PM, Andrew Jones wrote:
> The virt machine already checks KVM_CAP_ARM_VM_IPA_SIZE to get the
> upper bound of the IPA size. If that bound is lower than the highest
> possible GPA for the machine, then QEMU will error out. However, the
> IPA is set to 40 when the highest GPA is less than or equal to 40,
> even when KVM may support an IPA limit as low as 32. This means KVM
> may fail the VM creation unnecessarily. Additionally, 40 is selected
> with the value 0, which means use the default, and that gets around
> a check in some versions of KVM, causing a difficult to debug fail.
> Always use the IPA size that corresponds to the highest possible GPA,
> unless it's lower than 32, in which case use 32. Also, we must still
> use 0 when KVM only supports the legacy fixed 40 bit IPA.
> 
> Suggested-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---

>  hw/arm/virt.c        | 23 ++++++++++++++++-------
>  target/arm/kvm.c     |  4 +++-
>  target/arm/kvm_arm.h |  6 ++++--
>  3 files changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 371147f3ae9c..3ed94d24d70b 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2534,27 +2534,36 @@ static HotplugHandler 
> *virt_machine_get_hotplug_handler(MachineState *machine,
>  static int virt_kvm_type(MachineState *ms, const char *type_str)
>  {
>      VirtMachineState *vms = VIRT_MACHINE(ms);
> -    int max_vm_pa_size = kvm_arm_get_max_vm_ipa_size(ms);
> -    int requested_pa_size;
> +    int max_vm_pa_size, requested_pa_size;
> +    bool fixed_ipa;
> +
> +    max_vm_pa_size = kvm_arm_get_max_vm_ipa_size(ms, &fixed_ipa);
>  
>      /* we freeze the memory map to compute the highest gpa */
>      virt_set_memmap(vms);
>  
>      requested_pa_size = 64 - clz64(vms->highest_gpa);
>  
> +    /*
> +     * KVM requires the IPA size to be at least 32 bits.
> +     */
> +    if (requested_pa_size < 32) {
> +        requested_pa_size = 32;
> +    }
> +
>      if (requested_pa_size > max_vm_pa_size) {
>          error_report("-m and ,maxmem option values "
>                       "require an IPA range (%d bits) larger than "
>                       "the one supported by the host (%d bits)",
>                       requested_pa_size, max_vm_pa_size);
> -       exit(1);
> +        exit(1);
>      }
>      /*
> -     * By default we return 0 which corresponds to an implicit legacy
> -     * 40b IPA setting. Otherwise we return the actual requested PA
> -     * logsize
> +     * We return the requested PA log size, unless KVM only supports
> +     * the implicit legacy 40b IPA setting, in which case the kvm_type
> +     * must be 0.
>       */
> -    return requested_pa_size > 40 ? requested_pa_size : 0;
> +    return fixed_ipa ? 0 : requested_pa_size;
>  }
>  
>  static void virt_machine_class_init(ObjectClass *oc, void *data)
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 00e124c81239..1fcab0e1d37b 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -230,12 +230,14 @@ bool kvm_arm_pmu_supported(void)
>      return kvm_check_extension(kvm_state, KVM_CAP_ARM_PMU_V3);
>  }
>  
> -int kvm_arm_get_max_vm_ipa_size(MachineState *ms)
> +int kvm_arm_get_max_vm_ipa_size(MachineState *ms, bool *fixed_ipa)
>  {
>      KVMState *s = KVM_STATE(ms->accelerator);
>      int ret;
>  
>      ret = kvm_check_extension(s, KVM_CAP_ARM_VM_IPA_SIZE);
> +    *fixed_ipa = ret <= 0;
> +
>      return ret > 0 ? ret : 40;
>  }
>  
> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> index eb81b7059eb1..d36d76403ff2 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -311,10 +311,12 @@ bool kvm_arm_sve_supported(void);
>  /**
>   * kvm_arm_get_max_vm_ipa_size:
>   * @ms: Machine state handle
> + * @fixed_ipa: True when the IPA limit is fixed at 40. This is the case
> + * for legacy KVM.
>   *
>   * Returns the number of bits in the IPA address space supported by KVM
>   */
> -int kvm_arm_get_max_vm_ipa_size(MachineState *ms);
> +int kvm_arm_get_max_vm_ipa_size(MachineState *ms, bool *fixed_ipa);
>  
>  /**
>   * kvm_arm_sync_mpstate_to_kvm:
> @@ -409,7 +411,7 @@ static inline void kvm_arm_add_vcpu_properties(Object 
> *obj)
>      g_assert_not_reached();
>  }
>  
> -static inline int kvm_arm_get_max_vm_ipa_size(MachineState *ms)
> +static inline int kvm_arm_get_max_vm_ipa_size(MachineState *ms, bool 
> *fixed_ipa)
>  {
>      g_assert_not_reached();
>  }
>