Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 15 Jun 2023 18:58:32 -0700
From:   Ashok Raj <ashok.raj@intel.com>
To:     Thomas Gleixner <tglx@linutronix.de>
CC:     LKML <linux-kernel@vger.kernel.org>, <x86@kernel.org>,
        Mario Limonciello <mario.limonciello@amd.com>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        "Tony Battersby" <tonyb@cybernetics.com>,
        Ashok Raj <ashok.raj@linux.intel.com>,
        Tony Luck <tony.luck@intel.com>,
        Arjan van de Veen <arjan@linux.intel.com>,
        Eric Biederman <ebiederm@xmission.com>,
        Ashok Raj <ashok.raj@intel.com>
Subject: Re: [patch v3 1/7] x86/smp: Make stop_other_cpus() more robust
Message-ID: <ZIvByEFqiJZOyau2@a4bf019067fa.jf.intel.com>
References: <20230615190036.898273129@linutronix.de>
 <20230615193330.263684884@linutronix.de>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230615193330.263684884@linutronix.de>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?EqtWuP2tfk2aWGCFn6QodSa20atZpg7fXby2Ikzw+V4zjRQG6YQNk87TamVv?=
 =?us-ascii?Q?3TrSAed6Af5/CHdP4N9LHb1iB3Q+kT0yCQm+h9x0/3zvDs079txlpJSkGHT3?=
 =?us-ascii?Q?yfrPGhowf756qWtilgjRsvq7eVw5tEkTk+3a6eWL3V3vHDKW/qUuFm/MaRya?=
 =?us-ascii?Q?Nc/xgv59QzG/IVsRM6PgJNOUN+a2ZhixDmuAl7zgpi00XLvI//+7m2waf/ul?=
 =?us-ascii?Q?SFZKlifbl4Dt59kwfLyZJ3s+peIcnTl/WTncxjifIm9fQbJPmuIqrBULhohx?=
 =?us-ascii?Q?rc7H9SNd0ZXOyoCIGZxgKe4vwS8UscM1/kdPljK2Y77GH1zD73+29JGVHYuQ?=
 =?us-ascii?Q?0f9F31XsKk9lcg4xHneaWdYFku5tz8k5xc2nIXOUR5Odk/9xZEqTAc3QpjFq?=
 =?us-ascii?Q?pzREvOxzhp4DYEzuLugAAc8UHEla+YRbjPasRjdbxcPbek64c9JeiZT84gUm?=
 =?us-ascii?Q?W/WIhglaP/r47bh0JbyBAsIuFmHPeEIIWPs2Yy6JmUGPR+DU3Ozr2Pfmv4x3?=
 =?us-ascii?Q?Hmdr+/5EEOhxAt42ekbNBDWFDA1bilAVfcQrL2XN6DyLuqTcSICaghiKIH8K?=
 =?us-ascii?Q?1GHVjcXHNGJTcYoBH5AHtZynGrljTiqMm47R+wJs9encHx8tWtdSv2E4RThM?=
 =?us-ascii?Q?FRMw53KYhXRu9wW7ShM80xZMBDYKpS/B0mFZoUnJq5LhPsQITkuaUAO7E8W3?=
 =?us-ascii?Q?/8XLTOlpCpFMZDP/FzSsPoQltAtaRGgkayr7ilkmlF91mNkO9PzOf4fOEdMn?=
 =?us-ascii?Q?x19i8xtC6TsR4EGDWT9dov0dzx6+agzo6MXVJgiJV6rutRHR7lCKkF1DHsfF?=
 =?us-ascii?Q?78mipBhkOpyZSHeanQdroEoQuMGOium5MnR2oIDlhMS2Opt0VrZSNhiZHgHs?=
 =?us-ascii?Q?gPqWJ+WaF69JWfxNJOBVJWCnf98EeBnsJT48nTO69faXXL+5WAq0OVT5c8bk?=
 =?us-ascii?Q?A1UhxB5XOdlfTquX86eBi2mx9a7oIU2sckYE6bgoblaHMyplj8F2rAbMmj2J?=
 =?us-ascii?Q?RAaAj9PaGaxMJPqUv1d5zK/xOuUAel5hdBjBif1QKGzM/vDdeXb5ximbVaBr?=
 =?us-ascii?Q?DS6gXhTaBzghrJKkFUe34LTGbUPhC8jdeCmQh5w3FuMB4bLsABjm3KKoE4xm?=
 =?us-ascii?Q?BVTbOEibFqbID1RNboGSTaLqSgapHEiE7AKEWz7EYfkgE942nA4mS6KijEcI?=
 =?us-ascii?Q?98x7mUEdo7LunAX41Q8/1lgjbKBh6aGcVaNab9bcN4izb2BG6uX2tklmw6+g?=
 =?us-ascii?Q?CDBgn3Tlx4w2NTMevRMZ9U/928EGXT7vktg6nz77eTbat6j5ScJTkO9jfZMZ?=
 =?us-ascii?Q?4WIxiD0OpDoT+xT+7EoyZdRlK+Fv84PsUddZuk3yDwxd1Q5SgyEebVdPI9JO?=
 =?us-ascii?Q?vDwVy2IfRcFS9yuvwNgrntFXlq0heOINxhMxU31XCyY9GWHKIaIIOfhFrCvi?=
 =?us-ascii?Q?keTk/NepZkICJ2NOLpdcR79P9JmTHpkWv1kC25R/gh9sx8Z0tBa1vNL2dXhe?=
 =?us-ascii?Q?1mzHhZyRzZqw5LPl6lbSOa8KC/ALKyoLzu4L4gFwF4JKnnjNIim8T8bLVE7p?=
 =?us-ascii?Q?hl5hNu32nJ/Ekd/u9qhxnBiMtjZDG2AZMa9I748t?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2b559c8e-ed03-4889-3117-08db6e0d3401
X-MS-Exchange-CrossTenant-AuthSource: SJ1PR11MB6201.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Jun 2023 01:58:39.5809
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: bzEbDKs1Ir60X6UK4RkeLuTpUazaQV1xMbklvdEs43ACuKDwe1GAdQF9k6I3Oe5JKdzp50keH0mYmwwyOhdw5A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB7034
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Thomas,

On Thu, Jun 15, 2023 at 10:33:50PM +0200, Thomas Gleixner wrote:
> Tony reported intermittent lockups on poweroff. His analysis identified the
> wbinvd() in stop_this_cpu() as the culprit. This was added to ensure that
> on SME enabled machines a kexec() does not leave any stale data in the
> caches when switching from encrypted to non-encrypted mode or vice versa.
> 
> That wbindv() is conditional on the SME feature bit which is read directly
> from CPUID. But that readout does not check whether the CPUID leaf is
> available or not. If it's not available the CPU will return the value of
> the highest supported leaf instead. Depending on the content the "SME" bit
> might be set or not.
> 
> That's incorrect but harmless. Making the CPUID readout conditional makes
> the observed hangs go away, but it does not fix the underlying problem:
> 
> CPU0					CPU1
> 
>  stop_other_cpus()
>    send_IPIs(REBOOT);			stop_this_cpu()
>    while (num_online_cpus() > 1);         set_online(false);
>    proceed... -> hang
> 				          wbinvd()
> 
> WBINVD is an expensive operation and if multiple CPUs issue it at the same
> time the resulting delays are even larger.
> 
> But CPU0 already observed num_online_cpus() going down to 1 and proceeds
> which causes the system to hang.
> 
> This issue exists independent of WBINVD, but the delays caused by WBINVD
> make it more prominent.
> 
> Make this more robust by adding a cpumask which is initialized to the
> online CPU mask before sending the IPIs and CPUs clear their bit in
> stop_this_cpu() after the WBINVD completed. Check for that cpumask to
> become empty in stop_other_cpus() instead of watching num_online_cpus().
> 
> The cpumask cannot plug all holes either, but it's better than a raw
> counter and allows to restrict the NMI fallback IPI to be sent only to
> the CPUs which have not reported within the timeout window.
> 
> Fixes: 08f253ec3767 ("x86/cpu: Clear SME feature flag when not in use")
> Reported-by: Tony Battersby <tonyb@cybernetics.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Link: https://lore.kernel.org/all/3817d810-e0f1-8ef8-0bbd-663b919ca49b@cybernetics.com
> ---
> V3: Use a cpumask to make the NMI case slightly safer - Ashok
> ---
>  arch/x86/include/asm/cpu.h |    2 +
>  arch/x86/kernel/process.c  |   23 +++++++++++++-
>  arch/x86/kernel/smp.c      |   71 +++++++++++++++++++++++++++++++--------------
>  3 files changed, 73 insertions(+), 23 deletions(-)

I tested them and seems to work fine on my system.

Maybe Tony can check in his setup would be great.

One thought on sending NMI below.

[snip]

>  
>  	/* if the REBOOT_VECTOR didn't work, try with the NMI */
> -	if (num_online_cpus() > 1) {
> +	if (!cpumask_empty(&cpus_stop_mask)) {
>  		/*
>  		 * If NMI IPI is enabled, try to register the stop handler
>  		 * and send the IPI. In any case try to wait for the other
>  		 * CPUs to stop.
>  		 */
>  		if (!smp_no_nmi_ipi && !register_stop_handler()) {
> +			u32 dm;
> +
>  			/* Sync above data before sending IRQ */
>  			wmb();
>  
>  			pr_emerg("Shutting down cpus with NMI\n");
>  
> -			apic_send_IPI_allbutself(NMI_VECTOR);
> +			dm = apic->dest_mode_logical ? APIC_DEST_LOGICAL : APIC_DEST_PHYSICAL;
> +			dm |= APIC_DM_NMI;
> +
> +			for_each_cpu(cpu, &cpus_stop_mask) {
> +				u32 apicid = apic->cpu_present_to_apicid(cpu);
> +
> +				apic_icr_write(dm, apicid);
> +				apic_wait_icr_idle();

can we simplify this by just apic->send_IPI(cpu, NMI_VECTOR); ??