| Message ID | alpine.DEB.2.21.1811011032190.1642@nanos.tec.linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/build: Build VSMP support only if selected | expand |
On 01/11/18 11:37, Thomas Gleixner wrote: > VSMP support is built even if CONFIG_X86_VSMP is not set. This leads to a build > breakage when CONFIG_PCI is disabled as well. > > Build VSMP code only when selected. This patch disables detect_vsmp_box() on systems without CONFIG_X86_VSMP, due to the recent 6da63eb241a05b0e676d68975e793c0521387141. This is significant regression that will affect significant number of deployments. We will reply shortly with an updated patch that fix the dependency on pv_irq_ops, and revert to CONFIG_PARAVIRT, with proper protection for CONFIG_PCI.
Greetings, On 11/01/2018 12:39 PM, Shai Fultheim (Shai@ScaleMP.com) wrote: > On 01/11/18 11:37, Thomas Gleixner wrote: > >> VSMP support is built even if CONFIG_X86_VSMP is not set. This leads to a build >> breakage when CONFIG_PCI is disabled as well. >> >> Build VSMP code only when selected. > > This patch disables detect_vsmp_box() on systems without CONFIG_X86_VSMP, due to > the recent 6da63eb241a05b0e676d68975e793c0521387141. This is significant > regression that will affect significant number of deployments. > > We will reply shortly with an updated patch that fix the dependency on pv_irq_ops, > and revert to CONFIG_PARAVIRT, with proper protection for CONFIG_PCI. > here is the proper patch which fixes the issue on hand: From ebff534f8cfa55d7c3ab798c44abe879f3fbe2b8 Mon Sep 17 00:00:00 2001 From: Eial Czerwacki <eial@scalemp.com> Date: Thu, 1 Nov 2018 15:08:32 +0200 Subject: [PATCH] x86/build: Build VSMP support only if CONFIG_PCI is selected vsmp dependency of pv_irq_ops removed some years ago, so now let's clean it up from vsmp_64.c. In short, "cap & ctl & (1 << 4)" was always returning 0, as such we can remove all the PARAVIRT/PARAVIRT_XXL code handling that. However, the rest of the code depends on CONFIG_PCI, so fix it accordingly. Signed-off-by: Eial Czerwacki <eial@scalemp.com> Acked-by: Shai Fultheim <shai@scalemp.com> --- arch/x86/Kconfig | 1 - arch/x86/kernel/vsmp_64.c | 80 +++-------------------------------------------- 2 files changed, 5 insertions(+), 76 deletions(-) diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index c51c989..4b187ca 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -524,7 +524,6 @@ config X86_VSMP bool "ScaleMP vSMP" select HYPERVISOR_GUEST select PARAVIRT - select PARAVIRT_XXL depends on X86_64 && PCI depends on X86_EXTENDED_PLATFORM depends on SMP diff --git a/arch/x86/kernel/vsmp_64.c b/arch/x86/kernel/vsmp_64.c index 1eae5af..c6d2b76 100644 --- a/arch/x86/kernel/vsmp_64.c +++ b/arch/x86/kernel/vsmp_64.c @@ -26,64 +26,7 @@ #define TOPOLOGY_REGISTER_OFFSET 0x10 -#if defined CONFIG_PCI && defined CONFIG_PARAVIRT_XXL -/* - * Interrupt control on vSMPowered systems: - * ~AC is a shadow of IF. If IF is 'on' AC should be 'off' - * and vice versa. - */ - -asmlinkage __visible unsigned long vsmp_save_fl(void) -{ - unsigned long flags = native_save_fl(); - - if (!(flags & X86_EFLAGS_IF) || (flags & X86_EFLAGS_AC)) - flags &= ~X86_EFLAGS_IF; - return flags; -} -PV_CALLEE_SAVE_REGS_THUNK(vsmp_save_fl); - -__visible void vsmp_restore_fl(unsigned long flags) -{ - if (flags & X86_EFLAGS_IF) - flags &= ~X86_EFLAGS_AC; - else - flags |= X86_EFLAGS_AC; - native_restore_fl(flags); -} -PV_CALLEE_SAVE_REGS_THUNK(vsmp_restore_fl); - -asmlinkage __visible void vsmp_irq_disable(void) -{ - unsigned long flags = native_save_fl(); - - native_restore_fl((flags & ~X86_EFLAGS_IF) | X86_EFLAGS_AC); -} -PV_CALLEE_SAVE_REGS_THUNK(vsmp_irq_disable); - -asmlinkage __visible void vsmp_irq_enable(void) -{ - unsigned long flags = native_save_fl(); - - native_restore_fl((flags | X86_EFLAGS_IF) & (~X86_EFLAGS_AC)); -} -PV_CALLEE_SAVE_REGS_THUNK(vsmp_irq_enable); - -static unsigned __init vsmp_patch(u8 type, void *ibuf, - unsigned long addr, unsigned len) -{ - switch (type) { - case PARAVIRT_PATCH(irq.irq_enable): - case PARAVIRT_PATCH(irq.irq_disable): - case PARAVIRT_PATCH(irq.save_fl): - case PARAVIRT_PATCH(irq.restore_fl): - return paravirt_patch_default(type, ibuf, addr, len); - default: - return native_patch(type, ibuf, addr, len); - } - -} - +#if defined CONFIG_PCI static void __init set_vsmp_pv_ops(void) { void __iomem *address; @@ -109,28 +52,12 @@ static void __init set_vsmp_pv_ops(void) } #endif - if (cap & ctl & (1 << 4)) { - /* Setup irq ops and turn on vSMP IRQ fastpath handling */ - pv_ops.irq.irq_disable = PV_CALLEE_SAVE(vsmp_irq_disable); - pv_ops.irq.irq_enable = PV_CALLEE_SAVE(vsmp_irq_enable); - pv_ops.irq.save_fl = PV_CALLEE_SAVE(vsmp_save_fl); - pv_ops.irq.restore_fl = PV_CALLEE_SAVE(vsmp_restore_fl); - pv_ops.init.patch = vsmp_patch; - ctl &= ~(1 << 4); - } writel(ctl, address + 4); ctl = readl(address + 4); pr_info("vSMP CTL: control set to:0x%08x\n", ctl); early_iounmap(address, 8); } -#else -static void __init set_vsmp_pv_ops(void) -{ -} -#endif - -#ifdef CONFIG_PCI static int is_vsmp = -1; static void __init detect_vsmp_box(void) @@ -164,11 +91,14 @@ static int is_vsmp_box(void) { return 0; } +static void __init set_vsmp_pv_ops(void) +{ +} #endif static void __init vsmp_cap_cpus(void) { -#if !defined(CONFIG_X86_VSMP) && defined(CONFIG_SMP) +#if !defined(CONFIG_X86_VSMP) && defined(CONFIG_SMP) && defined(CONFIG_PCI) void __iomem *address; unsigned int cfg, topology, node_shift, maxcpus;
On Thu, 1 Nov 2018, Eial Czerwacki wrote: > Greetings, > > On 11/01/2018 12:39 PM, Shai Fultheim (Shai@ScaleMP.com) wrote: > > On 01/11/18 11:37, Thomas Gleixner wrote: > > > >> VSMP support is built even if CONFIG_X86_VSMP is not set. This leads to a build > >> breakage when CONFIG_PCI is disabled as well. > >> > >> Build VSMP code only when selected. > > > > This patch disables detect_vsmp_box() on systems without CONFIG_X86_VSMP, due to > > the recent 6da63eb241a05b0e676d68975e793c0521387141. This is significant > > regression that will affect significant number of deployments. > > > > We will reply shortly with an updated patch that fix the dependency on pv_irq_ops, > > and revert to CONFIG_PARAVIRT, with proper protection for CONFIG_PCI. > > > > here is the proper patch which fixes the issue on hand: > >From ebff534f8cfa55d7c3ab798c44abe879f3fbe2b8 Mon Sep 17 00:00:00 2001 > From: Eial Czerwacki <eial@scalemp.com> > Date: Thu, 1 Nov 2018 15:08:32 +0200 > Subject: [PATCH] x86/build: Build VSMP support only if CONFIG_PCI is > selected > > vsmp dependency of pv_irq_ops removed some years ago, so now let's clean > it up from vsmp_64.c. > > In short, "cap & ctl & (1 << 4)" was always returning 0, as such we can > remove all the PARAVIRT/PARAVIRT_XXL code handling that. > > However, the rest of the code depends on CONFIG_PCI, so fix it accordingly. > > Signed-off-by: Eial Czerwacki <eial@scalemp.com> > Acked-by: Shai Fultheim <shai@scalemp.com> Nice cleanup! Acked-by: Thomas Gleixner <tglx@linutronix.de>
On 01/11/2018 14:10, Eial Czerwacki wrote: > Greetings, > > On 11/01/2018 12:39 PM, Shai Fultheim (Shai@ScaleMP.com) wrote: >> On 01/11/18 11:37, Thomas Gleixner wrote: >> >>> VSMP support is built even if CONFIG_X86_VSMP is not set. This leads to a build >>> breakage when CONFIG_PCI is disabled as well. >>> >>> Build VSMP code only when selected. >> >> This patch disables detect_vsmp_box() on systems without CONFIG_X86_VSMP, due to >> the recent 6da63eb241a05b0e676d68975e793c0521387141. This is significant >> regression that will affect significant number of deployments. >> >> We will reply shortly with an updated patch that fix the dependency on pv_irq_ops, >> and revert to CONFIG_PARAVIRT, with proper protection for CONFIG_PCI. >> > > here is the proper patch which fixes the issue on hand: > From ebff534f8cfa55d7c3ab798c44abe879f3fbe2b8 Mon Sep 17 00:00:00 2001 > From: Eial Czerwacki <eial@scalemp.com> > Date: Thu, 1 Nov 2018 15:08:32 +0200 > Subject: [PATCH] x86/build: Build VSMP support only if CONFIG_PCI is > selected > > vsmp dependency of pv_irq_ops removed some years ago, so now let's clean > it up from vsmp_64.c. > > In short, "cap & ctl & (1 << 4)" was always returning 0, as such we can > remove all the PARAVIRT/PARAVIRT_XXL code handling that. > > However, the rest of the code depends on CONFIG_PCI, so fix it accordingly. > > Signed-off-by: Eial Czerwacki <eial@scalemp.com> > Acked-by: Shai Fultheim <shai@scalemp.com> > --- > arch/x86/Kconfig | 1 - > arch/x86/kernel/vsmp_64.c | 80 > +++-------------------------------------------- > 2 files changed, 5 insertions(+), 76 deletions(-) > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index c51c989..4b187ca 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -524,7 +524,6 @@ config X86_VSMP > bool "ScaleMP vSMP" > select HYPERVISOR_GUEST > select PARAVIRT Do you really still need PARAVIRT and HYPERVISOR_GUEST? Maybe you want IRQ_REMAP instead? > - select PARAVIRT_XXL > depends on X86_64 && PCI > depends on X86_EXTENDED_PLATFORM > depends on SMP > diff --git a/arch/x86/kernel/vsmp_64.c b/arch/x86/kernel/vsmp_64.c > index 1eae5af..c6d2b76 100644 > --- a/arch/x86/kernel/vsmp_64.c > +++ b/arch/x86/kernel/vsmp_64.c > @@ -26,64 +26,7 @@ > > #define TOPOLOGY_REGISTER_OFFSET 0x10 > > -#if defined CONFIG_PCI && defined CONFIG_PARAVIRT_XXL > -/* > - * Interrupt control on vSMPowered systems: > - * ~AC is a shadow of IF. If IF is 'on' AC should be 'off' > - * and vice versa. > - */ > - > -asmlinkage __visible unsigned long vsmp_save_fl(void) > -{ > - unsigned long flags = native_save_fl(); > - > - if (!(flags & X86_EFLAGS_IF) || (flags & X86_EFLAGS_AC)) > - flags &= ~X86_EFLAGS_IF; > - return flags; > -} > -PV_CALLEE_SAVE_REGS_THUNK(vsmp_save_fl); > - > -__visible void vsmp_restore_fl(unsigned long flags) > -{ > - if (flags & X86_EFLAGS_IF) > - flags &= ~X86_EFLAGS_AC; > - else > - flags |= X86_EFLAGS_AC; > - native_restore_fl(flags); > -} > -PV_CALLEE_SAVE_REGS_THUNK(vsmp_restore_fl); > - > -asmlinkage __visible void vsmp_irq_disable(void) > -{ > - unsigned long flags = native_save_fl(); > - > - native_restore_fl((flags & ~X86_EFLAGS_IF) | X86_EFLAGS_AC); > -} > -PV_CALLEE_SAVE_REGS_THUNK(vsmp_irq_disable); > - > -asmlinkage __visible void vsmp_irq_enable(void) > -{ > - unsigned long flags = native_save_fl(); > - > - native_restore_fl((flags | X86_EFLAGS_IF) & (~X86_EFLAGS_AC)); > -} > -PV_CALLEE_SAVE_REGS_THUNK(vsmp_irq_enable); > - > -static unsigned __init vsmp_patch(u8 type, void *ibuf, > - unsigned long addr, unsigned len) > -{ > - switch (type) { > - case PARAVIRT_PATCH(irq.irq_enable): > - case PARAVIRT_PATCH(irq.irq_disable): > - case PARAVIRT_PATCH(irq.save_fl): > - case PARAVIRT_PATCH(irq.restore_fl): > - return paravirt_patch_default(type, ibuf, addr, len); > - default: > - return native_patch(type, ibuf, addr, len); > - } > - > -} > - > +#if defined CONFIG_PCI > static void __init set_vsmp_pv_ops(void) Wouldn't be a rename of the function be appropriate now? Juergen
Greetings, On 11/01/2018 03:45 PM, Juergen Gross wrote: > On 01/11/2018 14:10, Eial Czerwacki wrote: >> Greetings, >> >> On 11/01/2018 12:39 PM, Shai Fultheim (Shai@ScaleMP.com) wrote: >>> On 01/11/18 11:37, Thomas Gleixner wrote: >>> >>>> VSMP support is built even if CONFIG_X86_VSMP is not set. This leads to a build >>>> breakage when CONFIG_PCI is disabled as well. >>>> >>>> Build VSMP code only when selected. >>> >>> This patch disables detect_vsmp_box() on systems without CONFIG_X86_VSMP, due to >>> the recent 6da63eb241a05b0e676d68975e793c0521387141. This is significant >>> regression that will affect significant number of deployments. >>> >>> We will reply shortly with an updated patch that fix the dependency on pv_irq_ops, >>> and revert to CONFIG_PARAVIRT, with proper protection for CONFIG_PCI. >>> >> >> here is the proper patch which fixes the issue on hand: >> From ebff534f8cfa55d7c3ab798c44abe879f3fbe2b8 Mon Sep 17 00:00:00 2001 >> From: Eial Czerwacki <eial@scalemp.com> >> Date: Thu, 1 Nov 2018 15:08:32 +0200 >> Subject: [PATCH] x86/build: Build VSMP support only if CONFIG_PCI is >> selected >> >> vsmp dependency of pv_irq_ops removed some years ago, so now let's clean >> it up from vsmp_64.c. >> >> In short, "cap & ctl & (1 << 4)" was always returning 0, as such we can >> remove all the PARAVIRT/PARAVIRT_XXL code handling that. >> >> However, the rest of the code depends on CONFIG_PCI, so fix it accordingly. >> >> Signed-off-by: Eial Czerwacki <eial@scalemp.com> >> Acked-by: Shai Fultheim <shai@scalemp.com> >> --- >> arch/x86/Kconfig | 1 - >> arch/x86/kernel/vsmp_64.c | 80 >> +++-------------------------------------------- >> 2 files changed, 5 insertions(+), 76 deletions(-) >> >> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig >> index c51c989..4b187ca 100644 >> --- a/arch/x86/Kconfig >> +++ b/arch/x86/Kconfig >> @@ -524,7 +524,6 @@ config X86_VSMP >> bool "ScaleMP vSMP" >> select HYPERVISOR_GUEST >> select PARAVIRT > > Do you really still need PARAVIRT and HYPERVISOR_GUEST? > Maybe you want IRQ_REMAP instead? > Better performance is achieved with PARAVIRTed kernel. Hence we keep them both in. >> - select PARAVIRT_XXL >> depends on X86_64 && PCI >> depends on X86_EXTENDED_PLATFORM >> depends on SMP >> diff --git a/arch/x86/kernel/vsmp_64.c b/arch/x86/kernel/vsmp_64.c >> index 1eae5af..c6d2b76 100644 >> --- a/arch/x86/kernel/vsmp_64.c >> +++ b/arch/x86/kernel/vsmp_64.c >> @@ -26,64 +26,7 @@ >> >> #define TOPOLOGY_REGISTER_OFFSET 0x10 >> >> -#if defined CONFIG_PCI && defined CONFIG_PARAVIRT_XXL >> -/* >> - * Interrupt control on vSMPowered systems: >> - * ~AC is a shadow of IF. If IF is 'on' AC should be 'off' >> - * and vice versa. >> - */ >> - >> -asmlinkage __visible unsigned long vsmp_save_fl(void) >> -{ >> - unsigned long flags = native_save_fl(); >> - >> - if (!(flags & X86_EFLAGS_IF) || (flags & X86_EFLAGS_AC)) >> - flags &= ~X86_EFLAGS_IF; >> - return flags; >> -} >> -PV_CALLEE_SAVE_REGS_THUNK(vsmp_save_fl); >> - >> -__visible void vsmp_restore_fl(unsigned long flags) >> -{ >> - if (flags & X86_EFLAGS_IF) >> - flags &= ~X86_EFLAGS_AC; >> - else >> - flags |= X86_EFLAGS_AC; >> - native_restore_fl(flags); >> -} >> -PV_CALLEE_SAVE_REGS_THUNK(vsmp_restore_fl); >> - >> -asmlinkage __visible void vsmp_irq_disable(void) >> -{ >> - unsigned long flags = native_save_fl(); >> - >> - native_restore_fl((flags & ~X86_EFLAGS_IF) | X86_EFLAGS_AC); >> -} >> -PV_CALLEE_SAVE_REGS_THUNK(vsmp_irq_disable); >> - >> -asmlinkage __visible void vsmp_irq_enable(void) >> -{ >> - unsigned long flags = native_save_fl(); >> - >> - native_restore_fl((flags | X86_EFLAGS_IF) & (~X86_EFLAGS_AC)); >> -} >> -PV_CALLEE_SAVE_REGS_THUNK(vsmp_irq_enable); >> - >> -static unsigned __init vsmp_patch(u8 type, void *ibuf, >> - unsigned long addr, unsigned len) >> -{ >> - switch (type) { >> - case PARAVIRT_PATCH(irq.irq_enable): >> - case PARAVIRT_PATCH(irq.irq_disable): >> - case PARAVIRT_PATCH(irq.save_fl): >> - case PARAVIRT_PATCH(irq.restore_fl): >> - return paravirt_patch_default(type, ibuf, addr, len); >> - default: >> - return native_patch(type, ibuf, addr, len); >> - } >> - >> -} >> - >> +#if defined CONFIG_PCI >> static void __init set_vsmp_pv_ops(void) > > Wouldn't be a rename of the function be appropriate now? you are correct, I'll fix and resend the patch. > > > Juergen >
On 01/11/2018 16:09, Eial Czerwacki wrote: > Greetings, > > On 11/01/2018 03:45 PM, Juergen Gross wrote: >> On 01/11/2018 14:10, Eial Czerwacki wrote: >>> Greetings, >>> >>> On 11/01/2018 12:39 PM, Shai Fultheim (Shai@ScaleMP.com) wrote: >>>> On 01/11/18 11:37, Thomas Gleixner wrote: >>>> >>>>> VSMP support is built even if CONFIG_X86_VSMP is not set. This leads to a build >>>>> breakage when CONFIG_PCI is disabled as well. >>>>> >>>>> Build VSMP code only when selected. >>>> >>>> This patch disables detect_vsmp_box() on systems without CONFIG_X86_VSMP, due to >>>> the recent 6da63eb241a05b0e676d68975e793c0521387141. This is significant >>>> regression that will affect significant number of deployments. >>>> >>>> We will reply shortly with an updated patch that fix the dependency on pv_irq_ops, >>>> and revert to CONFIG_PARAVIRT, with proper protection for CONFIG_PCI. >>>> >>> >>> here is the proper patch which fixes the issue on hand: >>> From ebff534f8cfa55d7c3ab798c44abe879f3fbe2b8 Mon Sep 17 00:00:00 2001 >>> From: Eial Czerwacki <eial@scalemp.com> >>> Date: Thu, 1 Nov 2018 15:08:32 +0200 >>> Subject: [PATCH] x86/build: Build VSMP support only if CONFIG_PCI is >>> selected >>> >>> vsmp dependency of pv_irq_ops removed some years ago, so now let's clean >>> it up from vsmp_64.c. >>> >>> In short, "cap & ctl & (1 << 4)" was always returning 0, as such we can >>> remove all the PARAVIRT/PARAVIRT_XXL code handling that. >>> >>> However, the rest of the code depends on CONFIG_PCI, so fix it accordingly. >>> >>> Signed-off-by: Eial Czerwacki <eial@scalemp.com> >>> Acked-by: Shai Fultheim <shai@scalemp.com> >>> --- >>> arch/x86/Kconfig | 1 - >>> arch/x86/kernel/vsmp_64.c | 80 >>> +++-------------------------------------------- >>> 2 files changed, 5 insertions(+), 76 deletions(-) >>> >>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig >>> index c51c989..4b187ca 100644 >>> --- a/arch/x86/Kconfig >>> +++ b/arch/x86/Kconfig >>> @@ -524,7 +524,6 @@ config X86_VSMP >>> bool "ScaleMP vSMP" >>> select HYPERVISOR_GUEST >>> select PARAVIRT >> >> Do you really still need PARAVIRT and HYPERVISOR_GUEST? >> Maybe you want IRQ_REMAP instead? >> > Better performance is achieved with PARAVIRTed kernel. Hence we keep > them both in. Do you have an explanation for that? Normally PARAVIRT is expected to have a small negative impact on performance. Juergen
diff --git a/arch/x86/include/asm/setup.h b/arch/x86/include/asm/setup.h index ae13bc974416..b6b911c4c7f3 100644 --- a/arch/x86/include/asm/setup.h +++ b/arch/x86/include/asm/setup.h @@ -33,7 +33,7 @@ extern u64 relocated_ramdisk; /* Interrupt control for vSMPowered x86_64 systems */ -#ifdef CONFIG_X86_64 +#if defined(CONFIG_X86_64) && defined(CONFIG_X86_VSMP) void vsmp_init(void); #else static inline void vsmp_init(void) { } diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile index 8824d01c0c35..647ce52b17d5 100644 --- a/arch/x86/kernel/Makefile +++ b/arch/x86/kernel/Makefile @@ -148,5 +148,5 @@ ifeq ($(CONFIG_X86_64),y) obj-$(CONFIG_CALGARY_IOMMU) += pci-calgary_64.o tce_64.o obj-$(CONFIG_MMCONF_FAM10H) += mmconf-fam10h_64.o - obj-y += vsmp_64.o + obj-$(CONFIG_X86_VSMP) += vsmp_64.o endif
VSMP support is built even if CONFIG_X86_VSMP is not set. This leads to a build breakage when CONFIG_PCI is disabled as well. Build VSMP code only when selected. Reported-by: Randy Dunlap <rdunlap@infradead.org> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> ---