[PATCH] KVM: X86: Expose KVM_HINTS_REALTIME in KVM_GET_SUPPORTED_CPUID

[PATCH] KVM: X86: Expose KVM_HINTS_REALTIME in KVM_GET_SUPPORTED_CPUID

[Wanpeng Li]

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>
---
 arch/x86/kvm/cpuid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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);
-- 
2.7.4

Re: [PATCH] KVM: X86: Expose KVM_HINTS_REALTIME in KVM_GET_SUPPORTED_CPUID

[Paolo Bonzini]

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

Re: [PATCH] KVM: X86: Expose KVM_HINTS_REALTIME in KVM_GET_SUPPORTED_CPUID

[Vitaly Kuznetsov]

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.

-- 
Vitaly

Re: [PATCH] KVM: X86: Expose KVM_HINTS_REALTIME in KVM_GET_SUPPORTED_CPUID

[Xiaoyao Li]

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
> 

Re: [PATCH] KVM: X86: Expose KVM_HINTS_REALTIME in KVM_GET_SUPPORTED_CPUID

[Paolo Bonzini]

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

Re: [PATCH] KVM: X86: Expose KVM_HINTS_REALTIME in KVM_GET_SUPPORTED_CPUID

[Xiaoyao Li]

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
> 

Re: [PATCH] KVM: X86: Expose KVM_HINTS_REALTIME in KVM_GET_SUPPORTED_CPUID

[Paolo Bonzini]

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

Re: [PATCH] KVM: X86: Expose KVM_HINTS_REALTIME in KVM_GET_SUPPORTED_CPUID

[Jim Mattson]

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

Re: [PATCH] KVM: X86: Expose KVM_HINTS_REALTIME in KVM_GET_SUPPORTED_CPUID

[Paolo Bonzini]

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

Re: [PATCH] KVM: X86: Expose KVM_HINTS_REALTIME in KVM_GET_SUPPORTED_CPUID

[Jim Mattson]

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?

Re: [PATCH] KVM: X86: Expose KVM_HINTS_REALTIME in KVM_GET_SUPPORTED_CPUID

[Paolo Bonzini]

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

Re: [PATCH] KVM: X86: Expose KVM_HINTS_REALTIME in KVM_GET_SUPPORTED_CPUID

[Jim Mattson]

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.

Re: [PATCH] KVM: X86: Expose KVM_HINTS_REALTIME in KVM_GET_SUPPORTED_CPUID

[Wanpeng Li]

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