From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753333AbdCHQJb (ORCPT ); Wed, 8 Mar 2017 11:09:31 -0500 Received: from foss.arm.com ([217.140.101.70]:32802 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752867AbdCHQJ2 (ORCPT ); Wed, 8 Mar 2017 11:09:28 -0500 Message-ID: <58C02CA2.4030601@arm.com> Date: Wed, 08 Mar 2017 16:09:06 +0000 From: James Morse User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.6.0 MIME-Version: 1.0 To: "Baicar, Tyler" CC: christoffer.dall@linaro.org, marc.zyngier@arm.com, 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 Subject: Re: [PATCH V12 10/10] arm/arm64: KVM: add guest SEA support References: <1488833103-21082-1-git-send-email-tbaicar@codeaurora.org> <1488833103-21082-11-git-send-email-tbaicar@codeaurora.org> <58BE9E14.6040208@arm.com> <783e18ac-9495-aaf6-8fbd-5a48c0b68f5b@codeaurora.org> In-Reply-To: <783e18ac-9495-aaf6-8fbd-5a48c0b68f5b@codeaurora.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/03/17 17:58, Baicar, Tyler wrote: > On 3/7/2017 4:48 AM, James Morse wrote: >> On 06/03/17 20:45, 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/kvm/mmu.c b/arch/arm/kvm/mmu.c >>> index a5265ed..f3608c9 100644 >>> --- a/arch/arm/kvm/mmu.c >>> +++ b/arch/arm/kvm/mmu.c >>> @@ -1444,8 +1463,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 (is_abort_synchronous(fault_status)) { >>> + if(handle_guest_sea((unsigned long)fault_ipa, >>> + kvm_vcpu_get_hsr(vcpu))) { >> >> ... Looking further up in this function: >>> is_iabt = kvm_vcpu_trap_is_iabt(vcpu); >>> if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu))) { >>> kvm_inject_vabt(vcpu); >>> return 1; >>> } >> ... so external data aborts will have already been 'claimed' by kvm and dealt >> with, and we already have a helper for spotting external aborts. (sorry I didn't >> spot it earlier). >> >> We need to do the handle_guest_sea() before this code. >> >> kvm_inject_vabt() makes an SError interrupt pending for the guest. This makes a >> synchronous error asynchronous as the guest may have SError interrupts masked. >> >> I guess this was the best that could be done at the time of (4055710baca8 >> "arm/arm64: KVM: Inject virtual abort when guest exits on external abort"), but >> in the light of this firmware-first handling, I'm not sure its the right thing >> to do. >> >> Is it possible for handle_guest_sea() to return whether it actually found any >> work to do? If there was none I think we should keep this kvm_inject_vabt() as >> it is the existing behaviour. > Yes, I'll move the handle_guest_sea() call above this. My testing didn't call > into that if statement for some reason...it made it to the handle_guest_sea() > call successfully. I guess you're not using data aborts for testing then. > If there is no work for the GHES code to do it will return and could still make > the kvm_inject_vabt() call. It will also return and do that same thing if the > error was not fatal in GHES...would that be an issue? We might inject a superfluous SError Interrupt in that case. For memory errors we may do the whole unmapping and signalling thing to handle the fault. For recoverable faults, QEMU can generate its own CPER records for the guest and do the work to notify the guest. Everything looks fine until the guest gets the extra SError interrupt. If there is firmware-first RAS data, we should skip the injected SError Interrupt as the host's RAS code should choose what to do. If this Synchronous External Abort was nothing to do with RAS, we should inject the SError interrupt as it's the existing behaviour, and not all platforms will have this Synchronous External Abort mechanism. x86's APEI NMI needs to know if the NMI was due to RAS, so we can probably borrow the same trick. ghes_notify_nmi() calls ghes_read_estatus() for each struct ghes. If they all return an error, there was no work to do. (I assume firmware will only generate one of these at a time, so there is no risk of one list-walker processing two entries, then the second finding nothing to do?) Thanks, James >>> + 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), >>