Subject: Re: [PATCH v2 3/3] kvm: svm: Use the hardware provided GPA instead of
 page walk
To: Brijesh Singh <brijesh.singh@amd.com>
References: <147992048887.27638.17559991037474542240.stgit@brijesh-build-machine>
 <147992052008.27638.18095073174935903705.stgit@brijesh-build-machine>
 <65a10dd8-5fae-350f-b597-f8f0261da766@redhat.com>
 <9820037d-e3ca-0131-3b04-2e51f2abc883@amd.com>
 <de0d35c7-1aa1-f7f1-fc59-1020b6982558@amd.com>
 <657442146.2535029.1481298111651.JavaMail.zimbra@redhat.com>
 <6e1fd4ba-016c-99ea-5b98-24f89479da4b@amd.com>
Cc: kvm@vger.kernel.org, thomas lendacky <thomas.lendacky@amd.com>,
        rkrcmar@redhat.com, joro@8bytes.org, x86@kernel.org,
        linux-kernel@vger.kernel.org, mingo@redhat.com, hpa@zytor.com,
        tglx@linutronix.de, bp@suse.de
From: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <057d8a2e-0a3a-38d3-d9bf-9301e3eb8238@redhat.com>
Date: Tue, 13 Dec 2016 18:09:57 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.5.1
MIME-Version: 1.0
In-Reply-To: <6e1fd4ba-016c-99ea-5b98-24f89479da4b@amd.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1465
Lines: 39


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