How to do fast accesses to LAPIC TPR under kvm?

Commit Message

Stefan Fritsch Oct. 24, 2012, 9:19 a.m. UTC

On Mon, 22 Oct 2012, Avi Kivity wrote:
>>> Right.  You will need to expose the alternate encoding of cr8 (IIRC
>>> lock mov reg, cr0) on AMD via cpuid, but otherwise it should just
>>> work.  Be aware that this will break cross-vendor migration.
>>
>> I get an exception and I am not sure why:
>>
>> kvm_entry: vcpu 0
>> kvm_exit: reason write_cr8 rip 0xd0203788 info 0 0
>> kvm_emulate_insn: 0:d0203788: f0 0f 22 c0 (prot32)
>> kvm_inj_exception: #UD (0x0)
>>
>> This is qemu-kvm 1.1.2 on Linux 3.2.
>>
>> When I look at arch/x86/kvm/emulate.c (both the current and the v3.2
>> version), I don't see any special case handling for "lock mov reg,
>> cr0" to mean "mov reg, cr8".
>
> emulate.c will #UD is the Lock flag is missing in the instruction decode
> table.
>
>> Before I spend lots of time on debugging my code, can you verify if
>> the alternate encoding of cr8 is actually supported in kvm or if it is
>> maybe missing? Thanks in advance.
>
> With the decode table fix I think it should work.

It needs some more changes. The patch below did the trick for me. It is 
against 3.5, because I didn't want to build a whole new kernel (my test 
machine is a dead slow AMD E-350).

The patch is definitely incomplete. It now allows the lock prefix for all 
mov operations on the cr1-7, which should not be the case. Apart from 
that, do the changes look reasonable? I have not checked that this is the 
minimal patch that works. But the LockReg bit was definitely necessary, 
that was the final piece to make it work.

Cheers,
Stefan

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

Comments

Avi Kivity Oct. 25, 2012, 1:34 p.m. UTC | #1

On 10/24/2012 11:19 AM, Stefan Fritsch wrote:
>>
>> With the decode table fix I think it should work.
> 
> It needs some more changes. The patch below did the trick for me. It is
> against 3.5, because I didn't want to build a whole new kernel (my test
> machine is a dead slow AMD E-350).
> 
> The patch is definitely incomplete. It now allows the lock prefix for
> all mov operations on the cr1-7, which should not be the case. Apart
> from that, do the changes look reasonable? I have not checked that this
> is the minimal patch that works. But the LockReg bit was definitely
> necessary, that was the final piece to make it work.
> 
> Cheers,
> Stefan
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 4837375..c7f0ec7 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -128,6 +128,7 @@
>  #define Priv        (1<<27) /* instruction generates #GP if current CPL
> != 0 */
>  #define No64        (1<<28)
>  #define PageTable   (1 << 29)   /* instruction used to write page table */
> +#define LockReg     (1<<30) /* lock prefix is allowed for the
> instruction even for reg destination */
>  /* Source 2 operand type */
>  #define Src2Shift   (30)

LockReg conflicts with Src2Shift.

>  #define Src2None    (OpNone << Src2Shift)
> @@ -420,6 +421,7 @@ static int emulator_check_intercept(struct
> x86_emulate_ctxt *ctxt,
>      struct x86_instruction_info info = {
>          .intercept  = intercept,
>          .rep_prefix = ctxt->rep_prefix,
> +        .lock_prefix = ctxt->lock_prefix,
>          .modrm_mod  = ctxt->modrm_mod,
>          .modrm_reg  = ctxt->modrm_reg,
>          .modrm_rm   = ctxt->modrm_rm,
> @@ -2874,7 +2876,10 @@ static int em_mov(struct x86_emulate_ctxt *ctxt)
> 
>  static int em_cr_write(struct x86_emulate_ctxt *ctxt)
>  {
> -    if (ctxt->ops->set_cr(ctxt, ctxt->modrm_reg, ctxt->src.val))
> +    int cr = ctxt->modrm_reg;

Blank line here.

> +    if (ctxt->lock_prefix && cr == 0)
> +        cr = 8;

But maybe this is better dealt with during general decode, and
ctxt->modrm_reg adjusted instead.  This removes the code triplicstion.
Please also #UD if modrm_reg != 0, and if the feature is not exposed to
the guest via cpuid.

Please regenerate against kvm.git next, there have been changes to
emulate.c.

Patch

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 4837375..c7f0ec7 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -128,6 +128,7 @@ 
  #define Priv        (1<<27) /* instruction generates #GP if current CPL != 0 */
  #define No64	    (1<<28)
  #define PageTable   (1 << 29)   /* instruction used to write page table */
+#define LockReg     (1<<30) /* lock prefix is allowed for the instruction even for reg destination */
  /* Source 2 operand type */
  #define Src2Shift   (30)
  #define Src2None    (OpNone << Src2Shift)
@@ -420,6 +421,7 @@  static int emulator_check_intercept(struct x86_emulate_ctxt *ctxt,
  	struct x86_instruction_info info = {
  		.intercept  = intercept,
  		.rep_prefix = ctxt->rep_prefix,
+		.lock_prefix = ctxt->lock_prefix,
  		.modrm_mod  = ctxt->modrm_mod,
  		.modrm_reg  = ctxt->modrm_reg,
  		.modrm_rm   = ctxt->modrm_rm,
@@ -2874,7 +2876,10 @@  static int em_mov(struct x86_emulate_ctxt *ctxt)

  static int em_cr_write(struct x86_emulate_ctxt *ctxt)
  {
-	if (ctxt->ops->set_cr(ctxt, ctxt->modrm_reg, ctxt->src.val))
+	int cr = ctxt->modrm_reg;
+	if (ctxt->lock_prefix && cr == 0)
+		cr = 8;
+	if (ctxt->ops->set_cr(ctxt, cr, ctxt->src.val))
  		return emulate_gp(ctxt, 0);

  	/* Disable writeback. */
@@ -3177,6 +3182,8 @@  static int check_cr_write(struct x86_emulate_ctxt *ctxt)
  		CR8_RESERVED_BITS,
  	};

+	if (ctxt->lock_prefix && cr == 0)
+		cr = 8;
  	if (!valid_cr(cr))
  		return emulate_ud(ctxt);

@@ -3599,9 +3606,9 @@  static struct opcode twobyte_table[256] = {
  	/* 0x10 - 0x1F */
  	N, N, N, N, N, N, N, N, D(ImplicitOps | ModRM), N, N, N, N, N, N, N,
  	/* 0x20 - 0x2F */
-	DIP(ModRM | DstMem | Priv | Op3264, cr_read, check_cr_read),
+	DIP(Lock | LockReg | ModRM | DstMem | Priv | Op3264, cr_read, check_cr_read),
  	DIP(ModRM | DstMem | Priv | Op3264, dr_read, check_dr_read),
-	IIP(ModRM | SrcMem | Priv | Op3264, em_cr_write, cr_write, check_cr_write),
+	IIP(Lock | LockReg | ModRM | SrcMem | Priv | Op3264, em_cr_write, cr_write, check_cr_write),
  	IIP(ModRM | SrcMem | Priv | Op3264, em_dr_write, dr_write, check_dr_write),
  	N, N, N, N,
  	N, N, N, GP(ModRM | DstMem | SrcReg | Sse | Mov | Aligned, &pfx_vmovntpx),
@@ -4130,7 +4137,8 @@  int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
  	}

  	/* LOCK prefix is allowed only with some instructions */
-	if (ctxt->lock_prefix && (!(ctxt->d & Lock) || ctxt->dst.type != OP_MEM)) {
+	if (ctxt->lock_prefix && (!(ctxt->d & Lock)
+	    || (ctxt->dst.type != OP_MEM && !(ctxt->d & LockReg)))) {
  		rc = emulate_ud(ctxt);
  		goto done;
  	}
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index f75af40..13d4f3b 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -4146,8 +4146,13 @@  static int svm_check_intercept(struct kvm_vcpu *vcpu,
  		unsigned long cr0, val;
  		u64 intercept;

-		if (info->intercept == x86_intercept_cr_write)
-			icpt_info.exit_code += info->modrm_reg;
+		if (info->intercept == x86_intercept_cr_write) {
+			int cr = info->modrm_reg;
+			if (cr == 0 && info->lock_prefix) {
+				cr = 8;
+			}
+			icpt_info.exit_code += cr;
+		}

  		if (icpt_info.exit_code != SVM_EXIT_WRITE_CR0)
  			break;
diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index 15f960c..305d804 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -33,6 +33,7 @@  struct x86_exception {
  struct x86_instruction_info {
  	u8  intercept;          /* which intercept                      */
  	u8  rep_prefix;         /* rep prefix?                          */
+	u8  lock_prefix;        /* lock prefix?                         */
  	u8  modrm_mod;		/* mod part of modrm			*/
  	u8  modrm_reg;          /* index of register used               */
  	u8  modrm_rm;		/* rm part of modrm			*/