[PATCH 0/6] KVM: x86: Miscellaneous bug fixes

[PATCH 0/6] KVM: x86: Miscellaneous bug fixes

[Nadav Amit]

This patch-set includes miscellaneous bug fixes.

Patches 1-2 fix KVM bugs that affect the correct behavior of VMs.

Patches 3-5 have no apparent implications in normal use. Nonetheless, they fix
code which is bluntly wrong.

Patch 6 is version 2 of previously submitted patch. It was revised according to
Paolo's comment (previously a fetch check was missing).

Thanks for reviewing the patches.

Nadav Amit (6):
  KVM: x86: DR7.GD should be cleared upon any #DB exception
  KVM: x86: Wrong error code on limit violation during emulation
  KVM: x86: NoBigReal was mistakenly considering la instead of ea
  KVM: x86: Fix determining flat mode in recalculate_apic_map
  KVM: x86: Wrong assertion on paging_tmpl.h
  KVM: x86: Emulator does not calculate address correctly

 arch/x86/kvm/emulate.c     | 29 ++++++++++++++++-------------
 arch/x86/kvm/lapic.c       |  2 +-
 arch/x86/kvm/paging_tmpl.h |  2 +-
 arch/x86/kvm/vmx.c         |  2 --
 arch/x86/kvm/x86.c         |  6 ++++++
 5 files changed, 24 insertions(+), 17 deletions(-)

-- 
1.9.1

[PATCH 1/6] KVM: x86: DR7.GD should be cleared upon any #DB exception

[Nadav Amit]

Intel SDM 17.2.4 (Debug Control Register (DR7)) says: "The processor clears the
GD flag upon entering to the debug exception handler." This sentence may be
misunderstood as if it happens only on #DB due to debug-register protection,
but it happens regardless to the cause of the #DB.

Fix the behavior.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/vmx.c | 2 --
 arch/x86/kvm/x86.c | 6 ++++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 04fa1b8..82d39f9 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5176,9 +5176,7 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 			vcpu->run->exit_reason = KVM_EXIT_DEBUG;
 			return 0;
 		} else {
-			vcpu->arch.dr7 &= ~DR7_GD;
 			vcpu->arch.dr6 |= DR6_BD | DR6_RTM;
-			vmcs_writel(GUEST_DR7, vcpu->arch.dr7);
 			kvm_queue_exception(vcpu, DB_VECTOR);
 			return 1;
 		}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6857257..fb02371 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5935,6 +5935,12 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool req_int_win)
 			__kvm_set_rflags(vcpu, kvm_get_rflags(vcpu) |
 					     X86_EFLAGS_RF);
 
+		if (vcpu->arch.exception.nr == DB_VECTOR &&
+		    (vcpu->arch.dr7 & DR7_GD)) {
+			vcpu->arch.dr7 &= ~DR7_GD;
+			kvm_update_dr7(vcpu);
+		}
+
 		kvm_x86_ops->queue_exception(vcpu, vcpu->arch.exception.nr,
 					  vcpu->arch.exception.has_error_code,
 					  vcpu->arch.exception.error_code,
-- 
1.9.1

Re: [PATCH 1/6] KVM: x86: DR7.GD should be cleared upon any #DB exception

[Radim Krčmář]

2014-09-30 20:49+0300, Nadav Amit:
> Intel SDM 17.2.4 (Debug Control Register (DR7)) says: "The processor clears the
> GD flag upon entering to the debug exception handler." This sentence may be
> misunderstood as if it happens only on #DB due to debug-register protection,
> but it happens regardless to the cause of the #DB.

All real hardware behaves that way?

Intel has another sentence after that

  [...], to allow the handler access to the debug registers.

I suppose that the "the" is important, but I haven't verified it.[1]
Clearing GD on every #DB would also make the stated purpose[2] harder to
achieve without adding any benefit;  it seems like a bug for Intel.


---
1: AMD [13.1.1.4 Debug-Control Register (DR7)] uses a similar wording

     General-Detect Enable (GD)—Bit 13. Software sets this bit to 1 to
     cause a debug exception to occur when an attempt is made to execute
     a MOV DRn instruction to any debug register (DR0–DR7). This bit is
     cleared to 0 by the processor when the #DB handler is entered,
     allowing the handler to read and write the DRn registers. The #DB
     exception occurs before executing the instruction, and DR6[BD] is
     set by the processor. Software debuggers can use this bit to
     prevent the currently-executing program from interfering with the
     debug operation.

2: Last sentence of [1] and also this from Intel
     This condition is provided to support in-circuit emulators.

     When the emulator needs to access the debug registers, emulator
     software can set the GD flag to prevent interference from the
     program currently executing on the processor.

Re: [PATCH 1/6] KVM: x86: DR7.GD should be cleared upon any #DB exception

[Nadav Amit]

On Oct 1, 2014, at 6:24 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:

> 2014-09-30 20:49+0300, Nadav Amit:
>> Intel SDM 17.2.4 (Debug Control Register (DR7)) says: "The processor clears the
>> GD flag upon entering to the debug exception handler." This sentence may be
>> misunderstood as if it happens only on #DB due to debug-register protection,
>> but it happens regardless to the cause of the #DB.
> 
> All real hardware behaves that way?
I have no way of knowing.

I know Intel’s phrasing is misleading, so I verified this behaviour in two ways:
1. I changed KVM not to trap #DB. I then changed kvm-unit-tests/debug.c to set DR7.GD prior to the watchpoint test, and printed once I entered the handler, before any DR was accessed by the handler.
The result: we entered the handler once (afterwards I printed DR7 and saw GD is indeed clear). If #DB due to watchpoint did not clear GD, we would enter the handler twice.

2. I looked at bochs: https://github.com/larsr/bochs-svn/blob/master/cpu/exception.cc :

  if (vector == BX_DB_EXCEPTION) {
    // Commit debug events to DR6: preserve DR5.BS and DR6.BD values,
    // only software can clear them
    BX_CPU_THIS_PTR dr6.val32 = (BX_CPU_THIS_PTR dr6.val32 & 0xffff6ff0) |
                          (BX_CPU_THIS_PTR debug_trap & 0x0000e00f);

    // clear GD flag in the DR7 prior entering debug exception handler
    BX_CPU_THIS_PTR dr7.set_GD(0);
  }

> 
> Intel has another sentence after that
> 
>  [...], to allow the handler access to the debug registers.
> 
> I suppose that the "the" is important, but I haven't verified it.[1]
> Clearing GD on every #DB would also make the stated purpose[2] harder to
> achieve without adding any benefit;  it seems like a bug for Intel.

The behaviour seems reasonable to me. Otherwise the CPU would re-enter the handler when the handler inspects DR6.

> 
> 
> ---
> 1: AMD [13.1.1.4 Debug-Control Register (DR7)] uses a similar wording
> 
>     General-Detect Enable (GD)—Bit 13. Software sets this bit to 1 to
>     cause a debug exception to occur when an attempt is made to execute
>     a MOV DRn instruction to any debug register (DR0–DR7). This bit is
>     cleared to 0 by the processor when the #DB handler is entered,
>     allowing the handler to read and write the DRn registers. The #DB
>     exception occurs before executing the instruction, and DR6[BD] is
>     set by the processor. Software debuggers can use this bit to
>     prevent the currently-executing program from interfering with the
>     debug operation.
> 
> 2: Last sentence of [1] and also this from Intel
>     This condition is provided to support in-circuit emulators.
> 
>     When the emulator needs to access the debug registers, emulator
>     software can set the GD flag to prevent interference from the
>     program currently executing on the processor.

Re: [PATCH 1/6] KVM: x86: DR7.GD should be cleared upon any #DB exception

[Radim Krčmář]

2014-10-01 21:22+0300, Nadav Amit:
> 
> On Oct 1, 2014, at 6:24 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:
> 
> > 2014-09-30 20:49+0300, Nadav Amit:
> >> Intel SDM 17.2.4 (Debug Control Register (DR7)) says: "The processor clears the
> >> GD flag upon entering to the debug exception handler." This sentence may be
> >> misunderstood as if it happens only on #DB due to debug-register protection,
> >> but it happens regardless to the cause of the #DB.
> > 
> > All real hardware behaves that way?
> I have no way of knowing.

:)

> I know Intel’s phrasing is misleading, so I verified this behaviour in two ways:
> 1. I changed KVM not to trap #DB. I then changed kvm-unit-tests/debug.c to set DR7.GD prior to the watchpoint test, and printed once I entered the handler, before any DR was accessed by the handler.
> The result: we entered the handler once (afterwards I printed DR7 and saw GD is indeed clear). If #DB due to watchpoint did not clear GD, we would enter the handler twice.

Thanks.

> 2. I looked at bochs: https://github.com/larsr/bochs-svn/blob/master/cpu/exception.cc :
> 
>   if (vector == BX_DB_EXCEPTION) {
>     // Commit debug events to DR6: preserve DR5.BS and DR6.BD values,
>     // only software can clear them
>     BX_CPU_THIS_PTR dr6.val32 = (BX_CPU_THIS_PTR dr6.val32 & 0xffff6ff0) |
>                           (BX_CPU_THIS_PTR debug_trap & 0x0000e00f);
> 
>     // clear GD flag in the DR7 prior entering debug exception handler
>     BX_CPU_THIS_PTR dr7.set_GD(0);
>   }

(Ok.)

> The behaviour seems reasonable to me. Otherwise the CPU would re-enter the handler when the handler inspects DR6.

It usually is sufficient just to read it, but yeah, re-entry for the
general usage isn't nice either.

To the patch itself:  We could use kvm_x86_ops->set_dr7() directly, but
maybe we are going to do something with on GD bit, so

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

[PATCH 2/6] KVM: x86: Wrong error code on limit violation during emulation

[Nadav Amit]

GP and SS exceptions deliver as error-code the segment selector if the
exception occurred when the segment is loaded.  However, if the exception
occurs during the memory access itself, due to limit violations, the error-code
should be zero.  Fix it.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/emulate.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index a46207a..13a1c76 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -621,7 +621,7 @@ static int __linearize(struct x86_emulate_ctxt *ctxt,
 	bool usable;
 	ulong la;
 	u32 lim;
-	u16 sel;
+	u16 sel, error_code = 0;
 	unsigned cpl;
 
 	la = seg_base(ctxt, addr.seg) + addr.ea;
@@ -634,14 +634,14 @@ static int __linearize(struct x86_emulate_ctxt *ctxt,
 		usable = ctxt->ops->get_segment(ctxt, &sel, &desc, NULL,
 						addr.seg);
 		if (!usable)
-			goto bad;
+			goto bad_sel;
 		/* code segment in protected mode or read-only data segment */
 		if ((((ctxt->mode != X86EMUL_MODE_REAL) && (desc.type & 8))
 					|| !(desc.type & 2)) && write)
-			goto bad;
+			goto bad_sel;
 		/* unreadable code segment */
 		if (!fetch && (desc.type & 8) && !(desc.type & 2))
-			goto bad;
+			goto bad_sel;
 		lim = desc_limit_scaled(&desc);
 		if ((ctxt->mode == X86EMUL_MODE_REAL) && !fetch &&
 		    (ctxt->d & NoBigReal)) {
@@ -664,15 +664,15 @@ static int __linearize(struct x86_emulate_ctxt *ctxt,
 		if (!(desc.type & 8)) {
 			/* data segment */
 			if (cpl > desc.dpl)
-				goto bad;
+				goto bad_sel;
 		} else if ((desc.type & 8) && !(desc.type & 4)) {
 			/* nonconforming code segment */
 			if (cpl != desc.dpl)
-				goto bad;
+				goto bad_sel;
 		} else if ((desc.type & 8) && (desc.type & 4)) {
 			/* conforming code segment */
 			if (cpl < desc.dpl)
-				goto bad;
+				goto bad_sel;
 		}
 		break;
 	}
@@ -682,11 +682,13 @@ static int __linearize(struct x86_emulate_ctxt *ctxt,
 		return emulate_gp(ctxt, 0);
 	*linear = la;
 	return X86EMUL_CONTINUE;
+bad_sel:
+	error_code = sel;
 bad:
 	if (addr.seg == VCPU_SREG_SS)
-		return emulate_ss(ctxt, sel);
+		return emulate_ss(ctxt, error_code);
 	else
-		return emulate_gp(ctxt, sel);
+		return emulate_gp(ctxt, error_code);
 }
 
 static int linearize(struct x86_emulate_ctxt *ctxt,
-- 
1.9.1

Re: [PATCH 2/6] KVM: x86: Wrong error code on limit violation during emulation

[Radim Krčmář]

2014-09-30 20:49+0300, Nadav Amit:
> GP and SS exceptions deliver as error-code the segment selector if the
> exception occurred when the segment is loaded.  However, if the exception
> occurs during the memory access itself, due to limit violations, the error-code
> should be zero.  Fix it.
> 
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

Re: [PATCH 2/6] KVM: x86: Wrong error code on limit violation during emulation

[Paolo Bonzini]

On 09/30/2014 07:49 PM, Nadav Amit wrote:
> GP and SS exceptions deliver as error-code the segment selector if the
> exception occurred when the segment is loaded.  However, if the exception
> occurs during the memory access itself, due to limit violations, the error-code
> should be zero.  Fix it.
> 
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  arch/x86/kvm/emulate.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index a46207a..13a1c76 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -621,7 +621,7 @@ static int __linearize(struct x86_emulate_ctxt *ctxt,
>  	bool usable;
>  	ulong la;
>  	u32 lim;
> -	u16 sel;
> +	u16 sel, error_code = 0;
>  	unsigned cpl;
>  
>  	la = seg_base(ctxt, addr.seg) + addr.ea;
> @@ -634,14 +634,14 @@ static int __linearize(struct x86_emulate_ctxt *ctxt,
>  		usable = ctxt->ops->get_segment(ctxt, &sel, &desc, NULL,
>  						addr.seg);
>  		if (!usable)
> -			goto bad;
> +			goto bad_sel;

This can only happen because of a NULL selector, which means the error
code is zero anyway.

>  		/* code segment in protected mode or read-only data segment */
>  		if ((((ctxt->mode != X86EMUL_MODE_REAL) && (desc.type & 8))
>  					|| !(desc.type & 2)) && write)
> -			goto bad;
> +			goto bad_sel;

This is not "detected while loading a segment descriptor", so the error
code should be zero.

>  		/* unreadable code segment */
>  		if (!fetch && (desc.type & 8) && !(desc.type & 2))
> -			goto bad;
> +			goto bad_sel;

Same here.

>  		lim = desc_limit_scaled(&desc);
>  		if ((ctxt->mode == X86EMUL_MODE_REAL) && !fetch &&
>  		    (ctxt->d & NoBigReal)) {
> @@ -664,15 +664,15 @@ static int __linearize(struct x86_emulate_ctxt *ctxt,
>  		if (!(desc.type & 8)) {
>  			/* data segment */
>  			if (cpl > desc.dpl)
> -				goto bad;
> +				goto bad_sel;
>  		} else if ((desc.type & 8) && !(desc.type & 4)) {
>  			/* nonconforming code segment */
>  			if (cpl != desc.dpl)
> -				goto bad;
> +				goto bad_sel;
>  		} else if ((desc.type & 8) && (desc.type & 4)) {
>  			/* conforming code segment */
>  			if (cpl < desc.dpl)
> -				goto bad;
> +				goto bad_sel;

These three should be deleted, as you pointed out in patch 5.

So I've dropped this patch, and posted a simpler alternative that just
uses error code 0 in __linearize.  Can you look at it?

Paolo

>  		}
>  		break;
>  	}
> @@ -682,11 +682,13 @@ static int __linearize(struct x86_emulate_ctxt *ctxt,
>  		return emulate_gp(ctxt, 0);
>  	*linear = la;
>  	return X86EMUL_CONTINUE;
> +bad_sel:
> +	error_code = sel;
>  bad:
>  	if (addr.seg == VCPU_SREG_SS)
> -		return emulate_ss(ctxt, sel);
> +		return emulate_ss(ctxt, error_code);
>  	else
> -		return emulate_gp(ctxt, sel);
> +		return emulate_gp(ctxt, error_code);
>  }
>  
>  static int linearize(struct x86_emulate_ctxt *ctxt,
> 

Re: [PATCH 2/6] KVM: x86: Wrong error code on limit violation during emulation

[Nadav Amit]

> On Oct 27, 2014, at 16:37, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> 
> 
> On 09/30/2014 07:49 PM, Nadav Amit wrote:
>> GP and SS exceptions deliver as error-code the segment selector if the
>> exception occurred when the segment is loaded.  However, if the exception
>> occurs during the memory access itself, due to limit violations, the error-code
>> should be zero.  Fix it.
>> 
>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>> ---
>> arch/x86/kvm/emulate.c | 20 +++++++++++---------
>> 1 file changed, 11 insertions(+), 9 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index a46207a..13a1c76 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -621,7 +621,7 @@ static int __linearize(struct x86_emulate_ctxt *ctxt,
>> 	bool usable;
>> 	ulong la;
>> 	u32 lim;
>> -	u16 sel;
>> +	u16 sel, error_code = 0;
>> 	unsigned cpl;
>> 
>> 	la = seg_base(ctxt, addr.seg) + addr.ea;
>> @@ -634,14 +634,14 @@ static int __linearize(struct x86_emulate_ctxt *ctxt,
>> 		usable = ctxt->ops->get_segment(ctxt, &sel, &desc, NULL,
>> 						addr.seg);
>> 		if (!usable)
>> -			goto bad;
>> +			goto bad_sel;
> 
> This can only happen because of a NULL selector, which means the error
> code is zero anyway.
> 
>> 		/* code segment in protected mode or read-only data segment */
>> 		if ((((ctxt->mode != X86EMUL_MODE_REAL) && (desc.type & 8))
>> 					|| !(desc.type & 2)) && write)
>> -			goto bad;
>> +			goto bad_sel;
> 
> This is not "detected while loading a segment descriptor", so the error
> code should be zero.
> 
>> 		/* unreadable code segment */
>> 		if (!fetch && (desc.type & 8) && !(desc.type & 2))
>> -			goto bad;
>> +			goto bad_sel;
> 
> Same here.
> 
>> 		lim = desc_limit_scaled(&desc);
>> 		if ((ctxt->mode == X86EMUL_MODE_REAL) && !fetch &&
>> 		    (ctxt->d & NoBigReal)) {
>> @@ -664,15 +664,15 @@ static int __linearize(struct x86_emulate_ctxt *ctxt,
>> 		if (!(desc.type & 8)) {
>> 			/* data segment */
>> 			if (cpl > desc.dpl)
>> -				goto bad;
>> +				goto bad_sel;
>> 		} else if ((desc.type & 8) && !(desc.type & 4)) {
>> 			/* nonconforming code segment */
>> 			if (cpl != desc.dpl)
>> -				goto bad;
>> +				goto bad_sel;
>> 		} else if ((desc.type & 8) && (desc.type & 4)) {
>> 			/* conforming code segment */
>> 			if (cpl < desc.dpl)
>> -				goto bad;
>> +				goto bad_sel;
> 
> These three should be deleted, as you pointed out in patch 5.
> 
> So I've dropped this patch, and posted a simpler alternative that just
> uses error code 0 in __linearize.  Can you look at it?

It looks fine. I noticed these mistakes also...
Please wait with the other emulator fixes I posted. I run some further tests, since I am annoyed of making mistakes and redoing patches.

Regards,
Nadav 

Re: [PATCH 2/6] KVM: x86: Wrong error code on limit violation during emulation

[Paolo Bonzini]

On 10/27/2014 03:46 PM, Nadav Amit wrote:
> > So I've dropped this patch, and posted a simpler alternative that just
> > uses error code 0 in __linearize.  Can you look at it?
>
> It looks fine. I noticed these mistakes also...
> Please wait with the other emulator fixes I posted. I run some further tests, since I am annoyed of making mistakes and redoing patches.

Yes, they are all in kvm/queue so they can be removed anytime.

Paolo

[PATCH 3/6] KVM: x86: NoBigReal was mistakenly considering la instead of ea

[Nadav Amit]

NoBigReal emulation should consider the effective address is between 0 and
0xffff instead of checking the logical address.  Currently there are no
instructions which are marked with NoBigReal flag, so this bug currently has no
impact.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/emulate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 13a1c76..4b687ff 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -645,8 +645,8 @@ static int __linearize(struct x86_emulate_ctxt *ctxt,
 		lim = desc_limit_scaled(&desc);
 		if ((ctxt->mode == X86EMUL_MODE_REAL) && !fetch &&
 		    (ctxt->d & NoBigReal)) {
-			/* la is between zero and 0xffff */
-			if (la > 0xffff || (u32)(la + size - 1) > 0xffff)
+			/* ea is between zero and 0xffff */
+			if ((u32)addr.ea + size - 1 > 0xfffful)
 				goto bad;
 		} else if ((desc.type & 8) || !(desc.type & 4)) {
 			/* expand-up segment */
-- 
1.9.1

Re: [PATCH 3/6] KVM: x86: NoBigReal was mistakenly considering la instead of ea

[Radim Krčmář]

2014-09-30 20:49+0300, Nadav Amit:
> NoBigReal emulation should consider the effective address is between 0 and
> 0xffff instead of checking the logical address.  Currently there are no
> instructions which are marked with NoBigReal flag, so this bug currently has no
> impact.

(Would be nice if we actually used it :)

> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---

Should have first looked at git-blame instead of SDM,
Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

Re: [PATCH 3/6] KVM: x86: NoBigReal was mistakenly considering la instead of ea

[Nadav Amit]

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


On Oct 1, 2014, at 6:58 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:

> 2014-09-30 20:49+0300, Nadav Amit:
>> NoBigReal emulation should consider the effective address is between 0 and
>> 0xffff instead of checking the logical address.  Currently there are no
>> instructions which are marked with NoBigReal flag, so this bug currently has no
>> impact.
> 
> (Would be nice if we actually used it :)
> 
>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>> ---
> 
> Should have first looked at git-blame instead of SDM,
> Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

Please don’t apply this patch (only this one - 3/6). 
I observe strange behaviour which still requires debugging. Perhaps Intel SDM is wrong.

Thanks,
Nadav

[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 3/6] KVM: x86: NoBigReal was mistakenly considering la instead of ea

[Radim Krčmář]

2014-10-02 17:52+0300, Nadav Amit:
> > 2014-09-30 20:49+0300, Nadav Amit:
> >> NoBigReal emulation should consider the effective address is between 0 and
> >> 0xffff instead of checking the logical address.
[...]
> Please don’t apply this patch (only this one - 3/6). 
> I observe strange behaviour which still requires debugging. Perhaps Intel SDM is wrong.

Should the original NoBigReal patch be reverted?

Re: [PATCH 3/6] KVM: x86: NoBigReal was mistakenly considering la instead of ea

[Nadav Amit]

On Oct 3, 2014, at 3:50 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:

> 2014-10-02 17:52+0300, Nadav Amit:
>>> 2014-09-30 20:49+0300, Nadav Amit:
>>>> NoBigReal emulation should consider the effective address is between 0 and
>>>> 0xffff instead of checking the logical address.
> [...]
>> Please don’t apply this patch (only this one - 3/6). 
>> I observe strange behaviour which still requires debugging. Perhaps Intel SDM is wrong.
> 
> Should the original NoBigReal patch be reverted?

I think so, but I want to run some tests to make sure.

Nadav

[PATCH 4/6] KVM: x86: Fix determining flat mode in recalculate_apic_map

[Nadav Amit]

Determining flat mode according to cid_mask is wrong, since currently KVM
supports zero clusters in x2apic mode. Use ldr_bits instead.

Since we recalculate the apic map whenever the DFR is changed, the bug appears
to have no effect, and perhaps the entire check can be removed.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/lapic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index b8345dd..cfce429 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -171,7 +171,7 @@ static void recalculate_apic_map(struct kvm *kvm)
 			new->cid_mask = (1 << KVM_X2APIC_CID_BITS) - 1;
 			new->lid_mask = 0xffff;
 		} else if (kvm_apic_sw_enabled(apic) &&
-				!new->cid_mask /* flat mode */ &&
+				new->ldr_bits == 8 /* flat mode */ &&
 				kvm_apic_get_reg(apic, APIC_DFR) == APIC_DFR_CLUSTER) {
 			new->cid_shift = 4;
 			new->cid_mask = 0xf;
-- 
1.9.1

Re: [PATCH 4/6] KVM: x86: Fix determining flat mode in recalculate_apic_map

[Radim Krčmář]

2014-09-30 20:49+0300, Nadav Amit:
> Determining flat mode according to cid_mask is wrong, since currently KVM
> supports zero clusters in x2apic mode. Use ldr_bits instead.

No, it is in else branch of 'if (apic_x2apic_mode(apic))', so it works
as intended now, and ldr_bits == 8 is always true.

> Since we recalculate the apic map whenever the DFR is changed, the bug appears
> to have no effect, and perhaps the entire check can be removed.

It could, for the check is just an optimization.
(This whole code deserves a rewrite though ...)

Re: [PATCH 4/6] KVM: x86: Fix determining flat mode in recalculate_apic_map

[Nadav Amit]

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


On Oct 1, 2014, at 7:04 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:

> 2014-09-30 20:49+0300, Nadav Amit:
>> Determining flat mode according to cid_mask is wrong, since currently KVM
>> supports zero clusters in x2apic mode. Use ldr_bits instead.
> 
> No, it is in else branch of 'if (apic_x2apic_mode(apic))', so it works
> as intended now, and ldr_bits == 8 is always true.
The condition “!new->cid_mask” is certainly always true (unless we already set the cid/lid according to cluster-mode settings).
I did not feel comfortable removing it without replacing it with something “equivalent”.

> 
>> Since we recalculate the apic map whenever the DFR is changed, the bug appears
>> to have no effect, and perhaps the entire check can be removed.
> 
> It could, for the check is just an optimization.
> (This whole code deserves a rewrite though ...)

I did not hit such bug, but IMO the code is buggy.
The APIC mode is determined by processors that enabled their apic enabled (in the spurious vector), and all the enabled one should configure the same mode.
Nonetheless, as stated in the SDM 10.4.7.2 "Local APIC State After It Has Been Software Disabled APICs” - the disabled APICs should still respond to NMIs, INIT, etc.
So it appears the loop should be broken into two loops - first determine the apic mode, according to the first enabled APIC. Then, iterate again over vCPUs and build the logical map.

Regards,
Nadav

[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 4/6] KVM: x86: Fix determining flat mode in recalculate_apic_map

[Radim Krčmář]

2014-10-01 20:30+0300, Nadav Amit:
> On Oct 1, 2014, at 7:04 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:
> > 2014-09-30 20:49+0300, Nadav Amit:
> The condition “!new->cid_mask” is certainly always true (unless we already set the cid/lid according to cluster-mode settings).

Exactly, it "optimizes" the switch to a non-default clustered mode.

> I did not feel comfortable removing it without replacing it with something “equivalent”.
> 
> > 
> >> Since we recalculate the apic map whenever the DFR is changed, the bug appears
> >> to have no effect, and perhaps the entire check can be removed.
> > 
> > It could, for the check is just an optimization.
> > (This whole code deserves a rewrite though ...)
> 
> I did not hit such bug, but IMO the code is buggy.
> The APIC mode is determined by processors that enabled their apic enabled (in the spurious vector), and all the enabled one should configure the same mode.
> Nonetheless, as stated in the SDM 10.4.7.2 "Local APIC State After It Has Been Software Disabled APICs” - the disabled APICs should still respond to NMIs, INIT, etc.
> So it appears the loop should be broken into two loops - first determine the apic mode, according to the first enabled APIC. Then, iterate again over vCPUs and build the logical map.

Our assumption that all have the same mode is horrible.
(Do they all have be the same?)

The only thing we allow out of your scenario that I can see is software
disabled x2apic after enabled clustered xapic processors and that
doesn't need two loops, just a sw check at x2apic.
Practically, it is a harmless bug :)

Re: [PATCH 4/6] KVM: x86: Fix determining flat mode in recalculate_apic_map

[Nadav Amit]

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


On Oct 1, 2014, at 9:27 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:

> 2014-10-01 20:30+0300, Nadav Amit:
>> On Oct 1, 2014, at 7:04 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:
>>> 2014-09-30 20:49+0300, Nadav Amit:
>> The condition “!new->cid_mask” is certainly always true (unless we already set the cid/lid according to cluster-mode settings).
> 
> Exactly, it "optimizes" the switch to a non-default clustered mode.
> 
>> I did not feel comfortable removing it without replacing it with something “equivalent”.
>> 
>>> 
>>>> Since we recalculate the apic map whenever the DFR is changed, the bug appears
>>>> to have no effect, and perhaps the entire check can be removed.
>>> 
>>> It could, for the check is just an optimization.
>>> (This whole code deserves a rewrite though ...)
>> 
>> I did not hit such bug, but IMO the code is buggy.
>> The APIC mode is determined by processors that enabled their apic enabled (in the spurious vector), and all the enabled one should configure the same mode.
>> Nonetheless, as stated in the SDM 10.4.7.2 "Local APIC State After It Has Been Software Disabled APICs” - the disabled APICs should still respond to NMIs, INIT, etc.
>> So it appears the loop should be broken into two loops - first determine the apic mode, according to the first enabled APIC. Then, iterate again over vCPUs and build the logical map.
> 
> Our assumption that all have the same mode is horrible.
> (Do they all have be the same?)
Yes: "All processors that have their APIC software enabled (using the spurious vector enable/disable bit) must have their DFRs (Destination Format Registers) programmed identically."
> 
> The only thing we allow out of your scenario that I can see is software
> disabled x2apic after enabled clustered xapic processors and that
> doesn't need two loops, just a sw check at x2apic.
> Practically, it is a harmless bug :)
So does xsa-108... ;-)
Now seriously: First, the bug may affect certain cases of cpu hot-plug, etc.
Second, there are additional implications. Consider a situation in which the first VCPUs have lapic disabled, and they do not have the same DFR/x2apic mode as the rest of the VCPUs.
This is ok according to the SDM, but in such case, the logical map they would have would not match the spic-mode. Therefore, they may not receive NMIs, INIT, etc. - which they should regardless to the fact their LAPIC is disabled.

Regards,
Nadav

[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 4/6] KVM: x86: Fix determining flat mode in recalculate_apic_map

[Radim Krčmář]

2014-10-01 22:16+0300, Nadav Amit:
> On Oct 1, 2014, at 9:27 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:
> > Our assumption that all have the same mode is horrible.
> > (Do they all have be the same?)
> Yes: "All processors that have their APIC software enabled (using the spurious vector enable/disable bit) must have their DFRs (Destination Format Registers) programmed identically."

Thanks.

> > The only thing we allow out of your scenario that I can see is software
> > disabled x2apic after enabled clustered xapic processors and that
> > doesn't need two loops, just a sw check at x2apic.
> > Practically, it is a harmless bug :)
> So does xsa-108... ;-)
> Now seriously: First, the bug may affect certain cases of cpu hot-plug, etc.
> Second, there are additional implications. Consider a situation in which the first VCPUs have lapic disabled, and they do not have the same DFR/x2apic mode as the rest of the VCPUs.

(I coudn't find anything that would affect the host.)

> This is ok according to the SDM, but in such case, the logical map they would have would not match the spic-mode. Therefore, they may not receive NMIs, INIT, etc. - which they should regardless to the fact their LAPIC is disabled.

The guest wouldn't work, more suprising is that it usually does ;)
I'll rewrite it later if you are pressed for other stuff.

Re: [PATCH 4/6] KVM: x86: Fix determining flat mode in recalculate_apic_map

[Gleb Natapov]

On Tue, Sep 30, 2014 at 08:49:17PM +0300, Nadav Amit wrote:
> Determining flat mode according to cid_mask is wrong, since currently KVM
> supports zero clusters in x2apic mode. Use ldr_bits instead.
As a comment above the 'if' you are fixing says the code assumes all APICs are in
the same mode (if they are not that's an OS bug). In that case the code never gets
here with x2apic mode.

> 
> Since we recalculate the apic map whenever the DFR is changed, the bug appears
> to have no effect, and perhaps the entire check can be removed.
> 
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  arch/x86/kvm/lapic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index b8345dd..cfce429 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -171,7 +171,7 @@ static void recalculate_apic_map(struct kvm *kvm)
>  			new->cid_mask = (1 << KVM_X2APIC_CID_BITS) - 1;
>  			new->lid_mask = 0xffff;
>  		} else if (kvm_apic_sw_enabled(apic) &&
> -				!new->cid_mask /* flat mode */ &&
> +				new->ldr_bits == 8 /* flat mode */ &&
>  				kvm_apic_get_reg(apic, APIC_DFR) == APIC_DFR_CLUSTER) {
>  			new->cid_shift = 4;
>  			new->cid_mask = 0xf;
> -- 
> 1.9.1
> 
> --
> 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

--
			Gleb.

[PATCH 5/6] KVM: x86: Wrong assertion on paging_tmpl.h

[Nadav Amit]

Even after the recent fix, the assertion on paging_tmpl.h is triggered.
Apparently, the assertion wants to check that the PAE is always set on
long-mode, but does it in incorrect way.  Note that the assertion is not
enabled unless the code is debugged by defining MMU_DEBUG.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/paging_tmpl.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 806d58e..faf7298 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -298,7 +298,7 @@ retry_walk:
 	}
 #endif
 	walker->max_level = walker->level;
-	ASSERT(!is_long_mode(vcpu) && is_pae(vcpu));
+	ASSERT(!is_long_mode(vcpu) || is_pae(vcpu));
 
 	accessed_dirty = PT_GUEST_ACCESSED_MASK;
 	pt_access = pte_access = ACC_ALL;
-- 
1.9.1

Re: [PATCH 5/6] KVM: x86: Wrong assertion on paging_tmpl.h

[Radim Krčmář]

2014-09-30 20:49+0300, Nadav Amit:
> Even after the recent fix, the assertion on paging_tmpl.h is triggered.
> Apparently, the assertion wants to check that the PAE is always set on
> long-mode, but does it in incorrect way.  Note that the assertion is not
> enabled unless the code is debugged by defining MMU_DEBUG.

I think it was only supposed to be used together with
  (vcpu->cr3 & CR3_NONPAE_RESERVED_BITS) == 0)
to checked if CR3 does not contain ones where it shouldn't when in short
mode without PAE, because SDM says
  the lower 12 bits of the address are assumed to be 0.
and when we (incorrectly) removed the second part of condition, it
started to bug.

I'd remove the new assert, it does not nothing useful, but is correct
Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

> -	ASSERT(!is_long_mode(vcpu) && is_pae(vcpu));
> +	ASSERT(!is_long_mode(vcpu) || is_pae(vcpu));

Re: [PATCH 5/6] KVM: x86: Wrong assertion on paging_tmpl.h

[Nadav Amit]

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


On Oct 1, 2014, at 7:26 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:

> 2014-09-30 20:49+0300, Nadav Amit:
>> Even after the recent fix, the assertion on paging_tmpl.h is triggered.
>> Apparently, the assertion wants to check that the PAE is always set on
>> long-mode, but does it in incorrect way.  Note that the assertion is not
>> enabled unless the code is debugged by defining MMU_DEBUG.
> 
> I think it was only supposed to be used together with
>  (vcpu->cr3 & CR3_NONPAE_RESERVED_BITS) == 0)
> to checked if CR3 does not contain ones where it shouldn't when in short
> mode without PAE, because SDM says
>  the lower 12 bits of the address are assumed to be 0.
> and when we (incorrectly) removed the second part of condition, it
> started to bug.
> 
> I'd remove the new assert, it does not nothing useful, but is correct
> Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
> 
>> -	ASSERT(!is_long_mode(vcpu) && is_pae(vcpu));
>> +	ASSERT(!is_long_mode(vcpu) || is_pae(vcpu));

I am ok with removing the assertion. Due to the multiple changes, I lost track what it was supposed to do.
Anyhow, removing the second part was required since there are no reserved bits in non-pae (they are ignored - not reserved).

Nadav

[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 5/6] KVM: x86: Wrong assertion on paging_tmpl.h

[Radim Krčmář]

2014-10-01 20:14+0300, Nadav Amit:
> On Oct 1, 2014, at 7:26 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:
> > 2014-09-30 20:49+0300, Nadav Amit:
> >> Even after the recent fix, the assertion on paging_tmpl.h is triggered.
> >> Apparently, the assertion wants to check that the PAE is always set on
> >> long-mode, but does it in incorrect way.  Note that the assertion is not
> >> enabled unless the code is debugged by defining MMU_DEBUG.
> > 
> > I think it was only supposed to be used together with
> >  (vcpu->cr3 & CR3_NONPAE_RESERVED_BITS) == 0)
> > to checked if CR3 does not contain ones where it shouldn't when in short
> > mode without PAE, because SDM says
> >  the lower 12 bits of the address are assumed to be 0.
> > and when we (incorrectly) removed the second part of condition, it
> > started to bug.
> > 
> > I'd remove the new assert, it does not nothing useful, but is correct
> > Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
> > 
> >> -	ASSERT(!is_long_mode(vcpu) && is_pae(vcpu));
> >> +	ASSERT(!is_long_mode(vcpu) || is_pae(vcpu));
> 
> I am ok with removing the assertion. Due to the multiple changes, I lost track what it was supposed to do.

(It didn't say reserved when it was introduced and refactoring was done
 by different author.)

> Anyhow, removing the second part was required since there are no reserved bits in non-pae (they are ignored - not reserved).

Thanks, I thought that "assumed" is "shit will hit the fan unless", and
that this assert made it instant and clear.

Re: [PATCH 5/6] KVM: x86: Wrong assertion on paging_tmpl.h

[Paolo Bonzini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 01/10/2014 19:14, Nadav Amit ha scritto:
>>>>> +	ASSERT(!is_long_mode(vcpu) || is_pae(vcpu));
> I am ok with removing the assertion. Due to the multiple changes, I
> lost track what it was supposed to do. Anyhow, removing the second
> part was required since there are no reserved bits in non-pae (they
> are ignored - not reserved).

It becomes a bit clearer if you apply De Morgan:

        ASSERT(!(is_long_mode(vcpu) && !is_pae(vcpu)));

or almost equivalently

        WARN_ON(is_long_mode(vcpu) && !is_pae(vcpu));

We should change ASSERT to positive logic (MMU_WARN_ON for example).

Paolo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBAgAGBQJUNQEgAAoJEBRUblpOawnX9hwH/AghSII0PKLF1RDC9GqvKBb6
m+UYFmYxcJjidhsvcZuNg/pRuRfJYNOFoJWO13eTYUL/eSnxXEZqy1nQTneFUFjm
WKFebVc5FWc8DAXpEehrHuKUn/QmOEDj8qo41Bf0kHiptzh6W1jIEjH1AaPIwta3
5MFmFN6T+BmU1pBqOgGY5OJkAQnM9WmnsjsFDRJEFW520GP1Xvws+XxRA31Q6Qol
1qLvK2kSeuCUlGDwNWTFT0w79wQwpwuXCJfII5vzRp02pVgDKtl6sNLyKKGthxtv
ONPpn0Uq0mFRbxasPk8glwqaZtqRNJKG+jysSYEf3aBmMhP4hAcGoADP6C5Umj8=
=7S50
-----END PGP SIGNATURE-----

[PATCH 6/6] KVM: x86: Emulator does not calculate address correctly

[Nadav Amit]

In long-mode, when the address size is 4 bytes, the linear address is not
truncated as the emulator mistakenly does.  Instead, the offset within the
segment (the ea field) should be truncated according to the address size.

As Intel SDM says: "In 64-bit mode, the effective address components are added
and the effective address is truncated ... before adding the full 64-bit
segment base."

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/emulate.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 4b687ff..755d703 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -624,7 +624,8 @@ static int __linearize(struct x86_emulate_ctxt *ctxt,
 	u16 sel, error_code = 0;
 	unsigned cpl;
 
-	la = seg_base(ctxt, addr.seg) + addr.ea;
+	la = seg_base(ctxt, addr.seg) +
+	    (fetch || ctxt->ad_bytes == 8 ? addr.ea : (u32)addr.ea);
 	switch (ctxt->mode) {
 	case X86EMUL_MODE_PROT64:
 		if (((signed long)la << 16) >> 16 != la)
@@ -676,7 +677,7 @@ static int __linearize(struct x86_emulate_ctxt *ctxt,
 		}
 		break;
 	}
-	if (fetch ? ctxt->mode != X86EMUL_MODE_PROT64 : ctxt->ad_bytes != 8)
+	if (ctxt->mode != X86EMUL_MODE_PROT64)
 		la &= (u32)-1;
 	if (insn_aligned(ctxt, size) && ((la & (size - 1)) != 0))
 		return emulate_gp(ctxt, 0);
-- 
1.9.1

Re: [PATCH 6/6] KVM: x86: Emulator does not calculate address correctly

[Radim Krčmář]

2014-09-30 20:49+0300, Nadav Amit:
> In long-mode, when the address size is 4 bytes, the linear address is not
> truncated as the emulator mistakenly does.  Instead, the offset within the
> segment (the ea field) should be truncated according to the address size.
> 
> As Intel SDM says: "In 64-bit mode, the effective address components are added
> and the effective address is truncated ... before adding the full 64-bit
> segment base."
> 
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>