| Message ID | 20210309185939.188431-1-drjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/virt: KVM: The IPA lower bound is 32 | expand |
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(); }
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. >
On Tue, 09 Mar 2021 20:06:06 +0000, Andrew Jones <drjones@redhat.com> wrote: > > 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. Yes, absolutely. > I can add that and test this tomorrow. Should I post it with your > authorship? No, please keep it with yours (add a Suggested-by: tag if you really want to). If you can give it a good shake, that'd be great. I'll try and repost the kernel fixes tomorrow. Thanks, M.
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; } static void virt_machine_class_init(ObjectClass *oc, void *data)
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(-)