arm: Don't crash if user tries to use a Cortex-M CPU without an NVIC

Message ID 20180601160355.15393-1-peter.maydell@linaro.org
State Superseded
Headers show
Series
  • arm: Don't crash if user tries to use a Cortex-M CPU without an NVIC
Related show

Commit Message

Peter Maydell June 1, 2018, 4:03 p.m.
The Cortex-M CPU and its NVIC are two intimately intertwined parts of
the same hardware; it is not possible to use one without the other.
Unfortunately a lot of our board models don't do any sanity checking
on the CPU type the user asks for, so a command line like
    qemu-system-arm -M versatilepb -cpu cortex-m3
will create an M3 without an NVIC, and coredump immediately.
In the other direction, trying a non-M-profile CPU in an M-profile
board won't blow up, but doesn't do anything useful either:
    qemu-system-arm -M lm3s6965evb -cpu arm926

Add some checking in the NVIC and CPU realize functions that the
user isn't trying to use an NVIC without an M-profile CPU or
an M-profile CPU without an NVIC, so we can produce a helpful
error message rather than a core dump.

Fixes: https://bugs.launchpad.net/qemu/+bug/1766896
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---
 hw/arm/armv7m.c       |  7 ++++++-
 hw/intc/armv7m_nvic.c |  6 +++++-
 target/arm/cpu.c      | 18 ++++++++++++++++++
 3 files changed, 29 insertions(+), 2 deletions(-)

-- 
2.17.1

Comments

Peter Maydell June 11, 2018, 2:05 p.m. | #1
Ping for code review?

thanks
-- PMM

On 1 June 2018 at 17:03, Peter Maydell <peter.maydell@linaro.org> wrote:
> The Cortex-M CPU and its NVIC are two intimately intertwined parts of

> the same hardware; it is not possible to use one without the other.

> Unfortunately a lot of our board models don't do any sanity checking

> on the CPU type the user asks for, so a command line like

>     qemu-system-arm -M versatilepb -cpu cortex-m3

> will create an M3 without an NVIC, and coredump immediately.

> In the other direction, trying a non-M-profile CPU in an M-profile

> board won't blow up, but doesn't do anything useful either:

>     qemu-system-arm -M lm3s6965evb -cpu arm926

>

> Add some checking in the NVIC and CPU realize functions that the

> user isn't trying to use an NVIC without an M-profile CPU or

> an M-profile CPU without an NVIC, so we can produce a helpful

> error message rather than a core dump.

>

> Fixes: https://bugs.launchpad.net/qemu/+bug/1766896

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> ---

>  hw/arm/armv7m.c       |  7 ++++++-

>  hw/intc/armv7m_nvic.c |  6 +++++-

>  target/arm/cpu.c      | 18 ++++++++++++++++++

>  3 files changed, 29 insertions(+), 2 deletions(-)

>

> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c

> index a4ab7d2069..9e00d4037c 100644

> --- a/hw/arm/armv7m.c

> +++ b/hw/arm/armv7m.c

> @@ -178,6 +178,12 @@ static void armv7m_realize(DeviceState *dev, Error **errp)

>              return;

>          }

>      }

> +

> +    /* Tell the CPU where the NVIC is; it will fail realize if it doesn't

> +     * have one.

> +     */

> +    s->cpu->env.nvic = &s->nvic;

> +

>      object_property_set_bool(OBJECT(s->cpu), true, "realized", &err);

>      if (err != NULL) {

>          error_propagate(errp, err);

> @@ -202,7 +208,6 @@ static void armv7m_realize(DeviceState *dev, Error **errp)

>      sbd = SYS_BUS_DEVICE(&s->nvic);

>      sysbus_connect_irq(sbd, 0,

>                         qdev_get_gpio_in(DEVICE(s->cpu), ARM_CPU_IRQ));

> -    s->cpu->env.nvic = &s->nvic;

>

>      memory_region_add_subregion(&s->container, 0xe000e000,

>                                  sysbus_mmio_get_region(sbd, 0));

> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c

> index c51151fa8a..661be8878a 100644

> --- a/hw/intc/armv7m_nvic.c

> +++ b/hw/intc/armv7m_nvic.c

> @@ -2183,7 +2183,11 @@ static void armv7m_nvic_realize(DeviceState *dev, Error **errp)

>      int regionlen;

>

>      s->cpu = ARM_CPU(qemu_get_cpu(0));

> -    assert(s->cpu);

> +

> +    if (!s->cpu || !arm_feature(&s->cpu->env, ARM_FEATURE_M)) {

> +        error_setg(errp, "The NVIC can only be used with a Cortex-M CPU");

> +        return;

> +    }

>

>      if (s->num_irq > NVIC_MAX_IRQ) {

>          error_setg(errp, "num-irq %d exceeds NVIC maximum", s->num_irq);

> diff --git a/target/arm/cpu.c b/target/arm/cpu.c

> index 5d60893a07..eda1ce14fc 100644

> --- a/target/arm/cpu.c

> +++ b/target/arm/cpu.c

> @@ -767,6 +767,24 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)

>          return;

>      }

>

> +#ifndef CONFIG_USER_ONLY

> +    /* The NVIC and M-profile CPU are two halves of a single piece of

> +     * hardware; trying to use one without the other is a command line

> +     * error and will result in segfaults if not caught here.

> +     */

> +    if (arm_feature(env, ARM_FEATURE_M)) {

> +        if (!env->nvic) {

> +            error_setg(errp, "This board cannot be used with Cortex-M CPUs");

> +            return;

> +        }

> +    } else {

> +        if (env->nvic) {

> +            error_setg(errp, "This board can only be used with Cortex-M CPUs");

> +            return;

> +        }

> +    }

> +#endif

> +

>      cpu_exec_realizefn(cs, &local_err);

>      if (local_err != NULL) {

>          error_propagate(errp, local_err);

> --

> 2.17.1

>

>
Philippe Mathieu-Daudé June 11, 2018, 3:19 p.m. | #2
On 06/11/2018 11:05 AM, Peter Maydell wrote:
> Ping for code review?

> 

> thanks

> -- PMM

> 

> On 1 June 2018 at 17:03, Peter Maydell <peter.maydell@linaro.org> wrote:

>> The Cortex-M CPU and its NVIC are two intimately intertwined parts of

>> the same hardware; it is not possible to use one without the other.

>> Unfortunately a lot of our board models don't do any sanity checking

>> on the CPU type the user asks for, so a command line like

>>     qemu-system-arm -M versatilepb -cpu cortex-m3

>> will create an M3 without an NVIC, and coredump immediately.

>> In the other direction, trying a non-M-profile CPU in an M-profile

>> board won't blow up, but doesn't do anything useful either:

>>     qemu-system-arm -M lm3s6965evb -cpu arm926

>>

>> Add some checking in the NVIC and CPU realize functions that the

>> user isn't trying to use an NVIC without an M-profile CPU or

>> an M-profile CPU without an NVIC, so we can produce a helpful

>> error message rather than a core dump.

>>

>> Fixes: https://bugs.launchpad.net/qemu/+bug/1766896

>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

>> ---

>>  hw/arm/armv7m.c       |  7 ++++++-

>>  hw/intc/armv7m_nvic.c |  6 +++++-

>>  target/arm/cpu.c      | 18 ++++++++++++++++++

>>  3 files changed, 29 insertions(+), 2 deletions(-)

>>

>> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c

>> index a4ab7d2069..9e00d4037c 100644

>> --- a/hw/arm/armv7m.c

>> +++ b/hw/arm/armv7m.c

>> @@ -178,6 +178,12 @@ static void armv7m_realize(DeviceState *dev, Error **errp)

>>              return;

>>          }

>>      }

>> +

>> +    /* Tell the CPU where the NVIC is; it will fail realize if it doesn't

>> +     * have one.

>> +     */

>> +    s->cpu->env.nvic = &s->nvic;

>> +

>>      object_property_set_bool(OBJECT(s->cpu), true, "realized", &err);

>>      if (err != NULL) {

>>          error_propagate(errp, err);

>> @@ -202,7 +208,6 @@ static void armv7m_realize(DeviceState *dev, Error **errp)

>>      sbd = SYS_BUS_DEVICE(&s->nvic);

>>      sysbus_connect_irq(sbd, 0,

>>                         qdev_get_gpio_in(DEVICE(s->cpu), ARM_CPU_IRQ));

>> -    s->cpu->env.nvic = &s->nvic;

>>

>>      memory_region_add_subregion(&s->container, 0xe000e000,

>>                                  sysbus_mmio_get_region(sbd, 0));

>> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c

>> index c51151fa8a..661be8878a 100644

>> --- a/hw/intc/armv7m_nvic.c

>> +++ b/hw/intc/armv7m_nvic.c

>> @@ -2183,7 +2183,11 @@ static void armv7m_nvic_realize(DeviceState *dev, Error **errp)

>>      int regionlen;

>>

>>      s->cpu = ARM_CPU(qemu_get_cpu(0));

>> -    assert(s->cpu);

>> +

>> +    if (!s->cpu || !arm_feature(&s->cpu->env, ARM_FEATURE_M)) {

>> +        error_setg(errp, "The NVIC can only be used with a Cortex-M CPU");

>> +        return;

>> +    }

>>

>>      if (s->num_irq > NVIC_MAX_IRQ) {

>>          error_setg(errp, "num-irq %d exceeds NVIC maximum", s->num_irq);

>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c

>> index 5d60893a07..eda1ce14fc 100644

>> --- a/target/arm/cpu.c

>> +++ b/target/arm/cpu.c

>> @@ -767,6 +767,24 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)

>>          return;

>>      }

>>

>> +#ifndef CONFIG_USER_ONLY

>> +    /* The NVIC and M-profile CPU are two halves of a single piece of

>> +     * hardware; trying to use one without the other is a command line

>> +     * error and will result in segfaults if not caught here.

>> +     */

>> +    if (arm_feature(env, ARM_FEATURE_M)) {

>> +        if (!env->nvic) {

>> +            error_setg(errp, "This board cannot be used with Cortex-M CPUs");

>> +            return;

>> +        }

>> +    } else {

>> +        if (env->nvic) {

>> +            error_setg(errp, "This board can only be used with Cortex-M CPUs");

>> +            return;

>> +        }

>> +    }

>> +#endif

>> +

>>      cpu_exec_realizefn(cs, &local_err);

>>      if (local_err != NULL) {

>>          error_propagate(errp, local_err);

>> --

>> 2.17.1

>>

>>


Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Igor Mammedov June 11, 2018, 3:26 p.m. | #3
On Fri,  1 Jun 2018 17:03:55 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> The Cortex-M CPU and its NVIC are two intimately intertwined parts of

> the same hardware; it is not possible to use one without the other.

> Unfortunately a lot of our board models don't do any sanity checking

> on the CPU type the user asks for, so a command line like

>     qemu-system-arm -M versatilepb -cpu cortex-m3

> will create an M3 without an NVIC, and coredump immediately.

> In the other direction, trying a non-M-profile CPU in an M-profile

> board won't blow up, but doesn't do anything useful either:

>     qemu-system-arm -M lm3s6965evb -cpu arm926

> 

> Add some checking in the NVIC and CPU realize functions that the

> user isn't trying to use an NVIC without an M-profile CPU or

> an M-profile CPU without an NVIC, so we can produce a helpful

> error message rather than a core dump.

> 

> Fixes: https://bugs.launchpad.net/qemu/+bug/1766896

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> ---

>  hw/arm/armv7m.c       |  7 ++++++-

>  hw/intc/armv7m_nvic.c |  6 +++++-

>  target/arm/cpu.c      | 18 ++++++++++++++++++

>  3 files changed, 29 insertions(+), 2 deletions(-)

> 

> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c

> index a4ab7d2069..9e00d4037c 100644

> --- a/hw/arm/armv7m.c

> +++ b/hw/arm/armv7m.c

> @@ -178,6 +178,12 @@ static void armv7m_realize(DeviceState *dev, Error **errp)

>              return;

>          }

>      }

> +

> +    /* Tell the CPU where the NVIC is; it will fail realize if it doesn't

> +     * have one.

> +     */

> +    s->cpu->env.nvic = &s->nvic;

> +

>      object_property_set_bool(OBJECT(s->cpu), true, "realized", &err);

>      if (err != NULL) {

>          error_propagate(errp, err);

> @@ -202,7 +208,6 @@ static void armv7m_realize(DeviceState *dev, Error **errp)

>      sbd = SYS_BUS_DEVICE(&s->nvic);

>      sysbus_connect_irq(sbd, 0,

>                         qdev_get_gpio_in(DEVICE(s->cpu), ARM_CPU_IRQ));

> -    s->cpu->env.nvic = &s->nvic;

>  

>      memory_region_add_subregion(&s->container, 0xe000e000,

>                                  sysbus_mmio_get_region(sbd, 0));

> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c

> index c51151fa8a..661be8878a 100644

> --- a/hw/intc/armv7m_nvic.c

> +++ b/hw/intc/armv7m_nvic.c

> @@ -2183,7 +2183,11 @@ static void armv7m_nvic_realize(DeviceState *dev, Error **errp)

>      int regionlen;

>  

>      s->cpu = ARM_CPU(qemu_get_cpu(0));

> -    assert(s->cpu);

> +

> +    if (!s->cpu || !arm_feature(&s->cpu->env, ARM_FEATURE_M)) {

> +        error_setg(errp, "The NVIC can only be used with a Cortex-M CPU");

> +        return;

> +    }

>  

>      if (s->num_irq > NVIC_MAX_IRQ) {

>          error_setg(errp, "num-irq %d exceeds NVIC maximum", s->num_irq);

> diff --git a/target/arm/cpu.c b/target/arm/cpu.c

> index 5d60893a07..eda1ce14fc 100644

> --- a/target/arm/cpu.c

> +++ b/target/arm/cpu.c

> @@ -767,6 +767,24 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)

>          return;

>      }

>  

> +#ifndef CONFIG_USER_ONLY

> +    /* The NVIC and M-profile CPU are two halves of a single piece of

> +     * hardware; trying to use one without the other is a command line

> +     * error and will result in segfaults if not caught here.

> +     */

> +    if (arm_feature(env, ARM_FEATURE_M)) {

> +        if (!env->nvic) {

> +            error_setg(errp, "This board cannot be used with Cortex-M CPUs");

mentioning board in CPU's realize seems a little bit strange,
maybe something similar to following would be better:

  "Cortex-M CPUs require device_foo for working"

> +            return;

> +        }

> +    } else {

> +        if (env->nvic) {

> +            error_setg(errp, "This board can only be used with Cortex-M CPUs");

> +            return;

> +        }

> +    }

> +#endif

> +

>      cpu_exec_realizefn(cs, &local_err);

>      if (local_err != NULL) {

>          error_propagate(errp, local_err);
Peter Maydell June 11, 2018, 3:33 p.m. | #4
On 11 June 2018 at 16:26, Igor Mammedov <imammedo@redhat.com> wrote:
> On Fri,  1 Jun 2018 17:03:55 +0100

> Peter Maydell <peter.maydell@linaro.org> wrote:

>

>> The Cortex-M CPU and its NVIC are two intimately intertwined parts of

>> the same hardware; it is not possible to use one without the other.

>> Unfortunately a lot of our board models don't do any sanity checking

>> on the CPU type the user asks for, so a command line like

>>     qemu-system-arm -M versatilepb -cpu cortex-m3

>> will create an M3 without an NVIC, and coredump immediately.

>> In the other direction, trying a non-M-profile CPU in an M-profile

>> board won't blow up, but doesn't do anything useful either:

>>     qemu-system-arm -M lm3s6965evb -cpu arm926


>> +#ifndef CONFIG_USER_ONLY

>> +    /* The NVIC and M-profile CPU are two halves of a single piece of

>> +     * hardware; trying to use one without the other is a command line

>> +     * error and will result in segfaults if not caught here.

>> +     */

>> +    if (arm_feature(env, ARM_FEATURE_M)) {

>> +        if (!env->nvic) {

>> +            error_setg(errp, "This board cannot be used with Cortex-M CPUs");

> mentioning board in CPU's realize seems a little bit strange,

> maybe something similar to following would be better:

>

>   "Cortex-M CPUs require device_foo for working"


I think the text as I have it is of more practical use to the
user, who is clearly not going to be very familiar with how
M-profile systems work if they've run into this error message.
"requires an NVIC" doesn't tell them what's actually wrong;
"this board doesn't work with this CPU" gives them a fighting
chance of understanding that they've given two incompatible
command line options.

thanks
-- PMM

Patch

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index a4ab7d2069..9e00d4037c 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -178,6 +178,12 @@  static void armv7m_realize(DeviceState *dev, Error **errp)
             return;
         }
     }
+
+    /* Tell the CPU where the NVIC is; it will fail realize if it doesn't
+     * have one.
+     */
+    s->cpu->env.nvic = &s->nvic;
+
     object_property_set_bool(OBJECT(s->cpu), true, "realized", &err);
     if (err != NULL) {
         error_propagate(errp, err);
@@ -202,7 +208,6 @@  static void armv7m_realize(DeviceState *dev, Error **errp)
     sbd = SYS_BUS_DEVICE(&s->nvic);
     sysbus_connect_irq(sbd, 0,
                        qdev_get_gpio_in(DEVICE(s->cpu), ARM_CPU_IRQ));
-    s->cpu->env.nvic = &s->nvic;
 
     memory_region_add_subregion(&s->container, 0xe000e000,
                                 sysbus_mmio_get_region(sbd, 0));
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index c51151fa8a..661be8878a 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -2183,7 +2183,11 @@  static void armv7m_nvic_realize(DeviceState *dev, Error **errp)
     int regionlen;
 
     s->cpu = ARM_CPU(qemu_get_cpu(0));
-    assert(s->cpu);
+
+    if (!s->cpu || !arm_feature(&s->cpu->env, ARM_FEATURE_M)) {
+        error_setg(errp, "The NVIC can only be used with a Cortex-M CPU");
+        return;
+    }
 
     if (s->num_irq > NVIC_MAX_IRQ) {
         error_setg(errp, "num-irq %d exceeds NVIC maximum", s->num_irq);
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 5d60893a07..eda1ce14fc 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -767,6 +767,24 @@  static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
+#ifndef CONFIG_USER_ONLY
+    /* The NVIC and M-profile CPU are two halves of a single piece of
+     * hardware; trying to use one without the other is a command line
+     * error and will result in segfaults if not caught here.
+     */
+    if (arm_feature(env, ARM_FEATURE_M)) {
+        if (!env->nvic) {
+            error_setg(errp, "This board cannot be used with Cortex-M CPUs");
+            return;
+        }
+    } else {
+        if (env->nvic) {
+            error_setg(errp, "This board can only be used with Cortex-M CPUs");
+            return;
+        }
+    }
+#endif
+
     cpu_exec_realizefn(cs, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);