| Message ID | 20200924145757.1035782-5-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: hyper-v: make KVM_GET_SUPPORTED_HV_CPUID more useful | expand |
On 24/09/20 16:57, Vitaly Kuznetsov wrote: > HV_STIMER_DIRECT_MODE_AVAILABLE is the last conditionally set feature bit > in KVM_GET_SUPPORTED_HV_CPUID but it doesn't have to be conditional: first, > this bit is only an indication to userspace VMM that direct mode stimers > are supported, it still requires manual enablement (enabling SynIC) to > work so no VMM should just blindly copy it to guest CPUIDs. Second, > lapic_in_kernel() is a must for SynIC. Expose the bit unconditionally. > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > arch/x86/kvm/hyperv.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > index 6da20f91cd59..503829f71270 100644 > --- a/arch/x86/kvm/hyperv.c > +++ b/arch/x86/kvm/hyperv.c > @@ -2028,13 +2028,7 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid, > ent->ebx |= HV_DEBUGGING; > ent->edx |= HV_X64_GUEST_DEBUGGING_AVAILABLE; > ent->edx |= HV_FEATURE_DEBUG_MSRS_AVAILABLE; > - > - /* > - * Direct Synthetic timers only make sense with in-kernel > - * LAPIC > - */ > - if (lapic_in_kernel(vcpu)) > - ent->edx |= HV_STIMER_DIRECT_MODE_AVAILABLE; > + ent->edx |= HV_STIMER_DIRECT_MODE_AVAILABLE; > > break; > > Sorry for the late reply. I think this is making things worse. It's obviously okay to add a system KVM_GET_SUPPORTED_HV_CPUID, and I guess it makes sense to have bits in there that require to enable a capability. For example, KVM_GET_SUPPORTED_CPUID has a couple bits such as X2APIC, that we return even if they require in-kernel irqchip. For the vCPU version however we should be able to copy the returned leaves to KVM_SET_CPUID2, meaning that unsupported features should be masked. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 24/09/20 16:57, Vitaly Kuznetsov wrote: >> HV_STIMER_DIRECT_MODE_AVAILABLE is the last conditionally set feature bit >> in KVM_GET_SUPPORTED_HV_CPUID but it doesn't have to be conditional: first, >> this bit is only an indication to userspace VMM that direct mode stimers >> are supported, it still requires manual enablement (enabling SynIC) to >> work so no VMM should just blindly copy it to guest CPUIDs. Second, >> lapic_in_kernel() is a must for SynIC. Expose the bit unconditionally. >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> >> --- >> arch/x86/kvm/hyperv.c | 8 +------- >> 1 file changed, 1 insertion(+), 7 deletions(-) >> >> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c >> index 6da20f91cd59..503829f71270 100644 >> --- a/arch/x86/kvm/hyperv.c >> +++ b/arch/x86/kvm/hyperv.c >> @@ -2028,13 +2028,7 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid, >> ent->ebx |= HV_DEBUGGING; >> ent->edx |= HV_X64_GUEST_DEBUGGING_AVAILABLE; >> ent->edx |= HV_FEATURE_DEBUG_MSRS_AVAILABLE; >> - >> - /* >> - * Direct Synthetic timers only make sense with in-kernel >> - * LAPIC >> - */ >> - if (lapic_in_kernel(vcpu)) >> - ent->edx |= HV_STIMER_DIRECT_MODE_AVAILABLE; >> + ent->edx |= HV_STIMER_DIRECT_MODE_AVAILABLE; >> >> break; >> >> > > Sorry for the late reply. I think this is making things worse. It's > obviously okay to add a system KVM_GET_SUPPORTED_HV_CPUID, and I guess > it makes sense to have bits in there that require to enable a > capability. For example, KVM_GET_SUPPORTED_CPUID has a couple bits such > as X2APIC, that we return even if they require in-kernel irqchip. > > For the vCPU version however we should be able to copy the returned > leaves to KVM_SET_CPUID2, meaning that unsupported features should be > masked. What I don't quite like about exposing HV_STIMER_DIRECT_MODE_AVAILABLE conditionally is that we're requiring userspace to have a certain control flow: first, it needs to create irqchip and only then call KVM_GET_SUPPORTED_HV_CPUID or it won't know that HV_STIMER_DIRECT_MODE_AVAILABLE is supported. Also, are you only concerned about HV_STIMER_DIRECT_MODE_AVAILABLE? E.g. PATCH3 of this series is somewhat similar, it exposes eVMCS even when the corresponding CAP wasn't enabled. While I slightly prefer to get rid of this conditional feature exposure once and for all, I don't really feel very strong about it. We can have the system ioctl which always exposes all supported features and vCPU version which only exposes what is currently enabled. We would, however, need to preserve some inconsistency as a legacy: e.g. SynIC bits are now exposed unconditionally, even before KVM_CAP_HYPERV_SYNIC[2] is enabled (and if we change that we will break at least QEMU).
On 29/09/20 12:36, Vitaly Kuznetsov wrote: >> Sorry for the late reply. I think this is making things worse. It's >> obviously okay to add a system KVM_GET_SUPPORTED_HV_CPUID, and I guess >> it makes sense to have bits in there that require to enable a >> capability. For example, KVM_GET_SUPPORTED_CPUID has a couple bits such >> as X2APIC, that we return even if they require in-kernel irqchip. >> >> For the vCPU version however we should be able to copy the returned >> leaves to KVM_SET_CPUID2, meaning that unsupported features should be >> masked. > What I don't quite like about exposing HV_STIMER_DIRECT_MODE_AVAILABLE > conditionally is that we're requiring userspace to have a certain > control flow: first, it needs to create irqchip and only then call > KVM_GET_SUPPORTED_HV_CPUID or it won't know that > HV_STIMER_DIRECT_MODE_AVAILABLE is supported. > > Also, are you only concerned about HV_STIMER_DIRECT_MODE_AVAILABLE? E.g. > PATCH3 of this series is somewhat similar, it exposes eVMCS even when > the corresponding CAP wasn't enabled. All of them, but this was only about the vCPU ioctl. I agree with you that the system ioctl should return everything unconditionally. But perhaps the best thing to do is to deprecate the vCPU ioctl and just leave it as is with all its quirks. Paolo > While I slightly prefer to get rid of this conditional feature exposure > once and for all, I don't really feel very strong about it. We can have > the system ioctl which always exposes all supported features and vCPU > version which only exposes what is currently enabled. We would, however, > need to preserve some inconsistency as a legacy: e.g. SynIC bits are now > exposed unconditionally, even before KVM_CAP_HYPERV_SYNIC[2] is enabled > (and if we change that we will break at least QEMU).
Paolo Bonzini <pbonzini@redhat.com> writes: > On 29/09/20 12:36, Vitaly Kuznetsov wrote: >>> Sorry for the late reply. I think this is making things worse. It's >>> obviously okay to add a system KVM_GET_SUPPORTED_HV_CPUID, and I guess >>> it makes sense to have bits in there that require to enable a >>> capability. For example, KVM_GET_SUPPORTED_CPUID has a couple bits such >>> as X2APIC, that we return even if they require in-kernel irqchip. >>> >>> For the vCPU version however we should be able to copy the returned >>> leaves to KVM_SET_CPUID2, meaning that unsupported features should be >>> masked. >> What I don't quite like about exposing HV_STIMER_DIRECT_MODE_AVAILABLE >> conditionally is that we're requiring userspace to have a certain >> control flow: first, it needs to create irqchip and only then call >> KVM_GET_SUPPORTED_HV_CPUID or it won't know that >> HV_STIMER_DIRECT_MODE_AVAILABLE is supported. >> >> Also, are you only concerned about HV_STIMER_DIRECT_MODE_AVAILABLE? E.g. >> PATCH3 of this series is somewhat similar, it exposes eVMCS even when >> the corresponding CAP wasn't enabled. > > All of them, but this was only about the vCPU ioctl. I agree with you > that the system ioctl should return everything unconditionally. > > But perhaps the best thing to do is to deprecate the vCPU ioctl and just > leave it as is with all its quirks. > Ok, I'll do exactly that. I'm not sure if there are any KVM_GET_SUPPORTED_HV_CPUID users out there bisedes QEMU/selftest but let's take the 'safest' approach.
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c index 6da20f91cd59..503829f71270 100644 --- a/arch/x86/kvm/hyperv.c +++ b/arch/x86/kvm/hyperv.c @@ -2028,13 +2028,7 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid, ent->ebx |= HV_DEBUGGING; ent->edx |= HV_X64_GUEST_DEBUGGING_AVAILABLE; ent->edx |= HV_FEATURE_DEBUG_MSRS_AVAILABLE; - - /* - * Direct Synthetic timers only make sense with in-kernel - * LAPIC - */ - if (lapic_in_kernel(vcpu)) - ent->edx |= HV_STIMER_DIRECT_MODE_AVAILABLE; + ent->edx |= HV_STIMER_DIRECT_MODE_AVAILABLE; break;
HV_STIMER_DIRECT_MODE_AVAILABLE is the last conditionally set feature bit in KVM_GET_SUPPORTED_HV_CPUID but it doesn't have to be conditional: first, this bit is only an indication to userspace VMM that direct mode stimers are supported, it still requires manual enablement (enabling SynIC) to work so no VMM should just blindly copy it to guest CPUIDs. Second, lapic_in_kernel() is a must for SynIC. Expose the bit unconditionally. Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- arch/x86/kvm/hyperv.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)