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

[Marc Zyngier <maz@kernel.org>]

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?

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;
 }
 
 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.