Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752820AbcJNJl0 (ORCPT ); Fri, 14 Oct 2016 05:41:26 -0400 Received: from fw-tnat.cambridge.arm.com ([217.140.96.140]:57285 "EHLO cam-smtp0.cambridge.arm.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753865AbcJNJkb (ORCPT ); Fri, 14 Oct 2016 05:40:31 -0400 From: Punit Agrawal To: "Baicar\, Tyler" Cc: linux-efi@vger.kernel.org, matt@codeblueprint.co.uk, catalin.marinas@arm.com, will.deacon@arm.com, robert.moore@intel.com, paul.gortmaker@windriver.com, lv.zheng@intel.com, kvmarm@lists.cs.columbia.edu, fu.wei@linaro.org, linux@armlinux.org.uk, linux-acpi@vger.kernel.org, shijie.huang@arm.com, lenb@kernel.org, marc.zyngier@arm.com, tomasz.nowicki@linaro.org, rostedt@goodmis.org, sandeepa.s.prabhu@gmail.com, Dkvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, devel@acpica.org, rjw@rjwysocki.net, linux-kernel@vger.kernel.org, pbonzini@redhat.com, akpm@linux-foundation.org, bristot@redhat.com Subject: Re: [PATCH V3 10/10] arm64: KVM: add guest SEA support References: <1475875882-2604-1-git-send-email-tbaicar@codeaurora.org> <1475875882-2604-11-git-send-email-tbaicar@codeaurora.org> <87h98gs853.fsf@e105922-lin.cambridge.arm.com> Date: Fri, 14 Oct 2016 10:38:16 +0100 In-Reply-To: (Tyler Baicar's message of "Thu, 13 Oct 2016 14:14:27 -0600") Message-ID: <878ttrs22f.fsf@e105922-lin.cambridge.arm.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6144 Lines: 155 "Baicar, Tyler" writes: > Hello Punit, > > On 10/13/2016 7:14 AM, Punit Agrawal wrote: >> Hi Tyler, >> >> I know I've had my last comment already ;), but I thought I'd rather >> raise the question than stay confused... >> >> Tyler Baicar writes: >> >>> 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. >>> >>> Signed-off-by: Tyler Baicar >>> --- >>> arch/arm/include/asm/kvm_arm.h | 1 + >>> arch/arm/include/asm/system_misc.h | 5 +++++ >>> arch/arm/kvm/mmu.c | 15 +++++++++++++-- >>> arch/arm64/include/asm/kvm_arm.h | 1 + >>> arch/arm64/include/asm/system_misc.h | 2 ++ >>> arch/arm64/mm/fault.c | 13 +++++++++++++ >>> 6 files changed, 35 insertions(+), 2 deletions(-) >>> >>> 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) >>> /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */ >>> #define HPFAR_MASK (~0xf) >>> diff --git a/arch/arm/include/asm/system_misc.h b/arch/arm/include/asm/system_misc.h >>> index a3d61ad..86e1faa 100644 >>> --- a/arch/arm/include/asm/system_misc.h >>> +++ b/arch/arm/include/asm/system_misc.h >>> @@ -24,4 +24,9 @@ extern unsigned int user_debug; >>> #endif /* !__ASSEMBLY__ */ >>> +inline int handle_guest_sea(unsigned long addr, unsigned int >>> esr) >>> +{ >>> + return -1; >>> +} >>> + >>> #endif /* __ASM_ARM_SYSTEM_MISC_H */ >>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c >>> index e9a5c0e..24cde84 100644 >>> --- a/arch/arm/kvm/mmu.c >>> +++ b/arch/arm/kvm/mmu.c >>> @@ -29,6 +29,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> #include "trace.h" >>> @@ -1441,8 +1442,18 @@ 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) { >>> + >>> + 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), >>> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h >>> index 4b5c977..be0efb6 100644 >>> --- a/arch/arm64/include/asm/kvm_arm.h >>> +++ b/arch/arm64/include/asm/kvm_arm.h >>> @@ -201,6 +201,7 @@ >>> #define FSC_FAULT ESR_ELx_FSC_FAULT >>> #define FSC_ACCESS ESR_ELx_FSC_ACCESS >>> #define FSC_PERM ESR_ELx_FSC_PERM >>> +#define FSC_EXTABT ESR_ELx_FSC_EXTABT >>> /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */ >>> #define HPFAR_MASK (~UL(0xf)) >>> diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h >>> index 90daf4a..8522fff 100644 >>> --- a/arch/arm64/include/asm/system_misc.h >>> +++ b/arch/arm64/include/asm/system_misc.h >>> @@ -77,4 +77,6 @@ extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd); >>> int sea_register_handler_chain(struct notifier_block *nb); >>> void sea_unregister_handler_chain(struct notifier_block *nb); >>> +int handle_guest_sea(unsigned long addr, unsigned int esr); >>> + >>> #endif /* __ASM_SYSTEM_MISC_H */ >>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c >>> index 81cb7ad..d714432 100644 >>> --- a/arch/arm64/mm/fault.c >>> +++ b/arch/arm64/mm/fault.c >>> @@ -597,6 +597,19 @@ 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) >>> +{ >>> + atomic_notifier_call_chain(&sea_handler_chain, 0, NULL); >>> + >>> + pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n", >>> + fault_name(esr), esr, addr); >>> + >>> + return 0; >>> +} >> Don't we need to pass the abort to the guest? > This requires some infrastructure to implement virtual "ACPI platform > error interface" to expose the details of the abort to the guest. This > patchset does not cover that and focuses on feature parity with other > architectures that support APEI. There are discussions among Linaro > partners about how this can be achieved in the long term, but that > work is outside the scope of this patchset. This patch will ensure > that if a guest encounters one of these errors then it will be > reported before getting killed. Before this patch we would just get an > unsupported FSC print out and then the guest would be killed. OK. I think we might be talking about different things though. I am referring to the injection of the synchronous external abort into the guest - similar to what's been done for prefetch abort in the kvm_guest_handle_abort. Maybe there is no benefit in passing the abort to the guest. In that case, can you please add a comment where you handle external abort (FSC_EXTABT) in kvm_guest_handle_abort. > > Thanks, > Tyler >> >> Thanks, >> Punit >> >>> + >>> +/* >>> * Dispatch a data abort to the relevant handler. >>> */ >>> asmlinkage void __exception do_mem_abort(unsigned long addr, unsigned int esr,