Re: [PATCH 1/8] KVM: x86: #PF error-code on R/W operations is wrong

2014-12-27 Thread Nadav Amit
Feng feng...@intel.com wrote:

 
 
 -Original Message-
 From: kvm-ow...@vger.kernel.org [mailto:kvm-ow...@vger.kernel.org] On
 Behalf Of Nadav Amit
 Sent: Thursday, December 25, 2014 8:52 AM
 To: pbonz...@redhat.com
 Cc: kvm@vger.kernel.org; Nadav Amit
 Subject: [PATCH 1/8] KVM: x86: #PF error-code on R/W operations is wrong
 
 When emulating an instruction that reads the destination memory operand
 (i.e.,
 instructions without the Mov flag in the emulator), the operand is first 
 read.
 If a page-fault is detected in this phase, the error-code which would be
 delivered to the VM does not indicate that the access that caused the
 exception
 is a write one. This does not conform with real hardware, and may cause the
 VM
 to enter the page-fault handler twice for no reason (once for read, once for
 write).
 
 Signed-off-by: Nadav Amit na...@cs.technion.ac.il
 ---
 arch/x86/include/asm/kvm_host.h | 12 
 arch/x86/kvm/emulate.c  |  6 +-
 arch/x86/kvm/mmu.h  | 12 
 3 files changed, 17 insertions(+), 13 deletions(-)
 
 diff --git a/arch/x86/include/asm/kvm_host.h
 b/arch/x86/include/asm/kvm_host.h
 index 73ccb12..d6f90ca 100644
 --- a/arch/x86/include/asm/kvm_host.h
 +++ b/arch/x86/include/asm/kvm_host.h
 @@ -162,6 +162,18 @@ enum {
 #define DR7_FIXED_1  0x0400
 #define DR7_VOLATILE 0x2bff
 
 +#define PFERR_PRESENT_BIT 0
 +#define PFERR_WRITE_BIT 1
 +#define PFERR_USER_BIT 2
 +#define PFERR_RSVD_BIT 3
 +#define PFERR_FETCH_BIT 4
 +
 +#define PFERR_PRESENT_MASK (1U  PFERR_PRESENT_BIT)
 +#define PFERR_WRITE_MASK (1U  PFERR_WRITE_BIT)
 +#define PFERR_USER_MASK (1U  PFERR_USER_BIT)
 +#define PFERR_RSVD_MASK (1U  PFERR_RSVD_BIT)
 +#define PFERR_FETCH_MASK (1U  PFERR_FETCH_BIT)
 +
 /* apic attention bits */
 #define KVM_APIC_CHECK_VAPIC 0
 /*
 diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
 index 7a9697f..e5a84be 100644
 --- a/arch/x86/kvm/emulate.c
 +++ b/arch/x86/kvm/emulate.c
 @@ -4941,8 +4941,12 @@ int x86_emulate_insn(struct x86_emulate_ctxt
 *ctxt)
  /* optimisation - avoid slow emulated read if Mov */
  rc = segmented_read(ctxt, ctxt-dst.addr.mem,
 ctxt-dst.val, ctxt-dst.bytes);
 
 This is a write operation, do you know why we need to read the dst operand 
 first here?
Some x86 instructions read the destination operand during their operation.

For instance - MOV [EAX], EBX (Intel ASM format), would perform 
[EAX] = [EAX] + EBX. Therefore, it would first read the memory of [EAX] at
this stage.

Nadav

--
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 1/8] KVM: x86: #PF error-code on R/W operations is wrong

2014-12-25 Thread Wu, Feng


 -Original Message-
 From: kvm-ow...@vger.kernel.org [mailto:kvm-ow...@vger.kernel.org] On
 Behalf Of Nadav Amit
 Sent: Thursday, December 25, 2014 8:52 AM
 To: pbonz...@redhat.com
 Cc: kvm@vger.kernel.org; Nadav Amit
 Subject: [PATCH 1/8] KVM: x86: #PF error-code on R/W operations is wrong
 
 When emulating an instruction that reads the destination memory operand
 (i.e.,
 instructions without the Mov flag in the emulator), the operand is first read.
 If a page-fault is detected in this phase, the error-code which would be
 delivered to the VM does not indicate that the access that caused the
 exception
 is a write one. This does not conform with real hardware, and may cause the
 VM
 to enter the page-fault handler twice for no reason (once for read, once for
 write).
 
 Signed-off-by: Nadav Amit na...@cs.technion.ac.il
 ---
  arch/x86/include/asm/kvm_host.h | 12 
  arch/x86/kvm/emulate.c  |  6 +-
  arch/x86/kvm/mmu.h  | 12 
  3 files changed, 17 insertions(+), 13 deletions(-)
 
 diff --git a/arch/x86/include/asm/kvm_host.h
 b/arch/x86/include/asm/kvm_host.h
 index 73ccb12..d6f90ca 100644
 --- a/arch/x86/include/asm/kvm_host.h
 +++ b/arch/x86/include/asm/kvm_host.h
 @@ -162,6 +162,18 @@ enum {
  #define DR7_FIXED_1  0x0400
  #define DR7_VOLATILE 0x2bff
 
 +#define PFERR_PRESENT_BIT 0
 +#define PFERR_WRITE_BIT 1
 +#define PFERR_USER_BIT 2
 +#define PFERR_RSVD_BIT 3
 +#define PFERR_FETCH_BIT 4
 +
 +#define PFERR_PRESENT_MASK (1U  PFERR_PRESENT_BIT)
 +#define PFERR_WRITE_MASK (1U  PFERR_WRITE_BIT)
 +#define PFERR_USER_MASK (1U  PFERR_USER_BIT)
 +#define PFERR_RSVD_MASK (1U  PFERR_RSVD_BIT)
 +#define PFERR_FETCH_MASK (1U  PFERR_FETCH_BIT)
 +
  /* apic attention bits */
  #define KVM_APIC_CHECK_VAPIC 0
  /*
 diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
 index 7a9697f..e5a84be 100644
 --- a/arch/x86/kvm/emulate.c
 +++ b/arch/x86/kvm/emulate.c
 @@ -4941,8 +4941,12 @@ int x86_emulate_insn(struct x86_emulate_ctxt
 *ctxt)
   /* optimisation - avoid slow emulated read if Mov */
   rc = segmented_read(ctxt, ctxt-dst.addr.mem,
  ctxt-dst.val, ctxt-dst.bytes);

This is a write operation, do you know why we need to read the dst operand 
first here?

Thanks,
Feng

 - if (rc != X86EMUL_CONTINUE)
 + if (rc != X86EMUL_CONTINUE) {
 + if (rc == X86EMUL_PROPAGATE_FAULT 
 + ctxt-exception.vector == PF_VECTOR)
 + ctxt-exception.error_code |= PFERR_WRITE_MASK;
   goto done;
 + }
   }
   ctxt-dst.orig_val = ctxt-dst.val;
 
 diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
 index 6b34876b..daae711 100644
 --- a/arch/x86/kvm/mmu.h
 +++ b/arch/x86/kvm/mmu.h
 @@ -44,18 +44,6 @@
  #define PT_DIRECTORY_LEVEL 2
  #define PT_PAGE_TABLE_LEVEL 1
 
 -#define PFERR_PRESENT_BIT 0
 -#define PFERR_WRITE_BIT 1
 -#define PFERR_USER_BIT 2
 -#define PFERR_RSVD_BIT 3
 -#define PFERR_FETCH_BIT 4
 -
 -#define PFERR_PRESENT_MASK (1U  PFERR_PRESENT_BIT)
 -#define PFERR_WRITE_MASK (1U  PFERR_WRITE_BIT)
 -#define PFERR_USER_MASK (1U  PFERR_USER_BIT)
 -#define PFERR_RSVD_MASK (1U  PFERR_RSVD_BIT)
 -#define PFERR_FETCH_MASK (1U  PFERR_FETCH_BIT)
 -
  static inline u64 rsvd_bits(int s, int e)
  {
   return ((1ULL  (e - s + 1)) - 1)  s;
 --
 1.9.1
 
 --
 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
--
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