From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1431418AbdDYOEE (ORCPT ); Tue, 25 Apr 2017 10:04:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38188 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1031433AbdDYOD4 (ORCPT ); Tue, 25 Apr 2017 10:03:56 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com F3C208123A Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=rkrcmar@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com F3C208123A Date: Tue, 25 Apr 2017 16:03:52 +0200 From: Radim =?utf-8?B?S3LEjW3DocWZ?= To: Brijesh Singh Cc: pbonzini@redhat.com, joro@8bytes.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com, x86@kernel.org, Thomas.Lendacky@amd.com Subject: Re: [PATCH] x86: kvm: Avoid guest page table walk when gpa_available is set Message-ID: <20170425140351.GF5713@potion> References: <1493049146-19261-1-git-send-email-brijesh.singh@amd.com> <20170424205236.GE5713@potion> <77f51978-5937-0c94-13b6-885345921b03@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <77f51978-5937-0c94-13b6-885345921b03@amd.com> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Tue, 25 Apr 2017 14:03:56 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 2017-04-24 17:14-0500, Brijesh Singh: >> > /* GPA available (AMD only) */ >> > bool gpa_available; >> > + gpa_t gpa_val; >> >> Can't we pass this information through function parameters? >> >> (I'd rather avoid intractable variables.) >> > > I also wanted to avoid adding yet another variable but we can't depend on > cr2 parameters passed into x86_emulate_instruction(). > > The x86_emulate_instruction() function is called from two places: > > 1) handling the page-fault. > pf_interception [svm.c] > kvm_mmu_page_fault [mmu.c] > x86_emulate_instruction [x86.c] > > 2) completing the IO/MMIO's from previous instruction decode > kvm_arch_vcpu_ioctl_run > complete_emulated_io > emulate_instruction > x86_emulate_instruction(vcpu, 0, emulation_type, NULL, 0) > > In #1, we are guaranteed that cr2 variable will contain a valid GPA but > in #2, CR2 is set to zero. We are setting up the completion in #1 x86_emulate_instruction(), where the gpa (cr2) is available, so we could store the value while arming vcpu->arch.complete_userspace_io. emulator_read_write_onepage() already saves gpa in frag->gpa, which is then passed into complete_emulated_mmio -- isn't that mechanism sufficient? >> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c >> > @@ -4159,6 +4159,10 @@ static int handle_exit(struct kvm_vcpu *vcpu) >> > >> > vcpu->arch.gpa_available = (exit_code == SVM_EXIT_NPF); >> > >> > + /* On #NPF, exit_info_2 contain a valid GPA */ >> > + if (vcpu->arch.gpa_available) >> > + vcpu->arch.gpa_val = svm->vmcb->control.exit_info_2; >> >> How is vcpu->arch.gpa_val used between here and the NPF handler? >> > > handle_exit [svm.c] > pf_interception [svm.c] > /* it invokes the fault handler with CR2 = svm->vmcb->control.exit_info_2 */ > kvm_mmu_page_fault [mmu.c] > x86_emulate_instruction [x86.c] > emulator_read_write_onepage [x86.c] > /* > *this is where we walk the guest page table to translate > * a GVA to GPA. If gpa_available is set then we use the > * gpa_val instead of walking the pgtable. > */ pf_interception is the NPF exit handler -- please move the setting there, at least. handle_exit() is a hot path that shouldn't contain code that isn't applicable to all exits. Btw. we need some other guarantees to declare it as GPA (cr2 is GPA in NPT exits, but might not be in other) ... isn't arch.mmu.direct_map a condition we are interested in? The other code uses it to interpret cr2 directly as gpa, so we might be able to avoid setting the arch.gpa_available in a hot path too. >> > @@ -5675,8 +5673,14 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, >> > } >> > >> > restart: >> > - /* Save the faulting GPA (cr2) in the address field */ >> > - ctxt->exception.address = cr2; >> > + /* >> > + * Save the faulting GPA (cr2) in the address field >> > + * NOTE: If gpa_available is set then gpa_val will contain a valid GPA >> > + */ >> > + if (vcpu->arch.gpa_available) >> > + ctxt->exception.address = vcpu->arch.gpa_val; >> > + else >> > + ctxt->exception.address = cr2; >> >> And related, shouldn't vcpu->arch.gpa_val be in cr2? >> > > See my previous comment. In some cases CR2 may be set to zero > (e.g when completing the instruction from previous io/mmio page-fault). > > If we are decide to add the gpa_val then we can remove above if > statement from x86_emulate_instruction() and update emulator_read_write_onepage > to use the vcpu->arch.gpa_val instead of exception->address. Yeah, that would be nicer than setting exception->address at a random place. We could also pass the value as cr2 in all cases if it made something better. > if (vcpu->arch.gpa_available && > emulator_can_use_gpa(ctxt) && > (addr & ~PAGE_MASK) == (exception->address & ~PAGE_MASK)) { > gpa = vcpu=>arch.gpa_val; > ... > ... > } If at all possible, I'd like to have the gpa passed with other relevant data, instead of having it isolated like this ... and we can't manage that, then at least good benchmark results to excuse the bad code. Thanks.