| Message ID | 20220714091327.1085353-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 Thu, Jul 14, 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. > > 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(+) > > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > index e2e95a6fccfd..e08189211d9a 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; Doesn't KVM also need to switch to enforcing the "access" flag? diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c index c284a605e453..ca91547034e4 100644 --- a/arch/x86/kvm/hyperv.c +++ b/arch/x86/kvm/hyperv.c @@ -1282,7 +1282,7 @@ static bool hv_check_msr_access(struct kvm_vcpu_hv *hv_vcpu, u32 msr) case HV_X64_MSR_SYNDBG_OPTIONS: case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER: return hv_vcpu->cpuid_cache.features_edx & - HV_FEATURE_DEBUG_MSRS_AVAILABLE; + HV_ACCESS_DEBUG_MSRS; default: break; }
On 7/21/22 23:43, Sean Christopherson wrote: > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > index c284a605e453..ca91547034e4 100644 > --- a/arch/x86/kvm/hyperv.c > +++ b/arch/x86/kvm/hyperv.c > @@ -1282,7 +1282,7 @@ static bool hv_check_msr_access(struct kvm_vcpu_hv *hv_vcpu, u32 msr) > case HV_X64_MSR_SYNDBG_OPTIONS: > case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER: > return hv_vcpu->cpuid_cache.features_edx & > - HV_FEATURE_DEBUG_MSRS_AVAILABLE; > + HV_ACCESS_DEBUG_MSRS; > default: > break; > } > Yes, and this will need some kind of hack in QEMU to expose both CPUID bits. Fortunately hv-syndbg shouldn't be in much use in the wild, so I think we can avoid quirks etc. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 7/21/22 23:43, Sean Christopherson wrote: >> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c >> index c284a605e453..ca91547034e4 100644 >> --- a/arch/x86/kvm/hyperv.c >> +++ b/arch/x86/kvm/hyperv.c >> @@ -1282,7 +1282,7 @@ static bool hv_check_msr_access(struct kvm_vcpu_hv *hv_vcpu, u32 msr) >> case HV_X64_MSR_SYNDBG_OPTIONS: >> case HV_X64_MSR_SYNDBG_CONTROL ... HV_X64_MSR_SYNDBG_PENDING_BUFFER: >> return hv_vcpu->cpuid_cache.features_edx & >> - HV_FEATURE_DEBUG_MSRS_AVAILABLE; >> + HV_ACCESS_DEBUG_MSRS; >> default: >> break; >> } >> > > Yes, and this will need some kind of hack in QEMU to expose both CPUID > bits. Fortunately hv-syndbg shouldn't be in much use in the wild, so I > think we can avoid quirks etc. Properly behaving VMM should always expose both bits. I'm not sure what would it mean if only 'access' bit is present: you can try accessing the missing feature but you get #GP anyway most likely. When the feature is available but 'access' bit is not set -- the result is also #GP. In case we really want to support this behavior in KVM we should probably check *both* bits in hv_check_msr_access() but I don't really see a use-case. I've lazily kept HV_FEATURE_DEBUG_MSRS_AVAILABLE here just to be QEMU compatible.
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c index e2e95a6fccfd..e08189211d9a 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. 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(+)