| Message ID | 20230109062259.79074-1-akihiko.odaki@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | accel/kvm: Specify default IPA size for arm64 | expand |
On 1/8/23 22:22, Akihiko Odaki wrote: > libvirt uses "none" machine type to test KVM availability. Before this > change, QEMU used to pass 0 as machine type when calling KVM_CREATE_VM. > > The kernel documentation says: >> On arm64, the physical address size for a VM (IPA Size limit) is >> limited to 40bits by default. The limit can be configured if the host >> supports the extension KVM_CAP_ARM_VM_IPA_SIZE. When supported, use >> KVM_VM_TYPE_ARM_IPA_SIZE(IPA_Bits) to set the size in the machine type >> identifier, where IPA_Bits is the maximum width of any physical >> address used by the VM. The IPA_Bits is encoded in bits[7-0] of the >> machine type identifier. >> >> e.g, to configure a guest to use 48bit physical address size:: >> >> vm_fd = ioctl(dev_fd, KVM_CREATE_VM, KVM_VM_TYPE_ARM_IPA_SIZE(48)); >> >> The requested size (IPA_Bits) must be: >> >> == ========================================================= >> 0 Implies default size, 40bits (for backward compatibility) >> N Implies N bits, where N is a positive integer such that, >> 32 <= N <= Host_IPA_Limit >> == ========================================================= > >> Host_IPA_Limit is the maximum possible value for IPA_Bits on the host >> and is dependent on the CPU capability and the kernel configuration. >> The limit can be retrieved using KVM_CAP_ARM_VM_IPA_SIZE of the >> KVM_CHECK_EXTENSION ioctl() at run-time. >> >> Creation of the VM will fail if the requested IPA size (whether it is >> implicit or explicit) is unsupported on the host. > https://docs.kernel.org/virt/kvm/api.html#kvm-create-vm > > So if Host_IPA_Limit < 40, such KVM_CREATE_VM will fail, and libvirt > incorrectly thinks KVM is not available. This actually happened on M2 > MacBook Air. > > Fix this by specifying 32 for IPA_Bits as any arm64 system should > support the value according to the documentation. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > accel/kvm/kvm-all.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index e86c33e0e6..776ac7efcc 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -2294,7 +2294,11 @@ static int kvm_init(MachineState *ms) > KVMState *s; > const KVMCapabilityInfo *missing_cap; > int ret; > +#ifdef TARGET_AARCH64 > + int type = 32; > +#else > int type = 0; > +#endif No need for an ifdef. Down below we have, if (object_property_find(OBJECT(current_machine), "kvm-type")) { g_autofree char *kvm_type = object_property_get_str(OBJECT(current_machine), "kvm-type", &error_abort); type = mc->kvm_type(ms, kvm_type); } else if (mc->kvm_type) { type = mc->kvm_type(ms, NULL); } and the aarch64 -M virt machine provides virt_kvm_type as mc->kvm_type. How did you hit this? Are you trying to implement your own board model? Looking at this, I'm surprised this is a board hook and not a cpu hook. But I suppose the architecture specific 'type' can hide any number of sins. Anyway, if you are doing your own board model, I suggest arranging to share the virt board hook -- maybe moving it to target/arm/kvm.c in the process? r~
On 2023/01/14 14:23, Richard Henderson wrote: > On 1/8/23 22:22, Akihiko Odaki wrote: >> libvirt uses "none" machine type to test KVM availability. Before this >> change, QEMU used to pass 0 as machine type when calling KVM_CREATE_VM. >> >> The kernel documentation says: >>> On arm64, the physical address size for a VM (IPA Size limit) is >>> limited to 40bits by default. The limit can be configured if the host >>> supports the extension KVM_CAP_ARM_VM_IPA_SIZE. When supported, use >>> KVM_VM_TYPE_ARM_IPA_SIZE(IPA_Bits) to set the size in the machine type >>> identifier, where IPA_Bits is the maximum width of any physical >>> address used by the VM. The IPA_Bits is encoded in bits[7-0] of the >>> machine type identifier. >>> >>> e.g, to configure a guest to use 48bit physical address size:: >>> >>> vm_fd = ioctl(dev_fd, KVM_CREATE_VM, KVM_VM_TYPE_ARM_IPA_SIZE(48)); >>> >>> The requested size (IPA_Bits) must be: >>> >>> == ========================================================= >>> 0 Implies default size, 40bits (for backward compatibility) >>> N Implies N bits, where N is a positive integer such that, >>> 32 <= N <= Host_IPA_Limit >>> == ========================================================= >> >>> Host_IPA_Limit is the maximum possible value for IPA_Bits on the host >>> and is dependent on the CPU capability and the kernel configuration. >>> The limit can be retrieved using KVM_CAP_ARM_VM_IPA_SIZE of the >>> KVM_CHECK_EXTENSION ioctl() at run-time. >>> >>> Creation of the VM will fail if the requested IPA size (whether it is >>> implicit or explicit) is unsupported on the host. >> https://docs.kernel.org/virt/kvm/api.html#kvm-create-vm >> >> So if Host_IPA_Limit < 40, such KVM_CREATE_VM will fail, and libvirt >> incorrectly thinks KVM is not available. This actually happened on M2 >> MacBook Air. >> >> Fix this by specifying 32 for IPA_Bits as any arm64 system should >> support the value according to the documentation. >> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >> --- >> accel/kvm/kvm-all.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >> index e86c33e0e6..776ac7efcc 100644 >> --- a/accel/kvm/kvm-all.c >> +++ b/accel/kvm/kvm-all.c >> @@ -2294,7 +2294,11 @@ static int kvm_init(MachineState *ms) >> KVMState *s; >> const KVMCapabilityInfo *missing_cap; >> int ret; >> +#ifdef TARGET_AARCH64 >> + int type = 32; >> +#else >> int type = 0; >> +#endif > > No need for an ifdef. Down below we have, > > if (object_property_find(OBJECT(current_machine), "kvm-type")) { > g_autofree char *kvm_type = > object_property_get_str(OBJECT(current_machine), > "kvm-type", > &error_abort); > type = mc->kvm_type(ms, kvm_type); > } else if (mc->kvm_type) { > type = mc->kvm_type(ms, NULL); > } > > and the aarch64 -M virt machine provides virt_kvm_type as mc->kvm_type. > > How did you hit this? Are you trying to implement your own board model? > > Looking at this, I'm surprised this is a board hook and not a cpu hook. > But I suppose the architecture specific 'type' can hide any number of > sins. Anyway, if you are doing your own board model, I suggest > arranging to share the virt board hook -- maybe moving it to > target/arm/kvm.c in the process? > > > r~ I hit this problem when I used libvirt; libvirt uses "none" machine type to probe the availability of KVM and "none" machine type does not provide kvm_type hook. As the implementation of "none" machine type is shared among different architectures, we cannot remove ifdef by moving it to the hook. Although implementing the hook for "none" machine type is still possible, I think the default type should provide the lowest common denominator and "none" machine type shouldn't try to work around when the type is wrong. Otherwise it doesn't make sense to provide the "default". The virt board hook depends on the memory map of the board so it is not straightforward to share it with "none" machine type. Regards, Akihiko Odaki
On Sat, 14 Jan 2023 at 06:49, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > On 2023/01/14 14:23, Richard Henderson wrote: > > On 1/8/23 22:22, Akihiko Odaki wrote: > >> libvirt uses "none" machine type to test KVM availability. Before this > >> change, QEMU used to pass 0 as machine type when calling KVM_CREATE_VM. > >> > >> The kernel documentation says: > >>> On arm64, the physical address size for a VM (IPA Size limit) is > >>> limited to 40bits by default. The limit can be configured if the host > >>> supports the extension KVM_CAP_ARM_VM_IPA_SIZE. When supported, use > >>> KVM_VM_TYPE_ARM_IPA_SIZE(IPA_Bits) to set the size in the machine type > >>> identifier, where IPA_Bits is the maximum width of any physical > >>> address used by the VM. The IPA_Bits is encoded in bits[7-0] of the > >>> machine type identifier. > >>> > >>> e.g, to configure a guest to use 48bit physical address size:: > >>> > >>> vm_fd = ioctl(dev_fd, KVM_CREATE_VM, KVM_VM_TYPE_ARM_IPA_SIZE(48)); > >>> > >>> The requested size (IPA_Bits) must be: > >>> > >>> == ========================================================= > >>> 0 Implies default size, 40bits (for backward compatibility) > >>> N Implies N bits, where N is a positive integer such that, > >>> 32 <= N <= Host_IPA_Limit > >>> == ========================================================= > >> > >>> Host_IPA_Limit is the maximum possible value for IPA_Bits on the host > >>> and is dependent on the CPU capability and the kernel configuration. > >>> The limit can be retrieved using KVM_CAP_ARM_VM_IPA_SIZE of the > >>> KVM_CHECK_EXTENSION ioctl() at run-time. > >>> > >>> Creation of the VM will fail if the requested IPA size (whether it is > >>> implicit or explicit) is unsupported on the host. > >> https://docs.kernel.org/virt/kvm/api.html#kvm-create-vm > >> > >> So if Host_IPA_Limit < 40, such KVM_CREATE_VM will fail, and libvirt > >> incorrectly thinks KVM is not available. This actually happened on M2 > >> MacBook Air. > >> > >> Fix this by specifying 32 for IPA_Bits as any arm64 system should > >> support the value according to the documentation. > >> > >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > >> --- > >> accel/kvm/kvm-all.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > >> index e86c33e0e6..776ac7efcc 100644 > >> --- a/accel/kvm/kvm-all.c > >> +++ b/accel/kvm/kvm-all.c > >> @@ -2294,7 +2294,11 @@ static int kvm_init(MachineState *ms) > >> KVMState *s; > >> const KVMCapabilityInfo *missing_cap; > >> int ret; > >> +#ifdef TARGET_AARCH64 > >> + int type = 32; > >> +#else > >> int type = 0; > >> +#endif > > > > No need for an ifdef. Down below we have, > > > > if (object_property_find(OBJECT(current_machine), "kvm-type")) { > > g_autofree char *kvm_type = > > object_property_get_str(OBJECT(current_machine), > > "kvm-type", > > &error_abort); > > type = mc->kvm_type(ms, kvm_type); > > } else if (mc->kvm_type) { > > type = mc->kvm_type(ms, NULL); > > } > > > > and the aarch64 -M virt machine provides virt_kvm_type as mc->kvm_type. > > > > How did you hit this? Are you trying to implement your own board model? > > > > Looking at this, I'm surprised this is a board hook and not a cpu hook. > > But I suppose the architecture specific 'type' can hide any number of > > sins. Anyway, if you are doing your own board model, I suggest > > arranging to share the virt board hook -- maybe moving it to > > target/arm/kvm.c in the process? > I hit this problem when I used libvirt; libvirt uses "none" machine type > to probe the availability of KVM and "none" machine type does not > provide kvm_type hook. > > As the implementation of "none" machine type is shared among different > architectures, we cannot remove ifdef by moving it to the hook. > > Although implementing the hook for "none" machine type is still > possible, I think the default type should provide the lowest common > denominator and "none" machine type shouldn't try to work around when > the type is wrong. Otherwise it doesn't make sense to provide the "default". Yes, the problem is that the 'none' board type is all architecture-independent code, and so is this kvm_init() code, so there's no obvious arm-specific place to say "pick the best IPA size that will work for this host". Perhaps we should create somewhere in here a target-arch specific hook: we already have ifdefs in this function for S390X and PPC (printing some special case error strings if the ioctl fails), so maybe a hook that does "take the type provided by the machine hook, if any, sanitize or reject it, do the ioctl call, print arch-specific help/error messages if relevant" ? Paolo, do you have an opinion? thanks -- PMM
On 2023/01/16 20:18, Peter Maydell wrote: > On Sat, 14 Jan 2023 at 06:49, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >> >> On 2023/01/14 14:23, Richard Henderson wrote: >>> On 1/8/23 22:22, Akihiko Odaki wrote: >>>> libvirt uses "none" machine type to test KVM availability. Before this >>>> change, QEMU used to pass 0 as machine type when calling KVM_CREATE_VM. >>>> >>>> The kernel documentation says: >>>>> On arm64, the physical address size for a VM (IPA Size limit) is >>>>> limited to 40bits by default. The limit can be configured if the host >>>>> supports the extension KVM_CAP_ARM_VM_IPA_SIZE. When supported, use >>>>> KVM_VM_TYPE_ARM_IPA_SIZE(IPA_Bits) to set the size in the machine type >>>>> identifier, where IPA_Bits is the maximum width of any physical >>>>> address used by the VM. The IPA_Bits is encoded in bits[7-0] of the >>>>> machine type identifier. >>>>> >>>>> e.g, to configure a guest to use 48bit physical address size:: >>>>> >>>>> vm_fd = ioctl(dev_fd, KVM_CREATE_VM, KVM_VM_TYPE_ARM_IPA_SIZE(48)); >>>>> >>>>> The requested size (IPA_Bits) must be: >>>>> >>>>> == ========================================================= >>>>> 0 Implies default size, 40bits (for backward compatibility) >>>>> N Implies N bits, where N is a positive integer such that, >>>>> 32 <= N <= Host_IPA_Limit >>>>> == ========================================================= >>>> >>>>> Host_IPA_Limit is the maximum possible value for IPA_Bits on the host >>>>> and is dependent on the CPU capability and the kernel configuration. >>>>> The limit can be retrieved using KVM_CAP_ARM_VM_IPA_SIZE of the >>>>> KVM_CHECK_EXTENSION ioctl() at run-time. >>>>> >>>>> Creation of the VM will fail if the requested IPA size (whether it is >>>>> implicit or explicit) is unsupported on the host. >>>> https://docs.kernel.org/virt/kvm/api.html#kvm-create-vm >>>> >>>> So if Host_IPA_Limit < 40, such KVM_CREATE_VM will fail, and libvirt >>>> incorrectly thinks KVM is not available. This actually happened on M2 >>>> MacBook Air. >>>> >>>> Fix this by specifying 32 for IPA_Bits as any arm64 system should >>>> support the value according to the documentation. >>>> >>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>>> --- >>>> accel/kvm/kvm-all.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >>>> index e86c33e0e6..776ac7efcc 100644 >>>> --- a/accel/kvm/kvm-all.c >>>> +++ b/accel/kvm/kvm-all.c >>>> @@ -2294,7 +2294,11 @@ static int kvm_init(MachineState *ms) >>>> KVMState *s; >>>> const KVMCapabilityInfo *missing_cap; >>>> int ret; >>>> +#ifdef TARGET_AARCH64 >>>> + int type = 32; >>>> +#else >>>> int type = 0; >>>> +#endif >>> >>> No need for an ifdef. Down below we have, >>> >>> if (object_property_find(OBJECT(current_machine), "kvm-type")) { >>> g_autofree char *kvm_type = >>> object_property_get_str(OBJECT(current_machine), >>> "kvm-type", >>> &error_abort); >>> type = mc->kvm_type(ms, kvm_type); >>> } else if (mc->kvm_type) { >>> type = mc->kvm_type(ms, NULL); >>> } >>> >>> and the aarch64 -M virt machine provides virt_kvm_type as mc->kvm_type. >>> >>> How did you hit this? Are you trying to implement your own board model? >>> >>> Looking at this, I'm surprised this is a board hook and not a cpu hook. >>> But I suppose the architecture specific 'type' can hide any number of >>> sins. Anyway, if you are doing your own board model, I suggest >>> arranging to share the virt board hook -- maybe moving it to >>> target/arm/kvm.c in the process? > >> I hit this problem when I used libvirt; libvirt uses "none" machine type >> to probe the availability of KVM and "none" machine type does not >> provide kvm_type hook. >> >> As the implementation of "none" machine type is shared among different >> architectures, we cannot remove ifdef by moving it to the hook. >> >> Although implementing the hook for "none" machine type is still >> possible, I think the default type should provide the lowest common >> denominator and "none" machine type shouldn't try to work around when >> the type is wrong. Otherwise it doesn't make sense to provide the "default". > > Yes, the problem is that the 'none' board type is all > architecture-independent code, and so is this kvm_init() code, so > there's no obvious arm-specific place to say "pick the best IPA size > that will work for this host". > > Perhaps we should create somewhere in here a target-arch specific > hook: we already have ifdefs in this function for S390X and PPC > (printing some special case error strings if the ioctl fails), so > maybe a hook that does "take the type provided by the machine hook, > if any, sanitize or reject it, do the ioctl call, print arch-specific > help/error messages if relevant" ? Paolo, do you have an opinion? > > thanks > -- PMM Hi Paolo, I have sent this patch a while ago but it's kind of missed so I'm about to push this forward again. Can you have a look at this? Regards, Akihiko Odaki
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index e86c33e0e6..776ac7efcc 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -2294,7 +2294,11 @@ static int kvm_init(MachineState *ms) KVMState *s; const KVMCapabilityInfo *missing_cap; int ret; +#ifdef TARGET_AARCH64 + int type = 32; +#else int type = 0; +#endif uint64_t dirty_log_manual_caps; qemu_mutex_init(&kml_slots_lock);