[Qemu-devel] [PATCH 0/2] Let boards state maximum RAM limits in QEMUMachine struct

[Qemu-devel] [PATCH 0/2] Let boards state maximum RAM limits in QEMUMachine struct

[Peter Maydell]

This fairly simple patchset adds a new 'max_ram' field to the QEMUMachine
structure so that a board model can specify the maximum RAM it will accept.
We can then produce a friendly diagnostic message when the user tries to
start qemu with a '-m' option asking for more RAM than that. (Currently
most of the ARM devboard models respond with an obscure guest crash when
the guest tries to access RAM and finds device registers instead.)

If no maximum size is specified we default to the old behaviour of
"do not impose any limit".

The advantage of doing this in vl.c rather than in each board (apart
from avoiding code duplication) is that we can distinguish between
"the user asked for more RAM than we support" (an error) and "the global
default RAM size is more than our maximum" (just cap the RAM size to
the board maximum).

I did think about also adding a "default_ram" field so each board could
default to the same amount of RAM that real hardware has, but since we
allocate RAM up front rather than only when accessed, this would push the
initial default allocated RAM size up from 128MB to 1GB on models like
the PBX A9, and it didn't seem very friendly to do that. I'm happy
to be persuaded back again, though :-)

This patchset was prompted by bugs like:
https://bugs.launchpad.net/ubuntu/+source/qemu-kvm/+bug/584480
https://bugs.launchpad.net/ubuntu/+source/rootstock/+bug/570588

Peter Maydell (2):
  Allow boards to specify maximum RAM size
  hw: Add maximum RAM specifications for ARM devboard models

 hw/boards.h       |    1 +
 hw/integratorcp.c |    1 +
 hw/realview.c     |   11 +++++++++++
 hw/versatilepb.c  |    5 +++++
 vl.c              |   16 +++++++++++++++-
 5 files changed, 33 insertions(+), 1 deletions(-)

[Qemu-devel] [PATCH 1/2] Allow boards to specify maximum RAM size

[Peter Maydell]

Allow boards to specify their maximum RAM size in the QEMUMachine struct.
This allows us to provide a useful diagnostic if the user tries to specify
a RAM size that the board cannot support.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/boards.h |    1 +
 vl.c        |   16 +++++++++++++++-
 2 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/hw/boards.h b/hw/boards.h
index 6f0f0d7..bf63f8d 100644
--- a/hw/boards.h
+++ b/hw/boards.h
@@ -19,6 +19,7 @@ typedef struct QEMUMachine {
     QEMUMachineInitFunc *init;
     int use_scsi;
     int max_cpus;
+    ram_addr_t max_ram;
     unsigned int no_serial:1,
         no_parallel:1,
         use_virtcon:1,
diff --git a/vl.c b/vl.c
index b1a94aa..971ad90 100644
--- a/vl.c
+++ b/vl.c
@@ -166,6 +166,9 @@ int main(int argc, char **argv)
 //#define DEBUG_NET
 //#define DEBUG_SLIRP
 
+/* Note that this default RAM size is capped to any maximum
+ * RAM size specified in the board's QEMUMachine struct.
+ */
 #define DEFAULT_RAM_SIZE 128
 
 #define MAX_VIRTIO_CONSOLES 1
@@ -2910,8 +2913,19 @@ int main(int argc, char **argv, char **envp)
         exit(1);
 
     /* init the memory */
-    if (ram_size == 0)
+    if (ram_size == 0) {
         ram_size = DEFAULT_RAM_SIZE * 1024 * 1024;
+        if (machine->max_ram) {
+            ram_size = MIN(ram_size, machine->max_ram);
+        }
+    } else if (machine->max_ram && ram_size > machine->max_ram) {
+        /* Since you can only specify ram_size on the command line in MB it's
+         * OK to round down when printing the machine's maximum.
+         */
+        fprintf(stderr, "qemu: maximum permitted RAM size for '%s' is %ldM\n",
+                machine->name, (machine->max_ram / (1024 * 1024)));
+        exit(1);
+    }
 
     /* init the dynamic translator */
     cpu_exec_init_all(tb_size * 1024 * 1024);
-- 
1.7.1

[Qemu-devel] [PATCH 2/2] hw: Add maximum RAM specifications for ARM devboard models

[Peter Maydell]

Specify the maximum memory permitted for the various ARM devboard
models (integratorcp, realview-eb, realview-eb-mpcore, realview-pb-a8,
realview-pbx-a9, versatilepb, versatileab). This means we now handle
attempts to specify too much RAM gracefully rather than causing
the guest to crash in an obscure fashion.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/integratorcp.c |    1 +
 hw/realview.c     |   11 +++++++++++
 hw/versatilepb.c  |    5 +++++
 3 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/hw/integratorcp.c b/hw/integratorcp.c
index b049940..ccc44db 100644
--- a/hw/integratorcp.c
+++ b/hw/integratorcp.c
@@ -516,6 +516,7 @@ static QEMUMachine integratorcp_machine = {
     .name = "integratorcp",
     .desc = "ARM Integrator/CP (ARM926EJ-S)",
     .init = integratorcp_init,
+    .max_ram = 256 * 1024 * 1024,
     .is_default = 1,
 };
 
diff --git a/hw/realview.c b/hw/realview.c
index a67861e..a158ade 100644
--- a/hw/realview.c
+++ b/hw/realview.c
@@ -432,6 +432,7 @@ static QEMUMachine realview_eb_machine = {
     .desc = "ARM RealView Emulation Baseboard (ARM926EJ-S)",
     .init = realview_eb_init,
     .use_scsi = 1,
+    .max_ram = 256 * 1024 * 1024,
 };
 
 static QEMUMachine realview_eb_mpcore_machine = {
@@ -440,12 +441,18 @@ static QEMUMachine realview_eb_mpcore_machine = {
     .init = realview_eb_mpcore_init,
     .use_scsi = 1,
     .max_cpus = 4,
+    .max_ram = 256 * 1024 * 1024,
 };
 
 static QEMUMachine realview_pb_a8_machine = {
     .name = "realview-pb-a8",
     .desc = "ARM RealView Platform Baseboard for Cortex-A8",
     .init = realview_pb_a8_init,
+    /* The PB-A8 has 512MB; qemu also supports an extra PBX-A9-like
+     * 512MB although strictly speaking that area of the address
+     * space is 'reserved' on the PB-A8.
+     */
+    .max_ram = 1024 * 1024 * 1024,
 };
 
 static QEMUMachine realview_pbx_a9_machine = {
@@ -454,6 +461,10 @@ static QEMUMachine realview_pbx_a9_machine = {
     .init = realview_pbx_a9_init,
     .use_scsi = 1,
     .max_cpus = 4,
+    /* Realview PBX has 1GB of RAM (512MB on the motherboard
+     * and another 512MB on the daughterboard)
+     */
+    .max_ram = 1024 * 1024 * 1024,
 };
 
 static void realview_machine_init(void)
diff --git a/hw/versatilepb.c b/hw/versatilepb.c
index 9f1bfcf..aeddd28 100644
--- a/hw/versatilepb.c
+++ b/hw/versatilepb.c
@@ -329,6 +329,10 @@ static QEMUMachine versatilepb_machine = {
     .desc = "ARM Versatile/PB (ARM926EJ-S)",
     .init = vpb_init,
     .use_scsi = 1,
+    /* Hardware allows for up to 512MB expansion memory in two
+     * non-contiguous sections, but we only support up to 256MB
+     */
+    .max_ram = 256 * 1024 * 1024,
 };
 
 static QEMUMachine versatileab_machine = {
@@ -336,6 +340,7 @@ static QEMUMachine versatileab_machine = {
     .desc = "ARM Versatile/AB (ARM926EJ-S)",
     .init = vab_init,
     .use_scsi = 1,
+    .max_ram = 256 * 1024 * 1024,
 };
 
 static void versatile_machine_init(void)
-- 
1.7.1

Re: [Qemu-devel] [PATCH 0/2] Let boards state maximum RAM limits in QEMUMachine struct

[Anthony Liguori]

On 03/21/2011 12:47 PM, Peter Maydell wrote:
> This fairly simple patchset adds a new 'max_ram' field to the QEMUMachine
> structure so that a board model can specify the maximum RAM it will accept.
> We can then produce a friendly diagnostic message when the user tries to
> start qemu with a '-m' option asking for more RAM than that. (Currently
> most of the ARM devboard models respond with an obscure guest crash when
> the guest tries to access RAM and finds device registers instead.)
>
> If no maximum size is specified we default to the old behaviour of
> "do not impose any limit".
>
> The advantage of doing this in vl.c rather than in each board (apart
> from avoiding code duplication) is that we can distinguish between
> "the user asked for more RAM than we support" (an error) and "the global
> default RAM size is more than our maximum" (just cap the RAM size to
> the board maximum).
>
> I did think about also adding a "default_ram" field so each board could
> default to the same amount of RAM that real hardware has, but since we
> allocate RAM up front rather than only when accessed, this would push the
> initial default allocated RAM size up from 128MB to 1GB on models like
> the PBX A9, and it didn't seem very friendly to do that. I'm happy
> to be persuaded back again, though :-)

I don't have any objections to these patches, but it makes me wonder if 
we should try to go further.

There are certainly a lot of use-cases where the boards being emulated 
are extremely sensitive to the options specified.   I wonder if we ought 
to provide a better way for boards to participate in command line 
validation.  For instance, I know that many boards can only accept 
certain RAM sizes that map to realistic DIMM sizes.

I don't have any great ideas here but wonder if it's something we should 
more seriously consider.

Regards,

Anthony Liguori

> This patchset was prompted by bugs like:
> https://bugs.launchpad.net/ubuntu/+source/qemu-kvm/+bug/584480
> https://bugs.launchpad.net/ubuntu/+source/rootstock/+bug/570588
>
> Peter Maydell (2):
>    Allow boards to specify maximum RAM size
>    hw: Add maximum RAM specifications for ARM devboard models
>
>   hw/boards.h       |    1 +
>   hw/integratorcp.c |    1 +
>   hw/realview.c     |   11 +++++++++++
>   hw/versatilepb.c  |    5 +++++
>   vl.c              |   16 +++++++++++++++-
>   5 files changed, 33 insertions(+), 1 deletions(-)
>
>

Re: [Qemu-devel] [PATCH 0/2] Let boards state maximum RAM limits in QEMUMachine struct

[Brad Hards]

On Tue, 22 Mar 2011 04:47:18 am Peter Maydell wrote:
> This fairly simple patchset adds a new 'max_ram' field to the QEMUMachine
> structure so that a board model can specify the maximum RAM it will accept.
> We can then produce a friendly diagnostic message when the user tries to
> start qemu with a '-m' option asking for more RAM than that. (Currently
> most of the ARM devboard models respond with an obscure guest crash when
> the guest tries to access RAM and finds device registers instead.)
As a user, I've been bitten by this. Without understanding how qemu works, the 
problem is quite surprising: "all I've done is increased the RAM, and now it 
just crashes".

I don't think my review of the code will count for much, and I'd prefer to see 
the code added rather than not, but you could move the cmd.c macros (MEGABYTES  
and TO_MEGABYTES) to some common header and just use those. They are pretty 
ugly though...

Brad

Re: [Qemu-devel] [PATCH 0/2] Let boards state maximum RAM limits in QEMUMachine struct

[Blue Swirl]

On Mon, Mar 21, 2011 at 7:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> This fairly simple patchset adds a new 'max_ram' field to the QEMUMachine
> structure so that a board model can specify the maximum RAM it will accept.
> We can then produce a friendly diagnostic message when the user tries to
> start qemu with a '-m' option asking for more RAM than that. (Currently
> most of the ARM devboard models respond with an obscure guest crash when
> the guest tries to access RAM and finds device registers instead.)
>
> If no maximum size is specified we default to the old behaviour of
> "do not impose any limit".
>
> The advantage of doing this in vl.c rather than in each board (apart
> from avoiding code duplication) is that we can distinguish between
> "the user asked for more RAM than we support" (an error) and "the global
> default RAM size is more than our maximum" (just cap the RAM size to
> the board maximum).

This could replace the field max_mem in hwdef structures in sun4m.c.

Another candidate for refactoring would be default_cpu_model.

Re: [Qemu-devel] [PATCH 0/2] Let boards state maximum RAM limits in QEMUMachine struct

[Peter Maydell]

On 26 March 2011 11:41, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Mon, Mar 21, 2011 at 7:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> This fairly simple patchset adds a new 'max_ram' field to the QEMUMachine
>> structure so that a board model can specify the maximum RAM it will accept.
>> We can then produce a friendly diagnostic message when the user tries to
>> start qemu with a '-m' option asking for more RAM than that. (Currently
>> most of the ARM devboard models respond with an obscure guest crash when
>> the guest tries to access RAM and finds device registers instead.)

> This could replace the field max_mem in hwdef structures in sun4m.c.

I've had a go at this, and it's revealed a flaw in this patchset,
which is that the max_ram field needs to be type target_physaddr_t
rather than ram_addr_t. I'll post another patchset including a sun4m
cleanup(*) once I've done some testing on it.

(*) includes combining the QEMUMachine struct into the sun4*_hwdef
struct, which is necessary so the init fn can get at it and also
I think neater.

-- PMM