| Message ID | 20220209131251.387-1-dinhngoc.tu@irit.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] x86: Use low memory size directly from Multiboot | expand |
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:
> -----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.
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
> -----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.
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.
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: