On 03/07/2012 05:58 PM, Nadav Har'El wrote:
> The existing code emulates the guest's use of the IA32_FEATURE_CONTROL MSR
> in a way that was enough to run nested VMX guests, but did not fully
> conform to the VMX specification, and in particular did not allow a guest
> BIOS to prevent the guest OS from using VMX by setting the lock bit on this
> MSR.
>
> This patch emulates this MSR better, allowing the guest to lock it, and
> verifying its setting on VMXON. Also make sure that this MSR (and of course,
> VMXON state) is reset on guest vcpu reset (via SIPI).
>
> Signed-off-by: Nadav Har'El <nyh@xxxxxxxxxx>
> Reported-by: Julian Stecklina <js@xxxxxxxxx>
> ---
> arch/x86/kvm/vmx.c | 24 +++++++++++++++++++++---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> --- .before/arch/x86/kvm/vmx.c 2012-03-07 17:52:02.000000000 +0200
> +++ .after/arch/x86/kvm/vmx.c 2012-03-07 17:52:02.000000000 +0200
> @@ -352,6 +352,7 @@ struct nested_vmx {
> * we must keep them pinned while L2 runs.
> */
> struct page *apic_access_page;
> + u64 msr_ia32_feature_control;
> };
Need to add to the list of exported MSRs so it can be live migrated
(msrs_to_save). The variable itself should live in vcpu->arch, even if
some bits are vendor specific.
>
> struct vcpu_vmx {
> @@ -1999,7 +2000,7 @@ static int vmx_get_vmx_msr(struct kvm_vc
>
> switch (msr_index) {
> case MSR_IA32_FEATURE_CONTROL:
> - *pdata = 0;
> + *pdata = to_vmx(vcpu)->nested.msr_ia32_feature_control;
> break;
In a separate patch, please move this outside vmx_get_vmx_msr(). It's
not a vmx msr.
> case MSR_IA32_VMX_BASIC:
> /*
> @@ -2077,9 +2078,13 @@ static int vmx_set_vmx_msr(struct kvm_vc
> if (!nested_vmx_allowed(vcpu))
> return 0;
>
> - if (msr_index == MSR_IA32_FEATURE_CONTROL)
> - /* TODO: the right thing. */
> + if (msr_index == MSR_IA32_FEATURE_CONTROL) {
> + if (to_vmx(vcpu)->nested.msr_ia32_feature_control
> + & FEATURE_CONTROL_LOCKED)
> + return 0;
> + to_vmx(vcpu)->nested.msr_ia32_feature_control = data;
> return 1;
> + }
> /*
> * No need to treat VMX capability MSRs specially: If we don't handle
> * them, handle_wrmsr will #GP(0), which is correct (they are readonly)
> @@ -3807,6 +3812,8 @@ static int vmx_vcpu_setup(struct vcpu_vm
> return 0;
> }
>
> +static void free_nested(struct vcpu_vmx *vmx);
> +
> static int vmx_vcpu_reset(struct kvm_vcpu *vcpu)
> {
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> @@ -3920,6 +3927,9 @@ static int vmx_vcpu_reset(struct kvm_vcp
> /* HACK: Don't enable emulation on guest boot/reset */
> vmx->emulation_required = 0;
>
> + /* Reset nested-VMX settings: */
> + vmx->nested.msr_ia32_feature_control = 0;
> + free_nested(vmx);
> out:
> return ret;
> }
> @@ -5031,6 +5041,14 @@ static int handle_vmon(struct kvm_vcpu *
> return 1;
> }
>
> +#define VMXON_NEEDED_FEATURES \
> + (FEATURE_CONTROL_LOCKED | FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX)
Use const u64 instead of #define please, it jars my eyes.
> + if ((vmx->nested.msr_ia32_feature_control & VMXON_NEEDED_FEATURES)
> + != VMXON_NEEDED_FEATURES) {
> + kvm_inject_gp(vcpu, 0);
> + return 1;
> + }
> +
> INIT_LIST_HEAD(&(vmx->nested.vmcs02_pool));
> vmx->nested.vmcs02_num = 0;
>
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
