| Message ID | 20190220201609.28290-2-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/KVM: Xen HVM guest support | expand |
On Wed, Feb 20, 2019 at 08:15:31PM +0000, Joao Martins wrote: > Xen usually places its MSR at 0x4000000 or 0x4000200 depending on > whether it is running in viridian mode or not. Note that this is not > ABI guaranteed, so it is possible for Xen to advertise the MSR some > place else. > > Given the way xen_hvm_config() is handled, if the former address is > selected, this will conflict with HyperV's MSR > (HV_X64_MSR_GUEST_OS_ID) which uses the same address. Unconditionally servicing Hyper-V and KVM MSRs seems wrong, i.e. KVM should only expose MSRs specific to a hypervisor if userspace has configured CPUID to advertise support for said hypervisor. If we do the same thing for Xen, then the common MSR code looks like: 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. > Given that the MSR location is arbitrary, move the xen_hvm_config() > handling to the top of kvm_set_msr_common() before falling through. > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > --- > arch/x86/kvm/x86.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 65e4559eef2f..47360a4b0d42 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -2429,6 +2429,9 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > u32 msr = msr_info->index; > u64 data = msr_info->data; > > + if (msr && (msr == vcpu->kvm->arch.xen_hvm_config.msr)) > + return xen_hvm_config(vcpu, data); > + > switch (msr) { > case MSR_AMD64_NB_CFG: > case MSR_IA32_UCODE_WRITE: > @@ -2644,8 +2647,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > vcpu->arch.msr_misc_features_enables = data; > break; > default: > - if (msr && (msr == vcpu->kvm->arch.xen_hvm_config.msr)) > - return xen_hvm_config(vcpu, data); > if (kvm_pmu_is_valid_msr(vcpu, msr)) > return kvm_pmu_set_msr(vcpu, msr_info); > if (!ignore_msrs) { > -- > 2.11.0 >
On 2/22/19 1:30 AM, Sean Christopherson wrote: > On Wed, Feb 20, 2019 at 08:15:31PM +0000, Joao Martins wrote: >> Xen usually places its MSR at 0x4000000 or 0x4000200 depending on >> whether it is running in viridian mode or not. Note that this is not >> ABI guaranteed, so it is possible for Xen to advertise the MSR some >> place else. >> >> Given the way xen_hvm_config() is handled, if the former address is >> selected, this will conflict with HyperV's MSR >> (HV_X64_MSR_GUEST_OS_ID) which uses the same address. > > Unconditionally servicing Hyper-V and KVM MSRs seems wrong, i.e. KVM > should only expose MSRs specific to a hypervisor if userspace has > configured CPUID to advertise support for said hypervisor. > Yeah, that makes sense. > If we do the > same thing for Xen, then the common MSR code looks like: > > 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. > One thing to consider on the above is that there might be multiple hypervisors in advertised in the hypervisor leaf. Which is used when you want to run a KVM guest but with Hyper-V drivers, or an Hyper-V guest with KVM extensions or VirtIO. The else part would probably need to be removed as IIUC when multiple hypervisors are advertised (e.g. KVM in leaf 0x40000000, Hyper-V in leaf 0x40000100) you are still allowed to use all of its features.
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. Paolo
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@amazon.co.uk> Cc: stable@vger.kernel.org
On 30/11/20 11:39, David Woodhouse wrote: > ... 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 You can use CPUID too (search for Hv#1 in leaf 0x40000000)? Paolo > 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.
On Mon, 2020-11-30 at 12:03 +0100, Paolo Bonzini wrote:
> You can use CPUID too (search for Hv#1 in leaf 0x40000000)?
That's leaf 0x40000001. Which is also the leaf used for Xen to indicate
the Xen version. So as long as we don't pretend to be Xen version
12759.30280 I suppose that's OK.
Or we could just check leaf 0x40000000 for 'Microsoft Hv'? Or both.
How about...
#define HYPERV_CPUID_INTERFACE_MAGIC 0x31237648 /* 'Hv#1' */
static inline bool guest_cpu_has_hyperv(struct kvm_vcpu *vcpu)
{
struct kvm_hv *hv = &vcpu->kvm->arch.hyperv;
const struct kvm_cpuid_entry2 *entry;
/*
* The call to kvm_find_cpuid_entry() is a bit much to put on
* the fast path of some of the Hyper-V MSRs, so bypass it if
* the guest OS ID has already been set.
*/
if (hv->hv_guest_os_id)
return true;
entry = kvm_find_cpuid_entry(vcpu, HYPERV_CPUID_INTERFACE, 0x0);
return entry && entry->eax == HYPERV_CPUID_INTERFACE_MAGIC;
}
I wonder if this is overengineering when 'let Xen take precedence'
works well enough?
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 65e4559eef2f..47360a4b0d42 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -2429,6 +2429,9 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) u32 msr = msr_info->index; u64 data = msr_info->data; + if (msr && (msr == vcpu->kvm->arch.xen_hvm_config.msr)) + return xen_hvm_config(vcpu, data); + switch (msr) { case MSR_AMD64_NB_CFG: case MSR_IA32_UCODE_WRITE: @@ -2644,8 +2647,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) vcpu->arch.msr_misc_features_enables = data; break; default: - if (msr && (msr == vcpu->kvm->arch.xen_hvm_config.msr)) - return xen_hvm_config(vcpu, data); if (kvm_pmu_is_valid_msr(vcpu, msr)) return kvm_pmu_set_msr(vcpu, msr_info); if (!ignore_msrs) {
Xen usually places its MSR at 0x4000000 or 0x4000200 depending on whether it is running in viridian mode or not. Note that this is not ABI guaranteed, so it is possible for Xen to advertise the MSR some place else. Given the way xen_hvm_config() is handled, if the former address is selected, this will conflict with HyperV's MSR (HV_X64_MSR_GUEST_OS_ID) which uses the same address. Given that the MSR location is arbitrary, move the xen_hvm_config() handling to the top of kvm_set_msr_common() before falling through. Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- arch/x86/kvm/x86.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)