[PATCH] x86/boot: Annotate the Real Mode entry points

[PATCH] x86/boot: Annotate the Real Mode entry points

[Andrew Cooper]

... because its already hard enough to follow.  Cross reference the locations
in C which set the entrypoints up, and state the alignment requirements and
entry conditions.

Drop a redundant .align 16, and panic() in do_boot_cpu() if the AP trampoline
isn't set up properly rather than blindly continuing an letting the APs
execute junk, or shifting part of the address into unrelated fields in ICR.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: David Woodhouse <dwmw2@infradead.org>
---
 xen/arch/x86/boot/trampoline.S |  6 ++++++
 xen/arch/x86/boot/wakeup.S     | 10 +++++++++-
 xen/arch/x86/smpboot.c         |  5 ++++-
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index 125bdb5..cac0f3e1 100644
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -38,6 +38,12 @@
 
         .code16
 
+/*
+ * do_boot_cpu() programs the Startup-IPI to point here.  Due to the SIPI
+ * format, the relocated entrypoint must be 4k aligned.
+ *
+ * It is entered in Real Mode, with %cs = wakeup_start >> 4 and %ip = 0.
+ */
 GLOBAL(trampoline_realmode_entry)
         mov     %cs,%ax
         mov     %ax,%ds
diff --git a/xen/arch/x86/boot/wakeup.S b/xen/arch/x86/boot/wakeup.S
index 89df261..e3cb9e0 100644
--- a/xen/arch/x86/boot/wakeup.S
+++ b/xen/arch/x86/boot/wakeup.S
@@ -2,7 +2,15 @@
 
 #define wakesym(sym) (sym - wakeup_start)
 
-        .align 16
+/*
+ * acpi_sleep_prepare() programs the S3 wakeup vector to point here.
+ *
+ * The ACPI spec says that we shall be entered in Real Mode with:
+ *   %cs = wakeup_start >> 4
+ *   %ip = wakeup_start & 0xf
+ *
+ * As wakeup_start is 16-byte aligned, %ip is 0 in practice.
+ */
 ENTRY(wakeup_start)
         cli
         cld
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index b7a0a4a..4f65c8d 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -548,9 +548,12 @@ static int do_boot_cpu(int apicid, int cpu)
 
     booting_cpu = cpu;
 
-    /* start_eip had better be page-aligned! */
     start_eip = setup_trampoline();
 
+    /* start_eip needs be page aligned, and below the 1M boundary. */
+    if ( start_eip & ~0xff000 )
+        panic("AP trampoline %#lx not suitably positioned\n", start_eip);
+
     /* So we see what's up   */
     if ( opt_cpu_info )
         printk("Booting processor %d/%d eip %lx\n",
-- 
2.1.4


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

[Xen-devel] [PATCH] x86/boot: Annotate the Real Mode entry points

[Andrew Cooper]

... because its already hard enough to follow.  Cross reference the locations
in C which set the entrypoints up, and state the alignment requirements and
entry conditions.

Drop a redundant .align 16, and panic() in do_boot_cpu() if the AP trampoline
isn't set up properly rather than blindly continuing an letting the APs
execute junk, or shifting part of the address into unrelated fields in ICR.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: David Woodhouse <dwmw2@infradead.org>
---
 xen/arch/x86/boot/trampoline.S |  6 ++++++
 xen/arch/x86/boot/wakeup.S     | 10 +++++++++-
 xen/arch/x86/smpboot.c         |  5 ++++-
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index 125bdb5..cac0f3e1 100644
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -38,6 +38,12 @@
 
         .code16
 
+/*
+ * do_boot_cpu() programs the Startup-IPI to point here.  Due to the SIPI
+ * format, the relocated entrypoint must be 4k aligned.
+ *
+ * It is entered in Real Mode, with %cs = wakeup_start >> 4 and %ip = 0.
+ */
 GLOBAL(trampoline_realmode_entry)
         mov     %cs,%ax
         mov     %ax,%ds
diff --git a/xen/arch/x86/boot/wakeup.S b/xen/arch/x86/boot/wakeup.S
index 89df261..e3cb9e0 100644
--- a/xen/arch/x86/boot/wakeup.S
+++ b/xen/arch/x86/boot/wakeup.S
@@ -2,7 +2,15 @@
 
 #define wakesym(sym) (sym - wakeup_start)
 
-        .align 16
+/*
+ * acpi_sleep_prepare() programs the S3 wakeup vector to point here.
+ *
+ * The ACPI spec says that we shall be entered in Real Mode with:
+ *   %cs = wakeup_start >> 4
+ *   %ip = wakeup_start & 0xf
+ *
+ * As wakeup_start is 16-byte aligned, %ip is 0 in practice.
+ */
 ENTRY(wakeup_start)
         cli
         cld
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index b7a0a4a..4f65c8d 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -548,9 +548,12 @@ static int do_boot_cpu(int apicid, int cpu)
 
     booting_cpu = cpu;
 
-    /* start_eip had better be page-aligned! */
     start_eip = setup_trampoline();
 
+    /* start_eip needs be page aligned, and below the 1M boundary. */
+    if ( start_eip & ~0xff000 )
+        panic("AP trampoline %#lx not suitably positioned\n", start_eip);
+
     /* So we see what's up   */
     if ( opt_cpu_info )
         printk("Booting processor %d/%d eip %lx\n",
-- 
2.1.4


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

Re: [PATCH] x86/boot: Annotate the Real Mode entry points

[Jan Beulich]

>>> On 02.05.19 at 15:27, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/boot/trampoline.S
> +++ b/xen/arch/x86/boot/trampoline.S
> @@ -38,6 +38,12 @@
>  
>          .code16
>  
> +/*
> + * do_boot_cpu() programs the Startup-IPI to point here.  Due to the SIPI
> + * format, the relocated entrypoint must be 4k aligned.
> + *
> + * It is entered in Real Mode, with %cs = wakeup_start >> 4 and %ip = 0.
> + */
>  GLOBAL(trampoline_realmode_entry)

The reference to wakeup_start looks to be a copy-and-paste
(or alike) mistake here.

> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -548,9 +548,12 @@ static int do_boot_cpu(int apicid, int cpu)
>  
>      booting_cpu = cpu;
>  
> -    /* start_eip had better be page-aligned! */
>      start_eip = setup_trampoline();
>  
> +    /* start_eip needs be page aligned, and below the 1M boundary. */
> +    if ( start_eip & ~0xff000 )
> +        panic("AP trampoline %#lx not suitably positioned\n", start_eip);

Seeing what setup_trampoline() really does, I'm not
convinced a panic() is of much value. The page-alignment
should be possible to check at build time, and the below-1M
requirement should be guaranteed by us allocating low
memory space in the first place. Nevertheless I won't insist,
so with the earlier comment corrected
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

Re: [Xen-devel] [PATCH] x86/boot: Annotate the Real Mode entry points

[Jan Beulich]

>>> On 02.05.19 at 15:27, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/boot/trampoline.S
> +++ b/xen/arch/x86/boot/trampoline.S
> @@ -38,6 +38,12 @@
>  
>          .code16
>  
> +/*
> + * do_boot_cpu() programs the Startup-IPI to point here.  Due to the SIPI
> + * format, the relocated entrypoint must be 4k aligned.
> + *
> + * It is entered in Real Mode, with %cs = wakeup_start >> 4 and %ip = 0.
> + */
>  GLOBAL(trampoline_realmode_entry)

The reference to wakeup_start looks to be a copy-and-paste
(or alike) mistake here.

> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -548,9 +548,12 @@ static int do_boot_cpu(int apicid, int cpu)
>  
>      booting_cpu = cpu;
>  
> -    /* start_eip had better be page-aligned! */
>      start_eip = setup_trampoline();
>  
> +    /* start_eip needs be page aligned, and below the 1M boundary. */
> +    if ( start_eip & ~0xff000 )
> +        panic("AP trampoline %#lx not suitably positioned\n", start_eip);

Seeing what setup_trampoline() really does, I'm not
convinced a panic() is of much value. The page-alignment
should be possible to check at build time, and the below-1M
requirement should be guaranteed by us allocating low
memory space in the first place. Nevertheless I won't insist,
so with the earlier comment corrected
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

Re: [PATCH] x86/boot: Annotate the Real Mode entry points

[Andrew Cooper]

On 02/05/2019 14:50, Jan Beulich wrote:
>>>> On 02.05.19 at 15:27, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/boot/trampoline.S
>> +++ b/xen/arch/x86/boot/trampoline.S
>> @@ -38,6 +38,12 @@
>>  
>>          .code16
>>  
>> +/*
>> + * do_boot_cpu() programs the Startup-IPI to point here.  Due to the SIPI
>> + * format, the relocated entrypoint must be 4k aligned.
>> + *
>> + * It is entered in Real Mode, with %cs = wakeup_start >> 4 and %ip = 0.
>> + */
>>  GLOBAL(trampoline_realmode_entry)
> The reference to wakeup_start looks to be a copy-and-paste
> (or alike) mistake here.

Oops, indeed.  Fixed.

>
>> --- a/xen/arch/x86/smpboot.c
>> +++ b/xen/arch/x86/smpboot.c
>> @@ -548,9 +548,12 @@ static int do_boot_cpu(int apicid, int cpu)
>>  
>>      booting_cpu = cpu;
>>  
>> -    /* start_eip had better be page-aligned! */
>>      start_eip = setup_trampoline();
>>  
>> +    /* start_eip needs be page aligned, and below the 1M boundary. */
>> +    if ( start_eip & ~0xff000 )
>> +        panic("AP trampoline %#lx not suitably positioned\n", start_eip);
> Seeing what setup_trampoline() really does, I'm not
> convinced a panic() is of much value. The page-alignment
> should be possible to check at build time, and the below-1M
> requirement should be guaranteed by us allocating low
> memory space in the first place.

Sadly it cant.

We have a number of alignment issues (well - confusions at least), most
obviously that trampoline_{start,end} in the linked Xen image has no
particular alignment, but the relocated trampoline_start has a 4k
requirement as a consequence of its alias with trampoline_realmode_entry.

All it takes is one error in the 32bit asm which relocates the
trampoline and this ends up exploding in a way which can only be
detected at runtime.

>  Nevertheless I won't insist,
> so with the earlier comment corrected
> 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

Re: [Xen-devel] [PATCH] x86/boot: Annotate the Real Mode entry points

[Andrew Cooper]

On 02/05/2019 14:50, Jan Beulich wrote:
>>>> On 02.05.19 at 15:27, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/boot/trampoline.S
>> +++ b/xen/arch/x86/boot/trampoline.S
>> @@ -38,6 +38,12 @@
>>  
>>          .code16
>>  
>> +/*
>> + * do_boot_cpu() programs the Startup-IPI to point here.  Due to the SIPI
>> + * format, the relocated entrypoint must be 4k aligned.
>> + *
>> + * It is entered in Real Mode, with %cs = wakeup_start >> 4 and %ip = 0.
>> + */
>>  GLOBAL(trampoline_realmode_entry)
> The reference to wakeup_start looks to be a copy-and-paste
> (or alike) mistake here.

Oops, indeed.  Fixed.

>
>> --- a/xen/arch/x86/smpboot.c
>> +++ b/xen/arch/x86/smpboot.c
>> @@ -548,9 +548,12 @@ static int do_boot_cpu(int apicid, int cpu)
>>  
>>      booting_cpu = cpu;
>>  
>> -    /* start_eip had better be page-aligned! */
>>      start_eip = setup_trampoline();
>>  
>> +    /* start_eip needs be page aligned, and below the 1M boundary. */
>> +    if ( start_eip & ~0xff000 )
>> +        panic("AP trampoline %#lx not suitably positioned\n", start_eip);
> Seeing what setup_trampoline() really does, I'm not
> convinced a panic() is of much value. The page-alignment
> should be possible to check at build time, and the below-1M
> requirement should be guaranteed by us allocating low
> memory space in the first place.

Sadly it cant.

We have a number of alignment issues (well - confusions at least), most
obviously that trampoline_{start,end} in the linked Xen image has no
particular alignment, but the relocated trampoline_start has a 4k
requirement as a consequence of its alias with trampoline_realmode_entry.

All it takes is one error in the 32bit asm which relocates the
trampoline and this ends up exploding in a way which can only be
detected at runtime.

>  Nevertheless I won't insist,
> so with the earlier comment corrected
> 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

Re: [PATCH] x86/boot: Annotate the Real Mode entry points

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 754 bytes --]

On Thu, 2019-05-02 at 15:08 +0100, Andrew Cooper wrote:
> Sadly it cant.
> 
> We have a number of alignment issues (well - confusions at least), most
> obviously that trampoline_{start,end} in the linked Xen image has no
> particular alignment, but the relocated trampoline_start has a 4k
> requirement as a consequence of its alias with trampoline_realmode_entry.
> 
> All it takes is one error in the 32bit asm which relocates the
> trampoline and this ends up exploding in a way which can only be
> detected at runtime.

I'm relocating the permanent trampoline from 64-bit C code now, not in
assembly. In the no-real-mode case (ignoring reloc(), qv) nothing is
put into low memory until __start_xen() calls relocate_trampoline().




[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [Xen-devel] [PATCH] x86/boot: Annotate the Real Mode entry points

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 754 bytes --]

On Thu, 2019-05-02 at 15:08 +0100, Andrew Cooper wrote:
> Sadly it cant.
> 
> We have a number of alignment issues (well - confusions at least), most
> obviously that trampoline_{start,end} in the linked Xen image has no
> particular alignment, but the relocated trampoline_start has a 4k
> requirement as a consequence of its alias with trampoline_realmode_entry.
> 
> All it takes is one error in the 32bit asm which relocates the
> trampoline and this ends up exploding in a way which can only be
> detected at runtime.

I'm relocating the permanent trampoline from 64-bit C code now, not in
assembly. In the no-real-mode case (ignoring reloc(), qv) nothing is
put into low memory until __start_xen() calls relocate_trampoline().




[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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