[Qemu-devel] [PATCH v3 0/2] Fix two bugs related to ram_size

[Qemu-devel] [PATCH v3 0/2] Fix two bugs related to ram_size

[Markus Armbruster]

There are more, but let's start with these two.

v1 -> v3:
* Use QEMU_ALIGN_UP (Avi)
* Cover size overflow
* Coding style (Blue)

Markus Armbruster (2):
  vl: Round argument of -m up to multiple of 8KiB
  pc: Fix RTC CMOS info on RAM for ram_size < 1MiB

 hw/pc.c | 31 ++++++++++++++++++-------------
 vl.c    |  7 ++++---
 2 files changed, 22 insertions(+), 16 deletions(-)

-- 
1.7.11.2

[Qemu-devel] [PATCH v3 1/2] vl: Round argument of -m up to multiple of 8KiB

[Markus Armbruster]

Partial pages make little sense and don't work.  Ensure the RAM size
is a multiple of any possible target's page size.

Fixes

    $ qemu-system-x86_64 -nodefaults -S -vnc :0 -m 0.8
    qemu-system-x86_64: /work/armbru/qemu/exec.c:2255: register_subpage: Assertion `existing->mr->subpage || existing->mr == &io_mem_unassigned' failed.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 vl.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/vl.c b/vl.c
index d01256a..a63e16b 100644
--- a/vl.c
+++ b/vl.c
@@ -2701,6 +2701,7 @@ int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_m: {
                 int64_t value;
+                uint64_t sz;
                 char *end;
 
                 value = strtosz(optarg, &end);
@@ -2708,12 +2709,12 @@ int main(int argc, char **argv, char **envp)
                     fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
                     exit(1);
                 }
-
-                if (value != (uint64_t)(ram_addr_t)value) {
+                sz = QEMU_ALIGN_UP((uint64_t)value, 8192);
+                ram_size = sz;
+                if (ram_size != sz) {
                     fprintf(stderr, "qemu: ram size too large\n");
                     exit(1);
                 }
-                ram_size = value;
                 break;
             }
             case QEMU_OPTION_mempath:
-- 
1.7.11.2

[Qemu-devel] [PATCH v3 2/2] pc: Fix RTC CMOS info on RAM for ram_size < 1MiB

[Markus Armbruster]

pc_cmos_init() always claims 640KiB base memory, and ram_size - 1MiB
extended memory.  The latter can underflow to "lots of extended
memory".  Fix both, and clean up some.

Note: SeaBIOS currently requires 1MiB of RAM, and doesn't check
whether it got enough.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/pc.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index e8bcfc0..24df1e0 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -337,32 +337,37 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
     /* various important CMOS locations needed by PC/Bochs bios */
 
     /* memory size */
-    val = 640; /* base memory in K */
+    /* base memory (first MiB) */
+    val = MIN(ram_size / 1024, 640);
     rtc_set_memory(s, 0x15, val);
     rtc_set_memory(s, 0x16, val >> 8);
-
-    val = (ram_size / 1024) - 1024;
+    /* extended memory (next 64MiB) */
+    if (ram_size > 1024 * 1024) {
+        val = (ram_size - 1024 * 1024) / 1024;
+    } else {
+        val = 0;
+    }
     if (val > 65535)
         val = 65535;
     rtc_set_memory(s, 0x17, val);
     rtc_set_memory(s, 0x18, val >> 8);
     rtc_set_memory(s, 0x30, val);
     rtc_set_memory(s, 0x31, val >> 8);
-
-    if (above_4g_mem_size) {
-        rtc_set_memory(s, 0x5b, (unsigned int)above_4g_mem_size >> 16);
-        rtc_set_memory(s, 0x5c, (unsigned int)above_4g_mem_size >> 24);
-        rtc_set_memory(s, 0x5d, (uint64_t)above_4g_mem_size >> 32);
-    }
-
-    if (ram_size > (16 * 1024 * 1024))
-        val = (ram_size / 65536) - ((16 * 1024 * 1024) / 65536);
-    else
+    /* memory between 16MiB and 4GiB */
+    if (ram_size > 16 * 1024 * 1024) {
+        val = (ram_size - 16 * 1024 * 1024) / 65536;
+    } else {
         val = 0;
+    }
     if (val > 65535)
         val = 65535;
     rtc_set_memory(s, 0x34, val);
     rtc_set_memory(s, 0x35, val >> 8);
+    /* memory above 4GiB */
+    val = above_4g_mem_size / 65536;
+    rtc_set_memory(s, 0x5b, val);
+    rtc_set_memory(s, 0x5c, val >> 8);
+    rtc_set_memory(s, 0x5d, val >> 16);
 
     /* set the number of CPU */
     rtc_set_memory(s, 0x5f, smp_cpus - 1);
-- 
1.7.11.2

Re: [Qemu-devel] [PATCH v3 0/2] Fix two bugs related to ram_size

[Blue Swirl]

On Wed, Aug 15, 2012 at 11:12 AM, Markus Armbruster <armbru@redhat.com> wrote:
> There are more, but let's start with these two.

Thanks, applied both.

>
> v1 -> v3:
> * Use QEMU_ALIGN_UP (Avi)
> * Cover size overflow
> * Coding style (Blue)
>
> Markus Armbruster (2):
>   vl: Round argument of -m up to multiple of 8KiB
>   pc: Fix RTC CMOS info on RAM for ram_size < 1MiB
>
>  hw/pc.c | 31 ++++++++++++++++++-------------
>  vl.c    |  7 ++++---
>  2 files changed, 22 insertions(+), 16 deletions(-)
>
> --
> 1.7.11.2
>

Re: [Qemu-devel] [PATCH v3 2/2] pc: Fix RTC CMOS info on RAM for ram_size < 1MiB

[Kevin O'Connor]

On Wed, Aug 15, 2012 at 01:12:20PM +0200, Markus Armbruster wrote:
> pc_cmos_init() always claims 640KiB base memory, and ram_size - 1MiB
> extended memory.  The latter can underflow to "lots of extended
> memory".  Fix both, and clean up some.
> 
> Note: SeaBIOS currently requires 1MiB of RAM, and doesn't check
> whether it got enough.

SeaBIOS (and Bochs BIOS before it) never checked cmos 0x15-0x18 for
memory sizes.  Thus, it has only ever supported passing memory in 1MiB
chunks.  If we really want to communicate fine grained memory sizes, I
suggest passing an e820 like map via fw_cfg to SeaBIOS.  I don't think
it makes sense for SeaBIOS to read new magic cmos settings just to
check if it should crash in a slightly different way.

-Kevin

Re: [Qemu-devel] [PATCH v3 2/2] pc: Fix RTC CMOS info on RAM for ram_size < 1MiB

[Markus Armbruster]

"Kevin O'Connor" <kevin@koconnor.net> writes:

> On Wed, Aug 15, 2012 at 01:12:20PM +0200, Markus Armbruster wrote:
>> pc_cmos_init() always claims 640KiB base memory, and ram_size - 1MiB
>> extended memory.  The latter can underflow to "lots of extended
>> memory".  Fix both, and clean up some.
>> 
>> Note: SeaBIOS currently requires 1MiB of RAM, and doesn't check
>> whether it got enough.
>
> SeaBIOS (and Bochs BIOS before it) never checked cmos 0x15-0x18 for
> memory sizes.  Thus, it has only ever supported passing memory in 1MiB
> chunks.

I'm not debating what SeaBIOS supports, not even what it should support,
just correcting two inaccuracies:

1. SeaBIOS indeed doesn't check CMOS 0x15/0x16 (base memory, below
   1MiB), but it *does* check the copy of 0x17/0x18 in 0x30/0x31
   (extended memory, between 1MiB and 64MiB).

2. Not checking 0x15-0x18 does *not* imply 1MiB granularity.  CMOS RAM
   size granularity is 64KiB above 16MiB, and 1KiB below.

>          If we really want to communicate fine grained memory sizes, I
> suggest passing an e820 like map via fw_cfg to SeaBIOS.  I don't think
> it makes sense for SeaBIOS to read new magic cmos settings just to
> check if it should crash in a slightly different way.

Where "slightly different" means "fail POST (early, thus very limited
ways to report to the user)" instead of "attempt to use non-existant RAM
and crash hard".  Sounds nice to have to me, but there are tons of
"nice" things that are plainly not worth the trouble.

Whether this one is worth the trouble for SeaBIOS is for SeaBIOS to
decide.  Likewise, whether switching from CMOS to fw_cfg for RAM size is
worth it.

A nice thing to have on the QEMU side would be refusing to set up
SeaBIOS for a necessarily obscure failure by not meeting its RAM
requirement.  Again, this may not be worth the trouble.  Is there a
reliable way to recognize SeaBIOS?