From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751962AbdB0Lkg (ORCPT ); Mon, 27 Feb 2017 06:40:36 -0500 Received: from mail-yw0-f194.google.com ([209.85.161.194]:34299 "EHLO mail-yw0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751660AbdB0Lkd (ORCPT ); Mon, 27 Feb 2017 06:40:33 -0500 MIME-Version: 1.0 In-Reply-To: <58B00E12.5020602@arm.com> References: <1487712121-16688-1-git-send-email-tbaicar@codeaurora.org> <1487712121-16688-11-git-send-email-tbaicar@codeaurora.org> <58B00E12.5020602@arm.com> From: gengdongjiu Date: Mon, 27 Feb 2017 19:31:21 +0800 Message-ID: Subject: Re: [PATCH V11 10/10] arm/arm64: KVM: add guest SEA support To: James Morse Cc: Tyler Baicar , christoffer.dall@linaro.org, Marc Zyngier , pbonzini@redhat.com, rkrcmar@redhat.com, linux@armlinux.org.uk, catalin.marinas@arm.com, will.deacon@arm.com, rjw@rjwysocki.net, lenb@kernel.org, matt@codeblueprint.co.uk, robert.moore@intel.com, lv.zheng@intel.com, nkaje@codeaurora.org, zjzhang@codeaurora.org, mark.rutland@arm.com, akpm@linux-foundation.org, eun.taik.lee@samsung.com, sandeepa.s.prabhu@gmail.com, labbott@redhat.com, shijie.huang@arm.com, rruigrok@codeaurora.org, paul.gortmaker@windriver.com, tn@semihalf.com, fu.wei@linaro.org, rostedt@goodmis.org, bristot@redhat.com, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org, linux-efi@vger.kernel.org, devel@acpica.org, Suzuki.Poulose@arm.com, punit.agrawal@arm.com, astone@redhat.com, harba@codeaurora.org, hanjun.guo@linaro.org, john.garry@huawei.com, shiju.jose@huawei.com, joe@perches.com, gengdongjiu@huawei.com Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org @@ -1444,8 +1445,21 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run) /* Check the stage-2 fault is trans. fault or write fault */ fault_status = kvm_vcpu_trap_get_fault_type(vcpu); - if (fault_status != FSC_FAULT && fault_status != FSC_PERM && - fault_status != FSC_ACCESS) { + + /* The host kernel will handle the synchronous external abort. There + * is no need to pass the error into the guest. + */ + if (fault_status == FSC_EXTABT) { + if(handle_guest_sea((unsigned long)fault_ipa, + kvm_vcpu_get_hsr(vcpu))) { + kvm_err("Failed to handle guest SEA, FSC: EC=%#x xFSC=%#lx ESR_EL2=%#lx\n", + kvm_vcpu_trap_get_class(vcpu), + (unsigned long)kvm_vcpu_trap_get_fault(vcpu), + (unsigned long)kvm_vcpu_get_hsr(vcpu)); + return -EFAULT; + } + } else if (fault_status != FSC_FAULT && fault_status != FSC_PERM && + fault_status != FSC_ACCESS) { kvm_err("Unsupported FSC: EC=%#x xFSC=%#lx ESR_EL2=%#lx\n", kvm_vcpu_trap_get_class(vcpu), (unsigned long)kvm_vcpu_trap_get_fault(vcpu), if the error is SEA and we want to inject the sea to guest OK, after finish the handle, whether we need to directly return? instead of continuation? as shown below: if (fault_status == FSC_EXTABT) { if(handle_guest_sea((unsigned long)fault_ipa, kvm_vcpu_get_hsr(vcpu))) { kvm_err("Failed to handle guest SEA, FSC: EC=%#x xFSC=%#lx ESR_EL2=%#lx\n", kvm_vcpu_trap_get_class(vcpu), (unsigned long)kvm_vcpu_trap_get_fault(vcpu), (unsigned long)kvm_vcpu_get_hsr(vcpu)); return -EFAULT; } else return 1; 2017-02-24 18:42 GMT+08:00 James Morse : > Hi Tyler, > > On 21/02/17 21:22, Tyler Baicar wrote: >> Currently external aborts are unsupported by the guest abort >> handling. Add handling for SEAs so that the host kernel reports >> SEAs which occur in the guest kernel. > >> diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h >> index e22089f..33a77509 100644 >> --- a/arch/arm/include/asm/kvm_arm.h >> +++ b/arch/arm/include/asm/kvm_arm.h >> @@ -187,6 +187,7 @@ >> #define FSC_FAULT (0x04) >> #define FSC_ACCESS (0x08) >> #define FSC_PERM (0x0c) >> +#define FSC_EXTABT (0x10) > > arm64 has ESR_ELx_FSC_EXTABT which is used in inject_abt64(), but for matching > an external abort coming from hardware the range is wider. > > Looking at the ARM-ARMs 'ISS encoding for an exception from an Instruction > Abort' in 'D7.2.27 ESR_ELx, Exception Syndrome Register (ELx)' (page D7-1954 of > version 'k'...iss10775), the ten flavours of you Synchronous abort you hooked > with do_sea() in patch 4 occupy 0x10 to 0x1f... > > >> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c >> index a5265ed..04f1dd50 100644 >> --- a/arch/arm/kvm/mmu.c >> +++ b/arch/arm/kvm/mmu.c >> @@ -29,6 +29,7 @@ >> #include >> #include >> #include >> +#include >> >> #include "trace.h" >> >> @@ -1444,8 +1445,21 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run) >> >> /* Check the stage-2 fault is trans. fault or write fault */ >> fault_status = kvm_vcpu_trap_get_fault_type(vcpu); > > ... kvm_vcpu_trap_get_fault_type() on both arm and arm64 masks the HSR/ESR_EL2 > with 0x3c ... > > >> - if (fault_status != FSC_FAULT && fault_status != FSC_PERM && >> - fault_status != FSC_ACCESS) { >> + >> + /* The host kernel will handle the synchronous external abort. There >> + * is no need to pass the error into the guest. >> + */ >> + if (fault_status == FSC_EXTABT) { > > ... but here we only check for 'Synchronous external abort, not on a translation > table walk'. Are the other types relevant? > > If so we need some helper as this range is sparse and 'all other values are > reserved'. The aarch32 HSR format is slightly different. (G6-4411 ISS encoding > from an exception from a Data Abort). > > If not, can we change patch 4 to check this type too so we don't call out to > APEI for a fault type we know isn't relevant. > > >> + if(handle_guest_sea((unsigned long)fault_ipa, >> + kvm_vcpu_get_hsr(vcpu))) { >> + kvm_err("Failed to handle guest SEA, FSC: EC=%#x xFSC=%#lx ESR_EL2=%#lx\n", >> + kvm_vcpu_trap_get_class(vcpu), >> + (unsigned long)kvm_vcpu_trap_get_fault(vcpu), >> + (unsigned long)kvm_vcpu_get_hsr(vcpu)); >> + return -EFAULT; >> + } >> + } else if (fault_status != FSC_FAULT && fault_status != FSC_PERM && >> + fault_status != FSC_ACCESS) { >> kvm_err("Unsupported FSC: EC=%#x xFSC=%#lx ESR_EL2=%#lx\n", >> kvm_vcpu_trap_get_class(vcpu), >> (unsigned long)kvm_vcpu_trap_get_fault(vcpu), > >> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c >> index b2d57fc..403277b 100644 >> --- a/arch/arm64/mm/fault.c >> +++ b/arch/arm64/mm/fault.c >> @@ -602,6 +602,24 @@ static const char *fault_name(unsigned int esr) >> } >> >> /* >> + * Handle Synchronous External Aborts that occur in a guest kernel. >> + */ >> +int handle_guest_sea(unsigned long addr, unsigned int esr) >> +{ > >> + if(IS_ENABLED(HAVE_ACPI_APEI_SEA)) { >> + nmi_enter(); >> + ghes_notify_sea(); >> + nmi_exit(); > > This nmi stuff was needed for synchronous aborts that may have interrupted > APEI's interrupts-masked code. We want to avoid trying to take the same set of > locks, hence taking the in_nmi() path through APEI. Here we know we interrupted > a guest, so there is no risk that we have interrupted APEI on the host. > ghes_notify_sea() can safely take the normal path. > > > Thanks, > > James