Subject: Re: [PATCH v2 3/3] kvm: svm: Use the hardware provided GPA instead of
 page walk
To: Paolo Bonzini <pbonzini@redhat.com>, <kvm@vger.kernel.org>
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>
CC: <brijesh.singh@amd.com>, <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: Brijesh Singh <brijesh.singh@amd.com>
Message-ID: <de0d35c7-1aa1-f7f1-fc59-1020b6982558@amd.com>
Date: Thu, 8 Dec 2016 13:00:26 -0600
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: <9820037d-e3ca-0131-3b04-2e51f2abc883@amd.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Dec 2016 19:00:33.3959 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLUPR12MB0657
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 6412
Lines: 186

Hi Paolo,

On 12/08/2016 09:39 AM, Brijesh Singh wrote:
> Hi Paolo,
>
> On 12/08/2016 08:52 AM, Paolo Bonzini wrote:
>>
>>
>> On 23/11/2016 18:02, Brijesh Singh wrote:
>>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>>
>>> When a guest causes a NPF which requires emulation, KVM sometimes walks
>>> the guest page tables to translate the GVA to a GPA. This is unnecessary
>>> most of the time on AMD hardware since the hardware provides the GPA in
>>> EXITINFO2.
>>>
>>> The only exception cases involve string operations involving rep or
>>> operations that use two memory locations. With rep, the GPA will only be
>>> the value of the initial NPF and with dual memory locations we won't
>>> know
>>> which memory address was translated into EXITINFO2.
>>>
>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>> Reviewed-by: Borislav Petkov <bp@suse.de>
>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>>
>> Tom, Brijesh,
>>
>> I would like to confirm that you have run kvm-unit-tests with this patch?
>> I haven't yet tried AMD (will do before sending the pull request to
>> Linus),
>> but a similar patch for Intel gives me these 4 failures on emulator.flat:
>>
>> FAIL: push mem
>> FAIL: pop mem
>> FAIL: cross-page mmio read
>> FAIL: cross-page mmio write
>>
>
> I did not ran kvm-unit-tests. However, I was able to boot Linux
> guest and run some stress test. I will run kvm-unit-tests and let
> you know soon.
>

I am able to reproduce it on AMD HW using kvm-unit-tests. Looking at 
test, the initial thought is "push mem" has two operands (the memory 
being pushed and the stack pointer). The provided GPA could be either 
one of those.

If we can detect those cases, we should not set the gpa_available on 
them (like what we do with string move).

We probably haven't hit this case in guest booting. Will investigate bit 
further and provide a updated patch to handle it.

> -Brijesh
>> The VMX patch to set gpa_available is just this:
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 25d48380c312..5d7b60d4795b 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -6393,6 +6393,7 @@ static int handle_ept_violation(struct kvm_vcpu
>> *vcpu)
>>      /* ept page table is present? */
>>      error_code |= (exit_qualification & 0x38) != 0;
>>
>> +    vcpu->arch.gpa_available = true;
>>      vcpu->arch.exit_qualification = exit_qualification;
>>
>>      return kvm_mmu_page_fault(vcpu, gpa, error_code, NULL, 0);
>> @@ -6410,6 +6411,7 @@ static int handle_ept_misconfig(struct kvm_vcpu
>> *vcpu)
>>      }
>>
>>      ret = handle_mmio_page_fault(vcpu, gpa, true);
>> +    vcpu->arch.gpa_available = true;
>>      if (likely(ret == RET_MMIO_PF_EMULATE))
>>          return x86_emulate_instruction(vcpu, gpa, 0, NULL, 0) ==
>>                            EMULATE_DONE;
>> @@ -8524,6 +8526,7 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu)
>>      u32 vectoring_info = vmx->idt_vectoring_info;
>>
>>      trace_kvm_exit(exit_reason, vcpu, KVM_ISA_VMX);
>> +    vcpu->arch.gpa_available = false;
>>
>>      /*
>>       * Flush logged GPAs PML buffer, this will make dirty_bitmap more
>>
>> Thanks,
>>
>> Paolo
>>
>>> ---
>>>  arch/x86/include/asm/kvm_emulate.h |    3 +++
>>>  arch/x86/include/asm/kvm_host.h    |    3 +++
>>>  arch/x86/kvm/svm.c                 |    2 ++
>>>  arch/x86/kvm/x86.c                 |   17 ++++++++++++++++-
>>>  4 files changed, 24 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/include/asm/kvm_emulate.h
>>> b/arch/x86/include/asm/kvm_emulate.h
>>> index e9cd7be..2d1ac09 100644
>>> --- a/arch/x86/include/asm/kvm_emulate.h
>>> +++ b/arch/x86/include/asm/kvm_emulate.h
>>> @@ -344,6 +344,9 @@ struct x86_emulate_ctxt {
>>>      struct read_cache mem_read;
>>>  };
>>>
>>> +/* String operation identifier (matches the definition in emulate.c) */
>>> +#define CTXT_STRING_OP    (1 << 13)
>>> +
>>>  /* Repeat String Operation Prefix */
>>>  #define REPE_PREFIX    0xf3
>>>  #define REPNE_PREFIX    0xf2
>>> diff --git a/arch/x86/include/asm/kvm_host.h
>>> b/arch/x86/include/asm/kvm_host.h
>>> index 77cb3f9..fd5b1c8 100644
>>> --- a/arch/x86/include/asm/kvm_host.h
>>> +++ b/arch/x86/include/asm/kvm_host.h
>>> @@ -668,6 +668,9 @@ struct kvm_vcpu_arch {
>>>
>>>      int pending_ioapic_eoi;
>>>      int pending_external_vector;
>>> +
>>> +    /* GPA available (AMD only) */
>>> +    bool gpa_available;
>>>  };
>>>
>>>  struct kvm_lpage_info {
>>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>>> index 5e64e656..1bbd04c 100644
>>> --- a/arch/x86/kvm/svm.c
>>> +++ b/arch/x86/kvm/svm.c
>>> @@ -4246,6 +4246,8 @@ static int handle_exit(struct kvm_vcpu *vcpu)
>>>          return 1;
>>>      }
>>>
>>> +    vcpu->arch.gpa_available = (exit_code == SVM_EXIT_NPF);
>>> +
>>>      return svm_exit_handlers[exit_code](svm);
>>>  }
>>>
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index c30f62dc..5002eea 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -4441,7 +4441,19 @@ static int vcpu_mmio_gva_to_gpa(struct
>>> kvm_vcpu *vcpu, unsigned long gva,
>>>          return 1;
>>>      }
>>>
>>> -    *gpa = vcpu->arch.walk_mmu->gva_to_gpa(vcpu, gva, access,
>>> exception);
>>> +    /*
>>> +     * If the exit was due to a NPF we may already have a GPA.
>>> +     * If the GPA is present, use it to avoid the GVA to GPA table
>>> +     * walk. Note, this cannot be used on string operations since
>>> +     * string operation using rep will only have the initial GPA
>>> +     * from when the NPF occurred.
>>> +     */
>>> +    if (vcpu->arch.gpa_available &&
>>> +        !(vcpu->arch.emulate_ctxt.d & CTXT_STRING_OP))
>>> +        *gpa = exception->address;
>>> +    else
>>> +        *gpa = vcpu->arch.walk_mmu->gva_to_gpa(vcpu, gva, access,
>>> +                               exception);
>>>
>>>      if (*gpa == UNMAPPED_GVA)
>>>          return -1;
>>> @@ -5563,6 +5575,9 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
>>>      }
>>>
>>>  restart:
>>> +    /* Save the faulting GPA (cr2) in the address field */
>>> +    ctxt->exception.address = cr2;
>>> +
>>>      r = x86_emulate_insn(ctxt);
>>>
>>>      if (r == EMULATION_INTERCEPTED)
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>