Message ID | 20230724132047.554355840@linutronix.de |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:9010:0:b0:3e4:2afc:c1 with SMTP id l16csp1806800vqg; Mon, 24 Jul 2023 06:40:03 -0700 (PDT) X-Google-Smtp-Source: APBJJlH9eugWzeAzsExNa168OL02DtFQ7grFIpLPG9SJO+WIfIJvD497PIoH2Ug0Oi5o+UndJJkL X-Received: by 2002:a05:6402:5249:b0:51e:5aac:6bad with SMTP id t9-20020a056402524900b0051e5aac6badmr16860675edd.11.1690206003347; Mon, 24 Jul 2023 06:40:03 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1690206003; cv=none; d=google.com; s=arc-20160816; b=VuAkhEvmWud/k/scG7USpoMzqKi09s6fsOx30Fd65FgDPbf7bAryq5nbXys1KS5ILt EDnZ2BqucFDxVyVYDAZVJB2uhfI3emubSCFy/15cWQZmNAi3tHH9DGX4RwAYDKFRnEsq yK1plMxBkUhAAqJN/9w2ngExEhpuj8nxXXHsqPJcBDDWqiSv6pxklFgtM0aQ+uqFz5G0 fYwhtnc+Oxxs1aA822brLvVpYp2bFtmkhGn3475JoFvrPktpDBuzxAzmSOOeRD4VG4qr d49iSzvkytLcIozV33BCeI6r+22gBZgKLQ4jA7S3+u7ETQeAgC3dEg/8NWycSkLc7E7W pNcg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:date:mime-version:references:subject:cc:to:from :dkim-signature:dkim-signature:message-id; bh=u1MBAiZCltHmWzuaN06FA6y4i5aqgal/njyPH8pNWsw=; fh=mE3iieUIZLcDfOLIULj9X6/p0bMDPZGFSVRsnyRUnTc=; b=t2LY6y8CpxFfEyEdlgmUV0S4BP2bxiZYfmXeUcJthqaJVAXmfHLJhirk3f3tDye7v4 0/gc9sXmdYIIh08rknZ20nlRHo0gcr4ptgXuwvzIlx6opNwE7SvlVfghGIbFqqkj1V44 MoDv462T8EEiYN6vAsLGMg141BFH4SlILOpFu3Vr+9t64lhEpG4kTHzxzh2oVjGTk424 qdU4k8jNcZU+0LSXvhGB4NhRwsG8xfcX6vmE8P7QVBb63wF0EEmjHu2Celd8M5j2+ksn ag2s8jQnskjwvrwGWoIrCYu+yO/fh/kG3Yrxmmg9w0C3KNxwJqLZ9r/K1dPGRMrYnVLE 3Gbw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linutronix.de header.s=2020 header.b=u9kgxt8S; dkim=neutral (no key) header.i=@linutronix.de header.s=2020e header.b="eA/edTzH"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=linutronix.de Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id c9-20020aa7d609000000b005222252bfafsi2455000edr.547.2023.07.24.06.39.39; Mon, 24 Jul 2023 06:40:03 -0700 (PDT) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@linutronix.de header.s=2020 header.b=u9kgxt8S; dkim=neutral (no key) header.i=@linutronix.de header.s=2020e header.b="eA/edTzH"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=linutronix.de Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231153AbjGXNid (ORCPT <rfc822;kautuk.consul.80@gmail.com> + 99 others); Mon, 24 Jul 2023 09:38:33 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37424 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231738AbjGXNhz (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 24 Jul 2023 09:37:55 -0400 Received: from galois.linutronix.de (Galois.linutronix.de [193.142.43.55]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1D70E213E for <linux-kernel@vger.kernel.org>; Mon, 24 Jul 2023 06:36:30 -0700 (PDT) Message-ID: <20230724132047.554355840@linutronix.de> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1690205711; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: references:references; bh=u1MBAiZCltHmWzuaN06FA6y4i5aqgal/njyPH8pNWsw=; b=u9kgxt8SOBounsX3W7OIiLnRFhv3i25Y2kpEWL/veN4l+tSBaQ/APwQdgMMBK6wBDqypOH 0ibngDw1lcb/EqdfCfXK4lVgqQmhb7uLfewv/XZuLmI7u2SRFCsKI24f9s/r4m2sud2cht 5Y69rknWEkW4SDcKh+QHMbV+467JeRd8kUDZ8qsc+mtWlylAQjR/JRiNrRbJdL5qPFGEAu no+51QdXU2f6NhcjkGj4KMxXFzb+xsOcdaf34eiI4Ci053O//HGW+08KKXmamdXQXonWvQ 7/UvO45itPBejqF5nGUnDY0IjtUKJob+HuBplmD26vtWduWNFPGXjRkPGn+kew== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1690205711; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: references:references; bh=u1MBAiZCltHmWzuaN06FA6y4i5aqgal/njyPH8pNWsw=; b=eA/edTzH+XyteYt7cDp/D6iynyMj8pju/Xb3PmjQNy0fXdzELBYW2tSIzNSHjD5d5ujCWT u2yXkPTA0MBiY2AQ== From: Thomas Gleixner <tglx@linutronix.de> To: LKML <linux-kernel@vger.kernel.org> Cc: x86@kernel.org, Andrew Cooper <andrew.cooper3@citrix.com>, Tom Lendacky <thomas.lendacky@amd.com>, Paolo Bonzini <pbonzini@redhat.com>, Wei Liu <wei.liu@kernel.org>, Arjan van de Ven <arjan@linux.intel.com>, Juergen Gross <jgross@suse.com>, Michael Kelley <mikelley@microsoft.com>, Peter Keresztes Schmidt <peter@keresztesschmidt.de>, "Peter Zijlstra (Intel)" <peterz@infradead.org> Subject: [patch V2 50/58] x86/apic: Provide common init infrastructure References: <20230724131206.500814398@linutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Date: Mon, 24 Jul 2023 15:35:10 +0200 (CEST) X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE 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 X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1772309450089212060 X-GMAIL-MSGID: 1772309450089212060 |
Series |
x86/apic: Decrapification and static calls
|
|
Commit Message
Thomas Gleixner
July 24, 2023, 1:35 p.m. UTC
In preparation for converting the hotpath APIC callbacks to static keys, provide common initialization inforastructure. Lift apic_install_drivers() from probe_64.c and convert all places which switch the apic instance by storing the pointer to use apic_install_driver() as a first step. Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- arch/x86/include/asm/apic.h | 2 + arch/x86/kernel/apic/Makefile | 2 - arch/x86/kernel/apic/apic.c | 31 ----------------------- arch/x86/kernel/apic/apic_flat_64.c | 6 ---- arch/x86/kernel/apic/bigsmp_32.c | 6 +--- arch/x86/kernel/apic/init.c | 47 ++++++++++++++++++++++++++++++++++++ arch/x86/kernel/apic/probe_32.c | 5 +-- arch/x86/kernel/apic/probe_64.c | 13 --------- arch/x86/xen/apic.c | 10 ++----- 9 files changed, 59 insertions(+), 63 deletions(-)
Comments
On 24.07.23 15:35, Thomas Gleixner wrote: > In preparation for converting the hotpath APIC callbacks to static keys, > provide common initialization inforastructure. > > Lift apic_install_drivers() from probe_64.c and convert all places which > switch the apic instance by storing the pointer to use apic_install_driver() > as a first step. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > arch/x86/include/asm/apic.h | 2 + > arch/x86/kernel/apic/Makefile | 2 - > arch/x86/kernel/apic/apic.c | 31 ----------------------- > arch/x86/kernel/apic/apic_flat_64.c | 6 ---- > arch/x86/kernel/apic/bigsmp_32.c | 6 +--- > arch/x86/kernel/apic/init.c | 47 ++++++++++++++++++++++++++++++++++++ > arch/x86/kernel/apic/probe_32.c | 5 +-- > arch/x86/kernel/apic/probe_64.c | 13 --------- > arch/x86/xen/apic.c | 10 ++----- > 9 files changed, 59 insertions(+), 63 deletions(-) > > --- a/arch/x86/include/asm/apic.h > +++ b/arch/x86/include/asm/apic.h > @@ -344,6 +344,8 @@ extern int lapic_can_unplug_cpu(void); > > #ifdef CONFIG_X86_LOCAL_APIC > > +void __init apic_install_driver(struct apic *driver); > + > static inline u32 apic_read(u32 reg) > { > return apic->read(reg); > --- a/arch/x86/kernel/apic/Makefile > +++ b/arch/x86/kernel/apic/Makefile > @@ -7,7 +7,7 @@ > # In particualr, smp_apic_timer_interrupt() is called in random places. > KCOV_INSTRUMENT := n > > -obj-$(CONFIG_X86_LOCAL_APIC) += apic.o apic_common.o apic_noop.o ipi.o vector.o > +obj-$(CONFIG_X86_LOCAL_APIC) += apic.o apic_common.o apic_noop.o ipi.o vector.o init.o > obj-y += hw_nmi.o > > obj-$(CONFIG_X86_IO_APIC) += io_apic.o > --- a/arch/x86/kernel/apic/apic.c > +++ b/arch/x86/kernel/apic/apic.c > @@ -236,8 +236,7 @@ static int modern_apic(void) > */ > static void __init apic_disable(void) > { > - pr_info("APIC: switched to apic NOOP\n"); > - apic = &apic_noop; > + apic_install_driver(&apic_noop); > } > > void native_apic_icr_write(u32 low, u32 id) > @@ -2486,34 +2485,6 @@ u32 x86_msi_msg_get_destid(struct msi_ms > } > EXPORT_SYMBOL_GPL(x86_msi_msg_get_destid); > > -#ifdef CONFIG_X86_64 > -void __init acpi_wake_cpu_handler_update(wakeup_cpu_handler handler) > -{ > - struct apic **drv; > - > - for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) > - (*drv)->wakeup_secondary_cpu_64 = handler; > -} > -#endif > - > -/* > - * Override the generic EOI implementation with an optimized version. > - * Only called during early boot when only one CPU is active and with > - * interrupts disabled, so we know this does not race with actual APIC driver > - * use. > - */ > -void __init apic_set_eoi_cb(void (*eoi)(void)) > -{ > - struct apic **drv; > - > - for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) { > - /* Should happen once for each apic */ > - WARN_ON((*drv)->eoi == eoi); > - (*drv)->native_eoi = (*drv)->eoi; > - (*drv)->eoi = eoi; > - } > -} > - > static void __init apic_bsp_up_setup(void) > { > #ifdef CONFIG_X86_64 > --- a/arch/x86/kernel/apic/apic_flat_64.c > +++ b/arch/x86/kernel/apic/apic_flat_64.c > @@ -143,11 +143,7 @@ static int physflat_acpi_madt_oem_check( > > static int physflat_probe(void) > { > - if (apic == &apic_physflat || num_possible_cpus() > 8 || > - jailhouse_paravirt()) > - return 1; > - > - return 0; > + return apic == &apic_physflat || num_possible_cpus() > 8 || jailhouse_paravirt(); > } > > static struct apic apic_physflat __ro_after_init = { > --- a/arch/x86/kernel/apic/bigsmp_32.c > +++ b/arch/x86/kernel/apic/bigsmp_32.c > @@ -119,10 +119,8 @@ bool __init apic_bigsmp_possible(bool cm > > void __init apic_bigsmp_force(void) > { > - if (apic != &apic_bigsmp) { > - apic = &apic_bigsmp; > - pr_info("Overriding APIC driver with bigsmp\n"); > - } > + if (apic != &apic_bigsmp) > + apic_install_driver(&apic_bigsmp); > } > > apic_driver(apic_bigsmp); > --- /dev/null > +++ b/arch/x86/kernel/apic/init.c > @@ -0,0 +1,47 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +#define pr_fmt(fmt) "APIC: " fmt > + > +#include <asm/apic.h> > + > +#include "local.h" > + > +void __init apic_install_driver(struct apic *driver) > +{ > + if (apic == driver) > + return; > + > + apic = driver; > + > + if (IS_ENABLED(CONFIG_X86_X2APIC) && apic->x2apic_set_max_apicid) > + apic->max_apic_id = x2apic_max_apicid; > + > + pr_info("Switched APIC routing to: %s\n", driver->name); > +} > + > +#ifdef CONFIG_X86_64 > +void __init acpi_wake_cpu_handler_update(wakeup_cpu_handler handler) > +{ > + struct apic **drv; > + > + for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) > + (*drv)->wakeup_secondary_cpu_64 = handler; > +} > +#endif > + > +/* > + * Override the generic EOI implementation with an optimized version. > + * Only called during early boot when only one CPU is active and with > + * interrupts disabled, so we know this does not race with actual APIC driver > + * use. > + */ > +void __init apic_set_eoi_cb(void (*eoi)(void)) > +{ > + struct apic **drv; > + > + for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) { > + /* Should happen once for each apic */ > + WARN_ON((*drv)->eoi == eoi); > + (*drv)->native_eoi = (*drv)->eoi; > + (*drv)->eoi = eoi; > + } > +} > --- a/arch/x86/kernel/apic/probe_32.c > +++ b/arch/x86/kernel/apic/probe_32.c > @@ -82,7 +82,7 @@ static int __init parse_apic(char *arg) > > for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) { > if (!strcmp((*drv)->name, arg)) { > - apic = *drv; > + apic_install_driver(*drv); > cmdline_apic = 1; > return 0; > } > @@ -129,7 +129,7 @@ void __init x86_32_probe_apic(void) > > for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) { > if ((*drv)->probe()) { > - apic = *drv; > + apic_install_driver(*drv); > break; > } > } > @@ -137,5 +137,4 @@ void __init x86_32_probe_apic(void) > if (drv == __apicdrivers_end) > panic("Didn't find an APIC driver"); > } > - printk(KERN_INFO "Using APIC driver %s\n", apic->name); > } > --- a/arch/x86/kernel/apic/probe_64.c > +++ b/arch/x86/kernel/apic/probe_64.c > @@ -13,19 +13,6 @@ > > #include "local.h" > > -static __init void apic_install_driver(struct apic *driver) > -{ > - if (apic == driver) > - return; > - > - apic = driver; > - > - if (IS_ENABLED(CONFIG_X86_X2APIC) && apic->x2apic_set_max_apicid) > - apic->max_apic_id = x2apic_max_apicid; > - > - pr_info("Switched APIC routing to %s:\n", apic->name); > -} > - > /* Select the appropriate APIC driver */ > void __init x86_64_probe_apic(void) > { > --- a/arch/x86/xen/apic.c > +++ b/arch/x86/xen/apic.c > @@ -160,20 +160,16 @@ static struct apic xen_pv_apic = { > > static void __init xen_apic_check(void) > { > - if (apic == &xen_pv_apic) > - return; > - > - pr_info("Switched APIC routing from %s to %s.\n", apic->name, > - xen_pv_apic.name); > - apic = &xen_pv_apic; > + apic_install_driver(&xen_pv_apic); > } > + > void __init xen_init_apic(void) > { > x86_apic_ops.io_apic_read = xen_io_apic_read; > /* On PV guests the APIC CPUID bit is disabled so none of the > * routines end up executing. */ > if (!xen_initial_domain()) > - apic = &xen_pv_apic; > + apic_install_driver(&xen_pv_apic); This is working, but it produces a WARN() splat when booting as an unprivileged Xen PV guest from static_call patching (static_call_init() hasn't been called yet). The diff below on top is fixing the issue: diff --git a/arch/x86/xen/apic.c b/arch/x86/xen/apic.c index 1838aefc632f..84f24268670b 100644 --- a/arch/x86/xen/apic.c +++ b/arch/x86/xen/apic.c @@ -163,14 +163,18 @@ static void __init xen_apic_check(void) apic_install_driver(&xen_pv_apic); } +void __init xen_apic_install(void) +{ + /* + * On PV guests the APIC CPUID bit is disabled so none of the + * routines end up executing. + */ + apic_install_driver(&xen_pv_apic); +} + void __init xen_init_apic(void) { x86_apic_ops.io_apic_read = xen_io_apic_read; - /* On PV guests the APIC CPUID bit is disabled so none of the - * routines end up executing. */ - if (!xen_initial_domain()) - apic_install_driver(&xen_pv_apic); - x86_platform.apic_post_init = xen_apic_check; } apic_driver(xen_pv_apic); diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c index c6b42c66c60c..ff2d0754ce62 100644 --- a/arch/x86/xen/smp_pv.c +++ b/arch/x86/xen/smp_pv.c @@ -188,6 +188,9 @@ static void __init _get_smp_config(unsigned int early) static void __init xen_pv_smp_prepare_boot_cpu(void) { BUG_ON(smp_processor_id() != 0); + + xen_apic_install(); + native_smp_prepare_boot_cpu(); if (!xen_feature(XENFEAT_writable_page_tables)) diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h index 408a2aa66c69..217e4b625e4d 100644 --- a/arch/x86/xen/xen-ops.h +++ b/arch/x86/xen/xen-ops.h @@ -119,6 +119,7 @@ static inline void __init xen_init_vga(const struct dom0_vga_console_info *info, void xen_add_preferred_consoles(void); void __init xen_init_apic(void); +void __init xen_apic_install(void); #ifdef CONFIG_XEN_EFI extern void xen_efi_init(struct boot_params *boot_params); Juergen
On Mon, Jul 31 2023 at 13:31, Juergen Gross wrote: > On 24.07.23 15:35, Thomas Gleixner wrote: >> static void __init xen_apic_check(void) >> { >> - if (apic == &xen_pv_apic) >> - return; >> - >> - pr_info("Switched APIC routing from %s to %s.\n", apic->name, >> - xen_pv_apic.name); >> - apic = &xen_pv_apic; >> + apic_install_driver(&xen_pv_apic); >> } >> + >> void __init xen_init_apic(void) >> { >> x86_apic_ops.io_apic_read = xen_io_apic_read; >> /* On PV guests the APIC CPUID bit is disabled so none of the >> * routines end up executing. */ >> if (!xen_initial_domain()) >> - apic = &xen_pv_apic; >> + apic_install_driver(&xen_pv_apic); > > This is working, but it produces a WARN() splat when booting as an unprivileged > Xen PV guest from static_call patching (static_call_init() hasn't been called > yet). Duh, yes. It's too early. > The diff below on top is fixing the issue: > > diff --git a/arch/x86/xen/apic.c b/arch/x86/xen/apic.c > index 1838aefc632f..84f24268670b 100644 > --- a/arch/x86/xen/apic.c > +++ b/arch/x86/xen/apic.c > @@ -163,14 +163,18 @@ static void __init xen_apic_check(void) > apic_install_driver(&xen_pv_apic); > } > > +void __init xen_apic_install(void) > +{ > + /* > + * On PV guests the APIC CPUID bit is disabled so none of the > + * routines end up executing. > + */ > + apic_install_driver(&xen_pv_apic); > +} > + > void __init xen_init_apic(void) > { > x86_apic_ops.io_apic_read = xen_io_apic_read; > - /* On PV guests the APIC CPUID bit is disabled so none of the > - * routines end up executing. */ > - if (!xen_initial_domain()) > - apic_install_driver(&xen_pv_apic); > - > x86_platform.apic_post_init = xen_apic_check; > } > apic_driver(xen_pv_apic); I wonder whether this explicit install is actually needed at all. Shouldn't the driver be installed via the APIC probing mechanism automagically? Thanks, tglx
On 31.07.23 15:01, Thomas Gleixner wrote: > On Mon, Jul 31 2023 at 13:31, Juergen Gross wrote: >> On 24.07.23 15:35, Thomas Gleixner wrote: >>> static void __init xen_apic_check(void) >>> { >>> - if (apic == &xen_pv_apic) >>> - return; >>> - >>> - pr_info("Switched APIC routing from %s to %s.\n", apic->name, >>> - xen_pv_apic.name); >>> - apic = &xen_pv_apic; >>> + apic_install_driver(&xen_pv_apic); >>> } >>> + >>> void __init xen_init_apic(void) >>> { >>> x86_apic_ops.io_apic_read = xen_io_apic_read; >>> /* On PV guests the APIC CPUID bit is disabled so none of the >>> * routines end up executing. */ >>> if (!xen_initial_domain()) >>> - apic = &xen_pv_apic; >>> + apic_install_driver(&xen_pv_apic); >> >> This is working, but it produces a WARN() splat when booting as an unprivileged >> Xen PV guest from static_call patching (static_call_init() hasn't been called >> yet). > > Duh, yes. It's too early. > >> The diff below on top is fixing the issue: >> >> diff --git a/arch/x86/xen/apic.c b/arch/x86/xen/apic.c >> index 1838aefc632f..84f24268670b 100644 >> --- a/arch/x86/xen/apic.c >> +++ b/arch/x86/xen/apic.c >> @@ -163,14 +163,18 @@ static void __init xen_apic_check(void) >> apic_install_driver(&xen_pv_apic); >> } >> >> +void __init xen_apic_install(void) >> +{ >> + /* >> + * On PV guests the APIC CPUID bit is disabled so none of the >> + * routines end up executing. >> + */ >> + apic_install_driver(&xen_pv_apic); >> +} >> + >> void __init xen_init_apic(void) >> { >> x86_apic_ops.io_apic_read = xen_io_apic_read; >> - /* On PV guests the APIC CPUID bit is disabled so none of the >> - * routines end up executing. */ >> - if (!xen_initial_domain()) >> - apic_install_driver(&xen_pv_apic); >> - >> x86_platform.apic_post_init = xen_apic_check; >> } >> apic_driver(xen_pv_apic); > > I wonder whether this explicit install is actually needed at all. > Shouldn't the driver be installed via the APIC probing mechanism > automagically? Only in case x86_init.irq.intr_mode_init is set appropriately. Today it is a nop for Xen PV, but that can be changed. I'll have a look. Juergen
On Mon, Jul 31 2023 at 15:10, Juergen Gross wrote: > On 31.07.23 15:01, Thomas Gleixner wrote: >>> apic_driver(xen_pv_apic); >> >> I wonder whether this explicit install is actually needed at all. >> Shouldn't the driver be installed via the APIC probing mechanism >> automagically? > > Only in case x86_init.irq.intr_mode_init is set appropriately. Today it is > a nop for Xen PV, but that can be changed. I'll have a look. You could simply set that callback to default_setup_apic_routing() and be done with it.
On Mon, Jul 31 2023 at 17:48, Thomas Gleixner wrote: > On Mon, Jul 31 2023 at 15:10, Juergen Gross wrote: >> On 31.07.23 15:01, Thomas Gleixner wrote: >>>> apic_driver(xen_pv_apic); >>> >>> I wonder whether this explicit install is actually needed at all. >>> Shouldn't the driver be installed via the APIC probing mechanism >>> automagically? >> >> Only in case x86_init.irq.intr_mode_init is set appropriately. Today it is >> a nop for Xen PV, but that can be changed. I'll have a look. > > You could simply set that callback to default_setup_apic_routing() and > be done with it. Doesn't work because XEN overrides it already. So sure, lets just go with the solution you proposed. One more ugly or less in XEN/PV does not really matter much :) Let me grab this and put it into the right position in the queue. Thanks, tglx
On 01.08.23 08:41, Thomas Gleixner wrote: > On Mon, Jul 31 2023 at 17:48, Thomas Gleixner wrote: > >> On Mon, Jul 31 2023 at 15:10, Juergen Gross wrote: >>> On 31.07.23 15:01, Thomas Gleixner wrote: >>>>> apic_driver(xen_pv_apic); >>>> >>>> I wonder whether this explicit install is actually needed at all. >>>> Shouldn't the driver be installed via the APIC probing mechanism >>>> automagically? >>> >>> Only in case x86_init.irq.intr_mode_init is set appropriately. Today it is >>> a nop for Xen PV, but that can be changed. I'll have a look. >> >> You could simply set that callback to default_setup_apic_routing() and >> be done with it. > > Doesn't work because XEN overrides it already. So sure, lets just go It is overriding it with x86_init_noop(). > with the solution you proposed. One more ugly or less in XEN/PV does not > really matter much :) > > Let me grab this and put it into the right position in the queue. Wait a few minutes, please. I'm just about to test your suggestion. Juergen
On 01.08.23 08:49, Juergen Gross wrote: > On 01.08.23 08:41, Thomas Gleixner wrote: >> On Mon, Jul 31 2023 at 17:48, Thomas Gleixner wrote: >> >>> On Mon, Jul 31 2023 at 15:10, Juergen Gross wrote: >>>> On 31.07.23 15:01, Thomas Gleixner wrote: >>>>>> apic_driver(xen_pv_apic); >>>>> >>>>> I wonder whether this explicit install is actually needed at all. >>>>> Shouldn't the driver be installed via the APIC probing mechanism >>>>> automagically? >>>> >>>> Only in case x86_init.irq.intr_mode_init is set appropriately. Today it is >>>> a nop for Xen PV, but that can be changed. I'll have a look. >>> >>> You could simply set that callback to default_setup_apic_routing() and >>> be done with it. >> >> Doesn't work because XEN overrides it already. So sure, lets just go > > It is overriding it with x86_init_noop(). > >> with the solution you proposed. One more ugly or less in XEN/PV does not >> really matter much :) >> >> Let me grab this and put it into the right position in the queue. > > Wait a few minutes, please. I'm just about to test your suggestion. Using the following diff on top of your patch is working fine: diff --git a/arch/x86/xen/apic.c b/arch/x86/xen/apic.c index 804a26b7c85e..d7743ba0212d 100644 --- a/arch/x86/xen/apic.c +++ b/arch/x86/xen/apic.c @@ -166,11 +166,6 @@ static void __init xen_apic_check(void) void __init xen_init_apic(void) { x86_apic_ops.io_apic_read = xen_io_apic_read; - /* On PV guests the APIC CPUID bit is disabled so none of the - * routines end up executing. */ - if (!xen_initial_domain()) - apic_install_driver(&xen_pv_apic); - x86_platform.apic_post_init = xen_apic_check; } apic_driver(xen_pv_apic); diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c index 93b658248d01..164e5be23a45 100644 --- a/arch/x86/xen/enlighten_pv.c +++ b/arch/x86/xen/enlighten_pv.c @@ -1326,7 +1326,7 @@ asmlinkage __visible void __init xen_start_kernel(struct start_info *si) x86_init.resources.memory_setup = xen_memory_setup; x86_init.irqs.intr_mode_select = x86_init_noop; - x86_init.irqs.intr_mode_init = x86_init_noop; + x86_init.irqs.intr_mode_init = x86_64_probe_apic; x86_init.oem.arch_setup = xen_arch_setup; x86_init.oem.banner = xen_banner; x86_init.hyper.init_platform = xen_pv_init_platform; Juergen
On Tue, Aug 01 2023 at 08:41, Thomas Gleixner wrote: > On Mon, Jul 31 2023 at 17:48, Thomas Gleixner wrote: > >> On Mon, Jul 31 2023 at 15:10, Juergen Gross wrote: >>> On 31.07.23 15:01, Thomas Gleixner wrote: >>>>> apic_driver(xen_pv_apic); >>>> >>>> I wonder whether this explicit install is actually needed at all. >>>> Shouldn't the driver be installed via the APIC probing mechanism >>>> automagically? >>> >>> Only in case x86_init.irq.intr_mode_init is set appropriately. Today it is >>> a nop for Xen PV, but that can be changed. I'll have a look. >> >> You could simply set that callback to default_setup_apic_routing() and >> be done with it. > > Doesn't work because XEN overrides it already. So sure, lets just go > with the solution you proposed. One more ugly or less in XEN/PV does not > really matter much :) > > Let me grab this and put it into the right position in the queue. Spoke too early. The below should work AFAICT from the code. --- a/arch/x86/xen/apic.c +++ b/arch/x86/xen/apic.c @@ -99,10 +99,7 @@ static void xen_apic_icr_write(u32 low, static int xen_apic_probe_pv(void) { - if (xen_pv_domain()) - return 1; - - return 0; + return xen_pv_domain(); } static int xen_madt_oem_check(char *oem_id, char *oem_table_id) @@ -157,20 +154,14 @@ static struct apic xen_pv_apic = { .icr_read = xen_apic_icr_read, .icr_write = xen_apic_icr_write, }; - -static void __init xen_apic_check(void) -{ - apic_install_driver(&xen_pv_apic); -} +apic_driver(xen_pv_apic); void __init xen_init_apic(void) { x86_apic_ops.io_apic_read = xen_io_apic_read; - /* On PV guests the APIC CPUID bit is disabled so none of the - * routines end up executing. */ - if (!xen_initial_domain()) - apic_install_driver(&xen_pv_apic); - - x86_platform.apic_post_init = xen_apic_check; + /* + * On PV guests the APIC CPUID bit is disabled so none of the + * routines end up executing. + */ + apic_install_driver(&xen_pv_apic); } -apic_driver(xen_pv_apic); --- a/arch/x86/xen/enlighten_pv.c +++ b/arch/x86/xen/enlighten_pv.c @@ -1325,7 +1325,7 @@ asmlinkage __visible void __init xen_sta x86_platform.realmode_init = x86_init_noop; x86_init.resources.memory_setup = xen_memory_setup; - x86_init.irqs.intr_mode_select = x86_init_noop; + x86_init.irqs.intr_mode_select = xen_init_apic; x86_init.irqs.intr_mode_init = x86_init_noop; x86_init.oem.arch_setup = xen_arch_setup; x86_init.oem.banner = xen_banner; @@ -1366,13 +1366,6 @@ asmlinkage __visible void __init xen_sta xen_init_capabilities(); -#ifdef CONFIG_X86_LOCAL_APIC - /* - * set up the basic apic ops. - */ - xen_init_apic(); -#endif - machine_ops = xen_machine_ops; /* --- a/arch/x86/xen/xen-ops.h +++ b/arch/x86/xen/xen-ops.h @@ -118,7 +118,11 @@ static inline void __init xen_init_vga(c void xen_add_preferred_consoles(void); +#ifdef CONFIG_X86_LOCAL_APIC void __init xen_init_apic(void); +#else +# define xen_init_apic x86_init_noop +#endif #ifdef CONFIG_XEN_EFI extern void xen_efi_init(struct boot_params *boot_params);
On Tue, Aug 01 2023 at 09:08, Juergen Gross wrote: > On 01.08.23 08:49, Juergen Gross wrote: > void __init xen_init_apic(void) > { > x86_apic_ops.io_apic_read = xen_io_apic_read; > - /* On PV guests the APIC CPUID bit is disabled so none of the > - * routines end up executing. */ > - if (!xen_initial_domain()) > - apic_install_driver(&xen_pv_apic); > - > x86_platform.apic_post_init = xen_apic_check; I don't think this one is needed.
On 01.08.23 09:32, Thomas Gleixner wrote: > On Tue, Aug 01 2023 at 09:08, Juergen Gross wrote: >> On 01.08.23 08:49, Juergen Gross wrote: >> void __init xen_init_apic(void) >> { >> x86_apic_ops.io_apic_read = xen_io_apic_read; >> - /* On PV guests the APIC CPUID bit is disabled so none of the >> - * routines end up executing. */ >> - if (!xen_initial_domain()) >> - apic_install_driver(&xen_pv_apic); >> - >> x86_platform.apic_post_init = xen_apic_check; > > I don't think this one is needed. Indeed. Juergen
On Tue, Aug 01 2023 at 09:37, Juergen Gross wrote: > On 01.08.23 09:32, Thomas Gleixner wrote: >> On Tue, Aug 01 2023 at 09:08, Juergen Gross wrote: >>> On 01.08.23 08:49, Juergen Gross wrote: >>> void __init xen_init_apic(void) >>> { >>> x86_apic_ops.io_apic_read = xen_io_apic_read; >>> - /* On PV guests the APIC CPUID bit is disabled so none of the >>> - * routines end up executing. */ >>> - if (!xen_initial_domain()) >>> - apic_install_driver(&xen_pv_apic); >>> - >>> x86_platform.apic_post_init = xen_apic_check; >> >> I don't think this one is needed. > > Indeed. Can you send a real patch please which I can add to that pile at the right place? Thanks, tglx
On 01.08.23 10:23, Thomas Gleixner wrote: > On Tue, Aug 01 2023 at 09:37, Juergen Gross wrote: >> On 01.08.23 09:32, Thomas Gleixner wrote: >>> On Tue, Aug 01 2023 at 09:08, Juergen Gross wrote: >>>> On 01.08.23 08:49, Juergen Gross wrote: >>>> void __init xen_init_apic(void) >>>> { >>>> x86_apic_ops.io_apic_read = xen_io_apic_read; >>>> - /* On PV guests the APIC CPUID bit is disabled so none of the >>>> - * routines end up executing. */ >>>> - if (!xen_initial_domain()) >>>> - apic_install_driver(&xen_pv_apic); >>>> - >>>> x86_platform.apic_post_init = xen_apic_check; >>> >>> I don't think this one is needed. >> >> Indeed. > > Can you send a real patch please which I can add to that pile at the > right place? I think adding it right after patch 50 should be fine? The WARN() will be issued only with patch 58. Juergen
On Tue, Aug 01 2023 at 10:25, Juergen Gross wrote: > On 01.08.23 10:23, Thomas Gleixner wrote: >> On Tue, Aug 01 2023 at 09:37, Juergen Gross wrote: >>> On 01.08.23 09:32, Thomas Gleixner wrote: >>>> On Tue, Aug 01 2023 at 09:08, Juergen Gross wrote: >>>>> On 01.08.23 08:49, Juergen Gross wrote: >>>>> void __init xen_init_apic(void) >>>>> { >>>>> x86_apic_ops.io_apic_read = xen_io_apic_read; >>>>> - /* On PV guests the APIC CPUID bit is disabled so none of the >>>>> - * routines end up executing. */ >>>>> - if (!xen_initial_domain()) >>>>> - apic_install_driver(&xen_pv_apic); >>>>> - >>>>> x86_platform.apic_post_init = xen_apic_check; >>>> >>>> I don't think this one is needed. >>> >>> Indeed. >> >> Can you send a real patch please which I can add to that pile at the >> right place? > > I think adding it right after patch 50 should be fine? > > The WARN() will be issued only with patch 58. Correct.
On 01.08.23 10:54, Thomas Gleixner wrote: > On Tue, Aug 01 2023 at 10:25, Juergen Gross wrote: >> On 01.08.23 10:23, Thomas Gleixner wrote: >>> On Tue, Aug 01 2023 at 09:37, Juergen Gross wrote: >>>> On 01.08.23 09:32, Thomas Gleixner wrote: >>>>> On Tue, Aug 01 2023 at 09:08, Juergen Gross wrote: >>>>>> On 01.08.23 08:49, Juergen Gross wrote: >>>>>> void __init xen_init_apic(void) >>>>>> { >>>>>> x86_apic_ops.io_apic_read = xen_io_apic_read; >>>>>> - /* On PV guests the APIC CPUID bit is disabled so none of the >>>>>> - * routines end up executing. */ >>>>>> - if (!xen_initial_domain()) >>>>>> - apic_install_driver(&xen_pv_apic); >>>>>> - >>>>>> x86_platform.apic_post_init = xen_apic_check; >>>>> >>>>> I don't think this one is needed. >>>> >>>> Indeed. >>> >>> Can you send a real patch please which I can add to that pile at the >>> right place? >> >> I think adding it right after patch 50 should be fine? >> >> The WARN() will be issued only with patch 58. > > Correct. Here it is. Juergen
--- a/arch/x86/include/asm/apic.h +++ b/arch/x86/include/asm/apic.h @@ -344,6 +344,8 @@ extern int lapic_can_unplug_cpu(void); #ifdef CONFIG_X86_LOCAL_APIC +void __init apic_install_driver(struct apic *driver); + static inline u32 apic_read(u32 reg) { return apic->read(reg); --- a/arch/x86/kernel/apic/Makefile +++ b/arch/x86/kernel/apic/Makefile @@ -7,7 +7,7 @@ # In particualr, smp_apic_timer_interrupt() is called in random places. KCOV_INSTRUMENT := n -obj-$(CONFIG_X86_LOCAL_APIC) += apic.o apic_common.o apic_noop.o ipi.o vector.o +obj-$(CONFIG_X86_LOCAL_APIC) += apic.o apic_common.o apic_noop.o ipi.o vector.o init.o obj-y += hw_nmi.o obj-$(CONFIG_X86_IO_APIC) += io_apic.o --- a/arch/x86/kernel/apic/apic.c +++ b/arch/x86/kernel/apic/apic.c @@ -236,8 +236,7 @@ static int modern_apic(void) */ static void __init apic_disable(void) { - pr_info("APIC: switched to apic NOOP\n"); - apic = &apic_noop; + apic_install_driver(&apic_noop); } void native_apic_icr_write(u32 low, u32 id) @@ -2486,34 +2485,6 @@ u32 x86_msi_msg_get_destid(struct msi_ms } EXPORT_SYMBOL_GPL(x86_msi_msg_get_destid); -#ifdef CONFIG_X86_64 -void __init acpi_wake_cpu_handler_update(wakeup_cpu_handler handler) -{ - struct apic **drv; - - for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) - (*drv)->wakeup_secondary_cpu_64 = handler; -} -#endif - -/* - * Override the generic EOI implementation with an optimized version. - * Only called during early boot when only one CPU is active and with - * interrupts disabled, so we know this does not race with actual APIC driver - * use. - */ -void __init apic_set_eoi_cb(void (*eoi)(void)) -{ - struct apic **drv; - - for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) { - /* Should happen once for each apic */ - WARN_ON((*drv)->eoi == eoi); - (*drv)->native_eoi = (*drv)->eoi; - (*drv)->eoi = eoi; - } -} - static void __init apic_bsp_up_setup(void) { #ifdef CONFIG_X86_64 --- a/arch/x86/kernel/apic/apic_flat_64.c +++ b/arch/x86/kernel/apic/apic_flat_64.c @@ -143,11 +143,7 @@ static int physflat_acpi_madt_oem_check( static int physflat_probe(void) { - if (apic == &apic_physflat || num_possible_cpus() > 8 || - jailhouse_paravirt()) - return 1; - - return 0; + return apic == &apic_physflat || num_possible_cpus() > 8 || jailhouse_paravirt(); } static struct apic apic_physflat __ro_after_init = { --- a/arch/x86/kernel/apic/bigsmp_32.c +++ b/arch/x86/kernel/apic/bigsmp_32.c @@ -119,10 +119,8 @@ bool __init apic_bigsmp_possible(bool cm void __init apic_bigsmp_force(void) { - if (apic != &apic_bigsmp) { - apic = &apic_bigsmp; - pr_info("Overriding APIC driver with bigsmp\n"); - } + if (apic != &apic_bigsmp) + apic_install_driver(&apic_bigsmp); } apic_driver(apic_bigsmp); --- /dev/null +++ b/arch/x86/kernel/apic/init.c @@ -0,0 +1,47 @@ +// SPDX-License-Identifier: GPL-2.0-only +#define pr_fmt(fmt) "APIC: " fmt + +#include <asm/apic.h> + +#include "local.h" + +void __init apic_install_driver(struct apic *driver) +{ + if (apic == driver) + return; + + apic = driver; + + if (IS_ENABLED(CONFIG_X86_X2APIC) && apic->x2apic_set_max_apicid) + apic->max_apic_id = x2apic_max_apicid; + + pr_info("Switched APIC routing to: %s\n", driver->name); +} + +#ifdef CONFIG_X86_64 +void __init acpi_wake_cpu_handler_update(wakeup_cpu_handler handler) +{ + struct apic **drv; + + for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) + (*drv)->wakeup_secondary_cpu_64 = handler; +} +#endif + +/* + * Override the generic EOI implementation with an optimized version. + * Only called during early boot when only one CPU is active and with + * interrupts disabled, so we know this does not race with actual APIC driver + * use. + */ +void __init apic_set_eoi_cb(void (*eoi)(void)) +{ + struct apic **drv; + + for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) { + /* Should happen once for each apic */ + WARN_ON((*drv)->eoi == eoi); + (*drv)->native_eoi = (*drv)->eoi; + (*drv)->eoi = eoi; + } +} --- a/arch/x86/kernel/apic/probe_32.c +++ b/arch/x86/kernel/apic/probe_32.c @@ -82,7 +82,7 @@ static int __init parse_apic(char *arg) for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) { if (!strcmp((*drv)->name, arg)) { - apic = *drv; + apic_install_driver(*drv); cmdline_apic = 1; return 0; } @@ -129,7 +129,7 @@ void __init x86_32_probe_apic(void) for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) { if ((*drv)->probe()) { - apic = *drv; + apic_install_driver(*drv); break; } } @@ -137,5 +137,4 @@ void __init x86_32_probe_apic(void) if (drv == __apicdrivers_end) panic("Didn't find an APIC driver"); } - printk(KERN_INFO "Using APIC driver %s\n", apic->name); } --- a/arch/x86/kernel/apic/probe_64.c +++ b/arch/x86/kernel/apic/probe_64.c @@ -13,19 +13,6 @@ #include "local.h" -static __init void apic_install_driver(struct apic *driver) -{ - if (apic == driver) - return; - - apic = driver; - - if (IS_ENABLED(CONFIG_X86_X2APIC) && apic->x2apic_set_max_apicid) - apic->max_apic_id = x2apic_max_apicid; - - pr_info("Switched APIC routing to %s:\n", apic->name); -} - /* Select the appropriate APIC driver */ void __init x86_64_probe_apic(void) { --- a/arch/x86/xen/apic.c +++ b/arch/x86/xen/apic.c @@ -160,20 +160,16 @@ static struct apic xen_pv_apic = { static void __init xen_apic_check(void) { - if (apic == &xen_pv_apic) - return; - - pr_info("Switched APIC routing from %s to %s.\n", apic->name, - xen_pv_apic.name); - apic = &xen_pv_apic; + apic_install_driver(&xen_pv_apic); } + void __init xen_init_apic(void) { x86_apic_ops.io_apic_read = xen_io_apic_read; /* On PV guests the APIC CPUID bit is disabled so none of the * routines end up executing. */ if (!xen_initial_domain()) - apic = &xen_pv_apic; + apic_install_driver(&xen_pv_apic); x86_platform.apic_post_init = xen_apic_check; }