[Qemu-devel] [PATCH v2 for-4.1] hw/arm/raspi: Diagnose requests for too much RAM

Peter Maydell posted 1 patch 4 years, 12 months ago
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test checkpatch passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190328152944.3199-1-peter.maydell@linaro.org
Maintainers: Peter Maydell <peter.maydell@linaro.org>, Andrew Baumann <Andrew.Baumann@microsoft.com>, "Philippe Mathieu-Daudé" <f4bug@amsat.org>
hw/arm/raspi.c | 7 +++++++
1 file changed, 7 insertions(+)
[Qemu-devel] [PATCH v2 for-4.1] hw/arm/raspi: Diagnose requests for too much RAM
Posted by Peter Maydell 4 years, 12 months ago
The Raspberry Pi boards have a physical memory map which does
not allow for more than 1GB of RAM. Currently if the user tries
to ask for more then we fail in a confusing way:

$ qemu-system-aarch64 --machine raspi3 -m 8G
Unexpected error in visit_type_uintN() at qapi/qapi-visit-core.c:164:
qemu-system-aarch64: Parameter 'vcram-base' expects uint32_t
Aborted (core dumped)

Catch this earlier and diagnose it with a more friendly message:
$ qemu-system-aarch64 --machine raspi3 -m 8G
qemu-system-aarch64: Requested ram size is too large for this machine: maximum is 1GB

Fixes: https://bugs.launchpad.net/qemu/+bug/1794187
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Changes v1->v2: use '>', not '>='...

 hw/arm/raspi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 66899c28dc1..fe2bb511b98 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -12,6 +12,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/units.h"
 #include "qapi/error.h"
 #include "qemu-common.h"
 #include "cpu.h"
@@ -175,6 +176,12 @@ static void raspi_init(MachineState *machine, int version)
     BusState *bus;
     DeviceState *carddev;
 
+    if (machine->ram_size > 1 * GiB) {
+        error_report("Requested ram size is too large for this machine: "
+                     "maximum is 1GB");
+        exit(1);
+    }
+
     object_initialize(&s->soc, sizeof(s->soc),
                       version == 3 ? TYPE_BCM2837 : TYPE_BCM2836);
     object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
-- 
2.20.1


Re: [Qemu-devel] [PATCH v2 for-4.1] hw/arm/raspi: Diagnose requests for too much RAM
Posted by Philippe Mathieu-Daudé 4 years, 12 months ago
Le jeu. 28 mars 2019 16:29, Peter Maydell <peter.maydell@linaro.org> a
écrit :

> The Raspberry Pi boards have a physical memory map which does
> not allow for more than 1GB of RAM. Currently if the user tries
> to ask for more then we fail in a confusing way:
>
> $ qemu-system-aarch64 --machine raspi3 -m 8G
> Unexpected error in visit_type_uintN() at qapi/qapi-visit-core.c:164:
> qemu-system-aarch64: Parameter 'vcram-base' expects uint32_t
> Aborted (core dumped)
>
> Catch this earlier and diagnose it with a more friendly message:
> $ qemu-system-aarch64 --machine raspi3 -m 8G
> qemu-system-aarch64: Requested ram size is too large for this machine:
> maximum is 1GB
>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1794187
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Changes v1->v2: use '>', not '>='...
>

Argh I missed this...

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


>  hw/arm/raspi.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 66899c28dc1..fe2bb511b98 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -12,6 +12,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/units.h"
>  #include "qapi/error.h"
>  #include "qemu-common.h"
>  #include "cpu.h"
> @@ -175,6 +176,12 @@ static void raspi_init(MachineState *machine, int
> version)
>      BusState *bus;
>      DeviceState *carddev;
>
> +    if (machine->ram_size > 1 * GiB) {
> +        error_report("Requested ram size is too large for this machine: "
> +                     "maximum is 1GB");
> +        exit(1);
> +    }
> +
>      object_initialize(&s->soc, sizeof(s->soc),
>                        version == 3 ? TYPE_BCM2837 : TYPE_BCM2836);
>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> --
> 2.20.1
>
>
Re: [Qemu-devel] [PATCH v2 for-4.1] hw/arm/raspi: Diagnose requests for too much RAM
Posted by Richard Henderson 4 years, 12 months ago
On 3/28/19 8:29 AM, Peter Maydell wrote:
> The Raspberry Pi boards have a physical memory map which does
> not allow for more than 1GB of RAM. Currently if the user tries
> to ask for more then we fail in a confusing way:
> 
> $ qemu-system-aarch64 --machine raspi3 -m 8G
> Unexpected error in visit_type_uintN() at qapi/qapi-visit-core.c:164:
> qemu-system-aarch64: Parameter 'vcram-base' expects uint32_t
> Aborted (core dumped)
> 
> Catch this earlier and diagnose it with a more friendly message:
> $ qemu-system-aarch64 --machine raspi3 -m 8G
> qemu-system-aarch64: Requested ram size is too large for this machine: maximum is 1GB
> 
> Fixes: https://bugs.launchpad.net/qemu/+bug/1794187
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Changes v1->v2: use '>', not '>='...

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v2 for-4.1] hw/arm/raspi: Diagnose requests for too much RAM
Posted by Wainer dos Santos Moschetta 4 years, 12 months ago
On 03/28/2019 12:29 PM, Peter Maydell wrote:
> The Raspberry Pi boards have a physical memory map which does
> not allow for more than 1GB of RAM. Currently if the user tries
> to ask for more then we fail in a confusing way:
>
> $ qemu-system-aarch64 --machine raspi3 -m 8G
> Unexpected error in visit_type_uintN() at qapi/qapi-visit-core.c:164:
> qemu-system-aarch64: Parameter 'vcram-base' expects uint32_t
> Aborted (core dumped)
>
> Catch this earlier and diagnose it with a more friendly message:
> $ qemu-system-aarch64 --machine raspi3 -m 8G
> qemu-system-aarch64: Requested ram size is too large for this machine: maximum is 1GB
>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1794187
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Changes v1->v2: use '>', not '>='...
>
>   hw/arm/raspi.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 66899c28dc1..fe2bb511b98 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -12,6 +12,7 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qemu/units.h"
>   #include "qapi/error.h"
>   #include "qemu-common.h"
>   #include "cpu.h"
> @@ -175,6 +176,12 @@ static void raspi_init(MachineState *machine, int version)
>       BusState *bus;
>       DeviceState *carddev;
>   
> +    if (machine->ram_size > 1 * GiB) {
> +        error_report("Requested ram size is too large for this machine: "
> +                     "maximum is 1GB");

1GB vs 1GiB... maybe the message should display "GiB" to avoid any 
confusion?

Also I was wondering what is need for Patchew to start catching errors 
like in v1.

- Wainer

> +        exit(1);
> +    }
> +
>       object_initialize(&s->soc, sizeof(s->soc),
>                         version == 3 ? TYPE_BCM2837 : TYPE_BCM2836);
>       object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),


Re: [Qemu-devel] [PATCH v2 for-4.1] hw/arm/raspi: Diagnose requests for too much RAM
Posted by Peter Maydell 4 years, 12 months ago
On Thu, 28 Mar 2019 at 21:29, Wainer dos Santos Moschetta
<wainersm@redhat.com> wrote:
>
>
> On 03/28/2019 12:29 PM, Peter Maydell wrote:
> > The Raspberry Pi boards have a physical memory map which does
> > not allow for more than 1GB of RAM. Currently if the user tries
> > to ask for more then we fail in a confusing way:
> >
> > $ qemu-system-aarch64 --machine raspi3 -m 8G
> > Unexpected error in visit_type_uintN() at qapi/qapi-visit-core.c:164:
> > qemu-system-aarch64: Parameter 'vcram-base' expects uint32_t
> > Aborted (core dumped)
> >
> > Catch this earlier and diagnose it with a more friendly message:
> > $ qemu-system-aarch64 --machine raspi3 -m 8G
> > qemu-system-aarch64: Requested ram size is too large for this machine: maximum is 1GB
> >
> > Fixes: https://bugs.launchpad.net/qemu/+bug/1794187
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > Changes v1->v2: use '>', not '>='...
> >
> >   hw/arm/raspi.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> > index 66899c28dc1..fe2bb511b98 100644
> > --- a/hw/arm/raspi.c
> > +++ b/hw/arm/raspi.c
> > @@ -12,6 +12,7 @@
> >    */
> >
> >   #include "qemu/osdep.h"
> > +#include "qemu/units.h"
> >   #include "qapi/error.h"
> >   #include "qemu-common.h"
> >   #include "cpu.h"
> > @@ -175,6 +176,12 @@ static void raspi_init(MachineState *machine, int version)
> >       BusState *bus;
> >       DeviceState *carddev;
> >
> > +    if (machine->ram_size > 1 * GiB) {
> > +        error_report("Requested ram size is too large for this machine: "
> > +                     "maximum is 1GB");
>
> 1GB vs 1GiB... maybe the message should display "GiB" to avoid any
> confusion?

I don't know why we call our #defined constant GiB -- to me
the unit is GB and I think printing anything else in user
messages is weird.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 for-4.1] hw/arm/raspi: Diagnose requests for too much RAM
Posted by Wainer dos Santos Moschetta 4 years, 12 months ago
On 03/29/2019 06:20 AM, Peter Maydell wrote:
> On Thu, 28 Mar 2019 at 21:29, Wainer dos Santos Moschetta
> <wainersm@redhat.com> wrote:
>>
>> On 03/28/2019 12:29 PM, Peter Maydell wrote:
>>> The Raspberry Pi boards have a physical memory map which does
>>> not allow for more than 1GB of RAM. Currently if the user tries
>>> to ask for more then we fail in a confusing way:
>>>
>>> $ qemu-system-aarch64 --machine raspi3 -m 8G
>>> Unexpected error in visit_type_uintN() at qapi/qapi-visit-core.c:164:
>>> qemu-system-aarch64: Parameter 'vcram-base' expects uint32_t
>>> Aborted (core dumped)
>>>
>>> Catch this earlier and diagnose it with a more friendly message:
>>> $ qemu-system-aarch64 --machine raspi3 -m 8G
>>> qemu-system-aarch64: Requested ram size is too large for this machine: maximum is 1GB
>>>
>>> Fixes: https://bugs.launchpad.net/qemu/+bug/1794187
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>> Changes v1->v2: use '>', not '>='...
>>>
>>>    hw/arm/raspi.c | 7 +++++++
>>>    1 file changed, 7 insertions(+)
>>>
>>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
>>> index 66899c28dc1..fe2bb511b98 100644
>>> --- a/hw/arm/raspi.c
>>> +++ b/hw/arm/raspi.c
>>> @@ -12,6 +12,7 @@
>>>     */
>>>
>>>    #include "qemu/osdep.h"
>>> +#include "qemu/units.h"
>>>    #include "qapi/error.h"
>>>    #include "qemu-common.h"
>>>    #include "cpu.h"
>>> @@ -175,6 +176,12 @@ static void raspi_init(MachineState *machine, int version)
>>>        BusState *bus;
>>>        DeviceState *carddev;
>>>
>>> +    if (machine->ram_size > 1 * GiB) {
>>> +        error_report("Requested ram size is too large for this machine: "
>>> +                     "maximum is 1GB");
>> 1GB vs 1GiB... maybe the message should display "GiB" to avoid any
>> confusion?
> I don't know why we call our #defined constant GiB -- to me
> the unit is GB and I think printing anything else in user
> messages is weird.

Ok, I find it weird as well. But I slightly remember that discussion 
before so I thought in raising it here too.

Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>


>
> thanks
> -- PMM
>


Re: [Qemu-devel] [PATCH v2 for-4.1] hw/arm/raspi: Diagnose requests for too much RAM
Posted by Philippe Mathieu-Daudé 4 years, 12 months ago
Le ven. 29 mars 2019 10:20, Peter Maydell <peter.maydell@linaro.org> a
écrit :

> On Thu, 28 Mar 2019 at 21:29, Wainer dos Santos Moschetta
> <wainersm@redhat.com> wrote:
> >
> >
> > On 03/28/2019 12:29 PM, Peter Maydell wrote:
> > > The Raspberry Pi boards have a physical memory map which does
> > > not allow for more than 1GB of RAM. Currently if the user tries
> > > to ask for more then we fail in a confusing way:
> > >
> > > $ qemu-system-aarch64 --machine raspi3 -m 8G
> > > Unexpected error in visit_type_uintN() at qapi/qapi-visit-core.c:164:
> > > qemu-system-aarch64: Parameter 'vcram-base' expects uint32_t
> > > Aborted (core dumped)
> > >
> > > Catch this earlier and diagnose it with a more friendly message:
> > > $ qemu-system-aarch64 --machine raspi3 -m 8G
> > > qemu-system-aarch64: Requested ram size is too large for this machine:
> maximum is 1GB
> > >
> > > Fixes: https://bugs.launchpad.net/qemu/+bug/1794187
> > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > > ---
> > > Changes v1->v2: use '>', not '>='...
> > >
> > >   hw/arm/raspi.c | 7 +++++++
> > >   1 file changed, 7 insertions(+)
> > >
> > > diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> > > index 66899c28dc1..fe2bb511b98 100644
> > > --- a/hw/arm/raspi.c
> > > +++ b/hw/arm/raspi.c
> > > @@ -12,6 +12,7 @@
> > >    */
> > >
> > >   #include "qemu/osdep.h"
> > > +#include "qemu/units.h"
> > >   #include "qapi/error.h"
> > >   #include "qemu-common.h"
> > >   #include "cpu.h"
> > > @@ -175,6 +176,12 @@ static void raspi_init(MachineState *machine, int
> version)
> > >       BusState *bus;
> > >       DeviceState *carddev;
> > >
> > > +    if (machine->ram_size > 1 * GiB) {
> > > +        error_report("Requested ram size is too large for this
> machine: "
> > > +                     "maximum is 1GB");
> >
> > 1GB vs 1GiB... maybe the message should display "GiB" to avoid any
> > confusion?
>
> I don't know why we call our #defined constant GiB -- to me
> the unit is GB and I think printing anything else in user
> messages is weird.
>

There was a discussion with Stefan and few other in 2017 the outcome was to
use the explicit IEC standard (and avoid confusion with 1kHz i.e.).

https://en.m.wikipedia.org/wiki/Kibibyte
"The *kibibyte* is a multiple of the unit byte
<https://en.m.wikipedia.org/wiki/Byte> for quantities of digital information
<https://en.m.wikipedia.org/wiki/Information>. The binary prefix
<https://en.m.wikipedia.org/wiki/Binary_prefix> *kibi
<https://en.m.wikipedia.org/wiki/Kibi->* means 210, or 1024; therefore,
1 kibibyte is 1024 bytes. The unit symbol for the kibibyte is *KiB*."


> thanks
> -- PMM
>