On Fri, 2019-02-22 at 13:51 +0100, Paolo Bonzini wrote:
> On 22/02/19 02:30, Sean Christopherson wrote:
> > if (kvm_advertise_kvm()) {
> > if (<handle kvm msr>)
> > return ...;
> > } else if (kvm_advertise_hyperv()) {
> > if (<handle hyperv msr>)
> > return ...;
> > } else if (kvm_advertise_xen()) {
> > if (<handle xen msrs>)
> > return ...;
> > }
> >
> > <fall through to main switch statement>
> >
> > Obviously assumes KVM only advertises itself as one hypervisor, and so
> > the ordering is arbitrary.
>
> No, KVM can advertise as both KVM and Hyper-V. CPUID 0x40000000 is used
> for Hyper-V, while 0x40000100 is used for KVM. The MSRs do not conflict.
The MSRs *do* conflict. Kind of...
Xen uses MSR 0x40000000 (not to be conflated with CPUID leaf
0x40000000) for the "write hypercall page" request.
That conflicts with Hyper-V's HV_X64_MSR_GUEST_OS_ID.
So when the Hyper-V extensions are enabled, Xen moves its own MSR to
0x40000200 to avoid the conflict.
The problem is that KVM services the Hyper-V MSRs unconditionally in
the switch statement in kvm_set_msr_common(). So if the Xen MSR is set
to 0x40000000 and Hyper-V is *not* enabled, the Hyper-V support still
stops the Xen MSR from working.
Joao's patch fixes that.
A nicer alternative might be to disable the Hyper-V MSRs when they
shouldn't be there. Something like...
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2788,15 +2788,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
* the need to ignore the workaround.
*/
break;
- case HV_X64_MSR_GUEST_OS_ID ... HV_X64_MSR_SINT15:
- case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4:
- case HV_X64_MSR_CRASH_CTL:
- case HV_X64_MSR_STIMER0_CONFIG ... HV_X64_MSR_STIMER3_COUNT:
- case HV_X64_MSR_REENLIGHTENMENT_CONTROL:
- case HV_X64_MSR_TSC_EMULATION_CONTROL:
- case HV_X64_MSR_TSC_EMULATION_STATUS:
- return kvm_hv_set_msr_common(vcpu, msr, data,
- msr_info->host_initiated);
case MSR_IA32_BBL_CR_CTL3:
/* Drop writes to this legacy MSR -- see rdmsr
* counterpart for further detail.
@@ -2829,6 +2820,18 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
return 1;
vcpu->arch.msr_misc_features_enables = data;
break;
+ case HV_X64_MSR_GUEST_OS_ID ... HV_X64_MSR_SINT15:
+ case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4:
+ case HV_X64_MSR_CRASH_CTL:
+ case HV_X64_MSR_STIMER0_CONFIG ... HV_X64_MSR_STIMER3_COUNT:
+ case HV_X64_MSR_REENLIGHTENMENT_CONTROL:
+ case HV_X64_MSR_TSC_EMULATION_CONTROL:
+ case HV_X64_MSR_TSC_EMULATION_STATUS:
+ if (kvm_hyperv_enabled(vcpu->kvm)) {
+ return kvm_hv_set_msr_common(vcpu, msr, data,
+ msr_info->host_initiated);
+ }
+ /* fall through */
default:
if (msr && (msr == vcpu->kvm->arch.xen_hvm_config.msr))
return xen_hvm_config(vcpu, data);
... except that's a bit icky because that trick of falling through to
the default case only works for *one* case statement. And more to the
point, the closest thing I can find to a 'kvm_hyperv_enabled()' flag is
what we do for setting the HV_X64_MSR_HYPERCALL_ENABLE flag... which is
based on whether the hv_guest_os_id is set, which in turn is done by
writing one of these MSRs :)
I suppose we could disable them just by letting Xen take precedence, if
kvm->arch.xen_hvm_config.msr == HV_X64_MSR_GUEST_OS_ID. But that's
basically what Joao's patch already does. It doesn't disable the
*other* Hyper-V MSRs except for the one Xen 'conflicts' with, but I
don't think that matters.
The patch stands alone to correct the *existing* functionality of
KVM_XEN_HVM_CONFIG, regardless of the additional functionality being
proposed in the rest of the series that followed it.
Reviewed-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
