| Message ID | 20220802160756.339464-2-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: VMX: Support updated eVMCSv1 revision + use vmcs_config for L1 VMX MSRs | expand |
On Tue, Aug 02, 2022, Vitaly Kuznetsov wrote: > For some features, Hyper-V spec defines two separate CPUID bits: one > listing whether the feature is supported or not and another one showing > whether guest partition was granted access to the feature ("partition > privilege mask"). 'Debug MSRs available' is one of such features. Add > the missing 'access' bit. > > Note: hv_check_msr_access() deliberately keeps checking > HV_FEATURE_DEBUG_MSRS_AVAILABLE bit instead of the new HV_ACCESS_DEBUG_MSRS > to not break existing VMMs (QEMU) which only expose one bit. Normally, VMMs > should set either both these bits or none. This is not the right approach long term. If KVM absolutely cannot unconditionally switch to checking HV_ACCESS_DEBUG_MSRS because it would break QEMU users, then we should add a quirk, but sweeping the whole thing under the rug is wrong.
Sean Christopherson <seanjc@google.com> writes: > On Tue, Aug 02, 2022, Vitaly Kuznetsov wrote: >> For some features, Hyper-V spec defines two separate CPUID bits: one >> listing whether the feature is supported or not and another one showing >> whether guest partition was granted access to the feature ("partition >> privilege mask"). 'Debug MSRs available' is one of such features. Add >> the missing 'access' bit. >> >> Note: hv_check_msr_access() deliberately keeps checking >> HV_FEATURE_DEBUG_MSRS_AVAILABLE bit instead of the new HV_ACCESS_DEBUG_MSRS >> to not break existing VMMs (QEMU) which only expose one bit. Normally, VMMs >> should set either both these bits or none. > > This is not the right approach long term. If KVM absolutely cannot unconditionally > switch to checking HV_ACCESS_DEBUG_MSRS because it would break QEMU users, then we > should add a quirk, but sweeping the whole thing under the rug is wrong. > First, this patch is kind of unrelated to the series so in case it's the only thing which blocks it from being merged -- let's just pull it out and discuss separately. My personal opinion is that in this particular case we actually can switch to checking HV_ACCESS_DEBUG_MSRS and possibly backport this patch to stable@ and be done with it as SynDBG is a debug feature which is not supposed to be used much in the wild. This, however, will not give us much besides 'purity' in KVM as no sane VMM is supposed to set just one of the HV_FEATURE_DEBUG_MSRS_AVAILABLE/HV_ACCESS_DEBUG_MSRS bits. TL;DR: I'm not against the change.
On Thu, Aug 18, 2022, Vitaly Kuznetsov wrote: > Sean Christopherson <seanjc@google.com> writes: > > > On Tue, Aug 02, 2022, Vitaly Kuznetsov wrote: > >> For some features, Hyper-V spec defines two separate CPUID bits: one > >> listing whether the feature is supported or not and another one showing > >> whether guest partition was granted access to the feature ("partition > >> privilege mask"). 'Debug MSRs available' is one of such features. Add > >> the missing 'access' bit. > >> > >> Note: hv_check_msr_access() deliberately keeps checking > >> HV_FEATURE_DEBUG_MSRS_AVAILABLE bit instead of the new HV_ACCESS_DEBUG_MSRS > >> to not break existing VMMs (QEMU) which only expose one bit. Normally, VMMs > >> should set either both these bits or none. > > > > This is not the right approach long term. If KVM absolutely cannot unconditionally > > switch to checking HV_ACCESS_DEBUG_MSRS because it would break QEMU users, then we > > should add a quirk, but sweeping the whole thing under the rug is wrong. > > > > First, this patch is kind of unrelated to the series so in case it's the > only thing which blocks it from being merged -- let's just pull it out > and discuss separately. Regarding the series, are there any true dependencies between the eVMCS patches (1 - 11) and the VMCS sanitization rework (12 - 26)? I.e. can the VMCS rework be queued ahead of the eVMCS v1 support?
Sean Christopherson <seanjc@google.com> writes: > On Thu, Aug 18, 2022, Vitaly Kuznetsov wrote: >> Sean Christopherson <seanjc@google.com> writes: >> >> > On Tue, Aug 02, 2022, Vitaly Kuznetsov wrote: >> >> For some features, Hyper-V spec defines two separate CPUID bits: one >> >> listing whether the feature is supported or not and another one showing >> >> whether guest partition was granted access to the feature ("partition >> >> privilege mask"). 'Debug MSRs available' is one of such features. Add >> >> the missing 'access' bit. >> >> >> >> Note: hv_check_msr_access() deliberately keeps checking >> >> HV_FEATURE_DEBUG_MSRS_AVAILABLE bit instead of the new HV_ACCESS_DEBUG_MSRS >> >> to not break existing VMMs (QEMU) which only expose one bit. Normally, VMMs >> >> should set either both these bits or none. >> > >> > This is not the right approach long term. If KVM absolutely cannot unconditionally >> > switch to checking HV_ACCESS_DEBUG_MSRS because it would break QEMU users, then we >> > should add a quirk, but sweeping the whole thing under the rug is wrong. >> > >> >> First, this patch is kind of unrelated to the series so in case it's the >> only thing which blocks it from being merged -- let's just pull it out >> and discuss separately. > > Regarding the series, are there any true dependencies between the eVMCS patches > (1 - 11) and the VMCS sanitization rework (12 - 26)? I.e. can the VMCS rework > be queued ahead of the eVMCS v1 support? My memory is a bit blurry already but I think PATCH11 ("KVM: VMX: Get rid of eVMCS specific VMX controls sanitization") needs to go before PATCH24 ("KVM: nVMX: Use sanitized allowed-1 bits for VMX control MSRs") to have "bug compatibility" and resolve Jim's concern: guest visible VMX feature MSR values are not supposed to change. Currently, we filter out unsupported features from eVMCS for KVM itself but not for L1 as we expose raw host MSR values there. This is likely broken if L1 decides to *use* these features for real but that's another story.
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c index ed804447589c..c284a605e453 100644 --- a/arch/x86/kvm/hyperv.c +++ b/arch/x86/kvm/hyperv.c @@ -2496,6 +2496,7 @@ int kvm_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid, ent->eax |= HV_MSR_RESET_AVAILABLE; ent->eax |= HV_MSR_REFERENCE_TSC_AVAILABLE; ent->eax |= HV_ACCESS_FREQUENCY_MSRS; + ent->eax |= HV_ACCESS_DEBUG_MSRS; ent->eax |= HV_ACCESS_REENLIGHTENMENT; ent->ebx |= HV_POST_MESSAGES; diff --git a/include/asm-generic/hyperv-tlfs.h b/include/asm-generic/hyperv-tlfs.h index fdce7a4cfc6f..1d99dd296a76 100644 --- a/include/asm-generic/hyperv-tlfs.h +++ b/include/asm-generic/hyperv-tlfs.h @@ -70,6 +70,8 @@ #define HV_MSR_GUEST_IDLE_AVAILABLE BIT(10) /* Partition local APIC and TSC frequency registers available */ #define HV_ACCESS_FREQUENCY_MSRS BIT(11) +/* Debug MSRs available */ +#define HV_ACCESS_DEBUG_MSRS BIT(12) /* AccessReenlightenmentControls privilege */ #define HV_ACCESS_REENLIGHTENMENT BIT(13) /* AccessTscInvariantControls privilege */
For some features, Hyper-V spec defines two separate CPUID bits: one listing whether the feature is supported or not and another one showing whether guest partition was granted access to the feature ("partition privilege mask"). 'Debug MSRs available' is one of such features. Add the missing 'access' bit. Note: hv_check_msr_access() deliberately keeps checking HV_FEATURE_DEBUG_MSRS_AVAILABLE bit instead of the new HV_ACCESS_DEBUG_MSRS to not break existing VMMs (QEMU) which only expose one bit. Normally, VMMs should set either both these bits or none. Fixes: f97f5a56f597 ("x86/kvm/hyper-v: Add support for synthetic debugger interface") Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- arch/x86/kvm/hyperv.c | 1 + include/asm-generic/hyperv-tlfs.h | 2 ++ 2 files changed, 3 insertions(+)