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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id E99F5C433F5 for ; Tue, 12 Apr 2022 11:45:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Subject:Cc:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=ISyjG6odo7w3g9s87hMF+OL81LnNHBQ8pvERu/MwCpg=; b=bY5CEmDBq+wVnC /rOt2DjkVCEqua9nDgPb66LstjKSsXH9s1TTrC1xlhcYFQRQufYOod66LiroeuXVHEMBJTaJaD1HI UwzS9PFOpWXQFnJnEUk262e6IGRnrgWIYDokMm0oi/kG6Ai9LWVkKSnfzes/2Mv24dKchHD0Iz5xB 3CTDDP5gUPsOTkKhYBe8StwQgf6Plj1DL7Dm3teCPML4JxTpHJs/Z2T7xJhpEKZf/GkpXybOWYAaz fk5bD8wUeTQomlqyKdpRa2SI/1X39CF0LR8g4+qQA8MvqtsjoiEGTAO+h6KYH7AOrDkDO/nF0BfON utuTS4E0gldC+oOE6Utw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1neEvy-00DnYr-Ec; Tue, 12 Apr 2022 11:43:47 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1neEjU-00Dhim-By for linux-arm-kernel@lists.infradead.org; Tue, 12 Apr 2022 11:30:54 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id F0544619D2; Tue, 12 Apr 2022 11:30:50 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 63C66C385A5; Tue, 12 Apr 2022 11:30:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1649763050; bh=uK7N36K2e4+xCv93RcufURlLVYNraBsK39xdEe7/GeA=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=ND5kNCf7TIkZreRHnS4vubaMurYaENmc1qkFxoCH1WOFPiqg3rzOwE1IQmyLb5KnT pA9bW2ZEnuU6U8B9y39V3c02FR2O+IJW/Nf0unjV5oppC2j8i+81vK9f3KdD5kHn6f yovd35lPYxFYaKV1f5+aX/nSZwsADsYnXeHU57RudRXdAHJEerpMaszrBRfvPLYVUu zK/3SWTM7TlDlcDva/xWI3K2MdrRsGCEu/Fx1RJwwfJpCVhd1/ktvkyYI6GNGcuze8 1oP9aZ9CFlp7o/tB+OaOWkLr2Wad8768jGmQ8zFFObeAop9aY0i2JTm+dI0CloKZkC 3PHWQeFt/21Pg== Received: from sofa.misterjones.org ([185.219.108.64] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1neEjP-003kda-Q0; Tue, 12 Apr 2022 12:30:47 +0100 Date: Tue, 12 Apr 2022 12:30:47 +0100 Message-ID: <87bkx6bkt4.wl-maz@kernel.org> From: Marc Zyngier To: Alexandru Elisei Cc: catalin.marinas@arm.com, will@kernel.org, linux-arm-kernel@lists.infradead.org, james.morse@arm.com, suzuki.poulose@arm.com, kvmarm@lists.cs.columbia.edu, mark.rutland@arm.com Subject: Re: [PATCH 5/5] KVM: arm64: uapi: Add kvm_debug_exit_arch.hsr_high In-Reply-To: References: <20220407162327.396183-1-alexandru.elisei@arm.com> <20220407162327.396183-6-alexandru.elisei@arm.com> <87ee28auff.wl-maz@kernel.org> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: alexandru.elisei@arm.com, catalin.marinas@arm.com, will@kernel.org, linux-arm-kernel@lists.infradead.org, james.morse@arm.com, suzuki.poulose@arm.com, kvmarm@lists.cs.columbia.edu, mark.rutland@arm.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220412_043052_562115_C17E76F2 X-CRM114-Status: GOOD ( 48.38 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, 11 Apr 2022 11:46:09 +0100, Alexandru Elisei wrote: > > Hi Marc, > > On Fri, Apr 08, 2022 at 08:47:00AM +0100, Marc Zyngier wrote: > > Hi Alex, > > > > On Thu, 07 Apr 2022 17:23:27 +0100, > > Alexandru Elisei wrote: > > > > > > When userspace is debugging a VM, the kvm_debug_exit_arch part of the > > > kvm_run struct contains arm64 specific debug information: the ESR_EL2 > > > value, encoded in the field "hsr", and the address of the instruction > > > that caused the exception, encoded in the field "far". > > > > > > Linux has moved to treating ESR_EL2 as a 64-bit register, but unfortunately > > > kvm_debug_exit_arch.hsr cannot be changed to match because that would > > > change the memory layout of the struct on big endian machines: > > > > > > Current layout: | Layout with "hsr" extended to 64 bits: > > > | > > > offset 0: ESR_EL2[31:0] (hsr) | offset 0: ESR_EL2[61:32] (hsr[61:32]) > > > offset 4: padding | offset 4: ESR_EL2[31:0] (hsr[31:0]) > > > offset 8: FAR_EL2[61:0] (far) | offset 8: FAR_EL2[61:0] (far) > > > > > > which breaks existing code. > > > > > > The padding is inserted by the compiler because the "far" field must be > > > aligned to 8 bytes (each field must be naturally aligned - aapcs64 [1], > > > page 18) and the struct itself must be aligned to 8 bytes (the struct must > > > be aligned to the maximum alignment of its fields - aapcs64, page 18), > > > which means that "hsr" must be aligned to 8 bytes as it is the first field > > > in the struct. > > > > > > To avoid changing the struct size and layout for the existing fields, add a > > > new field, "hsr_high", which replaces the existing padding. "hsr_high" will > > > be used to hold the ESR_EL2[61:32] bits of the register. The memory layout, > > > both on big and little endian machine, becomes: > > > > > > Layout with "hsr_high" added: > > > > > > offset 0: ESR_EL2[31:0] (hsr) > > > offset 4: ESR_EL2[61:32] (hsr_high) > > > offset 8: FAR_EL2[61:0] (far) > > > > My concern with this change is that it isn't clear what the padding is > > currently initialised to, and I don't think there is any guarantee > > that it is zeroed. With that, a new userspace on an old kernel would > > interpret hsr_high, and potentially observe stuff that wasn't supposed > > to be interpreted. > > You are right, I didn't think about this scenario. Did some digging, and > C99 explicitely states that padding is uninitialized (September 7, 2007, > draft, page 38), so I assume that it's the same for C89 (couldn't find a > free source for the standard). > > > > > That's yet another mistake in our userspace ABI (where is the time > > machine when you need it?). > > To avoid this sort of thing happening in the future, KVM/arm64 could > mandate that all structs that are part of the userspace API have the > padding explicitly declared as a struct field and that padding must always > be set to zero by userspace before a syscall. That's what we were aiming for for most structures, but clearly missed some. Debug is obviously an unloved part of the architecture. > KVM would then check that the > padding is zero and return -EINVAL if userspace didn't clear it correctly, > to enforce this convention. I think that should be forward compatible with > repurposing the padding to add another field, unless 0 has a special > meaning for the field that is added, which I believe to be highly unlikely. > > > > > In order to do this, we must advertise to userspace that we provide > > more information. This probably means adding a flag of some sort to > > kvm_run (there are at least 128 bits of x86 stuff that can be readily > > reclaimed). > > We could add a flag to kvm_run.flags that is set only when > kvm_run.exit_reason == KVM_EXIT_DEBUG, something like > KVM_DEBUG_ARM_HSR_HIGH_SET (or PRESENT). flags has 16 bits which are unused > today, so I don't think it's too costly to use one bit for this. That'd be a sensible approach. We use any for KVM/arm64 yet, and this is all arch-specific anyway. > > One other option would be to wait to expose the upper 32 bits until KVM > supports a hardware feature that makes use of those bits. That means > advertising the feature to userspace, which KVM might or might not want to > do. And KVM today doesn't do any sanitisation/masking on the hsr value that > is reported to userspace, and tying hsr_high to a particular feature might > mean that KVM will have to sanitise the upper bits if said feature is > opt-in by userspace. > > My preference is for the first approach because the second approach looks > more complicated. +1. > There's always the option to wait until KVM makes use of the upper 32 bits > and decide then, when we have more information. The sooner, the better. Updating userspace takes *years*, so let's do it ASAP. Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel