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=-13.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 251BAC4708F for ; Fri, 4 Jun 2021 09:49:38 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0C90861405 for ; Fri, 4 Jun 2021 09:49:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230150AbhFDJvW (ORCPT ); Fri, 4 Jun 2021 05:51:22 -0400 Received: from foss.arm.com ([217.140.110.172]:34468 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229930AbhFDJvV (ORCPT ); Fri, 4 Jun 2021 05:51:21 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id A71AC1063; Fri, 4 Jun 2021 02:49:35 -0700 (PDT) Received: from C02TD0UTHF1T.local (unknown [10.57.6.137]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 556D03F73D; Fri, 4 Jun 2021 02:49:31 -0700 (PDT) Date: Fri, 4 Jun 2021 10:49:26 +0100 From: Mark Rutland To: Will Deacon Cc: linux-arm-kernel@lists.infradead.org, linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org, Catalin Marinas , Marc Zyngier , Greg Kroah-Hartman , Peter Zijlstra , Morten Rasmussen , Qais Yousef , Suren Baghdasaryan , Quentin Perret , Tejun Heo , Johannes Weiner , Ingo Molnar , Juri Lelli , Vincent Guittot , "Rafael J. Wysocki" , Dietmar Eggemann , Daniel Bristot de Oliveira , Valentin Schneider , kernel-team@android.com Subject: Re: [PATCH v8 15/19] arm64: Prevent offlining first CPU with 32-bit EL0 on mismatched system Message-ID: <20210604094926.GB64162@C02TD0UTHF1T.local> References: <20210602164719.31777-1-will@kernel.org> <20210602164719.31777-16-will@kernel.org> <20210603125856.GC48596@C02TD0UTHF1T.local> <20210603174056.GB1170@willie-the-truck> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210603174056.GB1170@willie-the-truck> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jun 03, 2021 at 06:40:57PM +0100, Will Deacon wrote: > On Thu, Jun 03, 2021 at 01:58:56PM +0100, Mark Rutland wrote: > > On Wed, Jun 02, 2021 at 05:47:15PM +0100, Will Deacon wrote: > > > If we want to support 32-bit applications, then when we identify a CPU > > > with mismatched 32-bit EL0 support we must ensure that we will always > > > have an active 32-bit CPU available to us from then on. This is important > > > for the scheduler, because is_cpu_allowed() will be constrained to 32-bit > > > CPUs for compat tasks and forced migration due to a hotplug event will > > > hang if no 32-bit CPUs are available. > > > > > > On detecting a mismatch, prevent offlining of either the mismatching CPU > > > if it is 32-bit capable, or find the first active 32-bit capable CPU > > > otherwise. > > > > > > Reviewed-by: Catalin Marinas > > > Signed-off-by: Will Deacon > > > --- > > > arch/arm64/kernel/cpufeature.c | 20 +++++++++++++++++++- > > > 1 file changed, 19 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > > > index 4194a47de62d..b31d7a1eaed6 100644 > > > --- a/arch/arm64/kernel/cpufeature.c > > > +++ b/arch/arm64/kernel/cpufeature.c > > > @@ -2877,15 +2877,33 @@ void __init setup_cpu_features(void) > > > > > > static int enable_mismatched_32bit_el0(unsigned int cpu) > > > { > > > + static int lucky_winner = -1; > > > > This is cute, but could we please give it a meaningful name, e.g. > > `pinned_cpu` ? > > I really don't see the problem, nor why it's "cute". > > Tell you what, I'll add a comment instead: > > /* > * The first 32-bit-capable CPU we detected and so can no longer > * be offlined by userspace. -1 indicates we haven't yet onlined > * a 32-bit-capable CPU. > */ Thanks for the comment; that's helpful. However, my concern here is that when we inevitably have to discuss this with others in future, "lucky winner" is jarring (and also unclear to those where English is not their native language). For clarity, it would be really nice to use a term like "cpu", "chosen_cpu", "pinned_cpu", etc. However, you're the maintainer; choose what you think is appropriate. > > > struct cpuinfo_arm64 *info = &per_cpu(cpu_data, cpu); > > > bool cpu_32bit = id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0); > > > > > > if (cpu_32bit) { > > > cpumask_set_cpu(cpu, cpu_32bit_el0_mask); > > > static_branch_enable_cpuslocked(&arm64_mismatched_32bit_el0); > > > - setup_elf_hwcaps(compat_elf_hwcaps); > > > } > > > > > > + if (cpumask_test_cpu(0, cpu_32bit_el0_mask) == cpu_32bit) > > > + return 0; > > > + > > > + if (lucky_winner >= 0) > > > + return 0; > > > + > > > + /* > > > + * We've detected a mismatch. We need to keep one of our CPUs with > > > + * 32-bit EL0 online so that is_cpu_allowed() doesn't end up rejecting > > > + * every CPU in the system for a 32-bit task. > > > + */ > > > + lucky_winner = cpu_32bit ? cpu : cpumask_any_and(cpu_32bit_el0_mask, > > > + cpu_active_mask); > > > + get_cpu_device(lucky_winner)->offline_disabled = true; > > > + setup_elf_hwcaps(compat_elf_hwcaps); > > > + pr_info("Asymmetric 32-bit EL0 support detected on CPU %u; CPU hot-unplug disabled on CPU %u\n", > > > + cpu, lucky_winner); > > > return 0; > > > } > > > > I guess this is going to play havoc with kexec and hibernate. :/ > > The kernel can still offline the CPUs (see the whole freezer mess that I > linked to in the cover letter). What specific havoc are you thinking of? Ah. If this is just inhibiting userspace-driven offlining, that sounds fine. For kexec, I was concerned that either this would inhibit kexec, or smp_shutdown_nonboot_cpus() would fail to offline the pinned CPU, and that'd trigger a BUG(), which would be unfortunate. For hibernate, the equivalent is freeze_secondary_cpus(), which I guess is dealt with by the freezer bits you mention. Thanks, Mark. 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=-14.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 63985C07E94 for ; Fri, 4 Jun 2021 09:51:22 +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 1730F61405 for ; Fri, 4 Jun 2021 09:51:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1730F61405 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+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.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=aVnKDc57s4bmPjLITUxIGyGeqtFLibv2GCioWiqMfLc=; b=ueQHl/zFEZo/sf tEfc+lJHRvYbBvtLiR9UGgZ7SOFZ0CTEzQ7MAm3evbQLuL2mu85mF5T6XGep394hMLCT+KdAs2CgZ FIQHdLoAxQylbnu6wnydRBB5hV3KAAxUwZc3V6KfXfsOKNuatyetcoLpwiXuASDb1QJV7uaqjUuPB fCYMjC2WJibd7s0nr0/QPfm97xuaqX5KALXFQjQ3cqN8OJXXw5/bitqqmqtuKF/ZG2bKMXuJEEnML HuO68jVZJl7XKgdVT18GQABQLwz6jcjjAInFgBttJLEEFi0u/TyU88Pu82Ku8583Gtg4VQl0ojNqx 0kpJ6bF8uxDGv3TsBWyg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1lp6SV-00Ck1h-7a; Fri, 04 Jun 2021 09:49:43 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1lp6SP-00Cjzy-R6 for linux-arm-kernel@lists.infradead.org; Fri, 04 Jun 2021 09:49:39 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id A71AC1063; Fri, 4 Jun 2021 02:49:35 -0700 (PDT) Received: from C02TD0UTHF1T.local (unknown [10.57.6.137]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 556D03F73D; Fri, 4 Jun 2021 02:49:31 -0700 (PDT) Date: Fri, 4 Jun 2021 10:49:26 +0100 From: Mark Rutland To: Will Deacon Cc: linux-arm-kernel@lists.infradead.org, linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org, Catalin Marinas , Marc Zyngier , Greg Kroah-Hartman , Peter Zijlstra , Morten Rasmussen , Qais Yousef , Suren Baghdasaryan , Quentin Perret , Tejun Heo , Johannes Weiner , Ingo Molnar , Juri Lelli , Vincent Guittot , "Rafael J. Wysocki" , Dietmar Eggemann , Daniel Bristot de Oliveira , Valentin Schneider , kernel-team@android.com Subject: Re: [PATCH v8 15/19] arm64: Prevent offlining first CPU with 32-bit EL0 on mismatched system Message-ID: <20210604094926.GB64162@C02TD0UTHF1T.local> References: <20210602164719.31777-1-will@kernel.org> <20210602164719.31777-16-will@kernel.org> <20210603125856.GC48596@C02TD0UTHF1T.local> <20210603174056.GB1170@willie-the-truck> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20210603174056.GB1170@willie-the-truck> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210604_024938_025093_A48E3022 X-CRM114-Status: GOOD ( 40.36 ) 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 Thu, Jun 03, 2021 at 06:40:57PM +0100, Will Deacon wrote: > On Thu, Jun 03, 2021 at 01:58:56PM +0100, Mark Rutland wrote: > > On Wed, Jun 02, 2021 at 05:47:15PM +0100, Will Deacon wrote: > > > If we want to support 32-bit applications, then when we identify a CPU > > > with mismatched 32-bit EL0 support we must ensure that we will always > > > have an active 32-bit CPU available to us from then on. This is important > > > for the scheduler, because is_cpu_allowed() will be constrained to 32-bit > > > CPUs for compat tasks and forced migration due to a hotplug event will > > > hang if no 32-bit CPUs are available. > > > > > > On detecting a mismatch, prevent offlining of either the mismatching CPU > > > if it is 32-bit capable, or find the first active 32-bit capable CPU > > > otherwise. > > > > > > Reviewed-by: Catalin Marinas > > > Signed-off-by: Will Deacon > > > --- > > > arch/arm64/kernel/cpufeature.c | 20 +++++++++++++++++++- > > > 1 file changed, 19 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > > > index 4194a47de62d..b31d7a1eaed6 100644 > > > --- a/arch/arm64/kernel/cpufeature.c > > > +++ b/arch/arm64/kernel/cpufeature.c > > > @@ -2877,15 +2877,33 @@ void __init setup_cpu_features(void) > > > > > > static int enable_mismatched_32bit_el0(unsigned int cpu) > > > { > > > + static int lucky_winner = -1; > > > > This is cute, but could we please give it a meaningful name, e.g. > > `pinned_cpu` ? > > I really don't see the problem, nor why it's "cute". > > Tell you what, I'll add a comment instead: > > /* > * The first 32-bit-capable CPU we detected and so can no longer > * be offlined by userspace. -1 indicates we haven't yet onlined > * a 32-bit-capable CPU. > */ Thanks for the comment; that's helpful. However, my concern here is that when we inevitably have to discuss this with others in future, "lucky winner" is jarring (and also unclear to those where English is not their native language). For clarity, it would be really nice to use a term like "cpu", "chosen_cpu", "pinned_cpu", etc. However, you're the maintainer; choose what you think is appropriate. > > > struct cpuinfo_arm64 *info = &per_cpu(cpu_data, cpu); > > > bool cpu_32bit = id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0); > > > > > > if (cpu_32bit) { > > > cpumask_set_cpu(cpu, cpu_32bit_el0_mask); > > > static_branch_enable_cpuslocked(&arm64_mismatched_32bit_el0); > > > - setup_elf_hwcaps(compat_elf_hwcaps); > > > } > > > > > > + if (cpumask_test_cpu(0, cpu_32bit_el0_mask) == cpu_32bit) > > > + return 0; > > > + > > > + if (lucky_winner >= 0) > > > + return 0; > > > + > > > + /* > > > + * We've detected a mismatch. We need to keep one of our CPUs with > > > + * 32-bit EL0 online so that is_cpu_allowed() doesn't end up rejecting > > > + * every CPU in the system for a 32-bit task. > > > + */ > > > + lucky_winner = cpu_32bit ? cpu : cpumask_any_and(cpu_32bit_el0_mask, > > > + cpu_active_mask); > > > + get_cpu_device(lucky_winner)->offline_disabled = true; > > > + setup_elf_hwcaps(compat_elf_hwcaps); > > > + pr_info("Asymmetric 32-bit EL0 support detected on CPU %u; CPU hot-unplug disabled on CPU %u\n", > > > + cpu, lucky_winner); > > > return 0; > > > } > > > > I guess this is going to play havoc with kexec and hibernate. :/ > > The kernel can still offline the CPUs (see the whole freezer mess that I > linked to in the cover letter). What specific havoc are you thinking of? Ah. If this is just inhibiting userspace-driven offlining, that sounds fine. For kexec, I was concerned that either this would inhibit kexec, or smp_shutdown_nonboot_cpus() would fail to offline the pinned CPU, and that'd trigger a BUG(), which would be unfortunate. For hibernate, the equivalent is freeze_secondary_cpus(), which I guess is dealt with by the freezer bits you mention. Thanks, Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel