Patchwork [1.3?] multiboot: fix e801 memory map

login
register
mail settings
Submitter Paolo Bonzini
Date Nov. 29, 2012, 5:11 p.m.
Message ID <1354209111-18110-1-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/202786/
State New
Headers show

Comments

Paolo Bonzini - Nov. 29, 2012, 5:11 p.m.
The e801 memory sizes in the multiboot structures hard-code the available
low memory to 640.  However, the value should not include the size of the
EBDA.  Fill the value in the option ROM, getting the size of low memory
from the BIOS.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        Alex, can you ack this patch for 1.3?

 pc-bios/multiboot.bin         | Bin 1024 -> 1024 bytes
 pc-bios/optionrom/multiboot.S |   7 +++++++
 2 files changed, 7 insertions(+)
Alexander Graf - Nov. 29, 2012, 6:49 p.m.
On 29.11.2012, at 18:11, Paolo Bonzini wrote:

> The e801 memory sizes in the multiboot structures hard-code the available
> low memory to 640.  However, the value should not include the size of the
> EBDA.  Fill the value in the option ROM, getting the size of low memory
> from the BIOS.
> 
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>        Alex, can you ack this patch for 1.3?

Do you have a test case (OS that fails for example)?


Alex

> 
> pc-bios/multiboot.bin         | Bin 1024 -> 1024 bytes
> pc-bios/optionrom/multiboot.S |   7 +++++++
> 2 files changed, 7 insertions(+)
> 
> diff --git a/pc-bios/multiboot.bin b/pc-bios/multiboot.bin
> index f74a6e142fddc054d7f40ab346a108532afac40f..7b3c1745a430ea5e0e15b9aa817d1cbbaa40db14 100644
> GIT binary patch
> delta 81
> zcmZqRXyBNj#q7o8KT+38L4+xd@o;KdryK*rZVrajPB|8aZaEhwAcKt|ty|7*VtW(k
> kS)sHUDQSyY7$$Qv{%2ueV91@!$z;eVv)P(y2P2~%0BkrF9smFU
> 
> delta 72
> zcmZqRXyBNj#capqJW<z5frBZH@o;KdryK*rZVrajPB|8aZaE7kAcKt|ty|7&VtdnM
> bA;$lVDU(H+3>gJByEE-zoLs=f!*~_|qXQE}
> 
> diff --git a/pc-bios/optionrom/multiboot.S b/pc-bios/optionrom/multiboot.S
> index f08222a..003bcfb 100644
> --- a/pc-bios/optionrom/multiboot.S
> +++ b/pc-bios/optionrom/multiboot.S
> @@ -75,6 +75,13 @@ run_multiboot:
> 	shr		$4, %eax
> 	mov		%ax, %fs
> 
> +	/* Account for the EBDA in the multiboot structure's e801
> +	 * map.
> +	 */
> +	int		$0x12
> +	cwtl
> +	movl		%eax, %fs:4
> +
> 	/* ES = mmap_addr */
> 	mov 		%fs:48, %eax
> 	shr		$4, %eax
> -- 
> 1.8.0
>
Alexander Graf - Nov. 29, 2012, 6:51 p.m.
On 29.11.2012, at 18:11, Paolo Bonzini wrote:

> The e801 memory sizes in the multiboot structures hard-code the available
> low memory to 640.  However, the value should not include the size of the
> EBDA.  Fill the value in the option ROM, getting the size of low memory
> from the BIOS.

The description mentions that instead of hard coding, you fetch the value dynamically via a BIOS call. However, I don't see the code that hard codes it changed. Where is that one?


Alex

> 
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>        Alex, can you ack this patch for 1.3?
> 
> pc-bios/multiboot.bin         | Bin 1024 -> 1024 bytes
> pc-bios/optionrom/multiboot.S |   7 +++++++
> 2 files changed, 7 insertions(+)
> 
> diff --git a/pc-bios/multiboot.bin b/pc-bios/multiboot.bin
> index f74a6e142fddc054d7f40ab346a108532afac40f..7b3c1745a430ea5e0e15b9aa817d1cbbaa40db14 100644
> GIT binary patch
> delta 81
> zcmZqRXyBNj#q7o8KT+38L4+xd@o;KdryK*rZVrajPB|8aZaEhwAcKt|ty|7*VtW(k
> kS)sHUDQSyY7$$Qv{%2ueV91@!$z;eVv)P(y2P2~%0BkrF9smFU
> 
> delta 72
> zcmZqRXyBNj#capqJW<z5frBZH@o;KdryK*rZVrajPB|8aZaE7kAcKt|ty|7&VtdnM
> bA;$lVDU(H+3>gJByEE-zoLs=f!*~_|qXQE}
> 
> diff --git a/pc-bios/optionrom/multiboot.S b/pc-bios/optionrom/multiboot.S
> index f08222a..003bcfb 100644
> --- a/pc-bios/optionrom/multiboot.S
> +++ b/pc-bios/optionrom/multiboot.S
> @@ -75,6 +75,13 @@ run_multiboot:
> 	shr		$4, %eax
> 	mov		%ax, %fs
> 
> +	/* Account for the EBDA in the multiboot structure's e801
> +	 * map.
> +	 */
> +	int		$0x12
> +	cwtl
> +	movl		%eax, %fs:4
> +
> 	/* ES = mmap_addr */
> 	mov 		%fs:48, %eax
> 	shr		$4, %eax
> -- 
> 1.8.0
>
Paolo Bonzini - Nov. 29, 2012, 7:21 p.m.
----- Messaggio originale -----
> Da: "Alexander Graf" <agraf@suse.de>
> A: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org
> Inviato: Giovedì, 29 novembre 2012 19:51:07
> Oggetto: Re: [PATCH 1.3?] multiboot: fix e801 memory map
> 
> 
> On 29.11.2012, at 18:11, Paolo Bonzini wrote:
> 
> > The e801 memory sizes in the multiboot structures hard-code the available
> > low memory to 640.  However, the value should not include the size
> > of the EBDA.  Fill the value in the option ROM, getting the size of low
> > memory from the BIOS.
> 
> The description mentions that instead of hard coding, you fetch the
> value dynamically via a BIOS call. However, I don't see the code
> that hard codes it changed. Where is that one?

It is in hw/multiboot.c:

    stl_p(bootinfo + MBI_MEM_LOWER,   640);


Regarding the testcase, Xen will touch the EBDA without this patch and
with http://permalink.gmane.org/gmane.comp.emulators.xen.devel/144855.
However, SeaBIOS does not complain.

Paolo
Alexander Graf - Nov. 29, 2012, 7:24 p.m.
On 29.11.2012, at 20:21, Paolo Bonzini wrote:

> 
> 
> ----- Messaggio originale -----
>> Da: "Alexander Graf" <agraf@suse.de>
>> A: "Paolo Bonzini" <pbonzini@redhat.com>
>> Cc: qemu-devel@nongnu.org
>> Inviato: Giovedì, 29 novembre 2012 19:51:07
>> Oggetto: Re: [PATCH 1.3?] multiboot: fix e801 memory map
>> 
>> 
>> On 29.11.2012, at 18:11, Paolo Bonzini wrote:
>> 
>>> The e801 memory sizes in the multiboot structures hard-code the available
>>> low memory to 640.  However, the value should not include the size
>>> of the EBDA.  Fill the value in the option ROM, getting the size of low
>>> memory from the BIOS.
>> 
>> The description mentions that instead of hard coding, you fetch the
>> value dynamically via a BIOS call. However, I don't see the code
>> that hard codes it changed. Where is that one?
> 
> It is in hw/multiboot.c:
> 
>    stl_p(bootinfo + MBI_MEM_LOWER,   640);

You want to remove that one then.

> Regarding the testcase, Xen will touch the EBDA without this patch and
> with http://permalink.gmane.org/gmane.comp.emulators.xen.devel/144855.
> However, SeaBIOS does not complain.

I see :). Well, the patch is unintrusive enough for be ok for 1.3 IMHO.


Alex
Paolo Bonzini - Nov. 29, 2012, 7:40 p.m.
Il 29/11/2012 20:24, Alexander Graf ha scritto:
>> > It is in hw/multiboot.c:
>> > 
>> >    stl_p(bootinfo + MBI_MEM_LOWER,   640);
> You want to remove that one then.

I wasn't sure of what happens if the multiboot option ROM is old.  Do we
support that to any extent?

> > Regarding the testcase, Xen will touch the EBDA without this patch and
> > with http://permalink.gmane.org/gmane.comp.emulators.xen.devel/144855.
> > However, SeaBIOS does not complain.
>
> I see :). Well, the patch is unintrusive enough for be ok for 1.3 IMHO.

Actually it's relatively easy to see a failure with Xen and a need to
PCI passthrough something with a bulky boot ROM; iSCSI or FC cards will
do.  You'll need both the Xen patch above, and this patch to boot
successfully.  Otherwise, it will fail to boot.

Paolo
Anthony Liguori - Nov. 29, 2012, 9:12 p.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 29/11/2012 20:24, Alexander Graf ha scritto:
>>> > It is in hw/multiboot.c:
>>> > 
>>> >    stl_p(bootinfo + MBI_MEM_LOWER,   640);
>> You want to remove that one then.
>
> I wasn't sure of what happens if the multiboot option ROM is old.  Do we
> support that to any extent?

Option ROMs are migrated but reset after reboot.  I guess you could
migrate before the option ROM is executed but that seems extraordinarily
unlikely.

Regards,

Anthony Liguori

>
>> > Regarding the testcase, Xen will touch the EBDA without this patch and
>> > with http://permalink.gmane.org/gmane.comp.emulators.xen.devel/144855.
>> > However, SeaBIOS does not complain.
>>
>> I see :). Well, the patch is unintrusive enough for be ok for 1.3 IMHO.
>
> Actually it's relatively easy to see a failure with Xen and a need to
> PCI passthrough something with a bulky boot ROM; iSCSI or FC cards will
> do.  You'll need both the Xen patch above, and this patch to boot
> successfully.  Otherwise, it will fail to boot.
>
> Paolo
Anthony Liguori - Nov. 30, 2012, 4:15 p.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

> The e801 memory sizes in the multiboot structures hard-code the available
> low memory to 640.  However, the value should not include the size of the
> EBDA.  Fill the value in the option ROM, getting the size of low memory
> from the BIOS.
>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>         Alex, can you ack this patch for 1.3?

Applied. Thanks.

Didn't see a clear ack from Alex but since this does fix a bug, I
figured we should carry it for 1.3.

Regards,

Anthony Liguori

>
>  pc-bios/multiboot.bin         | Bin 1024 -> 1024 bytes
>  pc-bios/optionrom/multiboot.S |   7 +++++++
>  2 files changed, 7 insertions(+)
>
> diff --git a/pc-bios/multiboot.bin b/pc-bios/multiboot.bin
> index f74a6e142fddc054d7f40ab346a108532afac40f..7b3c1745a430ea5e0e15b9aa817d1cbbaa40db14 100644
> GIT binary patch
> delta 81
> zcmZqRXyBNj#q7o8KT+38L4+xd@o;KdryK*rZVrajPB|8aZaEhwAcKt|ty|7*VtW(k
> kS)sHUDQSyY7$$Qv{%2ueV91@!$z;eVv)P(y2P2~%0BkrF9smFU
>
> delta 72
> zcmZqRXyBNj#capqJW<z5frBZH@o;KdryK*rZVrajPB|8aZaE7kAcKt|ty|7&VtdnM
> bA;$lVDU(H+3>gJByEE-zoLs=f!*~_|qXQE}
>
> diff --git a/pc-bios/optionrom/multiboot.S b/pc-bios/optionrom/multiboot.S
> index f08222a..003bcfb 100644
> --- a/pc-bios/optionrom/multiboot.S
> +++ b/pc-bios/optionrom/multiboot.S
> @@ -75,6 +75,13 @@ run_multiboot:
>  	shr		$4, %eax
>  	mov		%ax, %fs
>  
> +	/* Account for the EBDA in the multiboot structure's e801
> +	 * map.
> +	 */
> +	int		$0x12
> +	cwtl
> +	movl		%eax, %fs:4
> +
>  	/* ES = mmap_addr */
>  	mov 		%fs:48, %eax
>  	shr		$4, %eax
> -- 
> 1.8.0

Patch

diff --git a/pc-bios/multiboot.bin b/pc-bios/multiboot.bin
index f74a6e142fddc054d7f40ab346a108532afac40f..7b3c1745a430ea5e0e15b9aa817d1cbbaa40db14 100644
GIT binary patch
delta 81
zcmZqRXyBNj#q7o8KT+38L4+xd@o;KdryK*rZVrajPB|8aZaEhwAcKt|ty|7*VtW(k
kS)sHUDQSyY7$$Qv{%2ueV91@!$z;eVv)P(y2P2~%0BkrF9smFU

delta 72
zcmZqRXyBNj#capqJW<z5frBZH@o;KdryK*rZVrajPB|8aZaE7kAcKt|ty|7&VtdnM
bA;$lVDU(H+3>gJByEE-zoLs=f!*~_|qXQE}

diff --git a/pc-bios/optionrom/multiboot.S b/pc-bios/optionrom/multiboot.S
index f08222a..003bcfb 100644
--- a/pc-bios/optionrom/multiboot.S
+++ b/pc-bios/optionrom/multiboot.S
@@ -75,6 +75,13 @@  run_multiboot:
 	shr		$4, %eax
 	mov		%ax, %fs
 
+	/* Account for the EBDA in the multiboot structure's e801
+	 * map.
+	 */
+	int		$0x12
+	cwtl
+	movl		%eax, %fs:4
+
 	/* ES = mmap_addr */
 	mov 		%fs:48, %eax
 	shr		$4, %eax