Re: [PATCH RFC 01/39] KVM: x86: fix Xen hypercall page msr handling

[David Woodhouse]

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