[Qemu-devel] [PATCH 0/3] multiboot: Fix memory information

[Qemu-devel] [PATCH 0/3] multiboot: Fix memory information

[Kevin Wolf]

Kevin Wolf (3):
  multiboot: Don't forget last mmap entry
  multiboot: Calculate upper_mem in the ROM
  multiboot: Updated ROM binary

 hw/i386/multiboot.c           |    2 -
 pc-bios/multiboot.bin         |  Bin 1024 -> 1024 bytes
 pc-bios/optionrom/multiboot.S |   75 +++++++++++++++++++++++++++++++----------
 3 files changed, 57 insertions(+), 20 deletions(-)
 mode change 100644 => 100755 pc-bios/multiboot.bin

-- 
1.7.7

[Qemu-devel] [PATCH 1/3] multiboot: Don't forget last mmap entry

[Kevin Wolf]

When the BIOS returns ebx = 0, the current entry is still valid and
needs to be included in the Multiboot memory map.

Fixing this meant that using bx as the entry index doesn't work any
more because it's 0 on the last entry (and it was SeaBIOS-specific
anyway), so the whole loop had to change a bit and should be more
generic as a result (ebx can be an arbitrary continuation number now,
and the entry size returned by the BIOS is used instead of hard-coding
20 bytes).

Signed-off-by: Kevin Wolf <mail@kevin-wolf.de>
---
 pc-bios/optionrom/multiboot.S |   35 +++++++++++++++++------------------
 1 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/pc-bios/optionrom/multiboot.S b/pc-bios/optionrom/multiboot.S
index 003bcfb..a0f3602 100644
--- a/pc-bios/optionrom/multiboot.S
+++ b/pc-bios/optionrom/multiboot.S
@@ -89,17 +89,14 @@ run_multiboot:
 
 	/* Initialize multiboot mmap structs using int 0x15(e820) */
 	xor		%ebx, %ebx
-	/* mmap start after first size */
-	movl		$4, %edi
+	/* Start storing mmap data at %es:0 */
+	xor		%edi, %edi
 
 mmap_loop:
+	/* The multiboot entry size has offset -4, so leave some space */
+	add		$4, %di
 	/* entry size (mmap struct) & max buffer size (int15) */
 	movl		$20, %ecx
-	/* store entry size */
-	/* old as(1) doesn't like this insn so emit the bytes instead:
-	movl		%ecx, %es:-4(%edi)
-	*/
-	.dc.b		0x26,0x67,0x66,0x89,0x4f,0xfc
 	/* e820 */
 	movl		$0x0000e820, %eax
 	/* 'SMAP' magic */
@@ -107,21 +104,23 @@ mmap_loop:
 	int		$0x15
 
 mmap_check_entry:
-	/* last entry? then we're done */
+	/* Error or last entry already done? */
 	jb		mmap_done
-	and		%bx, %bx
-	jz		mmap_done
-	/* valid entry, so let's loop on */
 
 mmap_store_entry:
-	/* %ax = entry_number * 24 */
-	mov		$24, %ax
-	mul		%bx
-	mov		%ax, %di
+	/* store entry size */
+	/* old as(1) doesn't like this insn so emit the bytes instead:
+	movl		%ecx, %es:-4(%edi)
+	*/
+	.dc.b		0x26,0x67,0x66,0x89,0x4f,0xfc
+
+	/* %edi += entry_size, store as mbs_mmap_length */
+	add		%ecx, %edi
 	movw		%di, %fs:0x2c
-	/* %di = 4 + (entry_number * 24) */
-	add		$4, %di
-	jmp		mmap_loop
+
+	/* Continuation value 0 means last entry */
+	test		%ebx, %ebx
+	jnz		mmap_loop
 
 mmap_done:
 real_to_prot:
-- 
1.7.7

[Qemu-devel] [PATCH 2/3] multiboot: Calculate upper_mem in the ROM

[Kevin Wolf]

The upper_mem field of the Multiboot information struct doesn't really
contain the RAM size - 1 MB like we used to calculate it, but only the
memory from 1 MB up to the first (upper) memory hole.

In order to correctly retrieve this information, the multiboot ROM now
looks at the mmap it creates anyway and tries to find the size of
contiguous usable memory from 1 MB.

Drop the multiboot.c definition of lower_mem and upper_mem because both
are queried at runtime now.

Signed-off-by: Kevin Wolf <mail@kevin-wolf.de>
---
 hw/i386/multiboot.c           |    2 --
 pc-bios/optionrom/multiboot.S |   40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index 09211e0..985ca1e 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -315,8 +315,6 @@ int load_multiboot(FWCfgState *fw_cfg,
                                 | MULTIBOOT_FLAGS_CMDLINE
                                 | MULTIBOOT_FLAGS_MODULES
                                 | MULTIBOOT_FLAGS_MMAP);
-    stl_p(bootinfo + MBI_MEM_LOWER,   640);
-    stl_p(bootinfo + MBI_MEM_UPPER,   (ram_size / 1024) - 1024);
     stl_p(bootinfo + MBI_BOOT_DEVICE, 0x8000ffff); /* XXX: use the -boot switch? */
     stl_p(bootinfo + MBI_MMAP_ADDR,   ADDR_E820_MAP);
 
diff --git a/pc-bios/optionrom/multiboot.S b/pc-bios/optionrom/multiboot.S
index a0f3602..b7efe4d 100644
--- a/pc-bios/optionrom/multiboot.S
+++ b/pc-bios/optionrom/multiboot.S
@@ -123,6 +123,46 @@ mmap_store_entry:
 	jnz		mmap_loop
 
 mmap_done:
+	/* Calculate upper_mem field: The amount of memory between 1 MB and
+	   the first upper memory hole. Get it from the mmap. */
+	xor		%di, %di
+	mov		$0x100000, %edx
+upper_mem_entry:
+	cmp		%fs:0x2c, %di
+	je		upper_mem_done
+	add		$4, %di
+
+	/* Skip if type != 1 */
+	cmpl		$1, %es:16(%di)
+	jne		upper_mem_next
+
+	/* Skip if > 4 GB */
+	movl		%es:4(%di), %eax
+	test		%eax, %eax
+	jnz		upper_mem_next
+
+	/* Check for contiguous extension (base <= %edx < base + length) */
+	movl		%es:(%di), %eax
+	cmp		%eax, %edx
+	jb		upper_mem_next
+	addl		%es:8(%di), %eax
+	cmp		%eax, %edx
+	jae		upper_mem_next
+
+	/* If so, update %edx, and restart the search (mmap isn't ordered) */
+	mov		%eax, %edx
+	xor		%di, %di
+	jmp		upper_mem_entry
+
+upper_mem_next:
+	addl		%es:-4(%di), %edi
+	jmp		upper_mem_entry
+
+upper_mem_done:
+	sub		$0x100000, %edx
+	shr		$10, %edx
+	mov		%edx, %fs:0x8
+
 real_to_prot:
 	/* Load the GDT before going into protected mode */
 lgdt:
-- 
1.7.7

[Qemu-devel] [PATCH 3/3] multiboot: Updated ROM binary

[Kevin Wolf]

Signed-off-by: Kevin Wolf <mail@kevin-wolf.de>
---
 pc-bios/multiboot.bin |  Bin 1024 -> 1024 bytes
 1 files changed, 0 insertions(+), 0 deletions(-)
 mode change 100644 => 100755 pc-bios/multiboot.bin

diff --git a/pc-bios/multiboot.bin b/pc-bios/multiboot.bin
old mode 100644
new mode 100755
index 7b3c1745a430ea5e0e15b9aa817d1cbbaa40db14..e772713c95749bee82c20002b50ec6d05b2d4987
GIT binary patch
delta 202
zcmZqRXyBNj#XOB^_C#GPg#@NF#>1&;opKBeyEzzAJLOmyy5%M?0U2xzY29*@CbrM2
zH~imxoF#3i2m=E{+75*mKyp`rqi^t8(IR2B^t4X@KWU8TQ#$Q*7}8pAmtHaapSFvE
zL4YB}8Yog?2GpvS)?6#VSgN9y*6qrY)_R~+3dmwjvpiHJsFud;$^m2*v!`_)GW`Gg
w6i}e{&+Ef!jjupvq#b<4mBO0VDaOGtIh=7f<L=3#OoogZo86gm7#aTn0N{Q_tpET3

delta 129
zcmZqRXyBNj#q7o8KT+38L4+xd@o;KdryK*rZVrajPB|8aZaEhwAcKt|ty|7*V*4z!
z{VWU&3~4(>fTUV_TBrY?v>ggBfFipB9DRe&iWZ3~-YyZ`A;Iwdap&=rPCFfj=Ho1{
g&re>;xSN@QA$M{jlOdzb=E+PsjFS&A`7r7M0H<Ln=l}o!

-- 
1.7.7

Re: [Qemu-devel] [PATCH 0/3] multiboot: Fix memory information

[Anthony Liguori]

Kevin Wolf <mail@kevin-wolf.de> writes:

> Kevin Wolf (3):
>   multiboot: Don't forget last mmap entry
>   multiboot: Calculate upper_mem in the ROM
>   multiboot: Updated ROM binary

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Do you have a test case that triggered this that you can share?

I'll apply this after a day or so when others have had a chance to review.

Regards,

Anthony Liguori

>
>  hw/i386/multiboot.c           |    2 -
>  pc-bios/multiboot.bin         |  Bin 1024 -> 1024 bytes
>  pc-bios/optionrom/multiboot.S |   75 +++++++++++++++++++++++++++++++----------
>  3 files changed, 57 insertions(+), 20 deletions(-)
>  mode change 100644 => 100755 pc-bios/multiboot.bin
>
> -- 
> 1.7.7

Re: [Qemu-devel] [PATCH 0/3] multiboot: Fix memory information

[Kevin Wolf]

Am 23.06.2013 um 23:39 hat Anthony Liguori geschrieben:
> Kevin Wolf <mail@kevin-wolf.de> writes:
> 
> > Kevin Wolf (3):
> >   multiboot: Don't forget last mmap entry
> >   multiboot: Calculate upper_mem in the ROM
> >   multiboot: Updated ROM binary
> 
> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> Do you have a test case that triggered this that you can share?

I haven't seen an actual kernel failure myself, this was reported by
someone else. But it's easy enough to check with a simple Multiboot
kernel that just outputs lower_mem/upper_mem and the mmap.

For debugging and fixing I added some throw-away debug code to an
existing kernel and compared the fixed version with the output when
loaded by GRUB and it matches now.

If we can have a real test case for this somewhere, I can write a small
kernel to do the check. Not sure where it would fit though - probably
kvm-unittests is the closest, even though the test wouldn't really have
anything to do with KVM.

> I'll apply this after a day or so when others have had a chance to review.

Sounds good, thanks.

Kevin

Re: [Qemu-devel] [PATCH 0/3] multiboot: Fix memory information

[Anthony Liguori]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 23.06.2013 um 23:39 hat Anthony Liguori geschrieben:
>> Kevin Wolf <mail@kevin-wolf.de> writes:
>> 
>> > Kevin Wolf (3):
>> >   multiboot: Don't forget last mmap entry
>> >   multiboot: Calculate upper_mem in the ROM
>> >   multiboot: Updated ROM binary
>> 
>> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
>> 
>> Do you have a test case that triggered this that you can share?
>
> I haven't seen an actual kernel failure myself, this was reported by
> someone else. But it's easy enough to check with a simple Multiboot
> kernel that just outputs lower_mem/upper_mem and the mmap.
>
> For debugging and fixing I added some throw-away debug code to an
> existing kernel and compared the fixed version with the output when
> loaded by GRUB and it matches now.
>
> If we can have a real test case for this somewhere, I can write a small
> kernel to do the check. Not sure where it would fit though - probably
> kvm-unittests is the closest, even though the test wouldn't really have
> anything to do with KVM.

We could put it in pc-bios even with just a README for now.  Would be
nice to figure out ways to test -kernel as we do find issues often
enough.

It's up to you.  If you had something handy, I was going to add it to my
local test setup.

Regards,

Anthony Liguori

>
>> I'll apply this after a day or so when others have had a chance to review.
>
> Sounds good, thanks.
>
> Kevin

Re: [Qemu-devel] [PATCH 0/3] multiboot: Fix memory information

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori