| Message ID | 20201214083905.2017260-3-dwmw2@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Add minimal support for Xen HVM guests | expand |
David Woodhouse <dwmw2@infradead.org> writes: > From: Joao Martins <joao.m.martins@oracle.com> > > Xen usually places its MSR at 0x40000000 or 0x40000200 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 Hyper-V's MSR > (HV_X64_MSR_GUEST_OS_ID) which unconditionally 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. > In case we're making MSR 0x40000000 something different from HV_X64_MSR_GUEST_OS_ID we can and probably should disable Hyper-V emulation in KVM completely -- or how else is it going to work? > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > --- > 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 c7f1ba21212e..13ba4a64f748 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -3001,6 +3001,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); > + Can we generalize this maybe? E.g. before handling KVM and architectural MSRs we check that the particular MSR is not overriden by an emulated hypervisor, e.g. if (kvm_emulating_hyperv(kvm) && kvm_hyperv_msr_overriden(kvm,msr) return kvm_hyperv_handle_msr(kvm, msr); if (kvm_emulating_xen(kvm) && kvm_xen_msr_overriden(kvm,msr) return kvm_xen_handle_msr(kvm, msr); switch (msr) { ... > switch (msr) { > case MSR_AMD64_NB_CFG: > case MSR_IA32_UCODE_WRITE: > @@ -3288,8 +3291,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); > return KVM_MSR_RET_INVALID;
On 14 December 2020 21:27:19 GMT, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >David Woodhouse <dwmw2@infradead.org> writes: > >> From: Joao Martins <joao.m.martins@oracle.com> >> >> Xen usually places its MSR at 0x40000000 or 0x40000200 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 Hyper-V's MSR >> (HV_X64_MSR_GUEST_OS_ID) which unconditionally 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. >> > >In case we're making MSR 0x40000000 something different from >HV_X64_MSR_GUEST_OS_ID we can and probably should disable Hyper-V >emulation in KVM completely -- or how else is it going to work? The way Xen itself does this — and the way we have to do it if we want to faithfully emulate Xen and support live migration from it — is to shift the Xen MSRs up to (from memory) 0x40000200 if Hyper-V is enabled. I did look at disabling Hyper-V entirely when it isn't enabled, but the only flag we have for it being enabled is the guest OS ID being set... which is done through that MSR :) My minimal version ended up being so close to Joao's original that it was not longer worth the bikeshedding and so I gave up on it and stuck with the original. >> @@ -3001,6 +3001,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); >> + > >Can we generalize this maybe? E.g. before handling KVM and >architectural >MSRs we check that the particular MSR is not overriden by an emulated >hypervisor, > >e.g. > if (kvm_emulating_hyperv(kvm) && kvm_hyperv_msr_overriden(kvm,msr) > return kvm_hyperv_handle_msr(kvm, msr); > if (kvm_emulating_xen(kvm) && kvm_xen_msr_overriden(kvm,msr) > return kvm_xen_handle_msr(kvm, msr); That smells a bit like overengineering. As I said, I did have a play with "improving" Joao's original patch but nothing I tried actually made more sense to me than this once the details were ironed out.
David Woodhouse <dwmw2@infradead.org> writes: >>> @@ -3001,6 +3001,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); >>> + >> >>Can we generalize this maybe? E.g. before handling KVM and >>architectural >>MSRs we check that the particular MSR is not overriden by an emulated >>hypervisor, >> >>e.g. >> if (kvm_emulating_hyperv(kvm) && kvm_hyperv_msr_overriden(kvm,msr) >> return kvm_hyperv_handle_msr(kvm, msr); >> if (kvm_emulating_xen(kvm) && kvm_xen_msr_overriden(kvm,msr) >> return kvm_xen_handle_msr(kvm, msr); > > That smells a bit like overengineering. As I said, I did have a play > with "improving" Joao's original patch but nothing I tried actually > made more sense to me than this once the details were ironed out. This actually looks more or less like hypercall distinction from after PATCH3: if (kvm_xen_hypercall_enabled(vcpu->kvm)) return kvm_xen_hypercall(vcpu); if (kvm_hv_hypercall_enabled(vcpu->kvm)) return kvm_hv_hypercall(vcpu); .... so my idea was why not do the same for MSRs?
On 14 December 2020 21:44:47 GMT, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >This actually looks more or less like hypercall distinction from after >PATCH3: > > if (kvm_xen_hypercall_enabled(vcpu->kvm)) > return kvm_xen_hypercall(vcpu); > > if (kvm_hv_hypercall_enabled(vcpu->kvm)) > return kvm_hv_hypercall(vcpu); > >.... > >so my idea was why not do the same for MSRs? Can you define kvm_hv_msr_enabled()? Note kvm_hv_hypercall_enabled() is based on a value that gets written through the MSR, so it can't be that.
David Woodhouse <dwmw2@infradead.org> writes: > On 14 December 2020 21:44:47 GMT, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >>This actually looks more or less like hypercall distinction from after >>PATCH3: >> >> if (kvm_xen_hypercall_enabled(vcpu->kvm)) >> return kvm_xen_hypercall(vcpu); >> >> if (kvm_hv_hypercall_enabled(vcpu->kvm)) >> return kvm_hv_hypercall(vcpu); >> >>.... >> >>so my idea was why not do the same for MSRs? > > Can you define kvm_hv_msr_enabled()? > > Note kvm_hv_hypercall_enabled() is based on a value that gets written > through the MSR, so it can't be that. When Hyper-V emulation appeared in KVM we (unfortunately) didn't add a capability to globaly enable and disable it so to be backwards compatible we'll have to define kvm_emulating_hyperv() as 'true' for now as that's how KVM behaves. This, however, doesn't mean we can't add e.g. a module parameter to disable Hyper-V emulation. Also, we can probably check guest CPUIDs and if Hyper-V's signature wasn't set we can return 'false'. <rant> Having Hyper-V emulation in KVM 'always enabled' may not be a big deal from functional point of view but may not be ideal from security standpoint as bugs in arch/x86/kvm/hyperv.c become exploitable even from Linux guests. </rant>
On Mon, 2020-12-14 at 23:22 +0100, Vitaly Kuznetsov wrote: > > Can you define kvm_hv_msr_enabled()? > > > > Note kvm_hv_hypercall_enabled() is based on a value that gets written > > through the MSR, so it can't be that. > > When Hyper-V emulation appeared in KVM we (unfortunately) didn't add a > capability to globaly enable and disable it so to be backwards > compatible we'll have to define kvm_emulating_hyperv() as 'true' for > now as that's how KVM behaves. This, however, doesn't mean we can't add > e.g. a module parameter to disable Hyper-V emulation. Also, we can > probably check guest CPUIDs and if Hyper-V's signature wasn't set we can > return 'false'. > > <rant> > Having Hyper-V emulation in KVM 'always enabled' may not be a big deal > from functional point of view but may not be ideal from security > standpoint as bugs in arch/x86/kvm/hyperv.c become exploitable even from > Linux guests. > </rant> Indeed. And yet it can coexist with Xen support too, so it isn't even as simple as turning it off when Xen is enabled. Which is why I ended up just using Joao's patch unchanged. Short of going back in time to make Hyper-V support conditional when it was first introduced, I couldn't see a better answer. And regardless of the Hyper-V mess, what this patch does for Xen is precisely what you suggest: handle it first, before the switch(), *if* the Xen MSR is enabled.
David Woodhouse <dwmw2@infradead.org> writes: > On Mon, 2020-12-14 at 23:22 +0100, Vitaly Kuznetsov wrote: >> > Can you define kvm_hv_msr_enabled()? >> > >> > Note kvm_hv_hypercall_enabled() is based on a value that gets written >> > through the MSR, so it can't be that. >> >> When Hyper-V emulation appeared in KVM we (unfortunately) didn't add a >> capability to globaly enable and disable it so to be backwards >> compatible we'll have to define kvm_emulating_hyperv() as 'true' for >> now as that's how KVM behaves. This, however, doesn't mean we can't add >> e.g. a module parameter to disable Hyper-V emulation. Also, we can >> probably check guest CPUIDs and if Hyper-V's signature wasn't set we can >> return 'false'. >> >> <rant> >> Having Hyper-V emulation in KVM 'always enabled' may not be a big deal >> from functional point of view but may not be ideal from security >> standpoint as bugs in arch/x86/kvm/hyperv.c become exploitable even from >> Linux guests. >> </rant> > > Indeed. And yet it can coexist with Xen support too, so it isn't even > as simple as turning it off when Xen is enabled. > > Which is why I ended up just using Joao's patch unchanged. Short of > going back in time to make Hyper-V support conditional when it was > first introduced, I couldn't see a better answer. > > And regardless of the Hyper-V mess, what this patch does for Xen is > precisely what you suggest: handle it first, before the switch(), *if* > the Xen MSR is enabled. Functionally I have no complaints, even with the suggested 'generalization' we'll be handling MSRs in the exact same sequence. You are, however, right calling Hyper-V mess 'mess' and if we want to make things cleaner we should probably start there (goes to my to-do list...).
> + if (msr && (msr == vcpu->kvm->arch.xen_hvm_config.msr)) > + return xen_hvm_config(vcpu, data); Nit: no need for the inner braces.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index c7f1ba21212e..13ba4a64f748 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3001,6 +3001,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: @@ -3288,8 +3291,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); return KVM_MSR_RET_INVALID;