The current code in do_boot_cpu() makes a CMOS write (even in the case of an
FADT reduced hardware configuration) and two writes into the BDA for the
start_eip segment and offset.

BDA 0x67 and 0x69 hail from the days of the DOS and the 286, when IBM put
together the fast way to return from Protected mode back to Real mode (via a
deliberate triple fault).  This vector, when set, redirects the early boot
logic back into OS control.

It is also used by early MP systems, before the Startup IPI message became
standard, which in practice was before Local APICs became integrated into CPU
cores.

Support for non-integrated APICs was dropped in c/s 7b0007af "xen/x86: Remove
APIC_INTEGRATED() checks" because there are no 64-bit capable systems without
them.  Therefore, drop smpboot_{setup,restore}_warm_reset_vector().

Dropping smpboot_setup_warm_reset_vector() also lets us drop
TRAMPOLINE_{HIGH,LOW}, which lets us drop mach_wakecpu.h entirely.  The final
function in smpboot_hooks.h is smpboot_setup_io_apic() and has a single
caller, so expand it inline and delete smpboot_hooks.h as well.

This removes all reliance on CMOS and the BDA from the AP boot path, which is
especially of interest on reduced_hardware boots and EFI systems.

Reported-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: David Woodhouse <dwmw2@infradead.org>

This has involved several months of on-and-off code archeology.  It turns out
that details of how the 486-era MP systems booted are feindishly difficult to
locate.
---
 xen/arch/x86/smpboot.c                           | 11 +-----
 xen/include/asm-x86/mach-default/mach_wakecpu.h  | 12 ------
 xen/include/asm-x86/mach-default/smpboot_hooks.h | 47 ------------------------
 3 files changed, 2 insertions(+), 68 deletions(-)
 delete mode 100644 xen/include/asm-x86/mach-default/mach_wakecpu.h
 delete mode 100644 xen/include/asm-x86/mach-default/smpboot_hooks.h

diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 274865a..730fe14 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -46,8 +46,6 @@
 #include <asm/time.h>
 #include <asm/tboot.h>
 #include <mach_apic.h>
-#include <mach_wakecpu.h>
-#include <smpboot_hooks.h>
 
 #define setup_trampoline()    (bootsym_phys(trampoline_realmode_entry))
 
@@ -565,10 +563,6 @@ static int do_boot_cpu(int apicid, int cpu)
 
     set_cpu_state(CPU_STATE_INIT);
 
-    Dprintk("Setting warm reset code and vector.\n");
-
-    smpboot_setup_warm_reset_vector(start_eip);
-
     /* Starting actual IPI sequence... */
     if ( !tboot_in_measured_env() || tboot_wake_ap(apicid, start_eip) )
         boot_error = wakeup_secondary_cpu(apicid, start_eip);
@@ -623,8 +617,6 @@ static int do_boot_cpu(int apicid, int cpu)
     bootsym(trampoline_cpu_started) = 0;
     smp_mb();
 
-    smpboot_restore_warm_reset_vector();
-
     return rc;
 }
 
@@ -1162,7 +1154,8 @@ void __init smp_prepare_cpus(void)
     connect_bsp_APIC();
     setup_local_APIC();
 
-    smpboot_setup_io_apic();
+    if ( !skip_ioapic_setup && nr_ioapics )
+        setup_IO_APIC();
 
     setup_boot_APIC_clock();
 }
diff --git a/xen/include/asm-x86/mach-default/mach_wakecpu.h b/xen/include/asm-x86/mach-default/mach_wakecpu.h
deleted file mode 100644
index 32555e1..0000000
--- a/xen/include/asm-x86/mach-default/mach_wakecpu.h
+++ /dev/null
@@ -1,12 +0,0 @@
-#ifndef __ASM_MACH_WAKECPU_H
-#define __ASM_MACH_WAKECPU_H
-
-/* 
- * This file copes with machines that wakeup secondary CPUs by the
- * INIT, INIT, STARTUP sequence.
- */
-
-#define TRAMPOLINE_LOW maddr_to_virt(0x467)
-#define TRAMPOLINE_HIGH maddr_to_virt(0x469)
-
-#endif /* __ASM_MACH_WAKECPU_H */
diff --git a/xen/include/asm-x86/mach-default/smpboot_hooks.h b/xen/include/asm-x86/mach-default/smpboot_hooks.h
deleted file mode 100644
index 14e1ee5..0000000
--- a/xen/include/asm-x86/mach-default/smpboot_hooks.h
+++ /dev/null
@@ -1,47 +0,0 @@
-/* two abstractions specific to kernel/smpboot.c, mainly to cater to visws
- * which needs to alter them. */
-
-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);
-	flush_tlb_local();
-	Dprintk("1.\n");
-	*((volatile unsigned short *) TRAMPOLINE_HIGH) = start_eip >> 4;
-	Dprintk("2.\n");
-	*((volatile unsigned short *) TRAMPOLINE_LOW) = start_eip & 0xf;
-	Dprintk("3.\n");
-}
-
-static inline void smpboot_restore_warm_reset_vector(void)
-{
-	unsigned long flags;
-
-	/*
-	 * Install writable page 0 entry to set BIOS data area.
-	 */
-	flush_tlb_local();
-
-	/*
-	 * 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 int *) maddr_to_virt(0x467)) = 0;
-}
-
-static inline void smpboot_setup_io_apic(void)
-{
-	/*
-	 * Here we can be sure that there is an IO-APIC in the system. Let's
-	 * go and set it up:
-	 */
-	if (!skip_ioapic_setup && nr_ioapics)
-		setup_IO_APIC();
-}
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

>>> On 12.06.19 at 13:05, <andrew.cooper3@citrix.com> wrote:
> The current code in do_boot_cpu() makes a CMOS write (even in the case of an
> FADT reduced hardware configuration) and two writes into the BDA for the
> start_eip segment and offset.
> 
> BDA 0x67 and 0x69 hail from the days of the DOS and the 286, when IBM put
> together the fast way to return from Protected mode back to Real mode (via a
> deliberate triple fault).  This vector, when set, redirects the early boot
> logic back into OS control.
> 
> It is also used by early MP systems, before the Startup IPI message became
> standard, which in practice was before Local APICs became integrated into CPU
> cores.
> 
> Support for non-integrated APICs was dropped in c/s 7b0007af "xen/x86: Remove
> APIC_INTEGRATED() checks" because there are no 64-bit capable systems without
> them.  Therefore, drop smpboot_{setup,restore}_warm_reset_vector().
> 
> Dropping smpboot_setup_warm_reset_vector() also lets us drop
> TRAMPOLINE_{HIGH,LOW}, which lets us drop mach_wakecpu.h entirely.  The final
> function in smpboot_hooks.h is smpboot_setup_io_apic() and has a single
> caller, so expand it inline and delete smpboot_hooks.h as well.
> 
> This removes all reliance on CMOS and the BDA from the AP boot path, which is
> especially of interest on reduced_hardware boots and EFI systems.
> 
> Reported-by: David Woodhouse <dwmw@amazon.co.uk>

Does this mean there was an actual problem resulting from this code
being there, or simply the observation that this code is all dead?
Clarifying one way or the other by half a sentence would be nice.

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 12/06/2019 12:51, Jan Beulich wrote:
>>>> On 12.06.19 at 13:05, <andrew.cooper3@citrix.com> wrote:
>> The current code in do_boot_cpu() makes a CMOS write (even in the case of an
>> FADT reduced hardware configuration) and two writes into the BDA for the
>> start_eip segment and offset.
>>
>> BDA 0x67 and 0x69 hail from the days of the DOS and the 286, when IBM put
>> together the fast way to return from Protected mode back to Real mode (via a
>> deliberate triple fault).  This vector, when set, redirects the early boot
>> logic back into OS control.
>>
>> It is also used by early MP systems, before the Startup IPI message became
>> standard, which in practice was before Local APICs became integrated into CPU
>> cores.
>>
>> Support for non-integrated APICs was dropped in c/s 7b0007af "xen/x86: Remove
>> APIC_INTEGRATED() checks" because there are no 64-bit capable systems without
>> them.  Therefore, drop smpboot_{setup,restore}_warm_reset_vector().
>>
>> Dropping smpboot_setup_warm_reset_vector() also lets us drop
>> TRAMPOLINE_{HIGH,LOW}, which lets us drop mach_wakecpu.h entirely.  The final
>> function in smpboot_hooks.h is smpboot_setup_io_apic() and has a single
>> caller, so expand it inline and delete smpboot_hooks.h as well.
>>
>> This removes all reliance on CMOS and the BDA from the AP boot path, which is
>> especially of interest on reduced_hardware boots and EFI systems.
>>
>> Reported-by: David Woodhouse <dwmw@amazon.co.uk>
> Does this mean there was an actual problem resulting from this code
> being there, or simply the observation that this code is all dead?
> Clarifying one way or the other by half a sentence would be nice.

It was more an observation that when you kexec Xen, it blindly writes
into the BDA even when that wasn't set up properly by the tools.

Arguably that may have been kexec setup problem more than a Xen problem,
but after looking at this code, it is obviously that what Xen was doing
definitely wasn't right to do unconditionally.  It just so happens that
it safe to unconditionally drop the logic, rather than to make it
dependant on other system properties.

I'm not sure how best to phrase this.

>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks,

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

>>> On 12.06.19 at 13:55, <andrew.cooper3@citrix.com> wrote:
> On 12/06/2019 12:51, Jan Beulich wrote:
>>>>> On 12.06.19 at 13:05, <andrew.cooper3@citrix.com> wrote:
>>> The current code in do_boot_cpu() makes a CMOS write (even in the case of an
>>> FADT reduced hardware configuration) and two writes into the BDA for the
>>> start_eip segment and offset.
>>>
>>> BDA 0x67 and 0x69 hail from the days of the DOS and the 286, when IBM put
>>> together the fast way to return from Protected mode back to Real mode (via a
>>> deliberate triple fault).  This vector, when set, redirects the early boot
>>> logic back into OS control.
>>>
>>> It is also used by early MP systems, before the Startup IPI message became
>>> standard, which in practice was before Local APICs became integrated into 
> CPU
>>> cores.
>>>
>>> Support for non-integrated APICs was dropped in c/s 7b0007af "xen/x86: 
> Remove
>>> APIC_INTEGRATED() checks" because there are no 64-bit capable systems 
> without
>>> them.  Therefore, drop smpboot_{setup,restore}_warm_reset_vector().
>>>
>>> Dropping smpboot_setup_warm_reset_vector() also lets us drop
>>> TRAMPOLINE_{HIGH,LOW}, which lets us drop mach_wakecpu.h entirely.  The 
> final
>>> function in smpboot_hooks.h is smpboot_setup_io_apic() and has a single
>>> caller, so expand it inline and delete smpboot_hooks.h as well.
>>>
>>> This removes all reliance on CMOS and the BDA from the AP boot path, which 
> is
>>> especially of interest on reduced_hardware boots and EFI systems.
>>>
>>> Reported-by: David Woodhouse <dwmw@amazon.co.uk>
>> Does this mean there was an actual problem resulting from this code
>> being there, or simply the observation that this code is all dead?
>> Clarifying one way or the other by half a sentence would be nice.
> 
> It was more an observation that when you kexec Xen, it blindly writes
> into the BDA even when that wasn't set up properly by the tools.
> 
> Arguably that may have been kexec setup problem more than a Xen problem,
> but after looking at this code, it is obviously that what Xen was doing
> definitely wasn't right to do unconditionally.  It just so happens that
> it safe to unconditionally drop the logic, rather than to make it
> dependant on other system properties.
> 
> I'm not sure how best to phrase this.

Maybe "In practice issues with this were observed only with kexec"?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On Wed, 2019-06-12 at 06:00 -0600, Jan Beulich wrote:
> > > > Reported-by: David Woodhouse <dwmw@amazon.co.uk>
> > > 
> > > Does this mean there was an actual problem resulting from this code
> > > being there, or simply the observation that this code is all dead?
> > > Clarifying one way or the other by half a sentence would be nice.
> > 
> > It was more an observation that when you kexec Xen, it blindly writes
> > into the BDA even when that wasn't set up properly by the tools.
> > 
> > Arguably that may have been kexec setup problem more than a Xen problem,
> > but after looking at this code, it is obviously that what Xen was doing
> > definitely wasn't right to do unconditionally.  It just so happens that
> > it safe to unconditionally drop the logic, rather than to make it
> > dependant on other system properties.
> > 
> > I'm not sure how best to phrase this.
> 
> Maybe "In practice issues with this were observed only with kexec"?

Not sure that's true either, is it? I found *lots* of issues when doing
kexec, and I should resend that series of boot code cleanups — but this
wasn't one of the ones I remember spotting :)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 12/06/2019 13:14, David Woodhouse wrote:
> On Wed, 2019-06-12 at 06:00 -0600, Jan Beulich wrote:
>>>>> Reported-by: David Woodhouse <dwmw@amazon.co.uk>
>>>> Does this mean there was an actual problem resulting from this code
>>>> being there, or simply the observation that this code is all dead?
>>>> Clarifying one way or the other by half a sentence would be nice.
>>> It was more an observation that when you kexec Xen, it blindly writes
>>> into the BDA even when that wasn't set up properly by the tools.
>>>
>>> Arguably that may have been kexec setup problem more than a Xen problem,
>>> but after looking at this code, it is obviously that what Xen was doing
>>> definitely wasn't right to do unconditionally.  It just so happens that
>>> it safe to unconditionally drop the logic, rather than to make it
>>> dependant on other system properties.
>>>
>>> I'm not sure how best to phrase this.
>> Maybe "In practice issues with this were observed only with kexec"?
> Not sure that's true either, is it? I found *lots* of issues when doing
> kexec, and I should resend that series of boot code cleanups — but this
> wasn't one of the ones I remember spotting :)

You definitely complained about the BDA on IRC, which is why I started
looking, but I'm happy to leave you out of the patch if you'd prefer.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On Wed, 2019-06-12 at 13:20 +0100, Andrew Cooper wrote:
> You definitely complained about the BDA on IRC, which is why I started
> looking, but I'm happy to leave you out of the patch if you'd prefer.

I did indeed complain about the BDA, but mostly when we were reading
the amount of memory available from the BDA... when that had been
overwritten by kexec loading data segments over it :)

I don't think I spotted this *particular* issue, so perhaps "Dave made
me start staring at this crap again" instead of literally "Reported by
Dave" is truer...
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 12/06/2019 13:23, David Woodhouse wrote:
> On Wed, 2019-06-12 at 13:20 +0100, Andrew Cooper wrote:
>> You definitely complained about the BDA on IRC, which is why I started
>> looking, but I'm happy to leave you out of the patch if you'd prefer.
> I did indeed complain about the BDA, but mostly when we were reading
> the amount of memory available from the BDA... when that had been
> overwritten by kexec loading data segments over it :)
>
> I don't think I spotted this *particular* issue, so perhaps "Dave made
> me start staring at this crap again" instead of literally "Reported by
> Dave" is truer...

Ok.  I'll figure out some suitable wording when I commit.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel