Patchwork [v3,2/2] pc: Fix RTC CMOS info on RAM for ram_size < 1MiB

login
register
mail settings
Submitter Markus Armbruster
Date Aug. 15, 2012, 11:12 a.m.
Message ID <1345029140-12338-3-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/177627/
State New
Headers show

Comments

Markus Armbruster - Aug. 15, 2012, 11:12 a.m.
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(-)
Kevin O'Connor - Aug. 19, 2012, 7:36 p.m.
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
Markus Armbruster - Aug. 20, 2012, 8:54 a.m.
"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?

Patch

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);