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 111F9C77B70 for ; Mon, 17 Apr 2023 12:36:54 +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=oUrE17L9VqPjHBkbQM72rxG0tiUfkLQgeWCsBLZbZEw=; b=zNORCr8xcg6wRb +vHk/eENxyPWAbfX6zK+uL7nusSg3kqabV8bwfZGJH14PlPIOB1u8zSyESkJsJC8i25aTb0F/Nqrl BnK7oFfu0WZY2dvwyV4iPOLVKLN4oymSwnCPmJvQeVJaWtL6Y4UfN6007YZj3Js95zEjsjhUagQzR kuQbxDWoeVvRVib6HnhZ25B/Hg+cZ76woa3iNGPQSJgjUMxMfWWUn7QScUK6N2kjK7duqG9+tMttR qmDK4LjgEzO/tevlBlc9MVLNHAkY+cvu4ZqKg6juj23ajrxB4k7JrJl2g+RTT4s11oZ9d86/PVrLL QnIIGT7Nc+19TPasyRMQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1poO5M-00G9n7-3C; Mon, 17 Apr 2023 12:35:57 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1poO5J-00G9mA-0K for linux-arm-kernel@lists.infradead.org; Mon, 17 Apr 2023 12:35: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 74B8561B6B; Mon, 17 Apr 2023 12:35:52 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id DC353C433D2; Mon, 17 Apr 2023 12:35:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1681734951; bh=6UXgo+VUKr4Q5O8Hy6t/zQ8QpSNaPZ2EAGJ/i9IICf0=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=FdNW6CfnyxPyjxXXaZruUZDJNBhsoq+8Meh1VglBN4oyIousieo0IzJLaIoHvHgSv e+9cCYyGm0Sj7526x9L0i6DwZvlYFSHZtnMBAlWiueRmz1Sa1YJjb3BU7Wrc/9+5Ad Ogb/CkOnLpO0CqX1flslD3v4CpgXFtDLyaglwIpXpLINiAk3XK3aHk/lGgYlzI1c6P ukWd496kcUnB+bCUSqUl00kVKEX+gzg6/s3Ce0+chljj91kLZTNVfD08/h7f7w4UZl W7QWWRRS7jz5YZbFAHI5+6BK0B1p2Ab5+M5MZx74SoeVfU+jVSarR1ObnRbOFcMTO5 AILePj3gUBKiA== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1poO5F-0090sd-KL; Mon, 17 Apr 2023 13:35:49 +0100 Date: Mon, 17 Apr 2023 13:35:49 +0100 Message-ID: <86bkjmlm8q.wl-maz@kernel.org> From: Marc Zyngier To: Will Deacon Cc: kvmarm@lists.linux.dev, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, James Morse , Suzuki K Poulose , Oliver Upton , Zenghui Yu , Catalin Marinas , Quentin Perret , Mostafa Saleh , stable@vger.kernel.org Subject: Re: [PATCH] KVM: arm64: Make vcpu flag updates non-preemptible In-Reply-To: <20230417114025.GA30826@willie-the-truck> References: <20230417093629.1440039-1-maz@kernel.org> <20230417114025.GA30826@willie-the-truck> 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/28.2 (aarch64-unknown-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: will@kernel.org, kvmarm@lists.linux.dev, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, james.morse@arm.com, suzuki.poulose@arm.com, oliver.upton@linux.dev, yuzenghui@huawei.com, catalin.marinas@arm.com, qperret@google.com, smostafa@google.com, stable@vger.kernel.org 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-20230417_053553_222761_61C61CAD X-CRM114-Status: GOOD ( 36.45 ) 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, 17 Apr 2023 12:40:26 +0100, Will Deacon wrote: > > On Mon, Apr 17, 2023 at 10:36:29AM +0100, Marc Zyngier wrote: > > Per-vcpu flags are updated using a non-atomic RMW operation. > > Which means it is possible to get preempted between the read and > > write operations. > > > > Another interesting thing to note is that preemption also updates > > flags, as we have some flag manipulation in both the load and put > > operations. > > > > It is thus possible to lose information communicated by either > > load or put, as the preempted flag update will overwrite the flags > > when the thread is resumed. This is specially critical if either > > load or put has stored information which depends on the physical > > CPU the vcpu runs on. > > > > This results in really elusive bugs, and kudos must be given to > > Mostafa for the long hours of debugging, and finally spotting > > the problem. > > > > Fixes: e87abb73e594 ("KVM: arm64: Add helpers to manipulate vcpu flags among a set") > > Reported-by: Mostafa Saleh > > Signed-off-by: Marc Zyngier > > Cc: stable@vger.kernel.org > > --- > > arch/arm64/include/asm/kvm_host.h | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index bcd774d74f34..d716cfd806e8 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -579,6 +579,19 @@ struct kvm_vcpu_arch { > > v->arch.flagset & (m); \ > > }) > > > > +/* > > + * Note that the set/clear accessors must be preempt-safe in order to > > + * avoid nesting them with load/put which also manipulate flags... > > + */ > > +#ifdef __KVM_NVHE_HYPERVISOR__ > > +/* the nVHE hypervisor is always non-preemptible */ > > +#define __vcpu_flags_preempt_disable() > > +#define __vcpu_flags_preempt_enable() > > +#else > > +#define __vcpu_flags_preempt_disable() preempt_disable() > > +#define __vcpu_flags_preempt_enable() preempt_enable() > > +#endif > > If it makes things cleaner, we could define local (empty) copies of these > preempt_*() macros at EL2 to save you having to wrap them here. Up to you. Nah, that's fine. This is subtle enough stuff that I'm happy to see it all exposed in the same location. > > #define __vcpu_set_flag(v, flagset, f, m) \ > > do { \ > > typeof(v->arch.flagset) *fset; \ > > @@ -586,9 +599,11 @@ struct kvm_vcpu_arch { > > __build_check_flag(v, flagset, f, m); \ > > \ > > fset = &v->arch.flagset; \ > > + __vcpu_flags_preempt_disable(); \ > > if (HWEIGHT(m) > 1) \ > > *fset &= ~(m); \ > > *fset |= (f); \ > > + __vcpu_flags_preempt_enable(); \ > > } while (0) > > > > #define __vcpu_clear_flag(v, flagset, f, m) \ > > @@ -598,7 +613,9 @@ struct kvm_vcpu_arch { > > __build_check_flag(v, flagset, f, m); \ > > \ > > fset = &v->arch.flagset; \ > > + __vcpu_flags_preempt_disable(); \ > > *fset &= ~(m); \ > > + __vcpu_flags_preempt_enable(); \ > > } while (0) > > > > #define vcpu_get_flag(v, ...) __vcpu_get_flag((v), __VA_ARGS__) > > Given that __vcpu_get_flag() is still preemptible, we should probably > add a READ_ONCE() in there when we access the relevant flags field. In > practice, they're all single-byte fields so it should be ok, but I think > the READ_ONCE() is still worthwhile. Yup, good point. People are already talking about expanding some of the fields for $REASON, so they may become larger than a single byte. And READ_ONCE() makes it clear that there is some level of atomicity required here as well. I'll respin this shortly. 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