[PATCH] x86: Use low memory size directly from Multiboot

[PATCH] x86: Use low memory size directly from Multiboot

[dinhngoc.tu]

Previously, Xen used information from the BDA to detect the amount of
available low memory. This does not work on some scenarios such as
Coreboot, or when booting from Kexec on a UEFI system without CSM.

Use the information directly supplied by Multiboot boot information
instead.
---
 xen/arch/x86/boot/head.S | 30 ++++++------------------------
 1 file changed, 6 insertions(+), 24 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index dd1bea0d10..31a0663e6d 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -524,33 +524,15 @@ trampoline_bios_setup:
         mov     %ecx,%fs
         mov     %ecx,%gs
 
-        /* Set up trampoline segment 64k below EBDA */
-        movzwl  0x40e,%ecx          /* EBDA segment */
-        cmp     $0xa000,%ecx        /* sanity check (high) */
-        jae     0f
-        cmp     $0x4000,%ecx        /* sanity check (low) */
-        jae     1f
-0:
-        movzwl  0x413,%ecx          /* use base memory size on failure */
-        shl     $10-4,%ecx
-1:
-        /*
-         * Compare the value in the BDA with the information from the
-         * multiboot structure (if available) and use the smallest.
-         */
-        cmp     $0x100,%edx         /* is the multiboot value too small? */
-        jb      2f                  /* if so, do not use it */
-        shl     $10-4,%edx
-        cmp     %ecx,%edx           /* compare with BDA value */
-        cmovb   %edx,%ecx           /* and use the smaller */
+        /* Use lower memory size directly from Multiboot */
+        mov     %edx,%ecx
+        /* From arch/x86/smpboot.c: start_eip had better be page-aligned!
*/
+        shr     $2,%ecx
 
-2:
         /* Reserve memory for the trampoline and the low-memory stack. */
-        sub     $((TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE)>>4),%ecx
+        sub     $((TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE)>>12),%ecx
 
-        /* From arch/x86/smpboot.c: start_eip had better be page-aligned!
*/
-        xor     %cl, %cl
-        shl     $4, %ecx
+        shl     $12,%ecx
         mov     %ecx,sym_esi(trampoline_phys)
 
 trampoline_setup:
-- 
2.25.1

Re: [PATCH] x86: Use low memory size directly from Multiboot

[Jan Beulich]

On 09.02.2022 12:00, dinhngoc.tu@irit.fr wrote:
> Previously, Xen used information from the BDA to detect the amount of
> available low memory. This does not work on some scenarios such as
> Coreboot, or when booting from Kexec on a UEFI system without CSM.
> 
> Use the information directly supplied by Multiboot boot information
> instead.
> ---
>  xen/arch/x86/boot/head.S | 30 ++++++------------------------
>  1 file changed, 6 insertions(+), 24 deletions(-)
> 
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index dd1bea0d10..31a0663e6d 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -524,33 +524,15 @@ trampoline_bios_setup:
>          mov     %ecx,%fs
>          mov     %ecx,%gs
>  
> -        /* Set up trampoline segment 64k below EBDA */
> -        movzwl  0x40e,%ecx          /* EBDA segment */
> -        cmp     $0xa000,%ecx        /* sanity check (high) */
> -        jae     0f
> -        cmp     $0x4000,%ecx        /* sanity check (low) */
> -        jae     1f
> -0:
> -        movzwl  0x413,%ecx          /* use base memory size on failure */
> -        shl     $10-4,%ecx
> -1:
> -        /*
> -         * Compare the value in the BDA with the information from the
> -         * multiboot structure (if available) and use the smallest.
> -         */
> -        cmp     $0x100,%edx         /* is the multiboot value too small? */
> -        jb      2f                  /* if so, do not use it */

The comment here is a pretty clear indication that bad values may have
been observed, even if this was only in the distant past. But we have
to not regress even on very old boot loaders.

Is the kexec case recognizable by any means (including to distinguish
kexec properly communicating the value vs it not doing so, as iirc it
was said on irc that this didn't always work correctly there), such
that we could skip using the BDA value in that case?

Jan

> -        shl     $10-4,%edx
> -        cmp     %ecx,%edx           /* compare with BDA value */
> -        cmovb   %edx,%ecx           /* and use the smaller */
> +        /* Use lower memory size directly from Multiboot */
> +        mov     %edx,%ecx
> +        /* From arch/x86/smpboot.c: start_eip had better be page-aligned!
> */
> +        shr     $2,%ecx
>  
> -2:
>          /* Reserve memory for the trampoline and the low-memory stack. */
> -        sub     $((TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE)>>4),%ecx
> +        sub     $((TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE)>>12),%ecx
>  
> -        /* From arch/x86/smpboot.c: start_eip had better be page-aligned!
> */
> -        xor     %cl, %cl
> -        shl     $4, %ecx
> +        shl     $12,%ecx
>          mov     %ecx,sym_esi(trampoline_phys)
>  
>  trampoline_setup:

RE: [PATCH] x86: Use low memory size directly from Multiboot

[dinhngoc.tu]

> The comment here is a pretty clear indication that bad values may have been
> observed, even if this was only in the distant past. But we have to not
> regress even on very old boot loaders.
> 
> Is the kexec case recognizable by any means (including to distinguish kexec
> properly communicating the value vs it not doing so, as iirc it was said on irc
> that this didn't always work correctly there), such that we could skip using
> the BDA value in that case?

Old Kexec used to report the value in bytes instead of kilobytes like it's supposed to. I can make a special case for that so that we won't have to rely on the BDA.

[PATCH v2] x86: Use low memory size directly from Multiboot

[Tu Dinh Ngoc]

Previously, Xen used information from the BDA to detect the amount of
available low memory. This does not work on some scenarios such as
Coreboot, or when booting from Kexec on a UEFI system without CSM.

Use the information directly supplied by Multiboot boot information
instead.
---
 xen/arch/x86/boot/head.S | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index dd1bea0d10..62fe3fe55b 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -524,33 +524,23 @@ trampoline_bios_setup:
         mov     %ecx,%fs
         mov     %ecx,%gs
 
-        /* Set up trampoline segment 64k below EBDA */
-        movzwl  0x40e,%ecx          /* EBDA segment */
-        cmp     $0xa000,%ecx        /* sanity check (high) */
-        jae     0f
-        cmp     $0x4000,%ecx        /* sanity check (low) */
-        jae     1f
-0:
-        movzwl  0x413,%ecx          /* use base memory size on failure */
-        shl     $10-4,%ecx
-1:
+        /* Use lower memory size directly from Multiboot */
+        mov     %edx,%ecx
         /*
-         * Compare the value in the BDA with the information from the
-         * multiboot structure (if available) and use the smallest.
+         * Old Kexec used to report the value in bytes instead of kilobytes
+         * like it's supposed to, so fix that if detected.
          */
-        cmp     $0x100,%edx         /* is the multiboot value too small? */
-        jb      2f                  /* if so, do not use it */
-        shl     $10-4,%edx
-        cmp     %ecx,%edx           /* compare with BDA value */
-        cmovb   %edx,%ecx           /* and use the smaller */
+        cmpl    $640,%ecx
+        jbe     1f
+        shr     $10,%ecx
+1:
+        /* From arch/x86/smpboot.c: start_eip had better be page-aligned! */
+        shr     $2,%ecx
 
-2:
         /* Reserve memory for the trampoline and the low-memory stack. */
-        sub     $((TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE)>>4),%ecx
+        sub     $((TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE)>>12),%ecx
 
-        /* From arch/x86/smpboot.c: start_eip had better be page-aligned! */
-        xor     %cl, %cl
-        shl     $4, %ecx
+        shl     $12,%ecx
         mov     %ecx,sym_esi(trampoline_phys)
 
 trampoline_setup:
-- 
2.25.1

Re: [PATCH v2] x86: Use low memory size directly from Multiboot

[Jan Beulich]

On 09.02.2022 14:12, Tu Dinh Ngoc wrote:
> Previously, Xen used information from the BDA to detect the amount of
> available low memory. This does not work on some scenarios such as
> Coreboot, or when booting from Kexec on a UEFI system without CSM.
> 
> Use the information directly supplied by Multiboot boot information
> instead.
> ---

Btw - please summarize here briefly what has changed from the earlier
version. As it stands your adjustment looks to take care of one third
of what I did say in reply to your v1. That's not enough for a v2, or
else you should have taken care of the remaining aspects verbally.

Jan

> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -524,33 +524,23 @@ trampoline_bios_setup:
>          mov     %ecx,%fs
>          mov     %ecx,%gs
>  
> -        /* Set up trampoline segment 64k below EBDA */
> -        movzwl  0x40e,%ecx          /* EBDA segment */
> -        cmp     $0xa000,%ecx        /* sanity check (high) */
> -        jae     0f
> -        cmp     $0x4000,%ecx        /* sanity check (low) */
> -        jae     1f
> -0:
> -        movzwl  0x413,%ecx          /* use base memory size on failure */
> -        shl     $10-4,%ecx
> -1:
> +        /* Use lower memory size directly from Multiboot */
> +        mov     %edx,%ecx
>          /*
> -         * Compare the value in the BDA with the information from the
> -         * multiboot structure (if available) and use the smallest.
> +         * Old Kexec used to report the value in bytes instead of kilobytes
> +         * like it's supposed to, so fix that if detected.
>           */
> -        cmp     $0x100,%edx         /* is the multiboot value too small? */
> -        jb      2f                  /* if so, do not use it */
> -        shl     $10-4,%edx
> -        cmp     %ecx,%edx           /* compare with BDA value */
> -        cmovb   %edx,%ecx           /* and use the smaller */
> +        cmpl    $640,%ecx
> +        jbe     1f
> +        shr     $10,%ecx
> +1:
> +        /* From arch/x86/smpboot.c: start_eip had better be page-aligned! */
> +        shr     $2,%ecx
>  
> -2:
>          /* Reserve memory for the trampoline and the low-memory stack. */
> -        sub     $((TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE)>>4),%ecx
> +        sub     $((TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE)>>12),%ecx
>  
> -        /* From arch/x86/smpboot.c: start_eip had better be page-aligned! */
> -        xor     %cl, %cl
> -        shl     $4, %ecx
> +        shl     $12,%ecx
>          mov     %ecx,sym_esi(trampoline_phys)
>  
>  trampoline_setup:

RE: [PATCH v2] x86: Use low memory size directly from Multiboot

[dinhngoc.tu]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, 9 February 2022 15:26
> To: Tu Dinh Ngoc <dinhngoc.tu@irit.fr>
> Cc: xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v2] x86: Use low memory size directly from Multiboot
> 
> On 09.02.2022 14:12, Tu Dinh Ngoc wrote:
> > Previously, Xen used information from the BDA to detect the amount of
> > available low memory. This does not work on some scenarios such as
> > Coreboot, or when booting from Kexec on a UEFI system without CSM.
> >
> > Use the information directly supplied by Multiboot boot information
> > instead.
> > ---
> 
> Btw - please summarize here briefly what has changed from the earlier
> version. As it stands your adjustment looks to take care of one third of what I
> did say in reply to your v1. That's not enough for a v2, or else you should have
> taken care of the remaining aspects verbally.
> 
> Jan

Hi,

> The comment here is a pretty clear indication that bad values may have been observed, even if this was only in the distant past. But we have to not regress even on very old boot loaders.

> Is the kexec case recognizable by any means (including to distinguish kexec properly communicating the value vs it not doing so, as iirc it was said on irc that this didn't always work correctly there), such that we could skip using the BDA value in that case?

As written in the comments, old versions of kexec (before 2.0.23) presented the amount of lower and upper memory in the BASIC_MEMINFO Multiboot2 tag in bytes instead of kilobytes. The v2 patch tries to detect this condition by checking if there's more than 640 KB of low memory and corrects the low memory size in that case.

This change should only affect the particular case of booting with Multiboot2 without EFI (e.g. legacy BIOS or Kexec). Other cases like Multiboot 0.x, EFI booting (with or without MB2), or bootloaders that generate the BASIC_MEMINFO tag correctly shouldn't be affected.

Re: [PATCH v2] x86: Use low memory size directly from Multiboot

[Jan Beulich]

On 09.02.2022 16:20, dinhngoc.tu@irit.fr wrote:
> This change should only affect the particular case of booting with Multiboot2 without EFI (e.g. legacy BIOS or Kexec). Other cases like Multiboot 0.x, EFI booting (with or without MB2), or bootloaders that generate the BASIC_MEMINFO tag correctly shouldn't be affected.

How that? You're taking out the reading of the BDA value altogether,
aren't you? This is certainly a change affecting other environments
as well.

Jan

RE: [PATCH v2] x86: Use low memory size directly from Multiboot

[dinhngoc.tu]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, 9 February 2022 16:23
> To: dinhngoc.tu@irit.fr
> Cc: xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v2] x86: Use low memory size directly from Multiboot
> 
> On 09.02.2022 16:20, dinhngoc.tu@irit.fr wrote:
> > This change should only affect the particular case of booting with
> Multiboot2 without EFI (e.g. legacy BIOS or Kexec). Other cases like
> Multiboot 0.x, EFI booting (with or without MB2), or bootloaders that
> generate the BASIC_MEMINFO tag correctly shouldn't be affected.
> 
> How that? You're taking out the reading of the BDA value altogether, aren't
> you? This is certainly a change affecting other environments as well.
> 
> Jan

I missed that the BDA was used when booting on Multiboot 0.x as well. I can add a fallback to use the BDA when the bootloader doesn't provide low memory size in the MBI.

Re: [PATCH v2] x86: Use low memory size directly from Multiboot

[Roger Pau Monné]

On Wed, Feb 09, 2022 at 02:12:51PM +0100, Tu Dinh Ngoc wrote:
> Previously, Xen used information from the BDA to detect the amount of
> available low memory. This does not work on some scenarios such as
> Coreboot, or when booting from Kexec on a UEFI system without CSM.
> 
> Use the information directly supplied by Multiboot boot information
> instead.

This is missing a SoB, see:

https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Signed-off-by

Thanks, Roger.