Re: [PATCH v2 3/3] kvm: svm: Use the hardware provided GPA instead of page walk

[Brijesh Singh <brijesh.singh@amd.com>]

Hi Paolo,

On 12/13/2016 11:09 AM, Paolo Bonzini wrote:
>
>
> On 12/12/2016 18:51, Brijesh Singh wrote:
>> As per the AMD BKDG [1] Section 2.7.1, we should not be using any of
>> these instruction for MMIO access, the behavior is undefined.
>>
>> The question is, do we really need to add logic to detect the cross-page
>> MMIO accesses and push/pop mem operations so that we pass the
>> kvm-unit-test or we should update the unit test? Like you said
>> cross-page MMIO access detection is going to be a bit tricky.
>
> Actually there is a nice trick you can do to support cross-page
> MMIO access detection:
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 37cd31645d45..754d251dc611 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4549,6 +4549,7 @@ static int emulator_read_write_onepage(unsigned long addr, void *val,
>  	 */
>  	if (vcpu->arch.gpa_available &&
>  	    !emulator_is_string_op(ctxt) &&
> +	    (addr & ~PAGE_MASK) == (exception->address & ~PAGE_MASK) &&
>  	    vcpu_is_mmio_gpa(vcpu, addr, exception->address, write)) {
>  		gpa = exception->address;
>  		goto mmio;
>
>
> It fixes the testcase for push/pop with two memory ops too,
> but it's not reliable, so your change for TwoMemOp is still
> necessary.  Feel free to include it in your patch!
>
> Regarding the replacement of emulator_is_string_op with
> emulator_is_two_memory_op, what about REP prefixes?  In that
> case I think that you do need to reject string ops.  So the
> function would have to reject all TwoMemOps, and REP-prefixed
> String operations.
>

Since now we are going to perform multiple conditional checks before 
concluding that its safe to use HW provided GPA. How about if we add two 
functions "emulator_is_rep_string_op" and "emulator_is_two_mem_op" into 
emulator.c and  use these functions inside the x86.c to determine if its 
safe to use HW provided gpa?

Please let me know if you are okay with this approach.

diff --git a/arch/x86/include/asm/kvm_emulate.h 
b/arch/x86/include/asm/kvm_emulate.h
index 777eea2..29e44cb 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -441,6 +441,7 @@ int emulator_task_switch(struct x86_emulate_ctxt *ctxt,
  int emulate_int_real(struct x86_emulate_ctxt *ctxt, int irq);
  void emulator_invalidate_register_cache(struct x86_emulate_ctxt *ctxt);
  void emulator_writeback_register_cache(struct x86_emulate_ctxt *ctxt);
-bool emulator_is_string_op(struct x86_emulate_ctxt *ctxt);
+bool emulator_is_rep_string_op(struct x86_emulate_ctxt *ctxt);
+bool emulator_is_two_mem_op(struct x86_emulate_ctxt *ctxt);

  #endif /* _ASM_X86_KVM_X86_EMULATE_H */
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 8e7d09f..16149ad 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -5485,10 +5485,12 @@ void emulator_writeback_register_cache(struct 
x86_emulate_ctxt *ctxt)
         writeback_registers(ctxt);
  }

-bool emulator_is_string_op(struct x86_emulate_ctxt *ctxt)
+bool emulator_is_rep_string_op(struct x86_emulate_ctxt *ctxt)
  {
-       if (ctxt->d & String)
-               return true;
+       return ctxt->rep_prefix && (ctxt->d & String) ? true: false;
+}

-       return false;
+bool emulator_is_two_mem_op(struct x86_emulate_ctxt *ctxt)
+{
+       return ctxt->d & TwoMemOp ? true : false;
  }
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 640527b..0bc814a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4548,6 +4548,12 @@ static const struct read_write_emulator_ops 
write_emultor = {
         .write = true,
  };

+static bool emulator_can_use_gpa(struct x86_emulate_ctxt *ctxt)
+{
+       return emulator_is_rep_string_op(ctxt) &&
+               emulator_is_two_mem_op(ctxt) ? true : false;
+}
+
  static int emulator_read_write_onepage(unsigned long addr, void *val,
                                        unsigned int bytes,
                                        struct x86_exception *exception,
@@ -4568,7 +4574,7 @@ static int emulator_read_write_onepage(unsigned 
long addr, void *val,
          * occurred.
          */
         if (vcpu->arch.gpa_available &&
-           !emulator_is_string_op(ctxt) &&
+           !emulator_can_use_gpa(ctxt) &&
             vcpu_is_mmio_gpa(vcpu, addr, exception->address, write) &&
             (addr & ~PAGE_MASK) == (exception->address & ~PAGE_MASK)) {