Re: [PATCH 2/4] hw/arm/armsse: add Arm Corstone SSE-310

On Fri, 24 Jul 2026 at 00:36, Simon Xu <[email protected]> wrote:
>
> Add Arm Corstone SSE-310 model.
> - Include new NPU power policy unit and configuration interface in device
> list as unimplemented devices.
> - sys_version and iidr values from the "SSE-310 with M85 and U55 FPGA"
> documentation for the mps3-an555.
> - Refractor comments and code pertaining to both the SSE-300 and SSE-310.
> - Add support for SSE-310 in the iotkit files.
> - Add SSE-310 PIDR and CIDR registers according to the SSE-310 TRM.
>
> Arm Corstone SSE-310 TRM:
> https://developer.arm.com/documentation/102778/0000/
>
> Reviewed-by: Owen Giles <[email protected]>
> Reviewed-by: Robert Elliott <[email protected]>
> Signed-off-by: Simon Xu <[email protected]>
> ---
>  hw/arm/armsse.c                 | 208 +++++++++++++++++++++++++++++++-
>  hw/misc/iotkit-secctl.c         |  18 +++
>  hw/misc/iotkit-sysctl.c         |  60 +++++++--
>  hw/misc/iotkit-sysinfo.c        |  12 +-
>  include/hw/arm/armsse-version.h |   2 +
>  include/hw/arm/armsse.h         |   1 +
>  6 files changed, 287 insertions(+), 14 deletions(-)

I would prefer if we split this patch up into smaller pieces.
Adding the secctl handling, the sysctl handling, sysinfo, and
the main armsse support itself are all pieces that are easier to
review separately. If you'd split this patch up I would have
been able to mark some of the subpatches as reviewed already.

> diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c
> index ddb210c895..1847c73322 100644
> --- a/hw/arm/armsse.c
> +++ b/hw/arm/armsse.c
> @@ -26,7 +26,7 @@
>  #include "hw/core/qdev-clock.h"
>
>  /*
> - * The SSE-300 puts some devices in different places to the
> + * The SSE-300 and SSE-310 put some devices in different places to the
>   * SSE-200 (and original IoTKit). We use an array of these structs
>   * to define how each variant lays out these devices. (Parts of the
>   * SoC that are the same for all variants aren't handled via these
> @@ -118,6 +118,18 @@ static const Property sse300_properties[] = {
>      DEFINE_PROP_UINT32("CPU0_MPU_S", ARMSSE, cpu_mpu_s[0], 8),
>  };
>
> +static const Property sse310_properties[] = {
> +    DEFINE_PROP_LINK("memory", ARMSSE, board_memory, TYPE_MEMORY_REGION,
> +                     MemoryRegion *),
> +    DEFINE_PROP_UINT32("EXP_NUMIRQ", ARMSSE, exp_numirq, 64),
> +    DEFINE_PROP_UINT32("SRAM_ADDR_WIDTH", ARMSSE, sram_addr_width, 21),
> +    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_UINT32("CPU0_MPU_NS", ARMSSE, cpu_mpu_ns[0], 8),
> +    DEFINE_PROP_UINT32("CPU0_MPU_S", ARMSSE, cpu_mpu_s[0], 8),
> +};
> +
>  static const ARMSSEDeviceInfo iotkit_devices[] = {
>      {
>          .name = "timer0",
> @@ -480,6 +492,146 @@ static const ARMSSEDeviceInfo sse300_devices[] = {
>      }
>  };
>
> +static const ARMSSEDeviceInfo sse310_devices[] = {
> +    {

This is a very long block of data about the SSE310 devices
which is almost but not quite identical to the existing one
for SSE300. It would be easier to review if your commit message
stated which devices are new in the SSE310 memory map and
which have moved compared to the SSE300.

> +        .name = "timer0",
> +        .type = TYPE_SSE_TIMER,
> +        .index = 0,
> +        .addr = 0x48000000,
> +        .ppc = 0,
> +        .ppc_port = 0,
> +        .irq = 3,
> +    },

> +    {
> +        .name = "s32ktimer",
> +        .type = TYPE_CMSDK_APB_TIMER,
> +        .index = 0,
> +        .addr = 0x4802f000,
> +        .ppc = 1,
> +        .ppc_port = 0,
> +        .irq = 2,
> +        .slowclk = true,
> +    },
> +    {
> +        .name = "s32kwatchdog",
> +        .type = TYPE_CMSDK_APB_WATCHDOG,
> +        .index = 0,
> +        .addr = 0x5802e000,

This is different to what we set for SSE-300, and it looks
like that's a bug in the SSE-300 implementation. This is
the "SLOWCLK Watchdog", and both the SSE-300 and SSE-310
docs list it as being only in the Secure access region
(0x58....), and not in the NS region (0x48...). We should
fix the SSE-300 bug.

> +        .ppc = NO_PPC,
> +        .irq = NMI_0,
> +        .slowclk = true,
> +    },

> diff --git a/hw/misc/iotkit-sysctl.c b/hw/misc/iotkit-sysctl.c
> index dff89c677f..e6b1fe5d32 100644
> --- a/hw/misc/iotkit-sysctl.c
> +++ b/hw/misc/iotkit-sysctl.c

> @@ -193,6 +204,7 @@ static uint64_t iotkit_sysctl_read(void *opaque, hwaddr offset,
>              break;
>          case ARMSSE_SSE300:
>              /* In SSE300 this is reserved (for INITSVTOR2) */
> +        case ARMSSE_SSE310:

For this and the others similar, you should put the two "case" lines
together, and update the comment.

>              goto bad_offset;
>          default:
>              g_assert_not_reached();

As well as all the registers that are the same as SSE300, the
SSE310 has one extra register in the sysctl block: PPUINTSTAT, at
offset 0x128. We don't implement the PPUs in QEMU, but we should at
least implement this register as RAZ with a LOG_UNIMP log if the guest
tries to read it.

> diff --git a/hw/misc/iotkit-sysinfo.c b/hw/misc/iotkit-sysinfo.c
> index 19f089e6ee..21e161bdc2 100644
> --- a/hw/misc/iotkit-sysinfo.c
> +++ b/hw/misc/iotkit-sysinfo.c
> @@ -58,6 +58,12 @@ static const int sysinfo_sse300_id[] = {
>      0x0d, 0xf0, 0x05, 0xb1, /* CID0..CID3 */
>  };
>
> +static const int sysinfo_sse310_id[] = {
> +    0x04, 0x00, 0x00, 0x00, /* PID4..PID7 */
> +    0x58, 0xb8, 0x2b, 0x00, /* PID0..PID3 */
> +    0x0d, 0xf0, 0x05, 0xb1, /* CID0..CID3 */
> +};
> +
>  static uint64_t iotkit_sysinfo_read(void *opaque, hwaddr offset,
>                                      unsigned size)
>  {
> @@ -68,13 +74,13 @@ static uint64_t iotkit_sysinfo_read(void *opaque, hwaddr offset,
>      case A_SYS_VERSION:
>          r = s->sys_version;
>          break;
> -
>      case A_SYS_CONFIG:
>          r = s->sys_config;
>          break;
>      case A_SYS_CONFIG1:
>          switch (s->sse_version) {
>          case ARMSSE_SSE300:
> +        case ARMSSE_SSE310:
>              return 0;
>              break;
>          default:
> @@ -84,6 +90,7 @@ static uint64_t iotkit_sysinfo_read(void *opaque, hwaddr offset,
>      case A_IIDR:
>          switch (s->sse_version) {
>          case ARMSSE_SSE300:
> +        case ARMSSE_SSE310:
>              return s->iidr;
>              break;
>          default:
> @@ -95,6 +102,9 @@ static uint64_t iotkit_sysinfo_read(void *opaque, hwaddr offset,
>          case ARMSSE_SSE300:
>              r = sysinfo_sse300_id[(offset - A_PID4) / 4];
>              break;
> +        case ARMSSE_SSE310:
> +            r = sysinfo_sse310_id[(offset - A_PID4) / 4];
> +            break;
>          default:
>              r = sysinfo_id[(offset - A_PID4) / 4];
>              break;

As well as these changes to sysinfo, which are all fine, the SSE310
has an extra SYS_CONFIG2 register which the SSE300 does not. We
can hardwire it to "return 0" like we do for SYS_CONFIG1,
because the only interesting field in it is "is there an NPU
present", which for us is always "no".

> diff --git a/include/hw/arm/armsse.h b/include/hw/arm/armsse.h
> index bdf2d4db8e..d0dcba1fbb 100644
> --- a/include/hw/arm/armsse.h
> +++ b/include/hw/arm/armsse.h
> @@ -127,6 +127,7 @@ OBJECT_DECLARE_TYPE(ARMSSE, ARMSSEClass,
>  #define TYPE_IOTKIT "iotkit"
>  #define TYPE_SSE200 "sse-200"
>  #define TYPE_SSE300 "sse-300"
> +#define TYPE_SSE310 "sse-310"

There's a comment at the top of this file starting "Currently we model:"
which documents what we implement. We should add SSE-310 to that.
(I forgot to do this for SSE-300, unfortunately.)

thanks
-- PMM