Re: [PATCH] kvm: svm: Only propagate next_rip when guest supports it

[Paolo Bonzini]

On 09/10/2015 11:51, Joerg Roedel wrote:
> From: Joerg Roedel 
> 
> Currently we always write the next_rip of the shadow vmcb to
> the guests vmcb when we emulate a vmexit. This could confuse
> the guest when its cpuid indicated no support for the
> next_rip feature.
> 
> Fix this by only propagating next_rip if the guest actually
> supports it.
> 
> Cc: Bandan Das 
> Cc: Dirk Mueller 
> Tested-by: Dirk Mueller 
> Signed-off-by: Joerg Roedel 
> ---
>  arch/x86/kvm/cpuid.h | 21 +
>  arch/x86/kvm/svm.c   |  4 +++-
>  2 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index dd05b9c..effca1f 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -133,4 +133,25 @@ static inline bool guest_cpuid_has_mpx(struct kvm_vcpu 
> *vcpu)
>   best = kvm_find_cpuid_entry(vcpu, 7, 0);
>   return best && (best->ebx & bit(X86_FEATURE_MPX));
>  }
> +
> +/*
> + * NRIPS is provided through cpuidfn 0x800a.edx bit 3
> + */
> +#define BIT_NRIPS3
> +
> +static inline bool guest_cpuid_has_nrips(struct kvm_vcpu *vcpu)
> +{
> + struct kvm_cpuid_entry2 *best;
> +
> + best = kvm_find_cpuid_entry(vcpu, 0x800a, 0);
> +
> + /*
> +  * NRIPS is a scattered cpuid feature, so we can't use
> +  * X86_FEATURE_NRIPS here (X86_FEATURE_NRIPS would be bit
> +  * position 8, not 3).
> +  */
> + return best && (best->edx & bit(BIT_NRIPS));
> +}
> +#undef BIT_NRIPS
> +
>  #endif
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 2f9ed1f..4084b33 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2365,7 +2365,9 @@ static int nested_svm_vmexit(struct vcpu_svm *svm)
>   nested_vmcb->control.exit_info_2   = vmcb->control.exit_info_2;
>   nested_vmcb->control.exit_int_info = vmcb->control.exit_int_info;
>   nested_vmcb->control.exit_int_info_err = 
> vmcb->control.exit_int_info_err;
> - nested_vmcb->control.next_rip  = vmcb->control.next_rip;
> +
> + if (guest_cpuid_has_nrips(>vcpu))

This could be a bit expensive to do on every vmexit.  Can you benchmark
it with kvm-unit-tests, or just cache the result in struct vcpu_svm?

Thanks,

Paolo

> + nested_vmcb->control.next_rip  = vmcb->control.next_rip;
>  
>   /*
>* If we emulate a VMRUN/#VMEXIT in the same host #vmexit cycle we have
> 
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kvm: svm: Only propagate next_rip when guest supports it

[kbuild test robot]

Hi Joerg,

[auto build test ERROR on v4.3-rc4 -- if it's inappropriate base, please ignore]

config: i386-randconfig-x009-201540 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All errors (new ones prefixed by >>):

   arch/x86/kvm/svm.c: In function 'nested_svm_vmexit':
>> arch/x86/kvm/svm.c:2369:28: error: 'vcpu' undeclared (first use in this 
>> function)
 if (guest_cpuid_has_nrips(vcpu))
   ^
   arch/x86/kvm/svm.c:2369:28: note: each undeclared identifier is reported 
only once for each function it appears in

vim +/vcpu +2369 arch/x86/kvm/svm.c

  2363  nested_vmcb->control.exit_code_hi  = 
vmcb->control.exit_code_hi;
  2364  nested_vmcb->control.exit_info_1   = 
vmcb->control.exit_info_1;
  2365  nested_vmcb->control.exit_info_2   = 
vmcb->control.exit_info_2;
  2366  nested_vmcb->control.exit_int_info = 
vmcb->control.exit_int_info;
  2367  nested_vmcb->control.exit_int_info_err = 
vmcb->control.exit_int_info_err;
  2368  
> 2369  if (guest_cpuid_has_nrips(vcpu))
  2370  nested_vmcb->control.next_rip  = vmcb->control.next_rip;
  2371  
  2372  /*

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH] kvm: svm: Only propagate next_rip when guest supports it

[Joerg Roedel]

On Wed, Oct 07, 2015 at 11:48:36AM -0400, Bandan Das wrote:
> Ok, understood now. The warn_on would trigger in L1 only if it has
> decided to disable nrips for some reason as was the case here. So,
> my reasoning behind putting the warning was incorrect.

Okay, so I think the warning can be removed.

> > +
> > +   if (guest_cpuid_has_nrips(vcpu))
> > +   nested_vmcb->control.next_rip  = vmcb->control.next_rip;

Note that there is a bug here, instead of vcpu it must be >vcpu.
Somehow I missed to at least compile-test this.

Dirk is currently testing whether this (fixed) patch solves the problem
in his setup.

> >  
> > /*
> >  * If we emulate a VMRUN/#VMEXIT in the same host #vmexit cycle we have
> > @@ -2714,6 +2716,9 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
> > svm->vmcb->control.event_inj = nested_vmcb->control.event_inj;
> > svm->vmcb->control.event_inj_err = nested_vmcb->control.event_inj_err;
> >  
> > +   /* Clear next_rip, as real hardware would do */
> > +   nested_vmcb->control.next_rip = 0;
> > +
> 
> Why do we need this ? And are you sure this is what real hardware does ?
> I couldn't find anything in the spec.

Yeah, probably right. Since we only write guests next_rip when the guest
supports it via cpuid, there is probably no point in resetting it at
vmrun emulation.


Joerg

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kvm: svm: Only propagate next_rip when guest supports it

[Bandan Das]

Joerg Roedel  writes:

> On Wed, Oct 07, 2015 at 01:03:35PM +0200, Joerg Roedel wrote:
>> But we don't care if L1 writes something into its own next_rip, as we
>> never read this value from its VMCB. We only copy the next_rip value we
>> get from our shadow-vmcb to it on an emulated vmexit. So I still don't
>> understand what triggers the reported problem or why the WARN_ON is
>> necessary.
>
> Okay, I think I have an idea now. I talked a bit with Dirk and the
> WARN_ON triggers in the guest, and not on the host. This makes a lot
> more sense.
>
> In nested-svm we always copy the next_rip from the shadow-vmcb to the
> guests vmcb, even when the nrips bit in cpuid is not set for the guest.
> This obviously triggers the WARN_ON() in the L1 KVM (I still don't
> understand why the WARN_ON was introduced in the first place).

Ok, understood now. The warn_on would trigger in L1 only if it has
decided to disable nrips for some reason as was the case here. So,
my reasoning behind putting the warning was incorrect. 

> So the right fix is to only copy next_rip to the guests vmcb when its
> cpuid indicates that next_rip is supported there, like in this patch:

Yep, agreed.

> From 019afc60507618b8e44e0c67d5ea2d850d88c9dd Mon Sep 17 00:00:00 2001
> From: Joerg Roedel 
> Date: Wed, 7 Oct 2015 13:38:19 +0200
> Subject: [PATCH] kvm: svm: Only propagate next_rip when guest supports it
>
> Currently we always write the next_rip of the shadow vmcb to
> the guests vmcb when we emulate a vmexit. This could confuse
> the guest when its cpuid indicated no support for the
> next_rip feature.
>
> Fix this by only propagating next_rip if the guest actually
> supports it.
>
> Signed-off-by: Joerg Roedel 
> ---
>  arch/x86/kvm/cpuid.h | 21 +
>  arch/x86/kvm/svm.c   |  7 ++-
>  2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index dd05b9c..effca1f 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -133,4 +133,25 @@ static inline bool guest_cpuid_has_mpx(struct kvm_vcpu 
> *vcpu)
>   best = kvm_find_cpuid_entry(vcpu, 7, 0);
>   return best && (best->ebx & bit(X86_FEATURE_MPX));
>  }
> +
> +/*
> + * NRIPS is provided through cpuidfn 0x800a.edx bit 3
> + */
> +#define BIT_NRIPS3
> +
> +static inline bool guest_cpuid_has_nrips(struct kvm_vcpu *vcpu)
> +{
> + struct kvm_cpuid_entry2 *best;
> +
> + best = kvm_find_cpuid_entry(vcpu, 0x800a, 0);
> +
> + /*
> +  * NRIPS is a scattered cpuid feature, so we can't use
> +  * X86_FEATURE_NRIPS here (X86_FEATURE_NRIPS would be bit
> +  * position 8, not 3).
> +  */
> + return best && (best->edx & bit(BIT_NRIPS));
> +}
> +#undef BIT_NRIPS
> +
>  #endif
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 94b7d15..e1a8824 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2459,7 +2459,9 @@ static int nested_svm_vmexit(struct vcpu_svm *svm)
>   nested_vmcb->control.exit_info_2   = vmcb->control.exit_info_2;
>   nested_vmcb->control.exit_int_info = vmcb->control.exit_int_info;
>   nested_vmcb->control.exit_int_info_err = 
> vmcb->control.exit_int_info_err;
> - nested_vmcb->control.next_rip  = vmcb->control.next_rip;
> +
> + if (guest_cpuid_has_nrips(vcpu))
> + nested_vmcb->control.next_rip  = vmcb->control.next_rip;
>  
>   /*
>* If we emulate a VMRUN/#VMEXIT in the same host #vmexit cycle we have
> @@ -2714,6 +2716,9 @@ static bool nested_svm_vmrun(struct vcpu_svm *svm)
>   svm->vmcb->control.event_inj = nested_vmcb->control.event_inj;
>   svm->vmcb->control.event_inj_err = nested_vmcb->control.event_inj_err;
>  
> + /* Clear next_rip, as real hardware would do */
> + nested_vmcb->control.next_rip = 0;
> +

Why do we need this ? And are you sure this is what real hardware does ?
I couldn't find anything in the spec.

>   nested_svm_unmap(page);
>  
>   /* Enter Guest-Mode */
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kvm: svm: Only propagate next_rip when guest supports it

[Dirk Müller]

On 07.10.2015 18:14, Joerg Roedel wrote:


Dirk is currently testing whether this (fixed) patch solves the problem
in his setup.


Tested-By: Dirk Mueller 

Works fine here. Thanks!


Greetings,
Dirk
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html