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

[Andrew Jones]

On Tue, Mar 09, 2021 at 07:21:58PM +0000, Marc Zyngier wrote:
> Hi Andrew,
> 
> On Tue, 09 Mar 2021 18:59:39 +0000,
> Andrew Jones <drjones@redhat.com> 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 only 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.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  hw/arm/virt.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 371147f3ae9c..7bf563715b4e 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -2547,14 +2547,13 @@ static int virt_kvm_type(MachineState *ms, const 
> > char *type_str)
> >                       "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
> > +     * KVM requires the IPA size to be at least 32 bits.
> >       */
> > -    return requested_pa_size > 40 ? requested_pa_size : 0;
> > +    return requested_pa_size < 32 ? 32 : requested_pa_size;
> 
> Doesn't this break on older kernels (pre 233a7cb23531, which is
> anything up 4.19), where the 'type' parameter passed at VM creation
> time was expected to be 0 and nothing else?

Doh, of course!

> 
> I had a quick go at qemu a couple of weeks back and came up with the
> following hack, but never actually tested it (there is no way qemu can
> fit in the initramfs I am feeding to this damn machine...).
> 
> Thanks,
> 
>       M.
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 371147f3ae..3301bb8dba 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2534,8 +2534,11 @@ 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 max_vm_pa_size;
>      int 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);
> @@ -2550,11 +2553,12 @@ static int virt_kvm_type(MachineState *ms, const char 
> *type_str)
>         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 we return 0.
>       */
> -    return requested_pa_size > 40 ? requested_pa_size : 0;
> +    if (fixed_ipa)
> +        return 0;
> +    return requested_pa_size;

Looks good to me. I think we should still ensure requested_pa_size is at
least 32 though. I can add that and test this tomorrow. Should I post it
with your authorship?

Thanks,
drew

>  }
>  
>  static void virt_machine_class_init(ObjectClass *oc, void *data)
> diff --git a/slirp b/slirp
> index 8f43a99191..ce94eba204 160000
> --- a/slirp
> +++ b/slirp
> @@ -1 +1 @@
> -Subproject commit 8f43a99191afb47ca3f3c6972f6306209f367ece
> +Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 00e124c812..5f576730e6 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 eb81b7059e..75d7133a35 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -311,10 +311,11 @@ bool kvm_arm_sve_supported(void);
>  /**
>   * kvm_arm_get_max_vm_ipa_size:
>   * @ms: Machine state handle
> + * @fixed_ipa: Indicate whether the IPA address space is a fixed value
>   *
>   * 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 +410,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();
>  }
> 
> -- 
> Without deviation from the norm, progress is not possible.
>