| Message ID | 20200115171014.56405-4-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/kvm/hyper-v: fix enlightened VMCS & QEMU4.2 | expand |
> On 15 Jan 2020, at 19:10, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > Sane L1 hypervisors are not supposed to turn any of the unsupported VMX > controls on for its guests and nested_vmx_check_controls() checks for > that. This is, however, not the case for the controls which are supported > on the host but are missing in enlightened VMCS and when eVMCS is in use. > > It would certainly be possible to add these missing checks to > nested_check_vm_execution_controls()/_vm_exit_controls()/.. but it seems > preferable to keep eVMCS-specific stuff in eVMCS and reduce the impact on > non-eVMCS guests by doing less unrelated checks. Create a separate > nested_evmcs_check_controls() for this purpose. > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > arch/x86/kvm/vmx/evmcs.c | 56 ++++++++++++++++++++++++++++++++++++++- > arch/x86/kvm/vmx/evmcs.h | 1 + > arch/x86/kvm/vmx/nested.c | 3 +++ > 3 files changed, 59 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c > index b5d6582ba589..88f462866396 100644 > --- a/arch/x86/kvm/vmx/evmcs.c > +++ b/arch/x86/kvm/vmx/evmcs.c > @@ -4,9 +4,11 @@ > #include <linux/smp.h> > > #include "../hyperv.h" > -#include "evmcs.h" > #include "vmcs.h" > +#include "vmcs12.h" > +#include "evmcs.h" > #include "vmx.h" > +#include "trace.h" > > DEFINE_STATIC_KEY_FALSE(enable_evmcs); > > @@ -378,6 +380,58 @@ void nested_evmcs_filter_control_msr(u32 msr_index, u64 *pdata) > *pdata = ctl_low | ((u64)ctl_high << 32); > } > > +int nested_evmcs_check_controls(struct vmcs12 *vmcs12) > +{ > + int ret = 0; > + u32 unsupp_ctl; > + > + unsupp_ctl = vmcs12->pin_based_vm_exec_control & > + EVMCS1_UNSUPPORTED_PINCTRL; > + if (unsupp_ctl) { > + trace_kvm_nested_vmenter_failed( > + "eVMCS: unsupported pin-based VM-execution controls", > + unsupp_ctl); Why not move "CC” macro from nested.c to nested.h and use it here as-well instead of replicating it’s logic? -Liran
Liran Alon <liran.alon@oracle.com> writes: >> On 15 Jan 2020, at 19:10, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >> >> Sane L1 hypervisors are not supposed to turn any of the unsupported VMX >> controls on for its guests and nested_vmx_check_controls() checks for >> that. This is, however, not the case for the controls which are supported >> on the host but are missing in enlightened VMCS and when eVMCS is in use. >> >> It would certainly be possible to add these missing checks to >> nested_check_vm_execution_controls()/_vm_exit_controls()/.. but it seems >> preferable to keep eVMCS-specific stuff in eVMCS and reduce the impact on >> non-eVMCS guests by doing less unrelated checks. Create a separate >> nested_evmcs_check_controls() for this purpose. >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> >> --- >> arch/x86/kvm/vmx/evmcs.c | 56 ++++++++++++++++++++++++++++++++++++++- >> arch/x86/kvm/vmx/evmcs.h | 1 + >> arch/x86/kvm/vmx/nested.c | 3 +++ >> 3 files changed, 59 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c >> index b5d6582ba589..88f462866396 100644 >> --- a/arch/x86/kvm/vmx/evmcs.c >> +++ b/arch/x86/kvm/vmx/evmcs.c >> @@ -4,9 +4,11 @@ >> #include <linux/smp.h> >> >> #include "../hyperv.h" >> -#include "evmcs.h" >> #include "vmcs.h" >> +#include "vmcs12.h" >> +#include "evmcs.h" >> #include "vmx.h" >> +#include "trace.h" >> >> DEFINE_STATIC_KEY_FALSE(enable_evmcs); >> >> @@ -378,6 +380,58 @@ void nested_evmcs_filter_control_msr(u32 msr_index, u64 *pdata) >> *pdata = ctl_low | ((u64)ctl_high << 32); >> } >> >> +int nested_evmcs_check_controls(struct vmcs12 *vmcs12) >> +{ >> + int ret = 0; >> + u32 unsupp_ctl; >> + >> + unsupp_ctl = vmcs12->pin_based_vm_exec_control & >> + EVMCS1_UNSUPPORTED_PINCTRL; >> + if (unsupp_ctl) { >> + trace_kvm_nested_vmenter_failed( >> + "eVMCS: unsupported pin-based VM-execution controls", >> + unsupp_ctl); > > Why not move "CC” macro from nested.c to nested.h and use it here as-well instead of replicating it’s logic? > Because error messages I add are both human readable and conform to SDM! :-) On a more serious not yes, we should probably do that. We may even want to use it in non-nesting (and non VMX) code in KVM. Thanks,
On Thu, Jan 16, 2020 at 09:55:57AM +0100, Vitaly Kuznetsov wrote: > Liran Alon <liran.alon@oracle.com> writes: > > >> On 15 Jan 2020, at 19:10, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > >> > >> Sane L1 hypervisors are not supposed to turn any of the unsupported VMX > >> controls on for its guests and nested_vmx_check_controls() checks for > >> that. This is, however, not the case for the controls which are supported > >> on the host but are missing in enlightened VMCS and when eVMCS is in use. > >> > >> It would certainly be possible to add these missing checks to > >> nested_check_vm_execution_controls()/_vm_exit_controls()/.. but it seems > >> preferable to keep eVMCS-specific stuff in eVMCS and reduce the impact on > >> non-eVMCS guests by doing less unrelated checks. Create a separate > >> nested_evmcs_check_controls() for this purpose. > >> > >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > >> --- > >> arch/x86/kvm/vmx/evmcs.c | 56 ++++++++++++++++++++++++++++++++++++++- > >> arch/x86/kvm/vmx/evmcs.h | 1 + > >> arch/x86/kvm/vmx/nested.c | 3 +++ > >> 3 files changed, 59 insertions(+), 1 deletion(-) > >> > >> diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c > >> index b5d6582ba589..88f462866396 100644 > >> --- a/arch/x86/kvm/vmx/evmcs.c > >> +++ b/arch/x86/kvm/vmx/evmcs.c > >> @@ -4,9 +4,11 @@ > >> #include <linux/smp.h> > >> > >> #include "../hyperv.h" > >> -#include "evmcs.h" > >> #include "vmcs.h" > >> +#include "vmcs12.h" > >> +#include "evmcs.h" > >> #include "vmx.h" > >> +#include "trace.h" > >> > >> DEFINE_STATIC_KEY_FALSE(enable_evmcs); > >> > >> @@ -378,6 +380,58 @@ void nested_evmcs_filter_control_msr(u32 msr_index, u64 *pdata) > >> *pdata = ctl_low | ((u64)ctl_high << 32); > >> } > >> > >> +int nested_evmcs_check_controls(struct vmcs12 *vmcs12) > >> +{ > >> + int ret = 0; > >> + u32 unsupp_ctl; > >> + > >> + unsupp_ctl = vmcs12->pin_based_vm_exec_control & > >> + EVMCS1_UNSUPPORTED_PINCTRL; > >> + if (unsupp_ctl) { > >> + trace_kvm_nested_vmenter_failed( > >> + "eVMCS: unsupported pin-based VM-execution controls", > >> + unsupp_ctl); > > > > Why not move "CC” macro from nested.c to nested.h and use it here as-well instead of replicating it’s logic? > > > > Because error messages I add are both human readable and conform to SDM! > :-) > > On a more serious not yes, we should probably do that. We may even want > to use it in non-nesting (and non VMX) code in KVM. No, the CC() macro is short for Consistency Check, i.e. specific to nVMX. Even if KVM ends up adding nested_evmcs_check_controls(), which I'm hoping can be avoided, I'd still be hesitant to expose CC() in nested.h.
On 16/01/20 17:21, Sean Christopherson wrote: > On Thu, Jan 16, 2020 at 09:55:57AM +0100, Vitaly Kuznetsov wrote: >> Liran Alon <liran.alon@oracle.com> writes: >> >>>> On 15 Jan 2020, at 19:10, Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >>>> >>>> Sane L1 hypervisors are not supposed to turn any of the unsupported VMX >>>> controls on for its guests and nested_vmx_check_controls() checks for >>>> that. This is, however, not the case for the controls which are supported >>>> on the host but are missing in enlightened VMCS and when eVMCS is in use. >>>> >>>> It would certainly be possible to add these missing checks to >>>> nested_check_vm_execution_controls()/_vm_exit_controls()/.. but it seems >>>> preferable to keep eVMCS-specific stuff in eVMCS and reduce the impact on >>>> non-eVMCS guests by doing less unrelated checks. Create a separate >>>> nested_evmcs_check_controls() for this purpose. >>>> >>>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> >>>> --- >>>> arch/x86/kvm/vmx/evmcs.c | 56 ++++++++++++++++++++++++++++++++++++++- >>>> arch/x86/kvm/vmx/evmcs.h | 1 + >>>> arch/x86/kvm/vmx/nested.c | 3 +++ >>>> 3 files changed, 59 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c >>>> index b5d6582ba589..88f462866396 100644 >>>> --- a/arch/x86/kvm/vmx/evmcs.c >>>> +++ b/arch/x86/kvm/vmx/evmcs.c >>>> @@ -4,9 +4,11 @@ >>>> #include <linux/smp.h> >>>> >>>> #include "../hyperv.h" >>>> -#include "evmcs.h" >>>> #include "vmcs.h" >>>> +#include "vmcs12.h" >>>> +#include "evmcs.h" >>>> #include "vmx.h" >>>> +#include "trace.h" >>>> >>>> DEFINE_STATIC_KEY_FALSE(enable_evmcs); >>>> >>>> @@ -378,6 +380,58 @@ void nested_evmcs_filter_control_msr(u32 msr_index, u64 *pdata) >>>> *pdata = ctl_low | ((u64)ctl_high << 32); >>>> } >>>> >>>> +int nested_evmcs_check_controls(struct vmcs12 *vmcs12) >>>> +{ >>>> + int ret = 0; >>>> + u32 unsupp_ctl; >>>> + >>>> + unsupp_ctl = vmcs12->pin_based_vm_exec_control & >>>> + EVMCS1_UNSUPPORTED_PINCTRL; >>>> + if (unsupp_ctl) { >>>> + trace_kvm_nested_vmenter_failed( >>>> + "eVMCS: unsupported pin-based VM-execution controls", >>>> + unsupp_ctl); >>> >>> Why not move "CC” macro from nested.c to nested.h and use it here as-well instead of replicating it’s logic? >>> >> >> Because error messages I add are both human readable and conform to SDM! >> :-) >> >> On a more serious not yes, we should probably do that. We may even want >> to use it in non-nesting (and non VMX) code in KVM. > > No, the CC() macro is short for Consistency Check, i.e. specific to nVMX. > Even if KVM ends up adding nested_evmcs_check_controls(), which I'm hoping > can be avoided, I'd still be hesitant to expose CC() in nested.h. > For now let's keep Vitaly's patch as is. It's definitely a good one as it would catch Hyper-V's issue immediately even without patch 2 (which is the only really contentious change). Paolo
diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c index b5d6582ba589..88f462866396 100644 --- a/arch/x86/kvm/vmx/evmcs.c +++ b/arch/x86/kvm/vmx/evmcs.c @@ -4,9 +4,11 @@ #include <linux/smp.h> #include "../hyperv.h" -#include "evmcs.h" #include "vmcs.h" +#include "vmcs12.h" +#include "evmcs.h" #include "vmx.h" +#include "trace.h" DEFINE_STATIC_KEY_FALSE(enable_evmcs); @@ -378,6 +380,58 @@ void nested_evmcs_filter_control_msr(u32 msr_index, u64 *pdata) *pdata = ctl_low | ((u64)ctl_high << 32); } +int nested_evmcs_check_controls(struct vmcs12 *vmcs12) +{ + int ret = 0; + u32 unsupp_ctl; + + unsupp_ctl = vmcs12->pin_based_vm_exec_control & + EVMCS1_UNSUPPORTED_PINCTRL; + if (unsupp_ctl) { + trace_kvm_nested_vmenter_failed( + "eVMCS: unsupported pin-based VM-execution controls", + unsupp_ctl); + ret = -EINVAL; + } + + unsupp_ctl = vmcs12->secondary_vm_exec_control & + EVMCS1_UNSUPPORTED_2NDEXEC; + if (unsupp_ctl) { + trace_kvm_nested_vmenter_failed( + "eVMCS: unsupported secondary VM-execution controls", + unsupp_ctl); + ret = -EINVAL; + } + + unsupp_ctl = vmcs12->vm_exit_controls & + EVMCS1_UNSUPPORTED_VMEXIT_CTRL; + if (unsupp_ctl) { + trace_kvm_nested_vmenter_failed( + "eVMCS: unsupported VM-exit controls", + unsupp_ctl); + ret = -EINVAL; + } + + unsupp_ctl = vmcs12->vm_entry_controls & + EVMCS1_UNSUPPORTED_VMENTRY_CTRL; + if (unsupp_ctl) { + trace_kvm_nested_vmenter_failed( + "eVMCS: unsupported VM-entry controls", + unsupp_ctl); + ret = -EINVAL; + } + + unsupp_ctl = vmcs12->vm_function_control & EVMCS1_UNSUPPORTED_VMFUNC; + if (unsupp_ctl) { + trace_kvm_nested_vmenter_failed( + "eVMCS: unsupported VM-function controls", + unsupp_ctl); + ret = -EINVAL; + } + + return ret; +} + int nested_enable_evmcs(struct kvm_vcpu *vcpu, uint16_t *vmcs_version) { diff --git a/arch/x86/kvm/vmx/evmcs.h b/arch/x86/kvm/vmx/evmcs.h index b88d9807a796..cb7517a5a41c 100644 --- a/arch/x86/kvm/vmx/evmcs.h +++ b/arch/x86/kvm/vmx/evmcs.h @@ -202,5 +202,6 @@ uint16_t nested_get_evmcs_version(struct kvm_vcpu *vcpu); int nested_enable_evmcs(struct kvm_vcpu *vcpu, uint16_t *vmcs_version); void nested_evmcs_filter_control_msr(u32 msr_index, u64 *pdata); +int nested_evmcs_check_controls(struct vmcs12 *vmcs12); #endif /* __KVM_X86_VMX_EVMCS_H */ diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 4aea7d304beb..7c720b095663 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -2767,6 +2767,9 @@ static int nested_vmx_check_controls(struct kvm_vcpu *vcpu, nested_check_vm_entry_controls(vcpu, vmcs12)) return -EINVAL; + if (to_vmx(vcpu)->nested.enlightened_vmcs_enabled) + return nested_evmcs_check_controls(vmcs12); + return 0; }
Sane L1 hypervisors are not supposed to turn any of the unsupported VMX controls on for its guests and nested_vmx_check_controls() checks for that. This is, however, not the case for the controls which are supported on the host but are missing in enlightened VMCS and when eVMCS is in use. It would certainly be possible to add these missing checks to nested_check_vm_execution_controls()/_vm_exit_controls()/.. but it seems preferable to keep eVMCS-specific stuff in eVMCS and reduce the impact on non-eVMCS guests by doing less unrelated checks. Create a separate nested_evmcs_check_controls() for this purpose. Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- arch/x86/kvm/vmx/evmcs.c | 56 ++++++++++++++++++++++++++++++++++++++- arch/x86/kvm/vmx/evmcs.h | 1 + arch/x86/kvm/vmx/nested.c | 3 +++ 3 files changed, 59 insertions(+), 1 deletion(-)