[PATCH 0/4] hw/misc/a9scu: Verify CPU count is valid and simplify a bit

[PATCH 0/4] hw/misc/a9scu: Verify CPU count is valid and simplify a bit

[Philippe Mathieu-Daudé]

Trivial patches:
- verify the A9 CPU count is in range,
- simplify using MemoryRegionOps valid/impl,
- log unimplemented registers.

Philippe Mathieu-Daudé (4):
  hw/misc/a9scu: Do not allow invalid CPU count
  hw/misc/a9scu: Simplify setting MemoryRegionOps::valid fields
  hw/misc/a9scu: Simplify setting MemoryRegionOps::impl fields
  hw/misc/a9scu: Report unimplemented accesses with qemu_log_mask(UNIMP)

 hw/misc/a9scu.c | 59 +++++++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 31 deletions(-)

-- 
2.26.2

[PATCH 1/4] hw/misc/a9scu: Do not allow invalid CPU count

[Philippe Mathieu-Daudé]

Per the datasheet (DDI0407 r2p0):

  "The SCU connects one to four Cortex-A9 processors to
   the memory system through the AXI interfaces."

Change the instance_init() handler to a device_realize()
one so we can verify the property is in range, and return
an error to the caller if not.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/misc/a9scu.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/hw/misc/a9scu.c b/hw/misc/a9scu.c
index 324371a1c00..915f127761e 100644
--- a/hw/misc/a9scu.c
+++ b/hw/misc/a9scu.c
@@ -12,8 +12,11 @@
 #include "hw/misc/a9scu.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
+#include "qapi/error.h"
 #include "qemu/module.h"
 
+#define A9_SCU_CPU_MAX  4
+
 static uint64_t a9_scu_read(void *opaque, hwaddr offset,
                             unsigned size)
 {
@@ -105,12 +108,17 @@ static void a9_scu_reset(DeviceState *dev)
     s->control = 0;
 }
 
-static void a9_scu_init(Object *obj)
+static void a9_scu_realize(DeviceState *dev, Error **errp)
 {
-    A9SCUState *s = A9_SCU(obj);
-    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+    A9SCUState *s = A9_SCU(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
-    memory_region_init_io(&s->iomem, obj, &a9_scu_ops, s,
+    if (!s->num_cpu || s->num_cpu > A9_SCU_CPU_MAX) {
+        error_setg(errp, "Illegal CPU count: %u", s->num_cpu);
+        return;
+    }
+
+    memory_region_init_io(&s->iomem, OBJECT(s), &a9_scu_ops, s,
                           "a9-scu", 0x100);
     sysbus_init_mmio(sbd, &s->iomem);
 }
@@ -138,13 +146,13 @@ static void a9_scu_class_init(ObjectClass *klass, void *data)
     device_class_set_props(dc, a9_scu_properties);
     dc->vmsd = &vmstate_a9_scu;
     dc->reset = a9_scu_reset;
+    dc->realize = a9_scu_realize;
 }
 
 static const TypeInfo a9_scu_info = {
     .name          = TYPE_A9_SCU,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(A9SCUState),
-    .instance_init = a9_scu_init,
     .class_init    = a9_scu_class_init,
 };
 
-- 
2.26.2

[PATCH 2/4] hw/misc/a9scu: Simplify setting MemoryRegionOps::valid fields

[Philippe Mathieu-Daudé]

Per the datasheet (DDI0407 r2p0):

  "All SCU registers are byte accessible" and are 32-bit aligned.

Set MemoryRegionOps::valid min/max fields and simplify the write()
handler.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/misc/a9scu.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/hw/misc/a9scu.c b/hw/misc/a9scu.c
index 915f127761e..3f3dcc414fe 100644
--- a/hw/misc/a9scu.c
+++ b/hw/misc/a9scu.c
@@ -52,23 +52,8 @@ static void a9_scu_write(void *opaque, hwaddr offset,
                          uint64_t value, unsigned size)
 {
     A9SCUState *s = (A9SCUState *)opaque;
-    uint32_t mask;
+    uint32_t mask = MAKE_64BIT_MASK(0, size * 8);
     uint32_t shift;
-    switch (size) {
-    case 1:
-        mask = 0xff;
-        break;
-    case 2:
-        mask = 0xffff;
-        break;
-    case 4:
-        mask = 0xffffffff;
-        break;
-    default:
-        fprintf(stderr, "Invalid size %u in write to a9 scu register %x\n",
-                size, (unsigned)offset);
-        return;
-    }
 
     switch (offset) {
     case 0x00: /* Control */
@@ -99,6 +84,10 @@ static void a9_scu_write(void *opaque, hwaddr offset,
 static const MemoryRegionOps a9_scu_ops = {
     .read = a9_scu_read,
     .write = a9_scu_write,
+    .valid = {
+        .min_access_size = 1,
+        .max_access_size = 4,
+    },
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-- 
2.26.2

[PATCH 3/4] hw/misc/a9scu: Simplify setting MemoryRegionOps::impl fields

[Philippe Mathieu-Daudé]

This model implementation is designed for 32-bit accesses.
We can simplify setting the MemoryRegionOps::impl min/max
fields to 32-bit (memory::access_with_adjusted_size() will
take care of the 8/16-bit accesses).

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/misc/a9scu.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/hw/misc/a9scu.c b/hw/misc/a9scu.c
index 3f3dcc414fe..47f948341f7 100644
--- a/hw/misc/a9scu.c
+++ b/hw/misc/a9scu.c
@@ -28,12 +28,6 @@ static uint64_t a9_scu_read(void *opaque, hwaddr offset,
         return (((1 << s->num_cpu) - 1) << 4) | (s->num_cpu - 1);
     case 0x08: /* CPU Power Status */
         return s->status;
-    case 0x09: /* CPU status.  */
-        return s->status >> 8;
-    case 0x0a: /* CPU status.  */
-        return s->status >> 16;
-    case 0x0b: /* CPU status.  */
-        return s->status >> 24;
     case 0x0c: /* Invalidate All Registers In Secure State */
         return 0;
     case 0x40: /* Filtering Start Address Register */
@@ -52,8 +46,6 @@ static void a9_scu_write(void *opaque, hwaddr offset,
                          uint64_t value, unsigned size)
 {
     A9SCUState *s = (A9SCUState *)opaque;
-    uint32_t mask = MAKE_64BIT_MASK(0, size * 8);
-    uint32_t shift;
 
     switch (offset) {
     case 0x00: /* Control */
@@ -62,9 +54,7 @@ static void a9_scu_write(void *opaque, hwaddr offset,
     case 0x4: /* Configuration: RO */
         break;
     case 0x08: case 0x09: case 0x0A: case 0x0B: /* Power Control */
-        shift = (offset - 0x8) * 8;
-        s->status &= ~(mask << shift);
-        s->status |= ((value & mask) << shift);
+        s->status = value;
         break;
     case 0x0c: /* Invalidate All Registers In Secure State */
         /* no-op as we do not implement caches */
@@ -84,6 +74,10 @@ static void a9_scu_write(void *opaque, hwaddr offset,
 static const MemoryRegionOps a9_scu_ops = {
     .read = a9_scu_read,
     .write = a9_scu_write,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
     .valid = {
         .min_access_size = 1,
         .max_access_size = 4,
-- 
2.26.2

[PATCH 4/4] hw/misc/a9scu: Report unimplemented accesses with qemu_log_mask(UNIMP)

[Philippe Mathieu-Daudé]

Report unimplemented register accesses using qemu_log_mask(UNIMP).

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/misc/a9scu.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/misc/a9scu.c b/hw/misc/a9scu.c
index 47f948341f7..a375ebc9878 100644
--- a/hw/misc/a9scu.c
+++ b/hw/misc/a9scu.c
@@ -13,6 +13,7 @@
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 #include "qapi/error.h"
+#include "qemu/log.h"
 #include "qemu/module.h"
 
 #define A9_SCU_CPU_MAX  4
@@ -38,6 +39,8 @@ static uint64_t a9_scu_read(void *opaque, hwaddr offset,
     case 0x54: /* SCU Non-secure Access Control Register */
         /* unimplemented, fall through */
     default:
+        qemu_log_mask(LOG_UNIMP, "%s: Unsupported offset 0x%"HWADDR_PRIx"\n",
+                      __func__, offset);
         return 0;
     }
 }
@@ -67,6 +70,9 @@ static void a9_scu_write(void *opaque, hwaddr offset,
     case 0x54: /* SCU Non-secure Access Control Register */
         /* unimplemented, fall through */
     default:
+        qemu_log_mask(LOG_UNIMP, "%s: Unsupported offset 0x%"HWADDR_PRIx
+                                 " value 0x%"PRIx64"\n",
+                      __func__, offset, value);
         break;
     }
 }
-- 
2.26.2

Re: [PATCH 0/4] hw/misc/a9scu: Verify CPU count is valid and simplify a bit

[Richard Henderson]

On 9/1/20 7:40 AM, Philippe Mathieu-Daudé wrote:
> Trivial patches:
> - verify the A9 CPU count is in range,
> - simplify using MemoryRegionOps valid/impl,
> - log unimplemented registers.
> 
> Philippe Mathieu-Daudé (4):
>   hw/misc/a9scu: Do not allow invalid CPU count
>   hw/misc/a9scu: Simplify setting MemoryRegionOps::valid fields
>   hw/misc/a9scu: Simplify setting MemoryRegionOps::impl fields
>   hw/misc/a9scu: Report unimplemented accesses with qemu_log_mask(UNIMP)

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

r~

Re: [PATCH 0/4] hw/misc/a9scu: Verify CPU count is valid and simplify a bit

[Peter Maydell]

On Tue, 1 Sep 2020 at 15:41, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Trivial patches:
> - verify the A9 CPU count is in range,
> - simplify using MemoryRegionOps valid/impl,
> - log unimplemented registers.
>
> Philippe Mathieu-Daudé (4):
>   hw/misc/a9scu: Do not allow invalid CPU count
>   hw/misc/a9scu: Simplify setting MemoryRegionOps::valid fields
>   hw/misc/a9scu: Simplify setting MemoryRegionOps::impl fields
>   hw/misc/a9scu: Report unimplemented accesses with qemu_log_mask(UNIMP)
>
>  hw/misc/a9scu.c | 59 +++++++++++++++++++++++--------------------------
>  1 file changed, 28 insertions(+), 31 deletions(-)



Applied to target-arm.next, thanks.

-- PMM