* [PATCH v2 0/2] hw/arm/virt: KVM: Set IPA limit when possible @ 2021-03-10 13:52 Andrew Jones 2021-03-10 13:52 ` [PATCH v2 1/2] accel: kvm: Fix kvm_type invocation Andrew Jones ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Andrew Jones @ 2021-03-10 13:52 UTC (permalink / raw) To: qemu-devel, qemu-arm; +Cc: pbonzini, maz, eric.auger, peter.maydell This series fixes IPA limit setting for mach-virt KVM guests. The first patch restores the setting of IPA limits for values greater than 40 (the default) when necessary. The second patch ensures values less than 40 may also be used. The default KVM type=0 (which means an IPA limit of 40) is still used for legacy KVM, since it must be. I tested this with a KVM that supports KVM_CAP_ARM_VM_IPA_SIZE and with a KVM that does not. mach-virt's memory map didn't allow me to test with less than 40 on the KVM_CAP_ARM_VM_IPA_SIZE supporting host, but a quick VM fd opening test seemed to prove KVM would be happy with that. Testing was done with a typical Linux guest and also with kvm-unit-tests. I caught the bug that the first patch fixes by instrumenting QEMU to observe which IPA limit was getting selected, and then seeing that QEMU wasn't actually running mach-virt's kvm_type method at all! Thanks, drew Andrew Jones (2): accel: kvm: Fix kvm_type invocation hw/arm/virt: KVM: The IPA lower bound is 32 accel/kvm/kvm-all.c | 2 ++ hw/arm/virt.c | 23 ++++++++++++++++------- include/hw/boards.h | 1 + target/arm/kvm.c | 4 +++- target/arm/kvm_arm.h | 6 ++++-- 5 files changed, 26 insertions(+), 10 deletions(-) -- 2.26.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] accel: kvm: Fix kvm_type invocation 2021-03-10 13:52 [PATCH v2 0/2] hw/arm/virt: KVM: Set IPA limit when possible Andrew Jones @ 2021-03-10 13:52 ` Andrew Jones 2021-03-10 14:19 ` Auger Eric 2021-03-10 13:52 ` [PATCH v2 2/2] hw/arm/virt: KVM: The IPA lower bound is 32 Andrew Jones 2021-03-12 12:47 ` [PATCH v2 0/2] hw/arm/virt: KVM: Set IPA limit when possible Peter Maydell 2 siblings, 1 reply; 7+ messages in thread From: Andrew Jones @ 2021-03-10 13:52 UTC (permalink / raw) To: qemu-devel, qemu-arm; +Cc: pbonzini, maz, eric.auger, peter.maydell Prior to commit f2ce39b4f067 a MachineClass kvm_type method only needed to be registered to ensure it would be executed. With commit f2ce39b4f067 a kvm-type machine property must also be specified. hw/arm/virt relies on the kvm_type method to pass its selected IPA limit to KVM, but this is not exposed as a machine property. Restore the previous functionality of invoking kvm_type when it's present. Fixes: f2ce39b4f067 ("vl: make qemu_get_machine_opts static") Signed-off-by: Andrew Jones <drjones@redhat.com> --- accel/kvm/kvm-all.c | 2 ++ include/hw/boards.h | 1 + 2 files changed, 3 insertions(+) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index f88a52393fe0..37b0a1861e72 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -2068,6 +2068,8 @@ static int kvm_init(MachineState *ms) "kvm-type", &error_abort); type = mc->kvm_type(ms, kvm_type); + } else if (mc->kvm_type) { + type = mc->kvm_type(ms, NULL); } do { diff --git a/include/hw/boards.h b/include/hw/boards.h index a46dfe5d1a6a..327949967609 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -128,6 +128,7 @@ typedef struct { * @kvm_type: * Return the type of KVM corresponding to the kvm-type string option or * computed based on other criteria such as the host kernel capabilities. + * kvm-type may be NULL if it is not needed. * @numa_mem_supported: * true if '--numa node.mem' option is supported and false otherwise * @smp_parse: -- 2.26.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] accel: kvm: Fix kvm_type invocation 2021-03-10 13:52 ` [PATCH v2 1/2] accel: kvm: Fix kvm_type invocation Andrew Jones @ 2021-03-10 14:19 ` Auger Eric 0 siblings, 0 replies; 7+ messages in thread From: Auger Eric @ 2021-03-10 14:19 UTC (permalink / raw) To: Andrew Jones, qemu-devel, qemu-arm; +Cc: peter.maydell, pbonzini, maz Hi Drew, On 3/10/21 2:52 PM, Andrew Jones wrote: > Prior to commit f2ce39b4f067 a MachineClass kvm_type method > only needed to be registered to ensure it would be executed. > With commit f2ce39b4f067 a kvm-type machine property must also > be specified. hw/arm/virt relies on the kvm_type method to pass > its selected IPA limit to KVM, but this is not exposed as a > machine property. Restore the previous functionality of invoking > kvm_type when it's present. Ouch, good catch for this regression > > Fixes: f2ce39b4f067 ("vl: make qemu_get_machine_opts static") > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > accel/kvm/kvm-all.c | 2 ++ > include/hw/boards.h | 1 + > 2 files changed, 3 insertions(+) > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index f88a52393fe0..37b0a1861e72 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -2068,6 +2068,8 @@ static int kvm_init(MachineState *ms) > "kvm-type", > &error_abort); > type = mc->kvm_type(ms, kvm_type); > + } else if (mc->kvm_type) { > + type = mc->kvm_type(ms, NULL); > } > > do { > diff --git a/include/hw/boards.h b/include/hw/boards.h > index a46dfe5d1a6a..327949967609 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -128,6 +128,7 @@ typedef struct { > * @kvm_type: > * Return the type of KVM corresponding to the kvm-type string option or > * computed based on other criteria such as the host kernel capabilities. > + * kvm-type may be NULL if it is not needed. > * @numa_mem_supported: > * true if '--numa node.mem' option is supported and false otherwise > * @smp_parse: > Reviewed-by: Eric Auger <eric.auger@redhat.com> Eric ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] hw/arm/virt: KVM: The IPA lower bound is 32 2021-03-10 13:52 [PATCH v2 0/2] hw/arm/virt: KVM: Set IPA limit when possible Andrew Jones 2021-03-10 13:52 ` [PATCH v2 1/2] accel: kvm: Fix kvm_type invocation Andrew Jones @ 2021-03-10 13:52 ` Andrew Jones 2021-03-10 14:58 ` Auger Eric 2021-03-11 9:59 ` Marc Zyngier 2021-03-12 12:47 ` [PATCH v2 0/2] hw/arm/virt: KVM: Set IPA limit when possible Peter Maydell 2 siblings, 2 replies; 7+ messages in thread From: Andrew Jones @ 2021-03-10 13:52 UTC (permalink / raw) To: qemu-devel, qemu-arm; +Cc: pbonzini, maz, eric.auger, peter.maydell 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> --- 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(); } -- 2.26.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] hw/arm/virt: KVM: The IPA lower bound is 32 2021-03-10 13:52 ` [PATCH v2 2/2] hw/arm/virt: KVM: The IPA lower bound is 32 Andrew Jones @ 2021-03-10 14:58 ` Auger Eric 2021-03-11 9:59 ` Marc Zyngier 1 sibling, 0 replies; 7+ messages in thread From: Auger Eric @ 2021-03-10 14:58 UTC (permalink / raw) To: Andrew Jones, qemu-devel, qemu-arm; +Cc: peter.maydell, pbonzini, maz 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(); > } > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] hw/arm/virt: KVM: The IPA lower bound is 32 2021-03-10 13:52 ` [PATCH v2 2/2] hw/arm/virt: KVM: The IPA lower bound is 32 Andrew Jones 2021-03-10 14:58 ` Auger Eric @ 2021-03-11 9:59 ` Marc Zyngier 1 sibling, 0 replies; 7+ messages in thread From: Marc Zyngier @ 2021-03-11 9:59 UTC (permalink / raw) To: Andrew Jones; +Cc: pbonzini, qemu-arm, qemu-devel, eric.auger, peter.maydell On Wed, 10 Mar 2021 13:52:18 +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 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: Marc Zyngier <maz@kernel.org> M. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/2] hw/arm/virt: KVM: Set IPA limit when possible 2021-03-10 13:52 [PATCH v2 0/2] hw/arm/virt: KVM: Set IPA limit when possible Andrew Jones 2021-03-10 13:52 ` [PATCH v2 1/2] accel: kvm: Fix kvm_type invocation Andrew Jones 2021-03-10 13:52 ` [PATCH v2 2/2] hw/arm/virt: KVM: The IPA lower bound is 32 Andrew Jones @ 2021-03-12 12:47 ` Peter Maydell 2 siblings, 0 replies; 7+ messages in thread From: Peter Maydell @ 2021-03-12 12:47 UTC (permalink / raw) To: Andrew Jones Cc: Paolo Bonzini, qemu-arm, Marc Zyngier, QEMU Developers, Eric Auger On Wed, 10 Mar 2021 at 13:52, Andrew Jones <drjones@redhat.com> wrote: > > This series fixes IPA limit setting for mach-virt KVM guests. The > first patch restores the setting of IPA limits for values greater > than 40 (the default) when necessary. The second patch ensures values > less than 40 may also be used. The default KVM type=0 (which means > an IPA limit of 40) is still used for legacy KVM, since it must be. > > I tested this with a KVM that supports KVM_CAP_ARM_VM_IPA_SIZE and > with a KVM that does not. mach-virt's memory map didn't allow me > to test with less than 40 on the KVM_CAP_ARM_VM_IPA_SIZE supporting > host, but a quick VM fd opening test seemed to prove KVM would be > happy with that. Testing was done with a typical Linux guest and also > with kvm-unit-tests. > > I caught the bug that the first patch fixes by instrumenting QEMU > to observe which IPA limit was getting selected, and then seeing > that QEMU wasn't actually running mach-virt's kvm_type method at > all! Applied to target-arm.next, thanks. -- PMM ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-03-12 12:50 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-10 13:52 [PATCH v2 0/2] hw/arm/virt: KVM: Set IPA limit when possible Andrew Jones 2021-03-10 13:52 ` [PATCH v2 1/2] accel: kvm: Fix kvm_type invocation Andrew Jones 2021-03-10 14:19 ` Auger Eric 2021-03-10 13:52 ` [PATCH v2 2/2] hw/arm/virt: KVM: The IPA lower bound is 32 Andrew Jones 2021-03-10 14:58 ` Auger Eric 2021-03-11 9:59 ` Marc Zyngier 2021-03-12 12:47 ` [PATCH v2 0/2] hw/arm/virt: KVM: Set IPA limit when possible Peter Maydell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.