| Message ID | 1603330475-7063-1-git-send-email-wanpengli@tencent.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: X86: Expose KVM_HINTS_REALTIME in KVM_GET_SUPPORTED_CPUID | expand |
On 22/10/20 03:34, Wanpeng Li wrote: > From: Wanpeng Li <wanpengli@tencent.com> > > Per KVM_GET_SUPPORTED_CPUID ioctl documentation: > > This ioctl returns x86 cpuid features which are supported by both the > hardware and kvm in its default configuration. > > A well-behaved userspace should not set the bit if it is not supported. > > Suggested-by: Jim Mattson <jmattson@google.com> > Signed-off-by: Wanpeng Li <wanpengli@tencent.com> It's common for userspace to copy all supported CPUID bits to KVM_SET_CPUID2, I don't think this is the right behavior for KVM_HINTS_REALTIME. (But maybe this was discussed already; if so, please point me to the previous discussion). Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 22/10/20 03:34, Wanpeng Li wrote: >> From: Wanpeng Li <wanpengli@tencent.com> >> >> Per KVM_GET_SUPPORTED_CPUID ioctl documentation: >> >> This ioctl returns x86 cpuid features which are supported by both the >> hardware and kvm in its default configuration. >> >> A well-behaved userspace should not set the bit if it is not supported. >> >> Suggested-by: Jim Mattson <jmattson@google.com> >> Signed-off-by: Wanpeng Li <wanpengli@tencent.com> > > It's common for userspace to copy all supported CPUID bits to > KVM_SET_CPUID2, I don't think this is the right behavior for > KVM_HINTS_REALTIME. > > (But maybe this was discussed already; if so, please point me to the > previous discussion). > Not for KVM_GET_SUPPORTED_CPUID but for KVM_GET_SUPPORTED_HV_CPUID: just copying all the bits blindly is incorrect as e.g. SYNIC needs to be enabled with KVM_CAP_HYPERV_SYNIC[2]. KVM PV features also have pre-requisites, e.g. KVM_ASYNC_PF_DELIVERY_AS_INT requires an irqchip so again copy/paste may not work.
On 10/22/2020 9:02 PM, Paolo Bonzini wrote: > On 22/10/20 03:34, Wanpeng Li wrote: >> From: Wanpeng Li <wanpengli@tencent.com> >> >> Per KVM_GET_SUPPORTED_CPUID ioctl documentation: >> >> This ioctl returns x86 cpuid features which are supported by both the >> hardware and kvm in its default configuration. >> >> A well-behaved userspace should not set the bit if it is not supported. >> >> Suggested-by: Jim Mattson <jmattson@google.com> >> Signed-off-by: Wanpeng Li <wanpengli@tencent.com> > > It's common for userspace to copy all supported CPUID bits to > KVM_SET_CPUID2, I don't think this is the right behavior for > KVM_HINTS_REALTIME. It reminds of X86_FEATURE_WAITPKG, which is added to supported CPUID recently as a fix but QEMU exposes it to guest only when "-overcommit cpu-pm" > (But maybe this was discussed already; if so, please point me to the > previous discussion). > > Paolo >
On 22/10/20 15:31, Xiaoyao Li wrote: >> >> It's common for userspace to copy all supported CPUID bits to >> KVM_SET_CPUID2, I don't think this is the right behavior for >> KVM_HINTS_REALTIME. > > It reminds of X86_FEATURE_WAITPKG, which is added to supported CPUID > recently as a fix but QEMU exposes it to guest only when "-overcommit > cpu-pm" WAITPKG is not included in KVM_GET_SUPPORTED_CPUID either. QEMU detects it through the MSR_IA32_UMWAIT register. Paolo
On 10/22/2020 10:06 PM, Paolo Bonzini wrote: > On 22/10/20 15:31, Xiaoyao Li wrote: >>> >>> It's common for userspace to copy all supported CPUID bits to >>> KVM_SET_CPUID2, I don't think this is the right behavior for >>> KVM_HINTS_REALTIME. >> >> It reminds of X86_FEATURE_WAITPKG, which is added to supported CPUID >> recently as a fix but QEMU exposes it to guest only when "-overcommit >> cpu-pm" > > WAITPKG is not included in KVM_GET_SUPPORTED_CPUID either. QEMU detects > it through the MSR_IA32_UMWAIT register. Doesn't 0abcc8f65cc2 ("KVM: VMX: enable X86_FEATURE_WAITPKG in KVM capabilities") add WAITPKG to KVM_GET_SUPPORTED_CPUID? > Paolo >
On 22/10/20 16:28, Xiaoyao Li wrote: > On 10/22/2020 10:06 PM, Paolo Bonzini wrote: >> On 22/10/20 15:31, Xiaoyao Li wrote: >>>> >>>> It's common for userspace to copy all supported CPUID bits to >>>> KVM_SET_CPUID2, I don't think this is the right behavior for >>>> KVM_HINTS_REALTIME. >>> >>> It reminds of X86_FEATURE_WAITPKG, which is added to supported CPUID >>> recently as a fix but QEMU exposes it to guest only when "-overcommit >>> cpu-pm" >> >> WAITPKG is not included in KVM_GET_SUPPORTED_CPUID either. QEMU detects >> it through the MSR_IA32_UMWAIT register. > > Doesn't 0abcc8f65cc2 ("KVM: VMX: enable X86_FEATURE_WAITPKG in KVM > capabilities") add WAITPKG to KVM_GET_SUPPORTED_CPUID? You're right, I shouldn't have checked only cpuid.c. :) Still I think WAITPKG is different, because it's only for userspace and it's always possible for userspace to do "for(;;)" and burn CPU. KVM_HINTS_REALTIME is more similar to MONITOR, which is not set. Paolo
On Thu, Oct 22, 2020 at 6:02 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 22/10/20 03:34, Wanpeng Li wrote: > > From: Wanpeng Li <wanpengli@tencent.com> > > > > Per KVM_GET_SUPPORTED_CPUID ioctl documentation: > > > > This ioctl returns x86 cpuid features which are supported by both the > > hardware and kvm in its default configuration. > > > > A well-behaved userspace should not set the bit if it is not supported. > > > > Suggested-by: Jim Mattson <jmattson@google.com> > > Signed-off-by: Wanpeng Li <wanpengli@tencent.com> > > It's common for userspace to copy all supported CPUID bits to > KVM_SET_CPUID2, I don't think this is the right behavior for > KVM_HINTS_REALTIME. That is not how the API is defined, but I'm sure you know that. :-) > (But maybe this was discussed already; if so, please point me to the > previous discussion). > > Paolo
On 22/10/20 18:35, Jim Mattson wrote: > On Thu, Oct 22, 2020 at 6:02 AM Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> On 22/10/20 03:34, Wanpeng Li wrote: >>> From: Wanpeng Li <wanpengli@tencent.com> >>> >>> Per KVM_GET_SUPPORTED_CPUID ioctl documentation: >>> >>> This ioctl returns x86 cpuid features which are supported by both the >>> hardware and kvm in its default configuration. >>> >>> A well-behaved userspace should not set the bit if it is not supported. >>> >>> Suggested-by: Jim Mattson <jmattson@google.com> >>> Signed-off-by: Wanpeng Li <wanpengli@tencent.com> >> >> It's common for userspace to copy all supported CPUID bits to >> KVM_SET_CPUID2, I don't think this is the right behavior for >> KVM_HINTS_REALTIME. > > That is not how the API is defined, but I'm sure you know that. :-) Yes, though in my defense :) KVM_HINTS_REALTIME is not a property of the kernel, it's a property of the environment that the guest runs in. This was the original reason to separate it from other feature bits in the same leaf. Paolo
On Thu, Oct 22, 2020 at 9:37 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 22/10/20 18:35, Jim Mattson wrote: > > On Thu, Oct 22, 2020 at 6:02 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > >> > >> On 22/10/20 03:34, Wanpeng Li wrote: > >>> From: Wanpeng Li <wanpengli@tencent.com> > >>> > >>> Per KVM_GET_SUPPORTED_CPUID ioctl documentation: > >>> > >>> This ioctl returns x86 cpuid features which are supported by both the > >>> hardware and kvm in its default configuration. > >>> > >>> A well-behaved userspace should not set the bit if it is not supported. > >>> > >>> Suggested-by: Jim Mattson <jmattson@google.com> > >>> Signed-off-by: Wanpeng Li <wanpengli@tencent.com> > >> > >> It's common for userspace to copy all supported CPUID bits to > >> KVM_SET_CPUID2, I don't think this is the right behavior for > >> KVM_HINTS_REALTIME. > > > > That is not how the API is defined, but I'm sure you know that. :-) > > Yes, though in my defense :) KVM_HINTS_REALTIME is not a property of the > kernel, it's a property of the environment that the guest runs in. This > was the original reason to separate it from other feature bits in the > same leaf. > > Paolo > We don't actually use KVM_GET_SUPPORTED_CPUID at all today. If it's commonly being misinterpreted as you say, perhaps we should add a KVM_GET_TRUE_SUPPORTED_CPUID ioctl. Or, perhaps we can just fix this in the documentation?
On Thu, 22 Oct 2020 at 21:02, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 22/10/20 03:34, Wanpeng Li wrote: > > From: Wanpeng Li <wanpengli@tencent.com> > > > > Per KVM_GET_SUPPORTED_CPUID ioctl documentation: > > > > This ioctl returns x86 cpuid features which are supported by both the > > hardware and kvm in its default configuration. > > > > A well-behaved userspace should not set the bit if it is not supported. > > > > Suggested-by: Jim Mattson <jmattson@google.com> > > Signed-off-by: Wanpeng Li <wanpengli@tencent.com> > > It's common for userspace to copy all supported CPUID bits to > KVM_SET_CPUID2, I don't think this is the right behavior for > KVM_HINTS_REALTIME. > > (But maybe this was discussed already; if so, please point me to the > previous discussion). The discussion is here. :) https://www.spinics.net/lists/kvm/msg227265.html Wanpeng
On 22/10/20 19:13, Jim Mattson wrote: > We don't actually use KVM_GET_SUPPORTED_CPUID at all today. If it's > commonly being misinterpreted as you say, perhaps we should add a > KVM_GET_TRUE_SUPPORTED_CPUID ioctl. Or, perhaps we can just fix this > in the documentation? Yes, I think we should fix the documentation and document the best practices around MSRs and CPUID bits. Mostly documenting what QEMU does, perhaps without all the quirks it has to support old kernels that messed things up even more. Paolo
On Fri, Oct 23, 2020 at 2:07 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 22/10/20 19:13, Jim Mattson wrote: > > We don't actually use KVM_GET_SUPPORTED_CPUID at all today. If it's > > commonly being misinterpreted as you say, perhaps we should add a > > KVM_GET_TRUE_SUPPORTED_CPUID ioctl. Or, perhaps we can just fix this > > in the documentation? > > Yes, I think we should fix the documentation and document the best > practices around MSRs and CPUID bits. Mostly documenting what QEMU > does, perhaps without all the quirks it has to support old kernels that > messed things up even more. > > Paolo I'd really like to be able to call an ioctl that will help me determine whether the host can support the guest CPUID information that I already have (e.g. on the target of a live migration). At first glance, KVM_GET_SUPPORTED_CPUID appeared to be that ioctl. Sadly, it appears that no such ioctl exists.
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 06a278b..225d251 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -789,7 +789,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function) entry->ebx = 0; entry->ecx = 0; - entry->edx = 0; + entry->edx = (1 << KVM_HINTS_REALTIME); break; case 0x80000000: entry->eax = min(entry->eax, 0x8000001f);