[PATCH] hw/misc/max78000_gcr: Do not ignore address_space_write() errors

[PATCH] hw/misc/max78000_gcr: Do not ignore address_space_write() errors

[Philippe Mathieu-Daudé]

Convert max78000_gcr_ops[] to take MemTxAttrs and return MemTxResult,
allowing the write() path to return error on failure.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Cc: Jackson Donaldson <jcksn@duck.com>
Cc: Jackson Donaldson <jackson88044@gmail.com>
---
 hw/misc/max78000_gcr.c | 95 +++++++++++++++++++++++++++---------------
 1 file changed, 61 insertions(+), 34 deletions(-)

diff --git a/hw/misc/max78000_gcr.c b/hw/misc/max78000_gcr.c
index fbbc92cca32..2d1d46cc26d 100644
--- a/hw/misc/max78000_gcr.c
+++ b/hw/misc/max78000_gcr.c
@@ -44,80 +44,106 @@ static void max78000_gcr_reset_hold(Object *obj, ResetType type)
     s->eccaddr = 0;
 }
 
-static uint64_t max78000_gcr_read(void *opaque, hwaddr addr,
-                                     unsigned int size)
+static MemTxResult max78000_gcr_read(void *opaque, hwaddr addr,
+                                     uint64_t *pdata, unsigned size,
+                                     MemTxAttrs attrs)
 {
     Max78000GcrState *s = opaque;
+    uint64_t r;
 
     switch (addr) {
     case SYSCTRL:
-        return s->sysctrl;
+        r = s->sysctrl;
+        break;
 
     case RST0:
-        return s->rst0;
+        r = s->rst0;
+        break;
 
     case CLKCTRL:
-        return s->clkctrl;
+        r = s->clkctrl;
+        break;
 
     case PM:
-        return s->pm;
+        r = s->pm;
+        break;
 
     case PCLKDIV:
-        return s->pclkdiv;
+        r = s->pclkdiv;
+        break;
 
     case PCLKDIS0:
-        return s->pclkdis0;
+        r = s->pclkdis0;
+        break;
 
     case MEMCTRL:
-        return s->memctrl;
+        r = s->memctrl;
+        break;
 
     case MEMZ:
-        return s->memz;
+        r = s->memz;
+        break;
 
     case SYSST:
-        return s->sysst;
+        r = s->sysst;
+        break;
 
     case RST1:
-        return s->rst1;
+        r = s->rst1;
+        break;
 
     case PCKDIS1:
-        return s->pckdis1;
+        r = s->pckdis1;
+        break;
 
     case EVENTEN:
-        return s->eventen;
+        r = s->eventen;
+        break;
 
     case REVISION:
-        return s->revision;
+        r = s->revision;
+        break;
 
     case SYSIE:
-        return s->sysie;
+        r = s->sysie;
+        break;
 
     case ECCERR:
-        return s->eccerr;
+        r = s->eccerr;
+        break;
 
     case ECCED:
-        return s->ecced;
+        r = s->ecced;
+        break;
 
     case ECCIE:
-        return s->eccie;
+        r = s->eccie;
+        break;
 
     case ECCADDR:
-        return s->eccaddr;
+        r = s->eccaddr;
+        break;
 
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%"
             HWADDR_PRIx "\n", __func__, addr);
-        return 0;
-
+        r = 0;
+        break;
     }
+    *pdata = r;
+
+    return MEMTX_OK;
 }
 
-static void max78000_gcr_write(void *opaque, hwaddr addr,
-                       uint64_t val64, unsigned int size)
+static MemTxResult max78000_gcr_write(void *opaque, hwaddr addr,
+                                      uint64_t val64, unsigned size,
+                                      MemTxAttrs attrs)
 {
     Max78000GcrState *s = opaque;
     uint32_t val = val64;
     uint8_t zero[0xc000] = {0};
+    MemTxResult res = MEMTX_OK;
+
     switch (addr) {
     case SYSCTRL:
         /* Checksum calculations always pass immediately */
@@ -190,20 +216,20 @@ static void max78000_gcr_write(void *opaque, hwaddr addr,
 
     case MEMZ:
         if (val & ram0) {
-            address_space_write(&s->sram_as, SYSRAM0_START,
-                                MEMTXATTRS_UNSPECIFIED, zero, 0x8000);
+            res |= address_space_write(&s->sram_as, SYSRAM0_START,
+                                       attrs, zero, 0x8000);
         }
         if (val & ram1) {
-            address_space_write(&s->sram_as, SYSRAM1_START,
-                                MEMTXATTRS_UNSPECIFIED, zero, 0x8000);
+            res |= address_space_write(&s->sram_as, SYSRAM1_START,
+                                       attrs, zero, 0x8000);
         }
         if (val & ram2) {
-            address_space_write(&s->sram_as, SYSRAM2_START,
-                                MEMTXATTRS_UNSPECIFIED, zero, 0xC000);
+            res |= address_space_write(&s->sram_as, SYSRAM2_START,
+                                       attrs, zero, 0xC000);
         }
         if (val & ram3) {
-            address_space_write(&s->sram_as, SYSRAM3_START,
-                                MEMTXATTRS_UNSPECIFIED, zero, 0x4000);
+            res |= address_space_write(&s->sram_as, SYSRAM3_START,
+                                       attrs, zero, 0x4000);
         }
         break;
 
@@ -254,6 +280,7 @@ static void max78000_gcr_write(void *opaque, hwaddr addr,
         break;
 
     }
+    return res;
 }
 
 static const Property max78000_gcr_properties[] = {
@@ -272,8 +299,8 @@ static const Property max78000_gcr_properties[] = {
 };
 
 static const MemoryRegionOps max78000_gcr_ops = {
-    .read = max78000_gcr_read,
-    .write = max78000_gcr_write,
+    .read_with_attrs = max78000_gcr_read,
+    .write_with_attrs = max78000_gcr_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
     .valid.min_access_size = 4,
     .valid.max_access_size = 4,
-- 
2.51.0

Re: [PATCH] hw/misc/max78000_gcr: Do not ignore address_space_write() errors

[Peter Maydell]

On Tue, 7 Oct 2025 at 03:40, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Convert max78000_gcr_ops[] to take MemTxAttrs and return MemTxResult,
> allowing the write() path to return error on failure.

*Should* it return a MEMTX error on this failure, though?
This is a question of what the hardware behaviour is,
and there's no reference to the datasheet in this
commit message...

thanks
-- PMM

Re: [PATCH] hw/misc/max78000_gcr: Do not ignore address_space_write() errors

[Philippe Mathieu-Daudé]

On 7/10/25 10:27, Peter Maydell wrote:
> On Tue, 7 Oct 2025 at 03:40, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Convert max78000_gcr_ops[] to take MemTxAttrs and return MemTxResult,
>> allowing the write() path to return error on failure.
> 
> *Should* it return a MEMTX error on this failure, though?
> This is a question of what the hardware behaviour is,
> and there's no reference to the datasheet in this
> commit message...

Right. Thanks!

> 
> thanks
> -- PMM

Re: [PATCH] hw/misc/max78000_gcr: Do not ignore address_space_write() errors

[Philippe Mathieu-Daudé]

On 7/10/25 15:48, Philippe Mathieu-Daudé wrote:
> On 7/10/25 10:27, Peter Maydell wrote:
>> On Tue, 7 Oct 2025 at 03:40, Philippe Mathieu-Daudé 
>> <philmd@linaro.org> wrote:
>>>
>>> Convert max78000_gcr_ops[] to take MemTxAttrs and return MemTxResult,
>>> allowing the write() path to return error on failure.
>>
>> *Should* it return a MEMTX error on this failure, though?
>> This is a question of what the hardware behaviour is,
>> and there's no reference to the datasheet in this
>> commit message...
> 
> Right. Thanks!

Looking at "MAX78000 User Guide (UG7456; Rev 1; 3/2024)",
chapter "4.7.2 RAM Zeroization" and table "4-67: Memory
Zeroize Control Register", IIUC failure can not happen.

Would that change be OK?

-      address_space_write(&s->sram_as, SYSRAM0_START,
-                          MEMTXATTRS_UNSPECIFIED, zero, 0x8000);
+      /* RAM Zeroization can not fail */
+      (void)address_space_write(&s->sram_as, SYSRAM0_START,
+                                MEMTXATTRS_UNSPECIFIED, zero, 0x8000);

Otherwise, what would be the recommended way to express return value
has to be ignored? Assertion doesn't seem right, since this is not a
programming error.

Thanks,

Phil.

Re: [PATCH] hw/misc/max78000_gcr: Do not ignore address_space_write() errors

[Peter Maydell]

On Tue, 7 Oct 2025 at 15:47, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 7/10/25 15:48, Philippe Mathieu-Daudé wrote:
> > On 7/10/25 10:27, Peter Maydell wrote:
> >> On Tue, 7 Oct 2025 at 03:40, Philippe Mathieu-Daudé
> >> <philmd@linaro.org> wrote:
> >>>
> >>> Convert max78000_gcr_ops[] to take MemTxAttrs and return MemTxResult,
> >>> allowing the write() path to return error on failure.
> >>
> >> *Should* it return a MEMTX error on this failure, though?
> >> This is a question of what the hardware behaviour is,
> >> and there's no reference to the datasheet in this
> >> commit message...
> >
> > Right. Thanks!
>
> Looking at "MAX78000 User Guide (UG7456; Rev 1; 3/2024)",
> chapter "4.7.2 RAM Zeroization" and table "4-67: Memory
> Zeroize Control Register", IIUC failure can not happen.
>
> Would that change be OK?
>
> -      address_space_write(&s->sram_as, SYSRAM0_START,
> -                          MEMTXATTRS_UNSPECIFIED, zero, 0x8000);
> +      /* RAM Zeroization can not fail */
> +      (void)address_space_write(&s->sram_as, SYSRAM0_START,
> +                                MEMTXATTRS_UNSPECIFIED, zero, 0x8000);

We don't generally use the void cast like that. Just
don't do anything with the return value.

-- PMM

Re: [PATCH] hw/misc/max78000_gcr: Do not ignore address_space_write() errors

[Philippe Mathieu-Daudé]

On 7/10/25 16:55, Peter Maydell wrote:
> On Tue, 7 Oct 2025 at 15:47, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 7/10/25 15:48, Philippe Mathieu-Daudé wrote:
>>> On 7/10/25 10:27, Peter Maydell wrote:
>>>> On Tue, 7 Oct 2025 at 03:40, Philippe Mathieu-Daudé
>>>> <philmd@linaro.org> wrote:
>>>>>
>>>>> Convert max78000_gcr_ops[] to take MemTxAttrs and return MemTxResult,
>>>>> allowing the write() path to return error on failure.
>>>>
>>>> *Should* it return a MEMTX error on this failure, though?
>>>> This is a question of what the hardware behaviour is,
>>>> and there's no reference to the datasheet in this
>>>> commit message...
>>>
>>> Right. Thanks!
>>
>> Looking at "MAX78000 User Guide (UG7456; Rev 1; 3/2024)",
>> chapter "4.7.2 RAM Zeroization" and table "4-67: Memory
>> Zeroize Control Register", IIUC failure can not happen.
>>
>> Would that change be OK?
>>
>> -      address_space_write(&s->sram_as, SYSRAM0_START,
>> -                          MEMTXATTRS_UNSPECIFIED, zero, 0x8000);
>> +      /* RAM Zeroization can not fail */
>> +      (void)address_space_write(&s->sram_as, SYSRAM0_START,
>> +                                MEMTXATTRS_UNSPECIFIED, zero, 0x8000);
> 
> We don't generally use the void cast like that. Just
> don't do anything with the return value.

That would allow to use the helpful __attribute__((warn_unused_result)).