From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_ADSP_ALL, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 15BF2C43331 for ; Fri, 6 Sep 2019 13:16:25 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id D7F21206BB for ; Fri, 6 Sep 2019 13:16:24 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="UJ3d4ZnO"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=amazon.com header.i=@amazon.com header.b="WkAUWXWJ" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D7F21206BB Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=amazon.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=KVLBArBokR1Wy94HYvOU8CxEjlgKGXZrLI7S7XUYlQM=; b=UJ3d4ZnODGQxsizG3vuKHxnUV bdmiI2wM6abV2B5r6Qn9liIagMnVfBpogOuN1P9fWfgMiWVMT5VSwbay7sruTBsZ6giUsv7D3ltWd djY3AgPt1YhnhoM+V0N7e+eF0Gz0pWuschHWShftF38peix6WzoG6N+fZ8cyc4lHkwKm32foIFReP px8o1ngVcSOAsnDnUR3eL9OmtLBwPiyrCdQBtTpNuVujpqsuz+4wAu114r8O6lIvbP4mrHjMWz5LA B5crH/Yi1jT6M2RIWK1yMxdoeD8bZq2hNhV40PCIYGCHfKaHnrnTyi1HV6POJI6NuOWyqC5Qelwb3 TpycWq7Sg==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92 #3 (Red Hat Linux)) id 1i6E69-0002Bu-FK; Fri, 06 Sep 2019 13:16:21 +0000 Received: from smtp-fw-2101.amazon.com ([72.21.196.25]) by bombadil.infradead.org with esmtps (Exim 4.92 #3 (Red Hat Linux)) id 1i6E65-0002BM-N4 for linux-arm-kernel@lists.infradead.org; Fri, 06 Sep 2019 13:16:19 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amazon.com; i=@amazon.com; q=dns/txt; s=amazon201209; t=1567775777; x=1599311777; h=subject:to:cc:references:from:message-id:date: mime-version:in-reply-to:content-transfer-encoding; bh=Z+c/2/FP7SqLIVegXjm3Q/SIZ39g6Irm8z2g2XMogZ4=; b=WkAUWXWJiV3xQiWMObhGhCAAi6F+u5wcPtVunfh2cRArzxvkghMqwmFg qe2b2HWKQPS/VXYGzPVmqZj+kaTGFmNFQig0kGSpMlyCuGjPdd2J/90f8 co4QJ34BXy74hRLXRD4x1fP46xE0eH3LpHVCLq/jWRb7TRVv0hhSrBasc U=; X-IronPort-AV: E=Sophos;i="5.64,473,1559520000"; d="scan'208";a="749382019" Received: from iad6-co-svc-p1-lb1-vlan2.amazon.com (HELO email-inbound-relay-1a-7d76a15f.us-east-1.amazon.com) ([10.124.125.2]) by smtp-border-fw-out-2101.iad2.amazon.com with ESMTP; 06 Sep 2019 13:16:14 +0000 Received: from EX13MTAUWC001.ant.amazon.com (iad55-ws-svc-p15-lb9-vlan2.iad.amazon.com [10.40.159.162]) by email-inbound-relay-1a-7d76a15f.us-east-1.amazon.com (Postfix) with ESMTPS id AE2F7A2CCE; Fri, 6 Sep 2019 13:16:12 +0000 (UTC) Received: from EX13D20UWC001.ant.amazon.com (10.43.162.244) by EX13MTAUWC001.ant.amazon.com (10.43.162.135) with Microsoft SMTP Server (TLS) id 15.0.1367.3; Fri, 6 Sep 2019 13:16:11 +0000 Received: from 38f9d3867b82.ant.amazon.com (10.43.160.149) by EX13D20UWC001.ant.amazon.com (10.43.162.244) with Microsoft SMTP Server (TLS) id 15.0.1367.3; Fri, 6 Sep 2019 13:16:09 +0000 Subject: Re: [PATCH 1/1] KVM: inject data abort if instruction cannot be decoded To: Christoffer Dall References: <20190904180736.29009-1-xypron.glpk@gmx.de> <86r24vrwyh.wl-maz@kernel.org> <86mufjrup7.wl-maz@kernel.org> <20190905092223.GC4320@e113682-lin.lund.arm.com> <4b6662bd-56e4-3c10-3b65-7c90828a22f9@kernel.org> <20190906080033.GF4320@e113682-lin.lund.arm.com> <20190906131252.GG4320@e113682-lin.lund.arm.com> From: Alexander Graf Message-ID: Date: Fri, 6 Sep 2019 15:16:07 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <20190906131252.GG4320@e113682-lin.lund.arm.com> Content-Language: en-US X-Originating-IP: [10.43.160.149] X-ClientProxiedBy: EX13D18UWC001.ant.amazon.com (10.43.162.105) To EX13D20UWC001.ant.amazon.com (10.43.162.244) Precedence: Bulk X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190906_061617_951920_43926C6B X-CRM114-Status: GOOD ( 29.91 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: =?UTF-8?Q?Daniel_P=2e_Berrang=c3=a9?= , Marc Zyngier , lkml - Kernel Mailing List , Stefan Hajnoczi , Heinrich Schuchardt , kvmarm@lists.cs.columbia.edu, arm-mail-list Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 06.09.19 15:12, Christoffer Dall wrote: > On Fri, Sep 06, 2019 at 02:08:15PM +0200, Alexander Graf wrote: >> >> >> On 06.09.19 10:00, Christoffer Dall wrote: >>> On Thu, Sep 05, 2019 at 02:09:18PM +0100, Marc Zyngier wrote: >>>> On 05/09/2019 10:22, Christoffer Dall wrote: >>>>> On Thu, Sep 05, 2019 at 09:56:44AM +0100, Peter Maydell wrote: >>>>>> On Thu, 5 Sep 2019 at 09:52, Marc Zyngier wrote: >>>>>>> >>>>>>> On Thu, 05 Sep 2019 09:16:54 +0100, >>>>>>> Peter Maydell wrote: >>>>>>>> This is true, but the problem is that barfing out to userspace >>>>>>>> makes it harder to debug the guest because it means that >>>>>>>> the VM is immediately destroyed, whereas AIUI if we >>>>>>>> inject some kind of exception then (assuming you're set up >>>>>>>> to do kernel-debug via gdbstub) you can actually examine >>>>>>>> the offending guest code with a debugger because at least >>>>>>>> your VM is still around to inspect... >>>>>>> >>>>>>> To Christoffer's point, I find the benefit a bit dubious. Yes, you get >>>>>>> an exception, but the instruction that caused it may be completely >>>>>>> legal (store with post-increment, for example), leading to an even >>>>>>> more puzzled developer (that exception should never have been >>>>>>> delivered the first place). >>>>>> >>>>>> Right, but the combination of "host kernel prints a message >>>>>> about an unsupported load/store insn" and "within-guest debug >>>>>> dump/stack trace/etc" is much more useful than just having >>>>>> "host kernel prints message" and "QEMU exits"; and it requires >>>>>> about 3 lines of code change... >>>>>> >>>>>>> I'm far more in favour of dumping the state of the access in the run >>>>>>> structure (much like we do for a MMIO access) and let userspace do >>>>>>> something about it (such as dumping information on the console or >>>>>>> breaking). It could even inject an exception *if* the user has asked >>>>>>> for it. >>>>>> >>>>>> ...whereas this requires agreement on a kernel-userspace API, >>>>>> larger changes in the kernel, somebody to implement the userspace >>>>>> side of things, and the user to update both the kernel and QEMU. >>>>>> It's hard for me to see that the benefit here over the 3-line >>>>>> approach really outweighs the extra effort needed. In practice >>>>>> saying "we should do this" is saying "we're going to do nothing", >>>>>> based on the historical record. >>>>>> >>>>> >>>>> How about something like the following (completely untested, liable for >>>>> ABI discussions etc. etc., but for illustration purposes). >>>>> >>>>> I think it raises the question (and likely many other) of whether we can >>>>> break the existing 'ABI' and change behavior for missing ISV >>>>> retrospectively for legacy user space when the issue has occurred? >>>>> Someone might have written code that reacts to the -ENOSYS, so I've >>>>> taken the conservative approach for this for the time being. >>>>> >>>>> >>>>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >>>>> index 8a37c8e89777..19a92c49039c 100644 >>>>> --- a/arch/arm/include/asm/kvm_host.h >>>>> +++ b/arch/arm/include/asm/kvm_host.h >>>>> @@ -76,6 +76,14 @@ struct kvm_arch { >>>>> /* Mandated version of PSCI */ >>>>> u32 psci_version; >>>>> + >>>>> + /* >>>>> + * If we encounter a data abort without valid instruction syndrome >>>>> + * information, report this to user space. User space can (and >>>>> + * should) opt in to this feature if KVM_CAP_ARM_NISV_TO_USER is >>>>> + * supported. >>>>> + */ >>>>> + bool return_nisv_io_abort_to_user; >>>>> }; >>>>> #define KVM_NR_MEM_OBJS 40 >>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >>>>> index f656169db8c3..019bc560edc1 100644 >>>>> --- a/arch/arm64/include/asm/kvm_host.h >>>>> +++ b/arch/arm64/include/asm/kvm_host.h >>>>> @@ -83,6 +83,14 @@ struct kvm_arch { >>>>> /* Mandated version of PSCI */ >>>>> u32 psci_version; >>>>> + >>>>> + /* >>>>> + * If we encounter a data abort without valid instruction syndrome >>>>> + * information, report this to user space. User space can (and >>>>> + * should) opt in to this feature if KVM_CAP_ARM_NISV_TO_USER is >>>>> + * supported. >>>>> + */ >>>>> + bool return_nisv_io_abort_to_user; >>>>> }; >>>>> #define KVM_NR_MEM_OBJS 40 >>>>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h >>>>> index 5e3f12d5359e..a4dd004d0db9 100644 >>>>> --- a/include/uapi/linux/kvm.h >>>>> +++ b/include/uapi/linux/kvm.h >>>>> @@ -235,6 +235,7 @@ struct kvm_hyperv_exit { >>>>> #define KVM_EXIT_S390_STSI 25 >>>>> #define KVM_EXIT_IOAPIC_EOI 26 >>>>> #define KVM_EXIT_HYPERV 27 >>>>> +#define KVM_EXIT_ARM_NISV 28 >>>>> /* For KVM_EXIT_INTERNAL_ERROR */ >>>>> /* Emulate instruction failed. */ >>>>> @@ -996,6 +997,7 @@ struct kvm_ppc_resize_hpt { >>>>> #define KVM_CAP_ARM_PTRAUTH_ADDRESS 171 >>>>> #define KVM_CAP_ARM_PTRAUTH_GENERIC 172 >>>>> #define KVM_CAP_PMU_EVENT_FILTER 173 >>>>> +#define KVM_CAP_ARM_NISV_TO_USER 174 >>>>> #ifdef KVM_CAP_IRQ_ROUTING >>>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c >>>>> index 35a069815baf..2ce94bd9d4a9 100644 >>>>> --- a/virt/kvm/arm/arm.c >>>>> +++ b/virt/kvm/arm/arm.c >>>>> @@ -98,6 +98,26 @@ int kvm_arch_check_processor_compat(void) >>>>> return 0; >>>>> } >>>>> +int kvm_vm_ioctl_enable_cap(struct kvm *kvm, >>>>> + struct kvm_enable_cap *cap) >>>>> +{ >>>>> + int r; >>>>> + >>>>> + if (cap->flags) >>>>> + return -EINVAL; >>>>> + >>>>> + switch (cap->cap) { >>>>> + case KVM_CAP_ARM_NISV_TO_USER: >>>>> + r = 0; >>>>> + kvm->arch.return_nisv_io_abort_to_user = true; >>>>> + break; >>>>> + default: >>>>> + r = -EINVAL; >>>>> + break; >>>>> + } >>>>> + >>>>> + return r; >>>>> +} >>>>> /** >>>>> * kvm_arch_init_vm - initializes a VM data structure >>>>> @@ -196,6 +216,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) >>>>> case KVM_CAP_MP_STATE: >>>>> case KVM_CAP_IMMEDIATE_EXIT: >>>>> case KVM_CAP_VCPU_EVENTS: >>>>> + case KVM_CAP_ARM_NISV_TO_USER: >>>>> r = 1; >>>>> break; >>>>> case KVM_CAP_ARM_SET_DEVICE_ADDR: >>>>> @@ -673,6 +694,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) >>>>> ret = kvm_handle_mmio_return(vcpu, vcpu->run); >>>>> if (ret) >>>>> return ret; >>>>> + } else if (run->exit_reason == KVM_EXIT_ARM_NISV) { >>>>> + kvm_inject_undefined(vcpu); >>>> >>>> Just to make sure I understand: Is the expectation here that userspace >>>> could clear the exit reason if it managed to handle the exit? And >>>> otherwise we'd inject an UNDEF on reentry? >>>> >>> >>> Yes, but I think we should change that to an external abort. I'll test >>> something and send a proper patch with more clear documentation. >> >> Why not leave the injection to user space in any case? API wise there is no >> need to be backwards compatible, as we require the CAP to be enabled, right? >> > > I'd prefer leaving it to userspace to worry about, but I thought Peter > said that had been problematic historically, which I took at face value, > but I could have misunderstood. > > If QEMU, kvmtool, and whatever the crazy^H cool kids are using in > userspace these days are happy emulating the exception, then that's a > viable approach. The main concern I have with that is whether they'll > all get it right, and since we already have the code in the kernel to do > this, it might make sense to re-use the kernel logic for it. You could make the same argument about injecting an #SError on an out of bounds access to MMIO. If injecting a fault is too complicated, we should fix that rather than create an unbalanced user space interface :). > I'll leave it in for v1 of the patch, and if based on how that code and > interface looks like, we agree it's better to leave it to userspace, I > can remove it in v2. Sure, works for me :). Please CC me on v1 so I can comment on it ;) Alex Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel