[PATCH] x86/smpboot: Remove 486-isms from the modern AP boot path

[PATCH] x86/smpboot: Remove 486-isms from the modern AP boot path

[Andrew Cooper]

Linux has an implementation of the Universal Start-up Algorithm (MP spec,
Appendix B.4, Application Processor Startup), which includes unconditionally
writing to the Bios Data Area and CMOS registers.

The warm reset vector is only necessary in the non-integrated Local APIC case.
UV and Jailhouse already have an opt-out for this behaviour, but blindly using
the BDA and CMOS on a UEFI or other reduced hardware system isn't clever.

Drop the warm_reset flag from struct x86_legacy_features, and tie the warm
vector modifications to the integrated-ness of the Local APIC.  This has the
advantage of compiling the warm reset logic out entirely for 64bit builds.

CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: Borislav Petkov <bp@alien8.de>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: x86@kernel.org
CC: Jan Kiszka <jan.kiszka@siemens.com>
CC: James Morris <jmorris@namei.org>
CC: David Howells <dhowells@redhat.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Matthew Garrett <mjg59@google.com>
CC: Josh Boyer <jwboyer@redhat.com>
CC: Zhenzhong Duan <zhenzhong.duan@oracle.com>
CC: Steve Wahl <steve.wahl@hpe.com>
CC: Mike Travis <mike.travis@hpe.com>
CC: Dimitri Sivanich <dimitri.sivanich@hpe.com>
CC: Arnd Bergmann <arnd@arndb.de>
CC: "Peter Zijlstra (Intel)" <peterz@infradead.org>
CC: Giovanni Gherdovich <ggherdovich@suse.cz>
CC: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
CC: Len Brown <len.brown@intel.com>
CC: Kees Cook <keescook@chromium.org>
CC: Martin Molnar <martin.molnar.programming@gmail.com>
CC: Pingfan Liu <kernelfans@gmail.com>
CC: linux-kernel@vger.kernel.org
CC: jailhouse-dev@googlegroups.com
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Thomas: I finally found the reference we were discussing in Portland.  Sorry
this patch took so long.

I don't have any non-integrated APIC hardware to test with.  Can anyone help
me out?
---
 arch/x86/include/asm/x86_init.h    |  1 -
 arch/x86/kernel/apic/x2apic_uv_x.c |  1 -
 arch/x86/kernel/jailhouse.c        |  1 -
 arch/x86/kernel/platform-quirks.c  |  1 -
 arch/x86/kernel/smpboot.c          | 21 ++++++++++++---------
 5 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 96d9cd208610..006a5d7fd7eb 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -229,7 +229,6 @@ enum x86_legacy_i8042_state {
 struct x86_legacy_features {
 	enum x86_legacy_i8042_state i8042;
 	int rtc;
-	int warm_reset;
 	int no_vga;
 	int reserve_bios_regions;
 	struct x86_legacy_devices devices;
diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
index ad53b2abc859..5afcfd193592 100644
--- a/arch/x86/kernel/apic/x2apic_uv_x.c
+++ b/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -343,7 +343,6 @@ static int __init uv_acpi_madt_oem_check(char *_oem_id, char *_oem_table_id)
 	} else if (!strcmp(oem_table_id, "UVH")) {
 		/* Only UV1 systems: */
 		uv_system_type = UV_NON_UNIQUE_APIC;
-		x86_platform.legacy.warm_reset = 0;
 		__this_cpu_write(x2apic_extra_bits, pnodeid << uvh_apicid.s.pnode_shift);
 		uv_set_apicid_hibit();
 		uv_apic = 1;
diff --git a/arch/x86/kernel/jailhouse.c b/arch/x86/kernel/jailhouse.c
index 6eb8b50ea07e..d628fe92d6af 100644
--- a/arch/x86/kernel/jailhouse.c
+++ b/arch/x86/kernel/jailhouse.c
@@ -210,7 +210,6 @@ static void __init jailhouse_init_platform(void)
 	x86_platform.calibrate_tsc	= jailhouse_get_tsc;
 	x86_platform.get_wallclock	= jailhouse_get_wallclock;
 	x86_platform.legacy.rtc		= 0;
-	x86_platform.legacy.warm_reset	= 0;
 	x86_platform.legacy.i8042	= X86_LEGACY_I8042_PLATFORM_ABSENT;
 
 	legacy_pic			= &null_legacy_pic;
diff --git a/arch/x86/kernel/platform-quirks.c b/arch/x86/kernel/platform-quirks.c
index b348a672f71d..d922c5e0c678 100644
--- a/arch/x86/kernel/platform-quirks.c
+++ b/arch/x86/kernel/platform-quirks.c
@@ -9,7 +9,6 @@ void __init x86_early_init_platform_quirks(void)
 {
 	x86_platform.legacy.i8042 = X86_LEGACY_I8042_EXPECTED_PRESENT;
 	x86_platform.legacy.rtc = 1;
-	x86_platform.legacy.warm_reset = 1;
 	x86_platform.legacy.reserve_bios_regions = 0;
 	x86_platform.legacy.devices.pnpbios = 1;
 
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index d85e91a8aa8c..e2ebb0be2ee3 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1049,18 +1049,21 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
 	 * the targeted processor.
 	 */
 
-	if (x86_platform.legacy.warm_reset) {
+	/*
+	 * APs are typically started in one of two ways:
+	 *
+	 * - On 486-era hardware with a non-integrated Local APIC, a single
+	 *   INIT IPI is sent.  When the AP comes out of reset, the BIOS
+	 *   follows the warm reset vector to start_ip.
+	 * - On everything with an integrated Local APIC, the start_ip is
+	 *   provided in the Startup IPI message, sent as part of an
+	 *   INIT-SIPI-SIPI sequence.
+	 */
+	if (!APIC_INTEGRATED(boot_cpu_apic_version)) {
 
 		pr_debug("Setting warm reset code and vector.\n");
 
 		smpboot_setup_warm_reset_vector(start_ip);
-		/*
-		 * Be paranoid about clearing APIC errors.
-		*/
-		if (APIC_INTEGRATED(boot_cpu_apic_version)) {
-			apic_write(APIC_ESR, 0);
-			apic_read(APIC_ESR);
-		}
 	}
 
 	/*
@@ -1118,7 +1121,7 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
 		}
 	}
 
-	if (x86_platform.legacy.warm_reset) {
+	if (!APIC_INTEGRATED(boot_cpu_apic_version)) {
 		/*
 		 * Cleanup possible dangling ends...
 		 */
-- 
2.11.0

Re: [PATCH] x86/smpboot: Remove 486-isms from the modern AP boot path

[hpa]

On March 25, 2020 3:14:31 AM PDT, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>Linux has an implementation of the Universal Start-up Algorithm (MP
>spec,
>Appendix B.4, Application Processor Startup), which includes
>unconditionally
>writing to the Bios Data Area and CMOS registers.
>
>The warm reset vector is only necessary in the non-integrated Local
>APIC case.
>UV and Jailhouse already have an opt-out for this behaviour, but
>blindly using
>the BDA and CMOS on a UEFI or other reduced hardware system isn't
>clever.
>
>Drop the warm_reset flag from struct x86_legacy_features, and tie the
>warm
>vector modifications to the integrated-ness of the Local APIC.  This
>has the
>advantage of compiling the warm reset logic out entirely for 64bit
>builds.
>
>CC: Thomas Gleixner <tglx@linutronix.de>
>CC: Ingo Molnar <mingo@redhat.com>
>CC: Borislav Petkov <bp@alien8.de>
>CC: "H. Peter Anvin" <hpa@zytor.com>
>CC: x86@kernel.org
>CC: Jan Kiszka <jan.kiszka@siemens.com>
>CC: James Morris <jmorris@namei.org>
>CC: David Howells <dhowells@redhat.com>
>CC: Andrew Cooper <andrew.cooper3@citrix.com>
>CC: Matthew Garrett <mjg59@google.com>
>CC: Josh Boyer <jwboyer@redhat.com>
>CC: Zhenzhong Duan <zhenzhong.duan@oracle.com>
>CC: Steve Wahl <steve.wahl@hpe.com>
>CC: Mike Travis <mike.travis@hpe.com>
>CC: Dimitri Sivanich <dimitri.sivanich@hpe.com>
>CC: Arnd Bergmann <arnd@arndb.de>
>CC: "Peter Zijlstra (Intel)" <peterz@infradead.org>
>CC: Giovanni Gherdovich <ggherdovich@suse.cz>
>CC: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
>CC: Len Brown <len.brown@intel.com>
>CC: Kees Cook <keescook@chromium.org>
>CC: Martin Molnar <martin.molnar.programming@gmail.com>
>CC: Pingfan Liu <kernelfans@gmail.com>
>CC: linux-kernel@vger.kernel.org
>CC: jailhouse-dev@googlegroups.com
>Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>---
>Thomas: I finally found the reference we were discussing in Portland. 
>Sorry
>this patch took so long.
>
>I don't have any non-integrated APIC hardware to test with.  Can anyone
>help
>me out?
>---
> arch/x86/include/asm/x86_init.h    |  1 -
> arch/x86/kernel/apic/x2apic_uv_x.c |  1 -
> arch/x86/kernel/jailhouse.c        |  1 -
> arch/x86/kernel/platform-quirks.c  |  1 -
> arch/x86/kernel/smpboot.c          | 21 ++++++++++++---------
> 5 files changed, 12 insertions(+), 13 deletions(-)
>
>diff --git a/arch/x86/include/asm/x86_init.h
>b/arch/x86/include/asm/x86_init.h
>index 96d9cd208610..006a5d7fd7eb 100644
>--- a/arch/x86/include/asm/x86_init.h
>+++ b/arch/x86/include/asm/x86_init.h
>@@ -229,7 +229,6 @@ enum x86_legacy_i8042_state {
> struct x86_legacy_features {
> 	enum x86_legacy_i8042_state i8042;
> 	int rtc;
>-	int warm_reset;
> 	int no_vga;
> 	int reserve_bios_regions;
> 	struct x86_legacy_devices devices;
>diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c
>b/arch/x86/kernel/apic/x2apic_uv_x.c
>index ad53b2abc859..5afcfd193592 100644
>--- a/arch/x86/kernel/apic/x2apic_uv_x.c
>+++ b/arch/x86/kernel/apic/x2apic_uv_x.c
>@@ -343,7 +343,6 @@ static int __init uv_acpi_madt_oem_check(char
>*_oem_id, char *_oem_table_id)
> 	} else if (!strcmp(oem_table_id, "UVH")) {
> 		/* Only UV1 systems: */
> 		uv_system_type = UV_NON_UNIQUE_APIC;
>-		x86_platform.legacy.warm_reset = 0;
>		__this_cpu_write(x2apic_extra_bits, pnodeid <<
>uvh_apicid.s.pnode_shift);
> 		uv_set_apicid_hibit();
> 		uv_apic = 1;
>diff --git a/arch/x86/kernel/jailhouse.c b/arch/x86/kernel/jailhouse.c
>index 6eb8b50ea07e..d628fe92d6af 100644
>--- a/arch/x86/kernel/jailhouse.c
>+++ b/arch/x86/kernel/jailhouse.c
>@@ -210,7 +210,6 @@ static void __init jailhouse_init_platform(void)
> 	x86_platform.calibrate_tsc	= jailhouse_get_tsc;
> 	x86_platform.get_wallclock	= jailhouse_get_wallclock;
> 	x86_platform.legacy.rtc		= 0;
>-	x86_platform.legacy.warm_reset	= 0;
> 	x86_platform.legacy.i8042	= X86_LEGACY_I8042_PLATFORM_ABSENT;
> 
> 	legacy_pic			= &null_legacy_pic;
>diff --git a/arch/x86/kernel/platform-quirks.c
>b/arch/x86/kernel/platform-quirks.c
>index b348a672f71d..d922c5e0c678 100644
>--- a/arch/x86/kernel/platform-quirks.c
>+++ b/arch/x86/kernel/platform-quirks.c
>@@ -9,7 +9,6 @@ void __init x86_early_init_platform_quirks(void)
> {
> 	x86_platform.legacy.i8042 = X86_LEGACY_I8042_EXPECTED_PRESENT;
> 	x86_platform.legacy.rtc = 1;
>-	x86_platform.legacy.warm_reset = 1;
> 	x86_platform.legacy.reserve_bios_regions = 0;
> 	x86_platform.legacy.devices.pnpbios = 1;
> 
>diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
>index d85e91a8aa8c..e2ebb0be2ee3 100644
>--- a/arch/x86/kernel/smpboot.c
>+++ b/arch/x86/kernel/smpboot.c
>@@ -1049,18 +1049,21 @@ static int do_boot_cpu(int apicid, int cpu,
>struct task_struct *idle,
> 	 * the targeted processor.
> 	 */
> 
>-	if (x86_platform.legacy.warm_reset) {
>+	/*
>+	 * APs are typically started in one of two ways:
>+	 *
>+	 * - On 486-era hardware with a non-integrated Local APIC, a single
>+	 *   INIT IPI is sent.  When the AP comes out of reset, the BIOS
>+	 *   follows the warm reset vector to start_ip.
>+	 * - On everything with an integrated Local APIC, the start_ip is
>+	 *   provided in the Startup IPI message, sent as part of an
>+	 *   INIT-SIPI-SIPI sequence.
>+	 */
>+	if (!APIC_INTEGRATED(boot_cpu_apic_version)) {
> 
> 		pr_debug("Setting warm reset code and vector.\n");
> 
> 		smpboot_setup_warm_reset_vector(start_ip);
>-		/*
>-		 * Be paranoid about clearing APIC errors.
>-		*/
>-		if (APIC_INTEGRATED(boot_cpu_apic_version)) {
>-			apic_write(APIC_ESR, 0);
>-			apic_read(APIC_ESR);
>-		}
> 	}
> 
> 	/*
>@@ -1118,7 +1121,7 @@ static int do_boot_cpu(int apicid, int cpu,
>struct task_struct *idle,
> 		}
> 	}
> 
>-	if (x86_platform.legacy.warm_reset) {
>+	if (!APIC_INTEGRATED(boot_cpu_apic_version)) {
> 		/*
> 		 * Cleanup possible dangling ends...
> 		 */

We don't support SMP on 486 and haven't for a very long time. Is there any reason to retain that code at all?
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH] x86/smpboot: Remove 486-isms from the modern AP boot path

[Thomas Gleixner]

hpa@zytor.com writes:
> On March 25, 2020 3:14:31 AM PDT, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>@@ -1118,7 +1121,7 @@ static int do_boot_cpu(int apicid, int cpu,
>>struct task_struct *idle,
>> 		}
>> 	}
>> 
>>-	if (x86_platform.legacy.warm_reset) {
>>+	if (!APIC_INTEGRATED(boot_cpu_apic_version)) {
>> 		/*
>> 		 * Cleanup possible dangling ends...
>> 		 */
>
> We don't support SMP on 486 and haven't for a very long time. Is there
> any reason to retain that code at all?

Not that I'm aware off.

Thanks,

        tglx

Re: [PATCH] x86/smpboot: Remove 486-isms from the modern AP boot path

[Maciej W. Rozycki]

On Wed, 25 Mar 2020, Thomas Gleixner wrote:

> >>@@ -1118,7 +1121,7 @@ static int do_boot_cpu(int apicid, int cpu,
> >>struct task_struct *idle,
> >> 		}
> >> 	}
> >> 
> >>-	if (x86_platform.legacy.warm_reset) {
> >>+	if (!APIC_INTEGRATED(boot_cpu_apic_version)) {
> >> 		/*
> >> 		 * Cleanup possible dangling ends...
> >> 		 */
> >
> > We don't support SMP on 486 and haven't for a very long time. Is there
> > any reason to retain that code at all?
> 
> Not that I'm aware off.

 For the record: this code is for Pentium really, covering original P5 
systems, which lacked integrated APIC, as well as P54C systems that went 
beyond dual (e.g. ALR made quad-SMP P54C systems).  They all used external 
i82489DX APICs for SMP support.  Few were ever manufactured and getting 
across one let alone running Linux might be tough these days.  I never 
managed to get one for myself, which would have been helpful for 
maintaining this code.

 Even though we supported them by spec I believe we never actually ran MP 
on any 486 SMP system (Alan Cox might be able to straighten me out on 
this); none that I know of implemented the MPS even though actual hardware 
might have used the APIC architecture.  Compaq had its competing solution 
for 486 and newer SMP, actually deployed, the name of which I long forgot.  
We never supported it due to the lack of documentation combined with the 
lack of enough incentive for someone to reverse-engineer it.

 FWIW,

  Maciej

RE: [PATCH] x86/smpboot: Remove 486-isms from the modern AP boot path

[David Laight]

From: Maciej W. Rozycki
> Sent: 01 April 2020 00:35
> On Wed, 25 Mar 2020, Thomas Gleixner wrote:
> 
> > >>@@ -1118,7 +1121,7 @@ static int do_boot_cpu(int apicid, int cpu,
> > >>struct task_struct *idle,
> > >> 		}
> > >> 	}
> > >>
> > >>-	if (x86_platform.legacy.warm_reset) {
> > >>+	if (!APIC_INTEGRATED(boot_cpu_apic_version)) {
> > >> 		/*
> > >> 		 * Cleanup possible dangling ends...
> > >> 		 */
> > >
> > > We don't support SMP on 486 and haven't for a very long time. Is there
> > > any reason to retain that code at all?
> >
> > Not that I'm aware off.
> 
>  For the record: this code is for Pentium really, covering original P5
> systems, which lacked integrated APIC, as well as P54C systems that went
> beyond dual (e.g. ALR made quad-SMP P54C systems).  They all used external
> i82489DX APICs for SMP support.  Few were ever manufactured and getting
> across one let alone running Linux might be tough these days.  I never
> managed to get one for myself, which would have been helpful for
> maintaining this code.

I remember ICL trying to get SVR4.2MP working on similar vintage hardware.
I wasn't directly involved (doing SMP sparc ethernet drivers) but ISTR
that the SMP support silicon they were using (or rather trying to use)
was basically broken.
By the time they got it (nearly) working single cpu systems were faster.

>  Even though we supported them by spec I believe we never actually ran MP
> on any 486 SMP system (Alan Cox might be able to straighten me out on
> this); none that I know of implemented the MPS even though actual hardware
> might have used the APIC architecture.  Compaq had its competing solution
> for 486 and newer SMP, actually deployed, the name of which I long forgot.
> We never supported it due to the lack of documentation combined with the
> lack of enough incentive for someone to reverse-engineer it.

I also remember one of those Compaq dual 486 boxes.
We must have has SVR4/Unixware running on it.

I suspect that any such systems would also be too slow and not have
enough memory to actually run anything recent.

OTOH there are modern 486 (like) systems than might have a reasonable
amount of memory and clock speed.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH] x86/smpboot: Remove 486-isms from the modern AP boot path

[Maciej W. Rozycki]

On Wed, 1 Apr 2020, David Laight wrote:

> >  Even though we supported them by spec I believe we never actually ran MP
> > on any 486 SMP system (Alan Cox might be able to straighten me out on
> > this); none that I know of implemented the MPS even though actual hardware
> > might have used the APIC architecture.  Compaq had its competing solution
> > for 486 and newer SMP, actually deployed, the name of which I long forgot.
> > We never supported it due to the lack of documentation combined with the
> > lack of enough incentive for someone to reverse-engineer it.
> 
> I also remember one of those Compaq dual 486 boxes.
> We must have has SVR4/Unixware running on it.
> 
> I suspect that any such systems would also be too slow and not have
> enough memory to actually run anything recent.

 For reasons mentioned above I cannot speak about 486 SMP systems.

 However I have a nice Dolch PAC 60 machine, which is a somewhat rugged 
luggable computer with an embedded LCD display and a detachable keyboard, 
built around a pure EISA 486 baseboard (wiring to an external display is 
also supported).  Its original purpose was an FDDI network sniffer with a 
DOS application meant to assist a field engineer with fault isolation, and 
as you may know FDDI used to have rings up to ~100km/60mi in length, so 
people often had to travel quite a distance to get a problem tracked down.

 It used to boot current Linux with somewhat dated userland until its PSU, 
an industrial unit, failed a couple years back, taking the hard disk with 
itself due to an overvoltage condition (its +12V output went up to +18V).  
I failed to repair the PSU (I suspect a fault in the transformer causing 
its windings to short-circuit intermittently, and only the +5V output is 
regulated with the remaining ones expected to maintain fixed correlation), 
which the box has been designed around, making it difficult to be replaced 
with a different PSU.

 However I have since managed to track down and install a compatible 
replacement PSU from the same manufacturer whose only difference are 
slightly higher power ratings, and I have a replacement hard disk for it 
too, so I plan to get it back in service soon.

 With 16MiB originally installed the machine is somewhat little usable 
with current Linux indeed, however the baseboard supports up to 512MiB of 
RAM and suitable modules are still available for purchase, even brand new 
ones.  Once expanded so that constant swapping stops I expect the machine 
to perform quite well, as the performance of the CPU/RAM didn't seem to be 
a problem with this machine.  We actually keep supporting slower systems 
in the non-x86 ports.

 And as I say, the userland is not (much of) our business and can be 
matched to actual hardware; not everyone needs a heavyweight graphical 
environment with all the bells and whistles burning machine cycles.

 Again, FWIW,

  Maciej

Re: [PATCH] x86/smpboot: Remove 486-isms from the modern AP boot path

[Andrew Cooper]

On 01/04/2020 00:35, Maciej W. Rozycki wrote:
> On Wed, 25 Mar 2020, Thomas Gleixner wrote:
>
>>>> @@ -1118,7 +1121,7 @@ static int do_boot_cpu(int apicid, int cpu,
>>>> struct task_struct *idle,
>>>> 		}
>>>> 	}
>>>>
>>>> -	if (x86_platform.legacy.warm_reset) {
>>>> +	if (!APIC_INTEGRATED(boot_cpu_apic_version)) {
>>>> 		/*
>>>> 		 * Cleanup possible dangling ends...
>>>> 		 */
>>> We don't support SMP on 486 and haven't for a very long time. Is there
>>> any reason to retain that code at all?
>> Not that I'm aware off.
>  For the record: this code is for Pentium really, covering original P5 
> systems, which lacked integrated APIC, as well as P54C systems that went 
> beyond dual (e.g. ALR made quad-SMP P54C systems).  They all used external 
> i82489DX APICs for SMP support.  Few were ever manufactured and getting 
> across one let alone running Linux might be tough these days.  I never 
> managed to get one for myself, which would have been helpful for 
> maintaining this code.
>
>  Even though we supported them by spec I believe we never actually ran MP 
> on any 486 SMP system (Alan Cox might be able to straighten me out on 
> this); none that I know of implemented the MPS even though actual hardware 
> might have used the APIC architecture.  Compaq had its competing solution 
> for 486 and newer SMP, actually deployed, the name of which I long forgot.  
> We never supported it due to the lack of documentation combined with the 
> lack of enough incentive for someone to reverse-engineer it.

:)

I chose "486-ism" based on what the MP spec said about external vs
integrated Local APICs.  I can't claim to have any experience of those days.

I guess given v2 of the patch, I guess this should become "Remove
external-LAPIC support from the AP boot path" ?

~Andrew

Re: [PATCH] x86/smpboot: Remove 486-isms from the modern AP boot path

[Maciej W. Rozycki]

On Wed, 1 Apr 2020, Andrew Cooper wrote:

> >  Even though we supported them by spec I believe we never actually ran MP 
> > on any 486 SMP system (Alan Cox might be able to straighten me out on 
> > this); none that I know of implemented the MPS even though actual hardware 
> > might have used the APIC architecture.  Compaq had its competing solution 
> > for 486 and newer SMP, actually deployed, the name of which I long forgot.  
> > We never supported it due to the lack of documentation combined with the 
> > lack of enough incentive for someone to reverse-engineer it.
> 
> :)
> 
> I chose "486-ism" based on what the MP spec said about external vs
> integrated Local APICs.  I can't claim to have any experience of those days.

 The spec is quite clear about the use of discrete APICs actually:

"5.1 Discrete APIC Configurations

"   Figure 5-1 shows the default configuration for systems that use the 
    discrete 82489 APIC.  The Intel486 processor is shown as an example; 
    however, this configuration can also employ Pentium processors.  In 
    Pentium processor systems, PRST is connected to INIT instead of to 
    RESET."

:)  And if in the way the internal local APIC in P54C processors can be
permanently disabled (causing it not to be reported in CPUID flags) via a 
reset strap, e.g. to support an unusual configuration.

 As I recall the integrated APIC would in principle support SMP configs 
beyond dual (the inter-APIC bus was serial at the time and supported 15 
APIC IDs with the P54C), but at the time the P54C processor was released 
the only compatible I/O APICs were available as a part of Intel PCI south 
bridges (the 82375EB/SB ESC and the 82379AB SIO.A).  Those chips were not 
necessarily compatible with whatever custom chipset was developed to 
support e.g. a quad-SMP P54C system.  Or there were political reasons 
preventing them from being used.

 Then the 82489DX used an incompatible protocol (supporting 254 APIC IDs 
among others, and as I recall the serial bus had a different number of 
wires even), so it couldn't be mixed with integrated local APICs.  That's 
why discrete APICs were sometimes used even with P54C processors.

 And the 82093AA standalone I/O APIC was only introduced a few years 
later, along with the Intel HX (Triton II) SMP chipset.  I still have a 
nice working machine equipped with this chipset and dual P55C processors 
@233MHz.  Even the original CPU fans are going strong. :)  Its MP table is 
however buggy and difficult to work with if the I/O APIC is to be used, 
especially if PCI-PCI bridges are involved (there's none onboard, but you 
can have these easily in multiple quantities on option cards nowadays).

  Maciej

[PATCH v2] x86/smpboot: Remove 486-isms from the modern AP boot path

[Andrew Cooper]

Linux has an implementation of the Universal Start-up Algorithm (MP spec,
Appendix B.4, Application Processor Startup), which includes unconditionally
writing to the Bios Data Area and CMOS registers.

The warm reset vector is only necessary in the non-integrated Local APIC case.
UV and Jailhouse already have an opt-out for this behaviour, but blindly using
the BDA and CMOS on a UEFI or other reduced hardware system isn't clever.

We could make this conditional on the integrated-ness of the Local APIC, but
486-era SMP isn't supported.  Drop the logic completely, tidying up the includ
list and header files as appropriate.

CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: Borislav Petkov <bp@alien8.de>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: x86@kernel.org
CC: Jan Kiszka <jan.kiszka@siemens.com>
CC: James Morris <jmorris@namei.org>
CC: David Howells <dhowells@redhat.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Matthew Garrett <mjg59@google.com>
CC: Josh Boyer <jwboyer@redhat.com>
CC: Steve Wahl <steve.wahl@hpe.com>
CC: Mike Travis <mike.travis@hpe.com>
CC: Dimitri Sivanich <dimitri.sivanich@hpe.com>
CC: Arnd Bergmann <arnd@arndb.de>
CC: "Peter Zijlstra (Intel)" <peterz@infradead.org>
CC: Giovanni Gherdovich <ggherdovich@suse.cz>
CC: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
CC: Len Brown <len.brown@intel.com>
CC: Kees Cook <keescook@chromium.org>
CC: Martin Molnar <martin.molnar.programming@gmail.com>
CC: Pingfan Liu <kernelfans@gmail.com>
CC: linux-kernel@vger.kernel.org
CC: jailhouse-dev@googlegroups.com
Suggested-by: "H. Peter Anvin" <hpa@zytor.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v2:
 * Drop logic entirely, rather than retaining support in 32bit builds.
---
 arch/x86/include/asm/apic.h        |  6 -----
 arch/x86/include/asm/x86_init.h    |  1 -
 arch/x86/kernel/apic/x2apic_uv_x.c |  1 -
 arch/x86/kernel/jailhouse.c        |  1 -
 arch/x86/kernel/platform-quirks.c  |  1 -
 arch/x86/kernel/smpboot.c          | 50 --------------------------------------
 6 files changed, 60 deletions(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 19e94af9cc5d..5c33f9374b28 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -472,12 +472,6 @@ static inline unsigned default_get_apic_id(unsigned long x)
 		return (x >> 24) & 0x0F;
 }
 
-/*
- * Warm reset vector position:
- */
-#define TRAMPOLINE_PHYS_LOW		0x467
-#define TRAMPOLINE_PHYS_HIGH		0x469
-
 extern void generic_bigsmp_probe(void);
 
 #ifdef CONFIG_X86_LOCAL_APIC
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 96d9cd208610..006a5d7fd7eb 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -229,7 +229,6 @@ enum x86_legacy_i8042_state {
 struct x86_legacy_features {
 	enum x86_legacy_i8042_state i8042;
 	int rtc;
-	int warm_reset;
 	int no_vga;
 	int reserve_bios_regions;
 	struct x86_legacy_devices devices;
diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
index ad53b2abc859..5afcfd193592 100644
--- a/arch/x86/kernel/apic/x2apic_uv_x.c
+++ b/arch/x86/kernel/apic/x2apic_uv_x.c
@@ -343,7 +343,6 @@ static int __init uv_acpi_madt_oem_check(char *_oem_id, char *_oem_table_id)
 	} else if (!strcmp(oem_table_id, "UVH")) {
 		/* Only UV1 systems: */
 		uv_system_type = UV_NON_UNIQUE_APIC;
-		x86_platform.legacy.warm_reset = 0;
 		__this_cpu_write(x2apic_extra_bits, pnodeid << uvh_apicid.s.pnode_shift);
 		uv_set_apicid_hibit();
 		uv_apic = 1;
diff --git a/arch/x86/kernel/jailhouse.c b/arch/x86/kernel/jailhouse.c
index 6eb8b50ea07e..d628fe92d6af 100644
--- a/arch/x86/kernel/jailhouse.c
+++ b/arch/x86/kernel/jailhouse.c
@@ -210,7 +210,6 @@ static void __init jailhouse_init_platform(void)
 	x86_platform.calibrate_tsc	= jailhouse_get_tsc;
 	x86_platform.get_wallclock	= jailhouse_get_wallclock;
 	x86_platform.legacy.rtc		= 0;
-	x86_platform.legacy.warm_reset	= 0;
 	x86_platform.legacy.i8042	= X86_LEGACY_I8042_PLATFORM_ABSENT;
 
 	legacy_pic			= &null_legacy_pic;
diff --git a/arch/x86/kernel/platform-quirks.c b/arch/x86/kernel/platform-quirks.c
index b348a672f71d..d922c5e0c678 100644
--- a/arch/x86/kernel/platform-quirks.c
+++ b/arch/x86/kernel/platform-quirks.c
@@ -9,7 +9,6 @@ void __init x86_early_init_platform_quirks(void)
 {
 	x86_platform.legacy.i8042 = X86_LEGACY_I8042_EXPECTED_PRESENT;
 	x86_platform.legacy.rtc = 1;
-	x86_platform.legacy.warm_reset = 1;
 	x86_platform.legacy.reserve_bios_regions = 0;
 	x86_platform.legacy.devices.pnpbios = 1;
 
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index fe3ab9632f3b..a9f5b511d0b4 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -72,7 +72,6 @@
 #include <asm/fpu/internal.h>
 #include <asm/setup.h>
 #include <asm/uv/uv.h>
-#include <linux/mc146818rtc.h>
 #include <asm/i8259.h>
 #include <asm/misc.h>
 #include <asm/qspinlock.h>
@@ -119,34 +118,6 @@ int arch_update_cpu_topology(void)
 	return retval;
 }
 
-static inline void smpboot_setup_warm_reset_vector(unsigned long start_eip)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&rtc_lock, flags);
-	CMOS_WRITE(0xa, 0xf);
-	spin_unlock_irqrestore(&rtc_lock, flags);
-	*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_HIGH)) =
-							start_eip >> 4;
-	*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) =
-							start_eip & 0xf;
-}
-
-static inline void smpboot_restore_warm_reset_vector(void)
-{
-	unsigned long flags;
-
-	/*
-	 * Paranoid:  Set warm reset code and vector here back
-	 * to default values.
-	 */
-	spin_lock_irqsave(&rtc_lock, flags);
-	CMOS_WRITE(0, 0xf);
-	spin_unlock_irqrestore(&rtc_lock, flags);
-
-	*((volatile u32 *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = 0;
-}
-
 static void init_freq_invariance(void);
 
 /*
@@ -1049,20 +1020,6 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
 	 * the targeted processor.
 	 */
 
-	if (x86_platform.legacy.warm_reset) {
-
-		pr_debug("Setting warm reset code and vector.\n");
-
-		smpboot_setup_warm_reset_vector(start_ip);
-		/*
-		 * Be paranoid about clearing APIC errors.
-		*/
-		if (APIC_INTEGRATED(boot_cpu_apic_version)) {
-			apic_write(APIC_ESR, 0);
-			apic_read(APIC_ESR);
-		}
-	}
-
 	/*
 	 * AP might wait on cpu_callout_mask in cpu_init() with
 	 * cpu_initialized_mask set if previous attempt to online
@@ -1118,13 +1075,6 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
 		}
 	}
 
-	if (x86_platform.legacy.warm_reset) {
-		/*
-		 * Cleanup possible dangling ends...
-		 */
-		smpboot_restore_warm_reset_vector();
-	}
-
 	return boot_error;
 }
 
-- 
2.11.0

Re: [PATCH v2] x86/smpboot: Remove 486-isms from the modern AP boot path

[Brian Gerst]

On Tue, Mar 31, 2020 at 1:59 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> Linux has an implementation of the Universal Start-up Algorithm (MP spec,
> Appendix B.4, Application Processor Startup), which includes unconditionally
> writing to the Bios Data Area and CMOS registers.
>
> The warm reset vector is only necessary in the non-integrated Local APIC case.
> UV and Jailhouse already have an opt-out for this behaviour, but blindly using
> the BDA and CMOS on a UEFI or other reduced hardware system isn't clever.
>
> We could make this conditional on the integrated-ness of the Local APIC, but
> 486-era SMP isn't supported.  Drop the logic completely, tidying up the includ
> list and header files as appropriate.
>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Borislav Petkov <bp@alien8.de>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: x86@kernel.org
> CC: Jan Kiszka <jan.kiszka@siemens.com>
> CC: James Morris <jmorris@namei.org>
> CC: David Howells <dhowells@redhat.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Matthew Garrett <mjg59@google.com>
> CC: Josh Boyer <jwboyer@redhat.com>
> CC: Steve Wahl <steve.wahl@hpe.com>
> CC: Mike Travis <mike.travis@hpe.com>
> CC: Dimitri Sivanich <dimitri.sivanich@hpe.com>
> CC: Arnd Bergmann <arnd@arndb.de>
> CC: "Peter Zijlstra (Intel)" <peterz@infradead.org>
> CC: Giovanni Gherdovich <ggherdovich@suse.cz>
> CC: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> CC: Len Brown <len.brown@intel.com>
> CC: Kees Cook <keescook@chromium.org>
> CC: Martin Molnar <martin.molnar.programming@gmail.com>
> CC: Pingfan Liu <kernelfans@gmail.com>
> CC: linux-kernel@vger.kernel.org
> CC: jailhouse-dev@googlegroups.com
> Suggested-by: "H. Peter Anvin" <hpa@zytor.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> v2:
>  * Drop logic entirely, rather than retaining support in 32bit builds.
> ---
>  arch/x86/include/asm/apic.h        |  6 -----
>  arch/x86/include/asm/x86_init.h    |  1 -
>  arch/x86/kernel/apic/x2apic_uv_x.c |  1 -
>  arch/x86/kernel/jailhouse.c        |  1 -
>  arch/x86/kernel/platform-quirks.c  |  1 -
>  arch/x86/kernel/smpboot.c          | 50 --------------------------------------
>  6 files changed, 60 deletions(-)
>
> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> index 19e94af9cc5d..5c33f9374b28 100644
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -472,12 +472,6 @@ static inline unsigned default_get_apic_id(unsigned long x)
>                 return (x >> 24) & 0x0F;
>  }
>
> -/*
> - * Warm reset vector position:
> - */
> -#define TRAMPOLINE_PHYS_LOW            0x467
> -#define TRAMPOLINE_PHYS_HIGH           0x469
> -
>  extern void generic_bigsmp_probe(void);
>
>  #ifdef CONFIG_X86_LOCAL_APIC
> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> index 96d9cd208610..006a5d7fd7eb 100644
> --- a/arch/x86/include/asm/x86_init.h
> +++ b/arch/x86/include/asm/x86_init.h
> @@ -229,7 +229,6 @@ enum x86_legacy_i8042_state {
>  struct x86_legacy_features {
>         enum x86_legacy_i8042_state i8042;
>         int rtc;
> -       int warm_reset;
>         int no_vga;
>         int reserve_bios_regions;
>         struct x86_legacy_devices devices;
> diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
> index ad53b2abc859..5afcfd193592 100644
> --- a/arch/x86/kernel/apic/x2apic_uv_x.c
> +++ b/arch/x86/kernel/apic/x2apic_uv_x.c
> @@ -343,7 +343,6 @@ static int __init uv_acpi_madt_oem_check(char *_oem_id, char *_oem_table_id)
>         } else if (!strcmp(oem_table_id, "UVH")) {
>                 /* Only UV1 systems: */
>                 uv_system_type = UV_NON_UNIQUE_APIC;
> -               x86_platform.legacy.warm_reset = 0;
>                 __this_cpu_write(x2apic_extra_bits, pnodeid << uvh_apicid.s.pnode_shift);
>                 uv_set_apicid_hibit();
>                 uv_apic = 1;
> diff --git a/arch/x86/kernel/jailhouse.c b/arch/x86/kernel/jailhouse.c
> index 6eb8b50ea07e..d628fe92d6af 100644
> --- a/arch/x86/kernel/jailhouse.c
> +++ b/arch/x86/kernel/jailhouse.c
> @@ -210,7 +210,6 @@ static void __init jailhouse_init_platform(void)
>         x86_platform.calibrate_tsc      = jailhouse_get_tsc;
>         x86_platform.get_wallclock      = jailhouse_get_wallclock;
>         x86_platform.legacy.rtc         = 0;
> -       x86_platform.legacy.warm_reset  = 0;
>         x86_platform.legacy.i8042       = X86_LEGACY_I8042_PLATFORM_ABSENT;
>
>         legacy_pic                      = &null_legacy_pic;
> diff --git a/arch/x86/kernel/platform-quirks.c b/arch/x86/kernel/platform-quirks.c
> index b348a672f71d..d922c5e0c678 100644
> --- a/arch/x86/kernel/platform-quirks.c
> +++ b/arch/x86/kernel/platform-quirks.c
> @@ -9,7 +9,6 @@ void __init x86_early_init_platform_quirks(void)
>  {
>         x86_platform.legacy.i8042 = X86_LEGACY_I8042_EXPECTED_PRESENT;
>         x86_platform.legacy.rtc = 1;
> -       x86_platform.legacy.warm_reset = 1;
>         x86_platform.legacy.reserve_bios_regions = 0;
>         x86_platform.legacy.devices.pnpbios = 1;
>
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index fe3ab9632f3b..a9f5b511d0b4 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -72,7 +72,6 @@
>  #include <asm/fpu/internal.h>
>  #include <asm/setup.h>
>  #include <asm/uv/uv.h>
> -#include <linux/mc146818rtc.h>
>  #include <asm/i8259.h>
>  #include <asm/misc.h>
>  #include <asm/qspinlock.h>
> @@ -119,34 +118,6 @@ int arch_update_cpu_topology(void)
>         return retval;
>  }
>
> -static inline void smpboot_setup_warm_reset_vector(unsigned long start_eip)
> -{
> -       unsigned long flags;
> -
> -       spin_lock_irqsave(&rtc_lock, flags);
> -       CMOS_WRITE(0xa, 0xf);
> -       spin_unlock_irqrestore(&rtc_lock, flags);
> -       *((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_HIGH)) =
> -                                                       start_eip >> 4;
> -       *((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) =
> -                                                       start_eip & 0xf;
> -}
> -
> -static inline void smpboot_restore_warm_reset_vector(void)
> -{
> -       unsigned long flags;
> -
> -       /*
> -        * Paranoid:  Set warm reset code and vector here back
> -        * to default values.
> -        */
> -       spin_lock_irqsave(&rtc_lock, flags);
> -       CMOS_WRITE(0, 0xf);
> -       spin_unlock_irqrestore(&rtc_lock, flags);
> -
> -       *((volatile u32 *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = 0;
> -}
> -
>  static void init_freq_invariance(void);
>
>  /*
> @@ -1049,20 +1020,6 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
>          * the targeted processor.
>          */
>
> -       if (x86_platform.legacy.warm_reset) {
> -
> -               pr_debug("Setting warm reset code and vector.\n");
> -
> -               smpboot_setup_warm_reset_vector(start_ip);
> -               /*
> -                * Be paranoid about clearing APIC errors.
> -               */
> -               if (APIC_INTEGRATED(boot_cpu_apic_version)) {
> -                       apic_write(APIC_ESR, 0);
> -                       apic_read(APIC_ESR);
> -               }
> -       }
> -
>         /*
>          * AP might wait on cpu_callout_mask in cpu_init() with
>          * cpu_initialized_mask set if previous attempt to online
> @@ -1118,13 +1075,6 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
>                 }
>         }
>
> -       if (x86_platform.legacy.warm_reset) {
> -               /*
> -                * Cleanup possible dangling ends...
> -                */
> -               smpboot_restore_warm_reset_vector();
> -       }
> -
>         return boot_error;
>  }

You removed x86_platform.legacy.warm_reset in the original patch, but
that is missing in V2.

--
Brian Gerst

Re: [PATCH v2] x86/smpboot: Remove 486-isms from the modern AP boot path

[Andrew Cooper]

On 31/03/2020 23:23, Brian Gerst wrote:
> On Tue, Mar 31, 2020 at 1:59 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> Linux has an implementation of the Universal Start-up Algorithm (MP spec,
>> Appendix B.4, Application Processor Startup), which includes unconditionally
>> writing to the Bios Data Area and CMOS registers.
>>
>> The warm reset vector is only necessary in the non-integrated Local APIC case.
>> UV and Jailhouse already have an opt-out for this behaviour, but blindly using
>> the BDA and CMOS on a UEFI or other reduced hardware system isn't clever.
>>
>> We could make this conditional on the integrated-ness of the Local APIC, but
>> 486-era SMP isn't supported.  Drop the logic completely, tidying up the includ
>> list and header files as appropriate.
>>
>> CC: Thomas Gleixner <tglx@linutronix.de>
>> CC: Ingo Molnar <mingo@redhat.com>
>> CC: Borislav Petkov <bp@alien8.de>
>> CC: "H. Peter Anvin" <hpa@zytor.com>
>> CC: x86@kernel.org
>> CC: Jan Kiszka <jan.kiszka@siemens.com>
>> CC: James Morris <jmorris@namei.org>
>> CC: David Howells <dhowells@redhat.com>
>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Matthew Garrett <mjg59@google.com>
>> CC: Josh Boyer <jwboyer@redhat.com>
>> CC: Steve Wahl <steve.wahl@hpe.com>
>> CC: Mike Travis <mike.travis@hpe.com>
>> CC: Dimitri Sivanich <dimitri.sivanich@hpe.com>
>> CC: Arnd Bergmann <arnd@arndb.de>
>> CC: "Peter Zijlstra (Intel)" <peterz@infradead.org>
>> CC: Giovanni Gherdovich <ggherdovich@suse.cz>
>> CC: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
>> CC: Len Brown <len.brown@intel.com>
>> CC: Kees Cook <keescook@chromium.org>
>> CC: Martin Molnar <martin.molnar.programming@gmail.com>
>> CC: Pingfan Liu <kernelfans@gmail.com>
>> CC: linux-kernel@vger.kernel.org
>> CC: jailhouse-dev@googlegroups.com
>> Suggested-by: "H. Peter Anvin" <hpa@zytor.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> v2:
>>  * Drop logic entirely, rather than retaining support in 32bit builds.
>> ---
>>  arch/x86/include/asm/apic.h        |  6 -----
>>  arch/x86/include/asm/x86_init.h    |  1 -
>>  arch/x86/kernel/apic/x2apic_uv_x.c |  1 -
>>  arch/x86/kernel/jailhouse.c        |  1 -
>>  arch/x86/kernel/platform-quirks.c  |  1 -
>>  arch/x86/kernel/smpboot.c          | 50 --------------------------------------
>>  6 files changed, 60 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
>> index 19e94af9cc5d..5c33f9374b28 100644
>> --- a/arch/x86/include/asm/apic.h
>> +++ b/arch/x86/include/asm/apic.h
>> @@ -472,12 +472,6 @@ static inline unsigned default_get_apic_id(unsigned long x)
>>                 return (x >> 24) & 0x0F;
>>  }
>>
>> -/*
>> - * Warm reset vector position:
>> - */
>> -#define TRAMPOLINE_PHYS_LOW            0x467
>> -#define TRAMPOLINE_PHYS_HIGH           0x469
>> -
>>  extern void generic_bigsmp_probe(void);
>>
>>  #ifdef CONFIG_X86_LOCAL_APIC
>> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
>> index 96d9cd208610..006a5d7fd7eb 100644
>> --- a/arch/x86/include/asm/x86_init.h
>> +++ b/arch/x86/include/asm/x86_init.h
>> @@ -229,7 +229,6 @@ enum x86_legacy_i8042_state {
>>  struct x86_legacy_features {
>>         enum x86_legacy_i8042_state i8042;
>>         int rtc;
>> -       int warm_reset;
>>         int no_vga;
>>         int reserve_bios_regions;
>>         struct x86_legacy_devices devices;
>> diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
>> index ad53b2abc859..5afcfd193592 100644
>> --- a/arch/x86/kernel/apic/x2apic_uv_x.c
>> +++ b/arch/x86/kernel/apic/x2apic_uv_x.c
>> @@ -343,7 +343,6 @@ static int __init uv_acpi_madt_oem_check(char *_oem_id, char *_oem_table_id)
>>         } else if (!strcmp(oem_table_id, "UVH")) {
>>                 /* Only UV1 systems: */
>>                 uv_system_type = UV_NON_UNIQUE_APIC;
>> -               x86_platform.legacy.warm_reset = 0;
>>                 __this_cpu_write(x2apic_extra_bits, pnodeid << uvh_apicid.s.pnode_shift);
>>                 uv_set_apicid_hibit();
>>                 uv_apic = 1;
>> diff --git a/arch/x86/kernel/jailhouse.c b/arch/x86/kernel/jailhouse.c
>> index 6eb8b50ea07e..d628fe92d6af 100644
>> --- a/arch/x86/kernel/jailhouse.c
>> +++ b/arch/x86/kernel/jailhouse.c
>> @@ -210,7 +210,6 @@ static void __init jailhouse_init_platform(void)
>>         x86_platform.calibrate_tsc      = jailhouse_get_tsc;
>>         x86_platform.get_wallclock      = jailhouse_get_wallclock;
>>         x86_platform.legacy.rtc         = 0;
>> -       x86_platform.legacy.warm_reset  = 0;
>>         x86_platform.legacy.i8042       = X86_LEGACY_I8042_PLATFORM_ABSENT;
>>
>>         legacy_pic                      = &null_legacy_pic;
>> diff --git a/arch/x86/kernel/platform-quirks.c b/arch/x86/kernel/platform-quirks.c
>> index b348a672f71d..d922c5e0c678 100644
>> --- a/arch/x86/kernel/platform-quirks.c
>> +++ b/arch/x86/kernel/platform-quirks.c
>> @@ -9,7 +9,6 @@ void __init x86_early_init_platform_quirks(void)
>>  {
>>         x86_platform.legacy.i8042 = X86_LEGACY_I8042_EXPECTED_PRESENT;
>>         x86_platform.legacy.rtc = 1;
>> -       x86_platform.legacy.warm_reset = 1;
>>         x86_platform.legacy.reserve_bios_regions = 0;
>>         x86_platform.legacy.devices.pnpbios = 1;
>>
>> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
>> index fe3ab9632f3b..a9f5b511d0b4 100644
>> --- a/arch/x86/kernel/smpboot.c
>> +++ b/arch/x86/kernel/smpboot.c
>> @@ -72,7 +72,6 @@
>>  #include <asm/fpu/internal.h>
>>  #include <asm/setup.h>
>>  #include <asm/uv/uv.h>
>> -#include <linux/mc146818rtc.h>
>>  #include <asm/i8259.h>
>>  #include <asm/misc.h>
>>  #include <asm/qspinlock.h>
>> @@ -119,34 +118,6 @@ int arch_update_cpu_topology(void)
>>         return retval;
>>  }
>>
>> -static inline void smpboot_setup_warm_reset_vector(unsigned long start_eip)
>> -{
>> -       unsigned long flags;
>> -
>> -       spin_lock_irqsave(&rtc_lock, flags);
>> -       CMOS_WRITE(0xa, 0xf);
>> -       spin_unlock_irqrestore(&rtc_lock, flags);
>> -       *((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_HIGH)) =
>> -                                                       start_eip >> 4;
>> -       *((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) =
>> -                                                       start_eip & 0xf;
>> -}
>> -
>> -static inline void smpboot_restore_warm_reset_vector(void)
>> -{
>> -       unsigned long flags;
>> -
>> -       /*
>> -        * Paranoid:  Set warm reset code and vector here back
>> -        * to default values.
>> -        */
>> -       spin_lock_irqsave(&rtc_lock, flags);
>> -       CMOS_WRITE(0, 0xf);
>> -       spin_unlock_irqrestore(&rtc_lock, flags);
>> -
>> -       *((volatile u32 *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = 0;
>> -}
>> -
>>  static void init_freq_invariance(void);
>>
>>  /*
>> @@ -1049,20 +1020,6 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
>>          * the targeted processor.
>>          */
>>
>> -       if (x86_platform.legacy.warm_reset) {
>> -
>> -               pr_debug("Setting warm reset code and vector.\n");
>> -
>> -               smpboot_setup_warm_reset_vector(start_ip);
>> -               /*
>> -                * Be paranoid about clearing APIC errors.
>> -               */
>> -               if (APIC_INTEGRATED(boot_cpu_apic_version)) {
>> -                       apic_write(APIC_ESR, 0);
>> -                       apic_read(APIC_ESR);
>> -               }
>> -       }
>> -
>>         /*
>>          * AP might wait on cpu_callout_mask in cpu_init() with
>>          * cpu_initialized_mask set if previous attempt to online
>> @@ -1118,13 +1075,6 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
>>                 }
>>         }
>>
>> -       if (x86_platform.legacy.warm_reset) {
>> -               /*
>> -                * Cleanup possible dangling ends...
>> -                */
>> -               smpboot_restore_warm_reset_vector();
>> -       }
>> -
>>         return boot_error;
>>  }
> You removed x86_platform.legacy.warm_reset in the original patch, but
> that is missing in V2.

Second hunk?  Or are you referring to something different?

~Andrew

Re: [PATCH v2] x86/smpboot: Remove 486-isms from the modern AP boot path

[Brian Gerst]

On Tue, Mar 31, 2020 at 6:44 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 31/03/2020 23:23, Brian Gerst wrote:
> > On Tue, Mar 31, 2020 at 1:59 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >> Linux has an implementation of the Universal Start-up Algorithm (MP spec,
> >> Appendix B.4, Application Processor Startup), which includes unconditionally
> >> writing to the Bios Data Area and CMOS registers.
> >>
> >> The warm reset vector is only necessary in the non-integrated Local APIC case.
> >> UV and Jailhouse already have an opt-out for this behaviour, but blindly using
> >> the BDA and CMOS on a UEFI or other reduced hardware system isn't clever.
> >>
> >> We could make this conditional on the integrated-ness of the Local APIC, but
> >> 486-era SMP isn't supported.  Drop the logic completely, tidying up the includ
> >> list and header files as appropriate.
> >>
> >> CC: Thomas Gleixner <tglx@linutronix.de>
> >> CC: Ingo Molnar <mingo@redhat.com>
> >> CC: Borislav Petkov <bp@alien8.de>
> >> CC: "H. Peter Anvin" <hpa@zytor.com>
> >> CC: x86@kernel.org
> >> CC: Jan Kiszka <jan.kiszka@siemens.com>
> >> CC: James Morris <jmorris@namei.org>
> >> CC: David Howells <dhowells@redhat.com>
> >> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> >> CC: Matthew Garrett <mjg59@google.com>
> >> CC: Josh Boyer <jwboyer@redhat.com>
> >> CC: Steve Wahl <steve.wahl@hpe.com>
> >> CC: Mike Travis <mike.travis@hpe.com>
> >> CC: Dimitri Sivanich <dimitri.sivanich@hpe.com>
> >> CC: Arnd Bergmann <arnd@arndb.de>
> >> CC: "Peter Zijlstra (Intel)" <peterz@infradead.org>
> >> CC: Giovanni Gherdovich <ggherdovich@suse.cz>
> >> CC: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> >> CC: Len Brown <len.brown@intel.com>
> >> CC: Kees Cook <keescook@chromium.org>
> >> CC: Martin Molnar <martin.molnar.programming@gmail.com>
> >> CC: Pingfan Liu <kernelfans@gmail.com>
> >> CC: linux-kernel@vger.kernel.org
> >> CC: jailhouse-dev@googlegroups.com
> >> Suggested-by: "H. Peter Anvin" <hpa@zytor.com>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> ---
> >> v2:
> >>  * Drop logic entirely, rather than retaining support in 32bit builds.
> >> ---
> >>  arch/x86/include/asm/apic.h        |  6 -----
> >>  arch/x86/include/asm/x86_init.h    |  1 -
> >>  arch/x86/kernel/apic/x2apic_uv_x.c |  1 -
> >>  arch/x86/kernel/jailhouse.c        |  1 -
> >>  arch/x86/kernel/platform-quirks.c  |  1 -
> >>  arch/x86/kernel/smpboot.c          | 50 --------------------------------------
> >>  6 files changed, 60 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> >> index 19e94af9cc5d..5c33f9374b28 100644
> >> --- a/arch/x86/include/asm/apic.h
> >> +++ b/arch/x86/include/asm/apic.h
> >> @@ -472,12 +472,6 @@ static inline unsigned default_get_apic_id(unsigned long x)
> >>                 return (x >> 24) & 0x0F;
> >>  }
> >>
> >> -/*
> >> - * Warm reset vector position:
> >> - */
> >> -#define TRAMPOLINE_PHYS_LOW            0x467
> >> -#define TRAMPOLINE_PHYS_HIGH           0x469
> >> -
> >>  extern void generic_bigsmp_probe(void);
> >>
> >>  #ifdef CONFIG_X86_LOCAL_APIC
> >> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> >> index 96d9cd208610..006a5d7fd7eb 100644
> >> --- a/arch/x86/include/asm/x86_init.h
> >> +++ b/arch/x86/include/asm/x86_init.h
> >> @@ -229,7 +229,6 @@ enum x86_legacy_i8042_state {
> >>  struct x86_legacy_features {
> >>         enum x86_legacy_i8042_state i8042;
> >>         int rtc;
> >> -       int warm_reset;
> >>         int no_vga;
> >>         int reserve_bios_regions;
> >>         struct x86_legacy_devices devices;
> >> diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
> >> index ad53b2abc859..5afcfd193592 100644
> >> --- a/arch/x86/kernel/apic/x2apic_uv_x.c
> >> +++ b/arch/x86/kernel/apic/x2apic_uv_x.c
> >> @@ -343,7 +343,6 @@ static int __init uv_acpi_madt_oem_check(char *_oem_id, char *_oem_table_id)
> >>         } else if (!strcmp(oem_table_id, "UVH")) {
> >>                 /* Only UV1 systems: */
> >>                 uv_system_type = UV_NON_UNIQUE_APIC;
> >> -               x86_platform.legacy.warm_reset = 0;
> >>                 __this_cpu_write(x2apic_extra_bits, pnodeid << uvh_apicid.s.pnode_shift);
> >>                 uv_set_apicid_hibit();
> >>                 uv_apic = 1;
> >> diff --git a/arch/x86/kernel/jailhouse.c b/arch/x86/kernel/jailhouse.c
> >> index 6eb8b50ea07e..d628fe92d6af 100644
> >> --- a/arch/x86/kernel/jailhouse.c
> >> +++ b/arch/x86/kernel/jailhouse.c
> >> @@ -210,7 +210,6 @@ static void __init jailhouse_init_platform(void)
> >>         x86_platform.calibrate_tsc      = jailhouse_get_tsc;
> >>         x86_platform.get_wallclock      = jailhouse_get_wallclock;
> >>         x86_platform.legacy.rtc         = 0;
> >> -       x86_platform.legacy.warm_reset  = 0;
> >>         x86_platform.legacy.i8042       = X86_LEGACY_I8042_PLATFORM_ABSENT;
> >>
> >>         legacy_pic                      = &null_legacy_pic;
> >> diff --git a/arch/x86/kernel/platform-quirks.c b/arch/x86/kernel/platform-quirks.c
> >> index b348a672f71d..d922c5e0c678 100644
> >> --- a/arch/x86/kernel/platform-quirks.c
> >> +++ b/arch/x86/kernel/platform-quirks.c
> >> @@ -9,7 +9,6 @@ void __init x86_early_init_platform_quirks(void)
> >>  {
> >>         x86_platform.legacy.i8042 = X86_LEGACY_I8042_EXPECTED_PRESENT;
> >>         x86_platform.legacy.rtc = 1;
> >> -       x86_platform.legacy.warm_reset = 1;
> >>         x86_platform.legacy.reserve_bios_regions = 0;
> >>         x86_platform.legacy.devices.pnpbios = 1;
> >>
> >> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> >> index fe3ab9632f3b..a9f5b511d0b4 100644
> >> --- a/arch/x86/kernel/smpboot.c
> >> +++ b/arch/x86/kernel/smpboot.c
> >> @@ -72,7 +72,6 @@
> >>  #include <asm/fpu/internal.h>
> >>  #include <asm/setup.h>
> >>  #include <asm/uv/uv.h>
> >> -#include <linux/mc146818rtc.h>
> >>  #include <asm/i8259.h>
> >>  #include <asm/misc.h>
> >>  #include <asm/qspinlock.h>
> >> @@ -119,34 +118,6 @@ int arch_update_cpu_topology(void)
> >>         return retval;
> >>  }
> >>
> >> -static inline void smpboot_setup_warm_reset_vector(unsigned long start_eip)
> >> -{
> >> -       unsigned long flags;
> >> -
> >> -       spin_lock_irqsave(&rtc_lock, flags);
> >> -       CMOS_WRITE(0xa, 0xf);
> >> -       spin_unlock_irqrestore(&rtc_lock, flags);
> >> -       *((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_HIGH)) =
> >> -                                                       start_eip >> 4;
> >> -       *((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) =
> >> -                                                       start_eip & 0xf;
> >> -}
> >> -
> >> -static inline void smpboot_restore_warm_reset_vector(void)
> >> -{
> >> -       unsigned long flags;
> >> -
> >> -       /*
> >> -        * Paranoid:  Set warm reset code and vector here back
> >> -        * to default values.
> >> -        */
> >> -       spin_lock_irqsave(&rtc_lock, flags);
> >> -       CMOS_WRITE(0, 0xf);
> >> -       spin_unlock_irqrestore(&rtc_lock, flags);
> >> -
> >> -       *((volatile u32 *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = 0;
> >> -}
> >> -
> >>  static void init_freq_invariance(void);
> >>
> >>  /*
> >> @@ -1049,20 +1020,6 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
> >>          * the targeted processor.
> >>          */
> >>
> >> -       if (x86_platform.legacy.warm_reset) {
> >> -
> >> -               pr_debug("Setting warm reset code and vector.\n");
> >> -
> >> -               smpboot_setup_warm_reset_vector(start_ip);
> >> -               /*
> >> -                * Be paranoid about clearing APIC errors.
> >> -               */
> >> -               if (APIC_INTEGRATED(boot_cpu_apic_version)) {
> >> -                       apic_write(APIC_ESR, 0);
> >> -                       apic_read(APIC_ESR);
> >> -               }
> >> -       }
> >> -
> >>         /*
> >>          * AP might wait on cpu_callout_mask in cpu_init() with
> >>          * cpu_initialized_mask set if previous attempt to online
> >> @@ -1118,13 +1075,6 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
> >>                 }
> >>         }
> >>
> >> -       if (x86_platform.legacy.warm_reset) {
> >> -               /*
> >> -                * Cleanup possible dangling ends...
> >> -                */
> >> -               smpboot_restore_warm_reset_vector();
> >> -       }
> >> -
> >>         return boot_error;
> >>  }
> > You removed x86_platform.legacy.warm_reset in the original patch, but
> > that is missing in V2.
>
> Second hunk?  Or are you referring to something different?

Removing the warm_reset field from struct x86_legacy_features.

--
Brian Gerst

Re: [PATCH v2] x86/smpboot: Remove 486-isms from the modern AP boot path

[Andrew Cooper]

On 31/03/2020 23:53, Brian Gerst wrote:
> On Tue, Mar 31, 2020 at 6:44 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 31/03/2020 23:23, Brian Gerst wrote:
>>> On Tue, Mar 31, 2020 at 1:59 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>> Linux has an implementation of the Universal Start-up Algorithm (MP spec,
>>>> Appendix B.4, Application Processor Startup), which includes unconditionally
>>>> writing to the Bios Data Area and CMOS registers.
>>>>
>>>> The warm reset vector is only necessary in the non-integrated Local APIC case.
>>>> UV and Jailhouse already have an opt-out for this behaviour, but blindly using
>>>> the BDA and CMOS on a UEFI or other reduced hardware system isn't clever.
>>>>
>>>> We could make this conditional on the integrated-ness of the Local APIC, but
>>>> 486-era SMP isn't supported.  Drop the logic completely, tidying up the includ
>>>> list and header files as appropriate.
>>>>
>>>> CC: Thomas Gleixner <tglx@linutronix.de>
>>>> CC: Ingo Molnar <mingo@redhat.com>
>>>> CC: Borislav Petkov <bp@alien8.de>
>>>> CC: "H. Peter Anvin" <hpa@zytor.com>
>>>> CC: x86@kernel.org
>>>> CC: Jan Kiszka <jan.kiszka@siemens.com>
>>>> CC: James Morris <jmorris@namei.org>
>>>> CC: David Howells <dhowells@redhat.com>
>>>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> CC: Matthew Garrett <mjg59@google.com>
>>>> CC: Josh Boyer <jwboyer@redhat.com>
>>>> CC: Steve Wahl <steve.wahl@hpe.com>
>>>> CC: Mike Travis <mike.travis@hpe.com>
>>>> CC: Dimitri Sivanich <dimitri.sivanich@hpe.com>
>>>> CC: Arnd Bergmann <arnd@arndb.de>
>>>> CC: "Peter Zijlstra (Intel)" <peterz@infradead.org>
>>>> CC: Giovanni Gherdovich <ggherdovich@suse.cz>
>>>> CC: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
>>>> CC: Len Brown <len.brown@intel.com>
>>>> CC: Kees Cook <keescook@chromium.org>
>>>> CC: Martin Molnar <martin.molnar.programming@gmail.com>
>>>> CC: Pingfan Liu <kernelfans@gmail.com>
>>>> CC: linux-kernel@vger.kernel.org
>>>> CC: jailhouse-dev@googlegroups.com
>>>> Suggested-by: "H. Peter Anvin" <hpa@zytor.com>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>> v2:
>>>>  * Drop logic entirely, rather than retaining support in 32bit builds.
>>>> ---
>>>>  arch/x86/include/asm/apic.h        |  6 -----
>>>>  arch/x86/include/asm/x86_init.h    |  1 -
>>>>  arch/x86/kernel/apic/x2apic_uv_x.c |  1 -
>>>>  arch/x86/kernel/jailhouse.c        |  1 -
>>>>  arch/x86/kernel/platform-quirks.c  |  1 -
>>>>  arch/x86/kernel/smpboot.c          | 50 --------------------------------------
>>>>  6 files changed, 60 deletions(-)
>>>>
>>>> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
>>>> index 19e94af9cc5d..5c33f9374b28 100644
>>>> --- a/arch/x86/include/asm/apic.h
>>>> +++ b/arch/x86/include/asm/apic.h
>>>> @@ -472,12 +472,6 @@ static inline unsigned default_get_apic_id(unsigned long x)
>>>>                 return (x >> 24) & 0x0F;
>>>>  }
>>>>
>>>> -/*
>>>> - * Warm reset vector position:
>>>> - */
>>>> -#define TRAMPOLINE_PHYS_LOW            0x467
>>>> -#define TRAMPOLINE_PHYS_HIGH           0x469
>>>> -
>>>>  extern void generic_bigsmp_probe(void);
>>>>
>>>>  #ifdef CONFIG_X86_LOCAL_APIC
>>>> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
>>>> index 96d9cd208610..006a5d7fd7eb 100644
>>>> --- a/arch/x86/include/asm/x86_init.h
>>>> +++ b/arch/x86/include/asm/x86_init.h
>>>> @@ -229,7 +229,6 @@ enum x86_legacy_i8042_state {
>>>>  struct x86_legacy_features {
>>>>         enum x86_legacy_i8042_state i8042;
>>>>         int rtc;
>>>> -       int warm_reset;
>>>>         int no_vga;
>>>>         int reserve_bios_regions;
>>>>         struct x86_legacy_devices devices;
>>>> diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
>>>> index ad53b2abc859..5afcfd193592 100644
>>>> --- a/arch/x86/kernel/apic/x2apic_uv_x.c
>>>> +++ b/arch/x86/kernel/apic/x2apic_uv_x.c
>>>> @@ -343,7 +343,6 @@ static int __init uv_acpi_madt_oem_check(char *_oem_id, char *_oem_table_id)
>>>>         } else if (!strcmp(oem_table_id, "UVH")) {
>>>>                 /* Only UV1 systems: */
>>>>                 uv_system_type = UV_NON_UNIQUE_APIC;
>>>> -               x86_platform.legacy.warm_reset = 0;
>>>>                 __this_cpu_write(x2apic_extra_bits, pnodeid << uvh_apicid.s.pnode_shift);
>>>>                 uv_set_apicid_hibit();
>>>>                 uv_apic = 1;
>>>> diff --git a/arch/x86/kernel/jailhouse.c b/arch/x86/kernel/jailhouse.c
>>>> index 6eb8b50ea07e..d628fe92d6af 100644
>>>> --- a/arch/x86/kernel/jailhouse.c
>>>> +++ b/arch/x86/kernel/jailhouse.c
>>>> @@ -210,7 +210,6 @@ static void __init jailhouse_init_platform(void)
>>>>         x86_platform.calibrate_tsc      = jailhouse_get_tsc;
>>>>         x86_platform.get_wallclock      = jailhouse_get_wallclock;
>>>>         x86_platform.legacy.rtc         = 0;
>>>> -       x86_platform.legacy.warm_reset  = 0;
>>>>         x86_platform.legacy.i8042       = X86_LEGACY_I8042_PLATFORM_ABSENT;
>>>>
>>>>         legacy_pic                      = &null_legacy_pic;
>>>> diff --git a/arch/x86/kernel/platform-quirks.c b/arch/x86/kernel/platform-quirks.c
>>>> index b348a672f71d..d922c5e0c678 100644
>>>> --- a/arch/x86/kernel/platform-quirks.c
>>>> +++ b/arch/x86/kernel/platform-quirks.c
>>>> @@ -9,7 +9,6 @@ void __init x86_early_init_platform_quirks(void)
>>>>  {
>>>>         x86_platform.legacy.i8042 = X86_LEGACY_I8042_EXPECTED_PRESENT;
>>>>         x86_platform.legacy.rtc = 1;
>>>> -       x86_platform.legacy.warm_reset = 1;
>>>>         x86_platform.legacy.reserve_bios_regions = 0;
>>>>         x86_platform.legacy.devices.pnpbios = 1;
>>>>
>>>> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
>>>> index fe3ab9632f3b..a9f5b511d0b4 100644
>>>> --- a/arch/x86/kernel/smpboot.c
>>>> +++ b/arch/x86/kernel/smpboot.c
>>>> @@ -72,7 +72,6 @@
>>>>  #include <asm/fpu/internal.h>
>>>>  #include <asm/setup.h>
>>>>  #include <asm/uv/uv.h>
>>>> -#include <linux/mc146818rtc.h>
>>>>  #include <asm/i8259.h>
>>>>  #include <asm/misc.h>
>>>>  #include <asm/qspinlock.h>
>>>> @@ -119,34 +118,6 @@ int arch_update_cpu_topology(void)
>>>>         return retval;
>>>>  }
>>>>
>>>> -static inline void smpboot_setup_warm_reset_vector(unsigned long start_eip)
>>>> -{
>>>> -       unsigned long flags;
>>>> -
>>>> -       spin_lock_irqsave(&rtc_lock, flags);
>>>> -       CMOS_WRITE(0xa, 0xf);
>>>> -       spin_unlock_irqrestore(&rtc_lock, flags);
>>>> -       *((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_HIGH)) =
>>>> -                                                       start_eip >> 4;
>>>> -       *((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) =
>>>> -                                                       start_eip & 0xf;
>>>> -}
>>>> -
>>>> -static inline void smpboot_restore_warm_reset_vector(void)
>>>> -{
>>>> -       unsigned long flags;
>>>> -
>>>> -       /*
>>>> -        * Paranoid:  Set warm reset code and vector here back
>>>> -        * to default values.
>>>> -        */
>>>> -       spin_lock_irqsave(&rtc_lock, flags);
>>>> -       CMOS_WRITE(0, 0xf);
>>>> -       spin_unlock_irqrestore(&rtc_lock, flags);
>>>> -
>>>> -       *((volatile u32 *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = 0;
>>>> -}
>>>> -
>>>>  static void init_freq_invariance(void);
>>>>
>>>>  /*
>>>> @@ -1049,20 +1020,6 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
>>>>          * the targeted processor.
>>>>          */
>>>>
>>>> -       if (x86_platform.legacy.warm_reset) {
>>>> -
>>>> -               pr_debug("Setting warm reset code and vector.\n");
>>>> -
>>>> -               smpboot_setup_warm_reset_vector(start_ip);
>>>> -               /*
>>>> -                * Be paranoid about clearing APIC errors.
>>>> -               */
>>>> -               if (APIC_INTEGRATED(boot_cpu_apic_version)) {
>>>> -                       apic_write(APIC_ESR, 0);
>>>> -                       apic_read(APIC_ESR);
>>>> -               }
>>>> -       }
>>>> -
>>>>         /*
>>>>          * AP might wait on cpu_callout_mask in cpu_init() with
>>>>          * cpu_initialized_mask set if previous attempt to online
>>>> @@ -1118,13 +1075,6 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
>>>>                 }
>>>>         }
>>>>
>>>> -       if (x86_platform.legacy.warm_reset) {
>>>> -               /*
>>>> -                * Cleanup possible dangling ends...
>>>> -                */
>>>> -               smpboot_restore_warm_reset_vector();
>>>> -       }
>>>> -
>>>>         return boot_error;
>>>>  }
>>> You removed x86_platform.legacy.warm_reset in the original patch, but
>>> that is missing in V2.
>> Second hunk?  Or are you referring to something different?
> Removing the warm_reset field from struct x86_legacy_features.

Ok, but that is still present as the 2nd hunk of the patch.

~Andrew

Re: [PATCH v2] x86/smpboot: Remove 486-isms from the modern AP boot path

[Brian Gerst]

On Wed, Apr 1, 2020 at 5:22 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 31/03/2020 23:53, Brian Gerst wrote:
> > On Tue, Mar 31, 2020 at 6:44 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >> On 31/03/2020 23:23, Brian Gerst wrote:
> >>> On Tue, Mar 31, 2020 at 1:59 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >>>> Linux has an implementation of the Universal Start-up Algorithm (MP spec,
> >>>> Appendix B.4, Application Processor Startup), which includes unconditionally
> >>>> writing to the Bios Data Area and CMOS registers.
> >>>>
> >>>> The warm reset vector is only necessary in the non-integrated Local APIC case.
> >>>> UV and Jailhouse already have an opt-out for this behaviour, but blindly using
> >>>> the BDA and CMOS on a UEFI or other reduced hardware system isn't clever.
> >>>>
> >>>> We could make this conditional on the integrated-ness of the Local APIC, but
> >>>> 486-era SMP isn't supported.  Drop the logic completely, tidying up the includ
> >>>> list and header files as appropriate.
> >>>>
> >>>> CC: Thomas Gleixner <tglx@linutronix.de>
> >>>> CC: Ingo Molnar <mingo@redhat.com>
> >>>> CC: Borislav Petkov <bp@alien8.de>
> >>>> CC: "H. Peter Anvin" <hpa@zytor.com>
> >>>> CC: x86@kernel.org
> >>>> CC: Jan Kiszka <jan.kiszka@siemens.com>
> >>>> CC: James Morris <jmorris@namei.org>
> >>>> CC: David Howells <dhowells@redhat.com>
> >>>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>> CC: Matthew Garrett <mjg59@google.com>
> >>>> CC: Josh Boyer <jwboyer@redhat.com>
> >>>> CC: Steve Wahl <steve.wahl@hpe.com>
> >>>> CC: Mike Travis <mike.travis@hpe.com>
> >>>> CC: Dimitri Sivanich <dimitri.sivanich@hpe.com>
> >>>> CC: Arnd Bergmann <arnd@arndb.de>
> >>>> CC: "Peter Zijlstra (Intel)" <peterz@infradead.org>
> >>>> CC: Giovanni Gherdovich <ggherdovich@suse.cz>
> >>>> CC: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> >>>> CC: Len Brown <len.brown@intel.com>
> >>>> CC: Kees Cook <keescook@chromium.org>
> >>>> CC: Martin Molnar <martin.molnar.programming@gmail.com>
> >>>> CC: Pingfan Liu <kernelfans@gmail.com>
> >>>> CC: linux-kernel@vger.kernel.org
> >>>> CC: jailhouse-dev@googlegroups.com
> >>>> Suggested-by: "H. Peter Anvin" <hpa@zytor.com>
> >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>> ---
> >>>> v2:
> >>>>  * Drop logic entirely, rather than retaining support in 32bit builds.
> >>>> ---
> >>>>  arch/x86/include/asm/apic.h        |  6 -----
> >>>>  arch/x86/include/asm/x86_init.h    |  1 -
> >>>>  arch/x86/kernel/apic/x2apic_uv_x.c |  1 -
> >>>>  arch/x86/kernel/jailhouse.c        |  1 -
> >>>>  arch/x86/kernel/platform-quirks.c  |  1 -
> >>>>  arch/x86/kernel/smpboot.c          | 50 --------------------------------------
> >>>>  6 files changed, 60 deletions(-)
> >>>>
> >>>> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> >>>> index 19e94af9cc5d..5c33f9374b28 100644
> >>>> --- a/arch/x86/include/asm/apic.h
> >>>> +++ b/arch/x86/include/asm/apic.h
> >>>> @@ -472,12 +472,6 @@ static inline unsigned default_get_apic_id(unsigned long x)
> >>>>                 return (x >> 24) & 0x0F;
> >>>>  }
> >>>>
> >>>> -/*
> >>>> - * Warm reset vector position:
> >>>> - */
> >>>> -#define TRAMPOLINE_PHYS_LOW            0x467
> >>>> -#define TRAMPOLINE_PHYS_HIGH           0x469
> >>>> -
> >>>>  extern void generic_bigsmp_probe(void);
> >>>>
> >>>>  #ifdef CONFIG_X86_LOCAL_APIC
> >>>> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> >>>> index 96d9cd208610..006a5d7fd7eb 100644
> >>>> --- a/arch/x86/include/asm/x86_init.h
> >>>> +++ b/arch/x86/include/asm/x86_init.h
> >>>> @@ -229,7 +229,6 @@ enum x86_legacy_i8042_state {
> >>>>  struct x86_legacy_features {
> >>>>         enum x86_legacy_i8042_state i8042;
> >>>>         int rtc;
> >>>> -       int warm_reset;
> >>>>         int no_vga;
> >>>>         int reserve_bios_regions;
> >>>>         struct x86_legacy_devices devices;
> >>>> diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
> >>>> index ad53b2abc859..5afcfd193592 100644
> >>>> --- a/arch/x86/kernel/apic/x2apic_uv_x.c
> >>>> +++ b/arch/x86/kernel/apic/x2apic_uv_x.c
> >>>> @@ -343,7 +343,6 @@ static int __init uv_acpi_madt_oem_check(char *_oem_id, char *_oem_table_id)
> >>>>         } else if (!strcmp(oem_table_id, "UVH")) {
> >>>>                 /* Only UV1 systems: */
> >>>>                 uv_system_type = UV_NON_UNIQUE_APIC;
> >>>> -               x86_platform.legacy.warm_reset = 0;
> >>>>                 __this_cpu_write(x2apic_extra_bits, pnodeid << uvh_apicid.s.pnode_shift);
> >>>>                 uv_set_apicid_hibit();
> >>>>                 uv_apic = 1;
> >>>> diff --git a/arch/x86/kernel/jailhouse.c b/arch/x86/kernel/jailhouse.c
> >>>> index 6eb8b50ea07e..d628fe92d6af 100644
> >>>> --- a/arch/x86/kernel/jailhouse.c
> >>>> +++ b/arch/x86/kernel/jailhouse.c
> >>>> @@ -210,7 +210,6 @@ static void __init jailhouse_init_platform(void)
> >>>>         x86_platform.calibrate_tsc      = jailhouse_get_tsc;
> >>>>         x86_platform.get_wallclock      = jailhouse_get_wallclock;
> >>>>         x86_platform.legacy.rtc         = 0;
> >>>> -       x86_platform.legacy.warm_reset  = 0;
> >>>>         x86_platform.legacy.i8042       = X86_LEGACY_I8042_PLATFORM_ABSENT;
> >>>>
> >>>>         legacy_pic                      = &null_legacy_pic;
> >>>> diff --git a/arch/x86/kernel/platform-quirks.c b/arch/x86/kernel/platform-quirks.c
> >>>> index b348a672f71d..d922c5e0c678 100644
> >>>> --- a/arch/x86/kernel/platform-quirks.c
> >>>> +++ b/arch/x86/kernel/platform-quirks.c
> >>>> @@ -9,7 +9,6 @@ void __init x86_early_init_platform_quirks(void)
> >>>>  {
> >>>>         x86_platform.legacy.i8042 = X86_LEGACY_I8042_EXPECTED_PRESENT;
> >>>>         x86_platform.legacy.rtc = 1;
> >>>> -       x86_platform.legacy.warm_reset = 1;
> >>>>         x86_platform.legacy.reserve_bios_regions = 0;
> >>>>         x86_platform.legacy.devices.pnpbios = 1;
> >>>>
> >>>> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> >>>> index fe3ab9632f3b..a9f5b511d0b4 100644
> >>>> --- a/arch/x86/kernel/smpboot.c
> >>>> +++ b/arch/x86/kernel/smpboot.c
> >>>> @@ -72,7 +72,6 @@
> >>>>  #include <asm/fpu/internal.h>
> >>>>  #include <asm/setup.h>
> >>>>  #include <asm/uv/uv.h>
> >>>> -#include <linux/mc146818rtc.h>
> >>>>  #include <asm/i8259.h>
> >>>>  #include <asm/misc.h>
> >>>>  #include <asm/qspinlock.h>
> >>>> @@ -119,34 +118,6 @@ int arch_update_cpu_topology(void)
> >>>>         return retval;
> >>>>  }
> >>>>
> >>>> -static inline void smpboot_setup_warm_reset_vector(unsigned long start_eip)
> >>>> -{
> >>>> -       unsigned long flags;
> >>>> -
> >>>> -       spin_lock_irqsave(&rtc_lock, flags);
> >>>> -       CMOS_WRITE(0xa, 0xf);
> >>>> -       spin_unlock_irqrestore(&rtc_lock, flags);
> >>>> -       *((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_HIGH)) =
> >>>> -                                                       start_eip >> 4;
> >>>> -       *((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) =
> >>>> -                                                       start_eip & 0xf;
> >>>> -}
> >>>> -
> >>>> -static inline void smpboot_restore_warm_reset_vector(void)
> >>>> -{
> >>>> -       unsigned long flags;
> >>>> -
> >>>> -       /*
> >>>> -        * Paranoid:  Set warm reset code and vector here back
> >>>> -        * to default values.
> >>>> -        */
> >>>> -       spin_lock_irqsave(&rtc_lock, flags);
> >>>> -       CMOS_WRITE(0, 0xf);
> >>>> -       spin_unlock_irqrestore(&rtc_lock, flags);
> >>>> -
> >>>> -       *((volatile u32 *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = 0;
> >>>> -}
> >>>> -
> >>>>  static void init_freq_invariance(void);
> >>>>
> >>>>  /*
> >>>> @@ -1049,20 +1020,6 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
> >>>>          * the targeted processor.
> >>>>          */
> >>>>
> >>>> -       if (x86_platform.legacy.warm_reset) {
> >>>> -
> >>>> -               pr_debug("Setting warm reset code and vector.\n");
> >>>> -
> >>>> -               smpboot_setup_warm_reset_vector(start_ip);
> >>>> -               /*
> >>>> -                * Be paranoid about clearing APIC errors.
> >>>> -               */
> >>>> -               if (APIC_INTEGRATED(boot_cpu_apic_version)) {
> >>>> -                       apic_write(APIC_ESR, 0);
> >>>> -                       apic_read(APIC_ESR);
> >>>> -               }
> >>>> -       }
> >>>> -
> >>>>         /*
> >>>>          * AP might wait on cpu_callout_mask in cpu_init() with
> >>>>          * cpu_initialized_mask set if previous attempt to online
> >>>> @@ -1118,13 +1075,6 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
> >>>>                 }
> >>>>         }
> >>>>
> >>>> -       if (x86_platform.legacy.warm_reset) {
> >>>> -               /*
> >>>> -                * Cleanup possible dangling ends...
> >>>> -                */
> >>>> -               smpboot_restore_warm_reset_vector();
> >>>> -       }
> >>>> -
> >>>>         return boot_error;
> >>>>  }
> >>> You removed x86_platform.legacy.warm_reset in the original patch, but
> >>> that is missing in V2.
> >> Second hunk?  Or are you referring to something different?
> > Removing the warm_reset field from struct x86_legacy_features.
>
> Ok, but that is still present as the 2nd hunk of the patch.

My apologies, Gmail was hiding that section of the patch because it
was a reply to the original patch.  For future reference, add the
version number to the title when resubmitting a patch (ie. [PATCH
v2]).

--
Brian Gerst

Re: [PATCH v2] x86/smpboot: Remove 486-isms from the modern AP boot path

[Andrew Cooper]

On 01/04/2020 12:39, Brian Gerst wrote:
> On Wed, Apr 1, 2020 at 5:22 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 31/03/2020 23:53, Brian Gerst wrote:
>>> On Tue, Mar 31, 2020 at 6:44 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>> On 31/03/2020 23:23, Brian Gerst wrote:
>>>>> On Tue, Mar 31, 2020 at 1:59 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>>>> Linux has an implementation of the Universal Start-up Algorithm (MP spec,
>>>>>> Appendix B.4, Application Processor Startup), which includes unconditionally
>>>>>> writing to the Bios Data Area and CMOS registers.
>>>>>>
>>>>>> The warm reset vector is only necessary in the non-integrated Local APIC case.
>>>>>> UV and Jailhouse already have an opt-out for this behaviour, but blindly using
>>>>>> the BDA and CMOS on a UEFI or other reduced hardware system isn't clever.
>>>>>>
>>>>>> We could make this conditional on the integrated-ness of the Local APIC, but
>>>>>> 486-era SMP isn't supported.  Drop the logic completely, tidying up the includ
>>>>>> list and header files as appropriate.
>>>>>>
>>>>>> CC: Thomas Gleixner <tglx@linutronix.de>
>>>>>> CC: Ingo Molnar <mingo@redhat.com>
>>>>>> CC: Borislav Petkov <bp@alien8.de>
>>>>>> CC: "H. Peter Anvin" <hpa@zytor.com>
>>>>>> CC: x86@kernel.org
>>>>>> CC: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> CC: James Morris <jmorris@namei.org>
>>>>>> CC: David Howells <dhowells@redhat.com>
>>>>>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>> CC: Matthew Garrett <mjg59@google.com>
>>>>>> CC: Josh Boyer <jwboyer@redhat.com>
>>>>>> CC: Steve Wahl <steve.wahl@hpe.com>
>>>>>> CC: Mike Travis <mike.travis@hpe.com>
>>>>>> CC: Dimitri Sivanich <dimitri.sivanich@hpe.com>
>>>>>> CC: Arnd Bergmann <arnd@arndb.de>
>>>>>> CC: "Peter Zijlstra (Intel)" <peterz@infradead.org>
>>>>>> CC: Giovanni Gherdovich <ggherdovich@suse.cz>
>>>>>> CC: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
>>>>>> CC: Len Brown <len.brown@intel.com>
>>>>>> CC: Kees Cook <keescook@chromium.org>
>>>>>> CC: Martin Molnar <martin.molnar.programming@gmail.com>
>>>>>> CC: Pingfan Liu <kernelfans@gmail.com>
>>>>>> CC: linux-kernel@vger.kernel.org
>>>>>> CC: jailhouse-dev@googlegroups.com
>>>>>> Suggested-by: "H. Peter Anvin" <hpa@zytor.com>
>>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>> ---
>>>>>> v2:
>>>>>>  * Drop logic entirely, rather than retaining support in 32bit builds.
>>>>>> ---
>>>>>>  arch/x86/include/asm/apic.h        |  6 -----
>>>>>>  arch/x86/include/asm/x86_init.h    |  1 -
>>>>>>  arch/x86/kernel/apic/x2apic_uv_x.c |  1 -
>>>>>>  arch/x86/kernel/jailhouse.c        |  1 -
>>>>>>  arch/x86/kernel/platform-quirks.c  |  1 -
>>>>>>  arch/x86/kernel/smpboot.c          | 50 --------------------------------------
>>>>>>  6 files changed, 60 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
>>>>>> index 19e94af9cc5d..5c33f9374b28 100644
>>>>>> --- a/arch/x86/include/asm/apic.h
>>>>>> +++ b/arch/x86/include/asm/apic.h
>>>>>> @@ -472,12 +472,6 @@ static inline unsigned default_get_apic_id(unsigned long x)
>>>>>>                 return (x >> 24) & 0x0F;
>>>>>>  }
>>>>>>
>>>>>> -/*
>>>>>> - * Warm reset vector position:
>>>>>> - */
>>>>>> -#define TRAMPOLINE_PHYS_LOW            0x467
>>>>>> -#define TRAMPOLINE_PHYS_HIGH           0x469
>>>>>> -
>>>>>>  extern void generic_bigsmp_probe(void);
>>>>>>
>>>>>>  #ifdef CONFIG_X86_LOCAL_APIC
>>>>>> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
>>>>>> index 96d9cd208610..006a5d7fd7eb 100644
>>>>>> --- a/arch/x86/include/asm/x86_init.h
>>>>>> +++ b/arch/x86/include/asm/x86_init.h
>>>>>> @@ -229,7 +229,6 @@ enum x86_legacy_i8042_state {
>>>>>>  struct x86_legacy_features {
>>>>>>         enum x86_legacy_i8042_state i8042;
>>>>>>         int rtc;
>>>>>> -       int warm_reset;
>>>>>>         int no_vga;
>>>>>>         int reserve_bios_regions;
>>>>>>         struct x86_legacy_devices devices;
>>>>>> diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
>>>>>> index ad53b2abc859..5afcfd193592 100644
>>>>>> --- a/arch/x86/kernel/apic/x2apic_uv_x.c
>>>>>> +++ b/arch/x86/kernel/apic/x2apic_uv_x.c
>>>>>> @@ -343,7 +343,6 @@ static int __init uv_acpi_madt_oem_check(char *_oem_id, char *_oem_table_id)
>>>>>>         } else if (!strcmp(oem_table_id, "UVH")) {
>>>>>>                 /* Only UV1 systems: */
>>>>>>                 uv_system_type = UV_NON_UNIQUE_APIC;
>>>>>> -               x86_platform.legacy.warm_reset = 0;
>>>>>>                 __this_cpu_write(x2apic_extra_bits, pnodeid << uvh_apicid.s.pnode_shift);
>>>>>>                 uv_set_apicid_hibit();
>>>>>>                 uv_apic = 1;
>>>>>> diff --git a/arch/x86/kernel/jailhouse.c b/arch/x86/kernel/jailhouse.c
>>>>>> index 6eb8b50ea07e..d628fe92d6af 100644
>>>>>> --- a/arch/x86/kernel/jailhouse.c
>>>>>> +++ b/arch/x86/kernel/jailhouse.c
>>>>>> @@ -210,7 +210,6 @@ static void __init jailhouse_init_platform(void)
>>>>>>         x86_platform.calibrate_tsc      = jailhouse_get_tsc;
>>>>>>         x86_platform.get_wallclock      = jailhouse_get_wallclock;
>>>>>>         x86_platform.legacy.rtc         = 0;
>>>>>> -       x86_platform.legacy.warm_reset  = 0;
>>>>>>         x86_platform.legacy.i8042       = X86_LEGACY_I8042_PLATFORM_ABSENT;
>>>>>>
>>>>>>         legacy_pic                      = &null_legacy_pic;
>>>>>> diff --git a/arch/x86/kernel/platform-quirks.c b/arch/x86/kernel/platform-quirks.c
>>>>>> index b348a672f71d..d922c5e0c678 100644
>>>>>> --- a/arch/x86/kernel/platform-quirks.c
>>>>>> +++ b/arch/x86/kernel/platform-quirks.c
>>>>>> @@ -9,7 +9,6 @@ void __init x86_early_init_platform_quirks(void)
>>>>>>  {
>>>>>>         x86_platform.legacy.i8042 = X86_LEGACY_I8042_EXPECTED_PRESENT;
>>>>>>         x86_platform.legacy.rtc = 1;
>>>>>> -       x86_platform.legacy.warm_reset = 1;
>>>>>>         x86_platform.legacy.reserve_bios_regions = 0;
>>>>>>         x86_platform.legacy.devices.pnpbios = 1;
>>>>>>
>>>>>> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
>>>>>> index fe3ab9632f3b..a9f5b511d0b4 100644
>>>>>> --- a/arch/x86/kernel/smpboot.c
>>>>>> +++ b/arch/x86/kernel/smpboot.c
>>>>>> @@ -72,7 +72,6 @@
>>>>>>  #include <asm/fpu/internal.h>
>>>>>>  #include <asm/setup.h>
>>>>>>  #include <asm/uv/uv.h>
>>>>>> -#include <linux/mc146818rtc.h>
>>>>>>  #include <asm/i8259.h>
>>>>>>  #include <asm/misc.h>
>>>>>>  #include <asm/qspinlock.h>
>>>>>> @@ -119,34 +118,6 @@ int arch_update_cpu_topology(void)
>>>>>>         return retval;
>>>>>>  }
>>>>>>
>>>>>> -static inline void smpboot_setup_warm_reset_vector(unsigned long start_eip)
>>>>>> -{
>>>>>> -       unsigned long flags;
>>>>>> -
>>>>>> -       spin_lock_irqsave(&rtc_lock, flags);
>>>>>> -       CMOS_WRITE(0xa, 0xf);
>>>>>> -       spin_unlock_irqrestore(&rtc_lock, flags);
>>>>>> -       *((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_HIGH)) =
>>>>>> -                                                       start_eip >> 4;
>>>>>> -       *((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) =
>>>>>> -                                                       start_eip & 0xf;
>>>>>> -}
>>>>>> -
>>>>>> -static inline void smpboot_restore_warm_reset_vector(void)
>>>>>> -{
>>>>>> -       unsigned long flags;
>>>>>> -
>>>>>> -       /*
>>>>>> -        * Paranoid:  Set warm reset code and vector here back
>>>>>> -        * to default values.
>>>>>> -        */
>>>>>> -       spin_lock_irqsave(&rtc_lock, flags);
>>>>>> -       CMOS_WRITE(0, 0xf);
>>>>>> -       spin_unlock_irqrestore(&rtc_lock, flags);
>>>>>> -
>>>>>> -       *((volatile u32 *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = 0;
>>>>>> -}
>>>>>> -
>>>>>>  static void init_freq_invariance(void);
>>>>>>
>>>>>>  /*
>>>>>> @@ -1049,20 +1020,6 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
>>>>>>          * the targeted processor.
>>>>>>          */
>>>>>>
>>>>>> -       if (x86_platform.legacy.warm_reset) {
>>>>>> -
>>>>>> -               pr_debug("Setting warm reset code and vector.\n");
>>>>>> -
>>>>>> -               smpboot_setup_warm_reset_vector(start_ip);
>>>>>> -               /*
>>>>>> -                * Be paranoid about clearing APIC errors.
>>>>>> -               */
>>>>>> -               if (APIC_INTEGRATED(boot_cpu_apic_version)) {
>>>>>> -                       apic_write(APIC_ESR, 0);
>>>>>> -                       apic_read(APIC_ESR);
>>>>>> -               }
>>>>>> -       }
>>>>>> -
>>>>>>         /*
>>>>>>          * AP might wait on cpu_callout_mask in cpu_init() with
>>>>>>          * cpu_initialized_mask set if previous attempt to online
>>>>>> @@ -1118,13 +1075,6 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
>>>>>>                 }
>>>>>>         }
>>>>>>
>>>>>> -       if (x86_platform.legacy.warm_reset) {
>>>>>> -               /*
>>>>>> -                * Cleanup possible dangling ends...
>>>>>> -                */
>>>>>> -               smpboot_restore_warm_reset_vector();
>>>>>> -       }
>>>>>> -
>>>>>>         return boot_error;
>>>>>>  }
>>>>> You removed x86_platform.legacy.warm_reset in the original patch, but
>>>>> that is missing in V2.
>>>> Second hunk?  Or are you referring to something different?
>>> Removing the warm_reset field from struct x86_legacy_features.
>> Ok, but that is still present as the 2nd hunk of the patch.
> My apologies, Gmail was hiding that section of the patch because it
> was a reply to the original patch.  For future reference, add the
> version number to the title when resubmitting a patch (ie. [PATCH
> v2]).

Erm... is Gmail hiding that too?

Lore thinks it is there:
https://lore.kernel.org/lkml/CAMzpN2g0LS5anGc7CXco4pgBHhGzc8hw+shMOg8WEWGsx+BHpg@mail.gmail.com/

~Andrew

Re: [PATCH v2] x86/smpboot: Remove 486-isms from the modern AP boot path

[Brian Gerst]

On Wed, Apr 1, 2020 at 8:14 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 01/04/2020 12:39, Brian Gerst wrote:
> > On Wed, Apr 1, 2020 at 5:22 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >> On 31/03/2020 23:53, Brian Gerst wrote:
> >>> On Tue, Mar 31, 2020 at 6:44 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >>>> On 31/03/2020 23:23, Brian Gerst wrote:
> >>>>> On Tue, Mar 31, 2020 at 1:59 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >>>>>> Linux has an implementation of the Universal Start-up Algorithm (MP spec,
> >>>>>> Appendix B.4, Application Processor Startup), which includes unconditionally
> >>>>>> writing to the Bios Data Area and CMOS registers.
> >>>>>>
> >>>>>> The warm reset vector is only necessary in the non-integrated Local APIC case.
> >>>>>> UV and Jailhouse already have an opt-out for this behaviour, but blindly using
> >>>>>> the BDA and CMOS on a UEFI or other reduced hardware system isn't clever.
> >>>>>>
> >>>>>> We could make this conditional on the integrated-ness of the Local APIC, but
> >>>>>> 486-era SMP isn't supported.  Drop the logic completely, tidying up the includ
> >>>>>> list and header files as appropriate.
> >>>>>>

> >>>>> You removed x86_platform.legacy.warm_reset in the original patch, but
> >>>>> that is missing in V2.
> >>>> Second hunk?  Or are you referring to something different?
> >>> Removing the warm_reset field from struct x86_legacy_features.
> >> Ok, but that is still present as the 2nd hunk of the patch.
> > My apologies, Gmail was hiding that section of the patch because it
> > was a reply to the original patch.  For future reference, add the
> > version number to the title when resubmitting a patch (ie. [PATCH
> > v2]).
>
> Erm... is Gmail hiding that too?
>
> Lore thinks it is there:
> https://lore.kernel.org/lkml/CAMzpN2g0LS5anGc7CXco4pgBHhGzc8hw+shMOg8WEWGsx+BHpg@mail.gmail.com/

Ugh, yes.  I thought it was the title that Gmail threaded on, but it
must be the In-Reply-To: header.  Sorry for the confusion.

That said, I think the v1 patch is probably the better way to go (but
adjusting the comments to include early Pentium-era systems without
integrated APICs).  Then the decision to drop support for external
APICs could be a separate patch.

--
Brian Gerst

Re: [PATCH v2] x86/smpboot: Remove 486-isms from the modern AP boot path

[Andrew Cooper]

On 01/04/2020 15:38, Brian Gerst wrote:
>>>>>>> You removed x86_platform.legacy.warm_reset in the original patch, but
>>>>>>> that is missing in V2.
>>>>>> Second hunk?  Or are you referring to something different?
>>>>> Removing the warm_reset field from struct x86_legacy_features.
>>>> Ok, but that is still present as the 2nd hunk of the patch.
>>> My apologies, Gmail was hiding that section of the patch because it
>>> was a reply to the original patch.  For future reference, add the
>>> version number to the title when resubmitting a patch (ie. [PATCH
>>> v2]).
>> Erm... is Gmail hiding that too?
>>
>> Lore thinks it is there:
>> https://lore.kernel.org/lkml/CAMzpN2g0LS5anGc7CXco4pgBHhGzc8hw+shMOg8WEWGsx+BHpg@mail.gmail.com/
> Ugh, yes.  I thought it was the title that Gmail threaded on, but it
> must be the In-Reply-To: header.  Sorry for the confusion.
>
> That said, I think the v1 patch is probably the better way to go (but
> adjusting the comments to include early Pentium-era systems without
> integrated APICs).

Yes - I'm afraid I'm showing my age here, being the same vintage as the 486.

I'll happily rephrase as suggested.

> Then the decision to drop support for external
> APICs could be a separate patch.

I have no vested interest.

This was a fix from Xen that I tried to upstream (if you can really call
it that, seeing as the common point in history was the Linux 2.4 days),
given the rather rude UEFI behaviour.

Ultimately, this will be down to the maintainers for which approach to take.

~Andrew