Re: [Qemu-arm] [PATCH 4/4] hw/arm: Correctly disable FPU/DSP for some ARMSSE-based boards

[Alex Bennée]

Peter Maydell <address@hidden> writes:

> The SSE-200 hardware has configurable integration settings which
> determine whether its two CPUs have the FPU and DSP:
>  * CPU0_FPU (default 0)
>  * CPU0_DSP (default 0)
>  * CPU1_FPU (default 1)
>  * CPU1_DSP (default 1)
>
> Similarly, the IoTKit has settings for its single CPU:
>  * CPU0_FPU (default 1)
>  * CPU0_DSP (default 1)
>
> Of our four boards that use either the IoTKit or the SSE-200:
>  * mps2-an505, mps2-an521 and musca-a use the default settings
>  * musca-b1 enables FPU and DSP on both CPUs
>
> Currently QEMU models all these boards using CPUs with
> both FPU and DSP enabled. This means that we are incorrect
> for mps2-an521 and musca-a, which should not have FPU or DSP
> on CPU0.
>
> Create QOM properties on the ARMSSE devices corresponding to the
> default h/w integration settings, and make the Musca-B1 board
> enable FPU and DSP on both CPUs. This fixes the mps2-an521
> and musca-a behaviour, and leaves the musca-b1 and mps2-an505
> behaviour unchanged.
>
> Signed-off-by: Peter Maydell <address@hidden>

Given the ever growing configurable nature of the v7m platform what do we do
to ensure the various combinations are tested on instantiating qemu? Or is
this a case of relying on the wider community to shout if users actually
find a combination that breaks? I guess fuzz testing would be a bit of a
sledgehammer approach.

Anyway:

Reviewed-by: Alex Bennée <address@hidden>

> ---
>  include/hw/arm/armsse.h |  7 +++++
>  hw/arm/armsse.c         | 58 ++++++++++++++++++++++++++++++++---------
>  hw/arm/musca.c          |  8 ++++++
>  3 files changed, 61 insertions(+), 12 deletions(-)
>
> diff --git a/include/hw/arm/armsse.h b/include/hw/arm/armsse.h
> index 81e082cccf8..84080c22993 100644
> --- a/include/hw/arm/armsse.h
> +++ b/include/hw/arm/armsse.h
> @@ -50,6 +50,11 @@
>   *    address of each SRAM bank (and thus the total amount of internal SRAM)
>   *  + QOM property "init-svtor" sets the initial value of the CPU SVTOR 
> register
>   *    (where it expects to load the PC and SP from the vector table on reset)
> + *  + QOM properties "CPU0_FPU", "CPU0_DSP", "CPU1_FPU" and "CPU1_DSP" which
> + *    set whether the CPUs have the FPU and DSP features present. The default
> + *    (matching the hardware) is that for CPU0 in an IoTKit and CPU1 in an
> + *    SSE-200 both are present; CPU0 in an SSE-200 has neither.
> + *    Since the IoTKit has only one CPU, it does not have the CPU1_* 
> properties.
>   *  + Named GPIO inputs "EXP_IRQ" 0..n are the expansion interrupts for CPU 
> 0,
>   *    which are wired to its NVIC lines 32 .. n+32
>   *  + Named GPIO inputs "EXP_CPU1_IRQ" 0..n are the expansion interrupts for
> @@ -208,6 +213,8 @@ typedef struct ARMSSE {
>      uint32_t mainclk_frq;
>      uint32_t sram_addr_width;
>      uint32_t init_svtor;
> +    bool cpu_fpu[SSE_MAX_CPUS];
> +    bool cpu_dsp[SSE_MAX_CPUS];
>  } ARMSSE;
>
>  typedef struct ARMSSEInfo ARMSSEInfo;
> diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c
> index 76cc6905798..e138aee673f 100644
> --- a/hw/arm/armsse.c
> +++ b/hw/arm/armsse.c
> @@ -37,6 +37,33 @@ struct ARMSSEInfo {
>      bool has_cachectrl;
>      bool has_cpusecctrl;
>      bool has_cpuid;
> +    Property *props;
> +};
> +
> +static Property iotkit_properties[] = {
> +    DEFINE_PROP_LINK("memory", ARMSSE, board_memory, TYPE_MEMORY_REGION,
> +                     MemoryRegion *),
> +    DEFINE_PROP_UINT32("EXP_NUMIRQ", ARMSSE, exp_numirq, 64),
> +    DEFINE_PROP_UINT32("MAINCLK", ARMSSE, mainclk_frq, 0),
> +    DEFINE_PROP_UINT32("SRAM_ADDR_WIDTH", ARMSSE, sram_addr_width, 15),
> +    DEFINE_PROP_UINT32("init-svtor", ARMSSE, init_svtor, 0x10000000),
> +    DEFINE_PROP_BOOL("CPU0_FPU", ARMSSE, cpu_fpu[0], true),
> +    DEFINE_PROP_BOOL("CPU0_DSP", ARMSSE, cpu_dsp[0], true),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
> +static Property armsse_properties[] = {
> +    DEFINE_PROP_LINK("memory", ARMSSE, board_memory, TYPE_MEMORY_REGION,
> +                     MemoryRegion *),
> +    DEFINE_PROP_UINT32("EXP_NUMIRQ", ARMSSE, exp_numirq, 64),
> +    DEFINE_PROP_UINT32("MAINCLK", ARMSSE, mainclk_frq, 0),
> +    DEFINE_PROP_UINT32("SRAM_ADDR_WIDTH", ARMSSE, sram_addr_width, 15),
> +    DEFINE_PROP_UINT32("init-svtor", ARMSSE, init_svtor, 0x10000000),
> +    DEFINE_PROP_BOOL("CPU0_FPU", ARMSSE, cpu_fpu[0], false),
> +    DEFINE_PROP_BOOL("CPU0_DSP", ARMSSE, cpu_dsp[0], false),
> +    DEFINE_PROP_BOOL("CPU1_FPU", ARMSSE, cpu_fpu[1], true),
> +    DEFINE_PROP_BOOL("CPU1_DSP", ARMSSE, cpu_dsp[1], true),
> +    DEFINE_PROP_END_OF_LIST()
>  };
>
>  static const ARMSSEInfo armsse_variants[] = {
> @@ -52,6 +79,7 @@ static const ARMSSEInfo armsse_variants[] = {
>          .has_cachectrl = false,
>          .has_cpusecctrl = false,
>          .has_cpuid = false,
> +        .props = iotkit_properties,
>      },
>      {
>          .name = TYPE_SSE200,
> @@ -65,6 +93,7 @@ static const ARMSSEInfo armsse_variants[] = {
>          .has_cachectrl = true,
>          .has_cpusecctrl = true,
>          .has_cpuid = true,
> +        .props = armsse_properties,
>      },
>  };
>
> @@ -532,6 +561,20 @@ static void armsse_realize(DeviceState *dev, Error 
> **errp)
>                  return;
>              }
>          }
> +        if (!s->cpu_fpu[i]) {
> +            object_property_set_bool(cpuobj, false, "vfp", &err);
> +            if (err) {
> +                error_propagate(errp, err);
> +                return;
> +            }
> +        }
> +        if (!s->cpu_dsp[i]) {
> +            object_property_set_bool(cpuobj, false, "dsp", &err);
> +            if (err) {
> +                error_propagate(errp, err);
> +                return;
> +            }
> +        }
>
>          if (i > 0) {
>              memory_region_add_subregion_overlap(&s->cpu_container[i], 0,
> @@ -1221,16 +1264,6 @@ static const VMStateDescription armsse_vmstate = {
>      }
>  };
>
> -static Property armsse_properties[] = {
> -    DEFINE_PROP_LINK("memory", ARMSSE, board_memory, TYPE_MEMORY_REGION,
> -                     MemoryRegion *),
> -    DEFINE_PROP_UINT32("EXP_NUMIRQ", ARMSSE, exp_numirq, 64),
> -    DEFINE_PROP_UINT32("MAINCLK", ARMSSE, mainclk_frq, 0),
> -    DEFINE_PROP_UINT32("SRAM_ADDR_WIDTH", ARMSSE, sram_addr_width, 15),
> -    DEFINE_PROP_UINT32("init-svtor", ARMSSE, init_svtor, 0x10000000),
> -    DEFINE_PROP_END_OF_LIST()
> -};
> -
>  static void armsse_reset(DeviceState *dev)
>  {
>      ARMSSE *s = ARMSSE(dev);
> @@ -1243,13 +1276,14 @@ static void armsse_class_init(ObjectClass *klass, 
> void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      IDAUInterfaceClass *iic = IDAU_INTERFACE_CLASS(klass);
>      ARMSSEClass *asc = ARMSSE_CLASS(klass);
> +    const ARMSSEInfo *info = data;
>
>      dc->realize = armsse_realize;
>      dc->vmsd = &armsse_vmstate;
> -    dc->props = armsse_properties;
> +    dc->props = info->props;
>      dc->reset = armsse_reset;
>      iic->check = armsse_idau_check;
> -    asc->info = data;
> +    asc->info = info;
>  }
>
>  static const TypeInfo armsse_info = {
> diff --git a/hw/arm/musca.c b/hw/arm/musca.c
> index 23aff43f4bc..736f37b774c 100644
> --- a/hw/arm/musca.c
> +++ b/hw/arm/musca.c
> @@ -385,6 +385,14 @@ static void musca_init(MachineState *machine)
>      qdev_prop_set_uint32(ssedev, "init-svtor", mmc->init_svtor);
>      qdev_prop_set_uint32(ssedev, "SRAM_ADDR_WIDTH", mmc->sram_addr_width);
>      qdev_prop_set_uint32(ssedev, "MAINCLK", SYSCLK_FRQ);
> +    /*
> +     * Musca-A takes the default SSE-200 FPU/DSP settings (ie no for
> +     * CPU0 and yes for CPU1); Musca-B1 explicitly enables them for CPU0.
> +     */
> +    if (mmc->type == MUSCA_B1) {
> +        qdev_prop_set_bit(ssedev, "CPU0_FPU", true);
> +        qdev_prop_set_bit(ssedev, "CPU0_DSP", true);
> +    }
>      object_property_set_bool(OBJECT(&mms->sse), true, "realized",
>                               &error_fatal);


--
Alex Bennée