Re: [PATCH v3] x86: Prioritize low memory size from Multiboot

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.02.2022 09:44, 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.
> 
> Prioritize the information supplied by Multiboot instead. If this is not
> available, fall back to the old BDA method.

You're still failing to provide information on why this would be a safe
thing to do. In the absence of such, prior behavior has to be retained,
and only the special case you're after wants adjusting for. This is
first and foremost (but not limited to) you moving to ...

> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -524,27 +524,41 @@ 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
> +        /* Check if Multiboot provides us with low memory size. */
> +        mov     %edx,%ecx
> +        test    %ecx,%ecx
> +        jz      1f

... checking for just zero, when originally ...

> +        /*
> +         * Old Kexec used to report memory sizes in bytes instead of 
> kilobytes
> +         * like it's supposed to.
> +         *
> +         * If Multiboot reports more than 640 KB of low memory, assume we 
> have
> +         * this problem.
> +         */
> +        cmp     $640,%ecx
> +        jbe     0f
> +        shr     $10,%ecx
>  0:
> -        movzwl  0x413,%ecx          /* use base memory size on failure */
> +        /* %ecx now contains the low memory size in kilobytes. */
>          shl     $10-4,%ecx
> +        jmp     3f
> +
>  1:
>          /*
> -         * Compare the value in the BDA with the information from the
> -         * multiboot structure (if available) and use the smallest.
> +         * Multiboot doesn't provide us with memory info. Set up trampoline
> +         * segment 64k below EBDA as fallback.
>           */
> -        cmp     $0x100,%edx         /* is the multiboot value too small? */
> -        jb      2f                  /* if so, do not use it */

... the boundary was 0x100.

It was for this reason why in reply to v1 I did ask "Is the kexec case
recognizable by any means (including [...]), such that we could skip
using the BDA value in that case?" If it wasn't clear, I did mean
_just_ in this case.

> -        shl     $10-4,%edx
> -        cmp     %ecx,%edx           /* compare with BDA value */
> -        cmovb   %edx,%ecx           /* and use the smaller */
> -
> +        movzwl  0x40e,%ecx          /* EBDA segment */
> +        cmp     $0xa000,%ecx        /* sanity check (high) */
> +        jae     2f
> +        cmp     $0x4000,%ecx        /* sanity check (low) */
> +        jae     3f
>  2:
> +        movzwl  0x413,%ecx          /* use base memory size on failure */
> +        shl     $10-4,%ecx

I don't see why this shift can't be folded with the other one, by
moving it ...

> +
> +3:

... below here (and removing the one further up).

Jan

>          /* Reserve memory for the trampoline and the low-memory stack. */
>          sub     $((TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE)>>4),%ecx
>