[PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Dirk Müller]

[-- Attachment #1: Type: text/plain, Size: 294 bytes --]

The cpu feature flags are not ever going to change, so warning
everytime can cause a lot of kernel log spam
(in our case more than 10GB/hour).

Signed-off-by: Dirk Mueller <dmueller@suse.com>

-- 
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, 
HRB 21284 (AG Nürnberg)

[-- Attachment #2: 0001-Use-WARN_ON_ONCE-for-missing-X86_FEATURE_NRIPS.patch --]
[-- Type: text/x-patch, Size: 943 bytes --]

>From 40c7213e34a1a1caf0b98db832c06c593f888b11 Mon Sep 17 00:00:00 2001
From: Dirk Mueller <dirk@dmllr.de>
Date: Thu, 1 Oct 2015 13:10:10 +0200
Subject: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

The cpu feature flags are not ever going to change, so warning
everytime can cause a lot of kernel log spam
(in our case more than 10GB/hour).

Signed-off-by: Dirk Mueller <dmueller@suse.com>
---
 arch/x86/kvm/svm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 94b7d15..0a42859 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -514,7 +514,7 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 	struct vcpu_svm *svm = to_svm(vcpu);
 
 	if (svm->vmcb->control.next_rip != 0) {
-		WARN_ON(!static_cpu_has(X86_FEATURE_NRIPS));
+		WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS));
 		svm->next_rip = svm->vmcb->control.next_rip;
 	}
 
-- 
2.5.1

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Paolo Bonzini]

On 01/10/2015 13:43, Dirk Müller wrote:
> The cpu feature flags are not ever going to change, so warning
> everytime can cause a lot of kernel log spam
> (in our case more than 10GB/hour).
> 
> Signed-off-by: Dirk Mueller <dmueller@suse.com>
> 

Applied.  Do you also know what caused the warning, and/or would you
like me to take a look?

Paolo

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Dirk Müller]

On 01.10.2015 14:25, Paolo Bonzini wrote:

Hi Paolo,

 > Applied.  Do you also know what caused the warning, and/or would you
> like me to take a look?

The trace we're getting is this:

  [<ffffffff8100471d>] dump_trace+0x7d/0x2d0
  [<ffffffff81004a04>] show_stack_log_lvl+0x94/0x170
  [<ffffffff81005cc1>] show_stack+0x21/0x50
  [<ffffffff81510f42>] dump_stack+0x41/0x51
  [<ffffffff81055362>] warn_slowpath_common+0x82/0xc0
  [<ffffffffa0388cc1>] skip_emulated_instruction+0xe1/0x160 [kvm_amd]
  [<ffffffffa038908b>] io_interception+0x3b/0x80 [kvm_amd]
  [<ffffffffa031667c>] vcpu_enter_guest+0x6bc/0xc70 [kvm]
  [<ffffffffa031aa08>] kvm_arch_vcpu_ioctl_run+0x1d8/0x450 [kvm]
  [<ffffffffa0305ec1>] kvm_vcpu_ioctl+0x2e1/0x580 [kvm]
  [<ffffffff811b3f24>] do_vfs_ioctl+0x2d4/0x4b0
  [<ffffffff811b4188>] SyS_ioctl+0x88/0xa0
  [<ffffffff8151f209>] system_call_fastpath+0x16/0x1b
  [<00007f8f7b5fabe7>] 0x7f8f7b5fabe6


The colleague who was investigating this is not here today, but from 
what I remember it seems to only happen when nested virtualisation is 
being used. I don't know more about it right now, bisecting pointed out 
f104765b4f81fd74d69e0eb161e89096deade2db . Since it was logging so 
rapidly on our autotester machines (which are running 200-250 VMs 
nested) that we are continuously run out of disk space I just tried to 
come up with a stop-gap fix until this has been properly analyzed.

Thanks,
Dirk

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Paolo Bonzini]

On 01/10/2015 13:43, Dirk Müller wrote:
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 94b7d15..0a42859 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -514,7 +514,7 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  
>  	if (svm->vmcb->control.next_rip != 0) {
> -		WARN_ON(!static_cpu_has(X86_FEATURE_NRIPS));
> +		WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS));
>  		svm->next_rip = svm->vmcb->control.next_rip;
>  	}
>  

Bandan, what was the reason for warning here?

Should we change the "if" condition to static_cpu_has(X86_FEATURE_NRIPS)
instead of Dirk's patch?

Paolo

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Bandan Das]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 01/10/2015 13:43, Dirk Müller wrote:
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 94b7d15..0a42859 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -514,7 +514,7 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
>>  	struct vcpu_svm *svm = to_svm(vcpu);
>>  
>>  	if (svm->vmcb->control.next_rip != 0) {
>> -		WARN_ON(!static_cpu_has(X86_FEATURE_NRIPS));
>> +		WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS));
>>  		svm->next_rip = svm->vmcb->control.next_rip;
>>  	}
>>  
>
> Bandan, what was the reason for warning here?

I added the warning so that we catch if the next_rip field is being written
to (even if the feature isn't supported) by a buggy L1 hypervisor.

From the commit:

 
-	if (svm->vmcb->control.next_rip != 0)
+	if (svm->vmcb->control.next_rip != 0) {
+		WARN_ON(!static_cpu_has(X86_FEATURE_NRIPS));
 		svm->next_rip = svm->vmcb->control.next_rip;
+	}
 
 	if (!svm->next_rip) {
 		if (emulate_instruction(vcpu, EMULTYPE_SKIP) !=
@@ -4317,7 +4319,9 @@ static int svm_check_intercept(struct kvm_vcpu *vcpu,
 		break;
 	}
 
-	vmcb->control.next_rip  = info->next_rip;
+	/* TODO: Advertise NRIPS to guest hypervisor unconditionally */
+	if (static_cpu_has(X86_FEATURE_NRIPS))
+		vmcb->control.next_rip  = info->next_rip;
 	vmcb->control.exit_code = icpt_info.exit_code;
 	vmexit = nested_svm_exit_handled(svm);
...

> Should we change the "if" condition to static_cpu_has(X86_FEATURE_NRIPS)
> instead of Dirk's patch?

Yes, seems ok to me. If decodeassist isn't supported then it's
mostly a stale value. It's interesting that that L1 still works even
after we hit this warning!

> Paolo

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Dirk Müller]

[-- Attachment #1: Type: text/plain, Size: 188 bytes --]


> I added the warning so that we catch if the next_rip field is being written
> to (even if the feature isn't supported) by a buggy L1 hypervisor.

Interesting, so how about this patch?


[-- Attachment #2: 0001-KVM-nSVM-Check-for-NRIP-support-before-accepting-con.patch --]
[-- Type: application/octet-stream, Size: 1009 bytes --]

From c5c8ea255d680f972cbdfc835cdf352fa78897ae Mon Sep 17 00:00:00 2001
From: Dirk Mueller <dirk@dmllr.de>
Date: Fri, 2 Oct 2015 08:35:24 +0200
Subject: [PATCH] KVM: nSVM: Check for NRIP support before accepting
 control.next_rip

NRIP support itself depends on cpuid Fn8000_000A_EDX[NRIPS], remove
a WARN_ON_(once) and check for it directly.

Signed-off-by: Dirk Mueller <dmueller@suse.com>
---
 arch/x86/kvm/svm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 0a42859..33d36da 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -513,8 +513,8 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
-	if (svm->vmcb->control.next_rip != 0) {
-		WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS));
+	if (static_cpu_has(X86_FEATURE_NRIPS) &&
+	    svm->vmcb->control.next_rip != 0) {
 		svm->next_rip = svm->vmcb->control.next_rip;
 	}
 
-- 
2.5.1


[-- Attachment #3: Type: text/plain, Size: 15 bytes --]




Thanks,
Dirk

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Bandan Das]

Dirk Müller <dmueller@suse.com> writes:

>> I added the warning so that we catch if the next_rip field is being written
>> to (even if the feature isn't supported) by a buggy L1 hypervisor.
>
> Interesting, so how about this patch?
>
>
> From c5c8ea255d680f972cbdfc835cdf352fa78897ae Mon Sep 17 00:00:00 2001
> From: Dirk Mueller <dirk@dmllr.de>
> Date: Fri, 2 Oct 2015 08:35:24 +0200
> Subject: [PATCH] KVM: nSVM: Check for NRIP support before accepting
>  control.next_rip
>
> NRIP support itself depends on cpuid Fn8000_000A_EDX[NRIPS], remove
> a WARN_ON_(once) and check for it directly.
>
> Signed-off-by: Dirk Mueller <dmueller@suse.com>
> ---
>  arch/x86/kvm/svm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 0a42859..33d36da 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -513,8 +513,8 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  
> -	if (svm->vmcb->control.next_rip != 0) {
> -		WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS));
> +	if (static_cpu_has(X86_FEATURE_NRIPS) &&
> +	    svm->vmcb->control.next_rip != 0) {
>  		svm->next_rip = svm->vmcb->control.next_rip;
>  	}

Ok, looks good to me. Still, probably a good idea to let the user know if this condition is
hit.

Bandan

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Joerg Roedel]

On Thu, Oct 01, 2015 at 06:31:27PM -0400, Bandan Das wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > On 01/10/2015 13:43, Dirk Müller wrote:
> >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> >> index 94b7d15..0a42859 100644
> >> --- a/arch/x86/kvm/svm.c
> >> +++ b/arch/x86/kvm/svm.c
> >> @@ -514,7 +514,7 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
> >>  	struct vcpu_svm *svm = to_svm(vcpu);
> >>  
> >>  	if (svm->vmcb->control.next_rip != 0) {
> >> -		WARN_ON(!static_cpu_has(X86_FEATURE_NRIPS));
> >> +		WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS));
> >>  		svm->next_rip = svm->vmcb->control.next_rip;
> >>  	}
> >>  
> >
> > Bandan, what was the reason for warning here?
> 
> I added the warning so that we catch if the next_rip field is being written
> to (even if the feature isn't supported) by a buggy L1 hypervisor.

Even if the L1 hypervisor writes to the next_rip field in the VMCB, we
would never see it in this code path, as we access the shadow VMCB in
this statement.

We don't even care if the L1 hypervisor writes to its next_rip field
because we only write to this field on an emulatated VMEXIT and never
read it back.

So what's the point in adding a guest-triggerable warning at all?



	Joerg

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Bandan Das]

Joerg Roedel <joro@8bytes.org> writes:

> On Thu, Oct 01, 2015 at 06:31:27PM -0400, Bandan Das wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>> > On 01/10/2015 13:43, Dirk Müller wrote:
>> >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> >> index 94b7d15..0a42859 100644
>> >> --- a/arch/x86/kvm/svm.c
>> >> +++ b/arch/x86/kvm/svm.c
>> >> @@ -514,7 +514,7 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
>> >>  	struct vcpu_svm *svm = to_svm(vcpu);
>> >>  
>> >>  	if (svm->vmcb->control.next_rip != 0) {
>> >> -		WARN_ON(!static_cpu_has(X86_FEATURE_NRIPS));
>> >> +		WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS));
>> >>  		svm->next_rip = svm->vmcb->control.next_rip;
>> >>  	}
>> >>  
>> >
>> > Bandan, what was the reason for warning here?
>> 
>> I added the warning so that we catch if the next_rip field is being written
>> to (even if the feature isn't supported) by a buggy L1 hypervisor.
>
> Even if the L1 hypervisor writes to the next_rip field in the VMCB, we
> would never see it in this code path, as we access the shadow VMCB in
> this statement.
>
> We don't even care if the L1 hypervisor writes to its next_rip field
> because we only write to this field on an emulatated VMEXIT and never
> read it back.

The problems is that the next_rip field could be stale. If the processor supports
next_rip, then it will clear it out on the next entry. If it doesn't,
an old value just sits there (no matter who wrote it) and the problem
happens when skip_emulated_instruction advances the rip with an incorrect
value.

> So what's the point in adding a guest-triggerable warning at all?

So, yes, maybe this doesn't have to be a guest specific warning but we still
need to warn if this unsupported field is being written to.

>
>
> 	Joerg

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Joerg Roedel]

On Mon, Oct 05, 2015 at 12:54:43PM -0400, Bandan Das wrote:
> Joerg Roedel <joro@8bytes.org> writes:
> The problems is that the next_rip field could be stale. If the processor supports
> next_rip, then it will clear it out on the next entry. If it doesn't,
> an old value just sits there (no matter who wrote it) and the problem
> happens when skip_emulated_instruction advances the rip with an incorrect
> value.

So the right fix would be to just set the guests next_rip to 0 when we
emulate vmrun, just like real hardware does, no?


	Joerg

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Bandan Das]

Joerg Roedel <joro@8bytes.org> writes:

> On Mon, Oct 05, 2015 at 12:54:43PM -0400, Bandan Das wrote:
>> Joerg Roedel <joro@8bytes.org> writes:
>> The problems is that the next_rip field could be stale. If the processor supports
>> next_rip, then it will clear it out on the next entry. If it doesn't,
>> an old value just sits there (no matter who wrote it) and the problem
>> happens when skip_emulated_instruction advances the rip with an incorrect
>> value.
>
> So the right fix would be to just set the guests next_rip to 0 when we
> emulate vmrun, just like real hardware does, no?

Agreed, resetting to 0 if nrips isn't supported seems right. It would still
help having a printk_once in this case IMO :)

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

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Joerg Roedel]

On Mon, Oct 05, 2015 at 01:42:43PM -0400, Bandan Das wrote:
> Joerg Roedel <joro@8bytes.org> writes:
> 
> > On Mon, Oct 05, 2015 at 12:54:43PM -0400, Bandan Das wrote:
> >> Joerg Roedel <joro@8bytes.org> writes:
> >> The problems is that the next_rip field could be stale. If the processor supports
> >> next_rip, then it will clear it out on the next entry. If it doesn't,
> >> an old value just sits there (no matter who wrote it) and the problem
> >> happens when skip_emulated_instruction advances the rip with an incorrect
> >> value.
> >
> > So the right fix would be to just set the guests next_rip to 0 when we
> > emulate vmrun, just like real hardware does, no?
> 
> Agreed, resetting to 0 if nrips isn't supported seems right. It would still
> help having a printk_once in this case IMO :)

I meant to reset it always to 0 on vmrun, like real hardware does.



	Joerg

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Bandan Das]

Joerg Roedel <joro@8bytes.org> writes:

> On Mon, Oct 05, 2015 at 01:42:43PM -0400, Bandan Das wrote:
>> Joerg Roedel <joro@8bytes.org> writes:
>> 
>> > On Mon, Oct 05, 2015 at 12:54:43PM -0400, Bandan Das wrote:
>> >> Joerg Roedel <joro@8bytes.org> writes:
>> >> The problems is that the next_rip field could be stale. If the processor supports
>> >> next_rip, then it will clear it out on the next entry. If it doesn't,
>> >> an old value just sits there (no matter who wrote it) and the problem
>> >> happens when skip_emulated_instruction advances the rip with an incorrect
>> >> value.
>> >
>> > So the right fix would be to just set the guests next_rip to 0 when we
>> > emulate vmrun, just like real hardware does, no?
>> 
>> Agreed, resetting to 0 if nrips isn't supported seems right. It would still
>> help having a printk_once in this case IMO :)
>
> I meant to reset it always to 0 on vmrun, like real hardware does.

Atleast the spec don't mention this, I don't know how I got that idea :) The spec
just say that it gets written to by hardware on certain intercepts and for others
it gets reset to 0 on #VMEXIT.

>
>
> 	Joerg

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Dirk Müller]

[-- Attachment #1: Type: text/plain, Size: 276 bytes --]

> So the right fix would be to just set the guests next_rip to 0 when we
> emulate vmrun, just like real hardware does, no?

Like this? (Note: I’m not sure what I’m doing here..). I Agree with you that the warning
for this seems excessive, I’ve just removed it. 


[-- Attachment #2: 0001-KVM-nSVM-Check-for-NRIP-support-before-passing-on-co.patch --]
[-- Type: application/octet-stream, Size: 1164 bytes --]

From 47db81837b6fe58aa302317bbf2a092b40af0a36 Mon Sep 17 00:00:00 2001
From: Dirk Mueller <dirk@dmllr.de>
Date: Fri, 2 Oct 2015 08:35:24 +0200
Subject: [PATCH] KVM: nSVM: Check for NRIP support before passing on
 control.next_rip

NRIP support itself depends on cpuid Fn8000_000A_EDX[NRIPS], so
ignore it if the cpu feature is not available. Remove the
guest-triggerable WARN_ON for this as it just emulates real
hardware behavior.

Signed-off-by: Dirk Mueller <dmueller@suse.com>
---
 arch/x86/kvm/svm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 0a42859..66e3a4c 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -514,8 +514,10 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 	struct vcpu_svm *svm = to_svm(vcpu);
 
 	if (svm->vmcb->control.next_rip != 0) {
-		WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS));
-		svm->next_rip = svm->vmcb->control.next_rip;
+		if (static_cpu_has(X86_FEATURE_NRIPS))
+			svm->next_rip = svm->vmcb->control.next_rip;
+		else
+			svm->next_rip = 0;
 	}
 
 	if (!svm->next_rip) {
-- 
2.5.1


[-- Attachment #3: Type: text/plain, Size: 14 bytes --]



Thanks,
Dirk

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Bandan Das]

Dirk Müller <dmueller@suse.com> writes:

>> So the right fix would be to just set the guests next_rip to 0 when we
>> emulate vmrun, just like real hardware does, no?
>
> Like this? (Note: I’m not sure what I’m doing here..). I Agree with you that the warning
> for this seems excessive, I’ve just removed it. 
>
>
> From 47db81837b6fe58aa302317bbf2a092b40af0a36 Mon Sep 17 00:00:00 2001
> From: Dirk Mueller <dirk@dmllr.de>
> Date: Fri, 2 Oct 2015 08:35:24 +0200
> Subject: [PATCH] KVM: nSVM: Check for NRIP support before passing on
>  control.next_rip
>
> NRIP support itself depends on cpuid Fn8000_000A_EDX[NRIPS], so
> ignore it if the cpu feature is not available. Remove the
> guest-triggerable WARN_ON for this as it just emulates real
> hardware behavior.
>
> Signed-off-by: Dirk Mueller <dmueller@suse.com>
> ---
>  arch/x86/kvm/svm.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 0a42859..66e3a4c 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -514,8 +514,10 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  
>  	if (svm->vmcb->control.next_rip != 0) {
> -		WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS));
> -		svm->next_rip = svm->vmcb->control.next_rip;
> +		if (static_cpu_has(X86_FEATURE_NRIPS))
> +			svm->next_rip = svm->vmcb->control.next_rip;
> +		else
> +			svm->next_rip = 0;

skip_emulated_instruction probably isn't the right place.

>  	}

So, L1 writes this field in svm_check_intercept.

I went back to the spec and it says (15.7.1):
The next sequential instruction pointer (nRIP) is saved in the guest VMCB
control area at location C8h on all #VMEXITs that are due to instruction
intercepts, as defined in Section 15.9 on page 458, as well as MSR and
IOIO intercepts and exceptions caused by the INT3, INTO, and BOUND
instructions. For all other intercepts, nRIP is reset to zero.

So, I think a better way would be to reset the field on vmexit from
L2 to L0 if NRIP isn't supported. Maybe it's enough to do this only for
the intercepts mentioned above but I am not sure.

Also, please include a debug message (pr_debug_once will do). It seems
unnecessary today, but in 2 years when someone is scratching his head why his
nested guests won't run, he will thank you!

BTW, it seems possible to unconditionally advertise SVM_FEATURE_NRIP..

>  	if (!svm->next_rip) {

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Joerg Roedel]

On Thu, Oct 01, 2015 at 06:31:27PM -0400, Bandan Das wrote:
> >> @@ -514,7 +514,7 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
> >>  	struct vcpu_svm *svm = to_svm(vcpu);
> >>  
> >>  	if (svm->vmcb->control.next_rip != 0) {
> >> -		WARN_ON(!static_cpu_has(X86_FEATURE_NRIPS));
> >> +		WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS));
> >>  		svm->next_rip = svm->vmcb->control.next_rip;
> >>  	}

I looked again how this could possibly be triggered, and I am somewhat
confused now.

So svm->vmcb->control.next_rip is only written by hardware or in
svm_check_intercept(). Both cases write only to this field, if the
hardware supports X86_FEATURE_NRIPS. The write in nested_svm_vmexit only
targets the guests VMCB, and we don't use that one again.

So I can't see how the WARN_ON above could be triggered. Do I miss
something or might this also be a miscompilation of static_cpu_has?


	Joerg

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Bandan Das]

Joerg Roedel <joro@8bytes.org> writes:

> On Thu, Oct 01, 2015 at 06:31:27PM -0400, Bandan Das wrote:
>> >> @@ -514,7 +514,7 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
>> >>  	struct vcpu_svm *svm = to_svm(vcpu);
>> >>  
>> >>  	if (svm->vmcb->control.next_rip != 0) {
>> >> -		WARN_ON(!static_cpu_has(X86_FEATURE_NRIPS));
>> >> +		WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS));
>> >>  		svm->next_rip = svm->vmcb->control.next_rip;
>> >>  	}
>
> I looked again how this could possibly be triggered, and I am somewhat
> confused now.
>
> So svm->vmcb->control.next_rip is only written by hardware or in
> svm_check_intercept(). Both cases write only to this field, if the
> hardware supports X86_FEATURE_NRIPS. The write in nested_svm_vmexit only

Not until commit f104765b4f81fd74d69e0eb161e89096deade2db. So, an older L1
kernel will trigger it.

> targets the guests VMCB, and we don't use that one again.
>
> So I can't see how the WARN_ON above could be triggered. Do I miss
> something or might this also be a miscompilation of static_cpu_has?
>
>
> 	Joerg

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Joerg Roedel]

On Tue, Oct 06, 2015 at 01:59:27PM -0400, Bandan Das wrote:
> Joerg Roedel <joro@8bytes.org> writes:
> >
> > So svm->vmcb->control.next_rip is only written by hardware or in
> > svm_check_intercept(). Both cases write only to this field, if the
> > hardware supports X86_FEATURE_NRIPS. The write in nested_svm_vmexit only
> 
> Not until commit f104765b4f81fd74d69e0eb161e89096deade2db. So, an older L1
> kernel will trigger it.

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.


	Joerg

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

[Joerg Roedel]

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).

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:

>From 019afc60507618b8e44e0c67d5ea2d850d88c9dd Mon Sep 17 00:00:00 2001
From: Joerg Roedel <jroedel@suse.de>
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 <jroedel@suse.de>
---
 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 0x8000000a.edx bit 3
+ */
+#define BIT_NRIPS	3
+
+static inline bool guest_cpuid_has_nrips(struct kvm_vcpu *vcpu)
+{
+	struct kvm_cpuid_entry2 *best;
+
+	best = kvm_find_cpuid_entry(vcpu, 0x8000000a, 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;
+
 	nested_svm_unmap(page);
 
 	/* Enter Guest-Mode */
-- 
1.8.4.5

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

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 infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 24334 bytes --]

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

[Bandan Das]

Joerg Roedel <joro@8bytes.org> 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 <jroedel@suse.de>
> 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 <jroedel@suse.de>
> ---
>  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 0x8000000a.edx bit 3
> + */
> +#define BIT_NRIPS	3
> +
> +static inline bool guest_cpuid_has_nrips(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_cpuid_entry2 *best;
> +
> +	best = kvm_find_cpuid_entry(vcpu, 0x8000000a, 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 */

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 &svm->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

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 <dmueller@suse.com>

Works fine here. Thanks!


Greetings,
Dirk

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Bandan Das]

Joerg Roedel <joro@8bytes.org> writes:

> On Tue, Oct 06, 2015 at 01:59:27PM -0400, Bandan Das wrote:
>> Joerg Roedel <joro@8bytes.org> writes:
>> >
>> > So svm->vmcb->control.next_rip is only written by hardware or in
>> > svm_check_intercept(). Both cases write only to this field, if the
>> > hardware supports X86_FEATURE_NRIPS. The write in nested_svm_vmexit only
>> 
>> Not until commit f104765b4f81fd74d69e0eb161e89096deade2db. So, an older L1
>> kernel will trigger it.
>
> 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.

Ok, looks like I am making some incorrect "vmx" assumptions here. What happens
when we exit from L2 to L0, arent' we looking at the VMCB L1 is using to run
L2 ? Wouldn't that trigger the warning if the host processor does not support
nrips and the field is set ?


>
> 	Joerg

Re: [PATCH] Use WARN_ON_ONCE for missing X86_FEATURE_NRIPS

[Joerg Roedel]

On Wed, Oct 07, 2015 at 10:58:07AM -0400, Bandan Das wrote:
> Ok, looks like I am making some incorrect "vmx" assumptions here. What happens
> when we exit from L2 to L0, arent' we looking at the VMCB L1 is using to run
> L2 ? Wouldn't that trigger the warning if the host processor does not support
> nrips and the field is set ?

No, because the L1 hypervisor can't write to the shadow-vmcb, and this
is the only one where we _read_ next_rip from.



	Joerg