Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1541298 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 3/3] kvm: svm: Use the hardware provided GPA instead of  page walk Paolo Bonzini <pbonzini@redhat.com> - 2016-12-13 18:20 +0100
    Re: [PATCH v2 3/3] kvm: svm: Use the hardware provided GPA instead of  page walk Paolo Bonzini <pbonzini@redhat.com> - 2016-12-14 18:30 +0100
      Re: [PATCH v2 3/3] kvm: svm: Use the hardware provided GPA instead of  page walk Paolo Bonzini <pbonzini@redhat.com> - 2016-12-14 19:50 +0100

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

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.

Paolo

[toc] | [next] | [standalone]

#1542073

On 14/12/2016 18:07, Brijesh Singh wrote:
>>
> 
> 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?

Why not export only emulator_can_use_gpa from emulate.c?  (So in the end
leaving emulator_is_string_op in emulate.c was the right thing to do, it
was just the test that was wrong :)).

The patch below is still missing the check for cross-page MMIO.  Your
reference to the BKDG only covers MMCONFIG (sometimes referred to as
ECAM), not MMIO in general.  Doing AND or OR into video memory for
example is perfectly legal, and I'm fairly sure that some obscure legacy
software does PUSH/POP into vram as well!

Thanks,

Paolo

[toc] | [prev] | [next] | [standalone]

#1542114

On 14/12/2016 19:39, Brijesh Singh wrote:
> 
> On 12/14/2016 11:23 AM, Paolo Bonzini wrote:
>>
>>
>> On 14/12/2016 18:07, Brijesh Singh wrote:
>>>>
>>>
>>> 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?
>>
>> Why not export only emulator_can_use_gpa from emulate.c?  (So in the end
>> leaving emulator_is_string_op in emulate.c was the right thing to do, it
>> was just the test that was wrong :)).
>>
> 
> Actually, I was not sure if putting emulator_can_use_gpa() in emulate.c
> was right thing - mainly because emulator.c does not deal with GPA. I
> will go with your advice and put it in emulator.c, it makes easy :)
> 
> 
>> The patch below is still missing the check for cross-page MMIO.  Your
>> reference to the BKDG only covers MMCONFIG (sometimes referred to as
>> ECAM), not MMIO in general.  Doing AND or OR into video memory for
>> example is perfectly legal, and I'm fairly sure that some obscure legacy
>> software does PUSH/POP into vram as well!
>>
>>
> 
> I used your below code snippet to detect cross-page MMIO access. After
> applying these changes cross-page MMIO read/write unit test is passing
> just fine. I will include it in patch.

Great, thanks.  I hope we can include it in 4.10.

Paolo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web