[PATCH 1/3] hw/ssi/aspeed_smc: Convert mem ops to read/write_with_attrs for error handling

Cédric Le Goater posted 3 patches 1 week, 5 days ago
There is a newer version of this series
[PATCH 1/3] hw/ssi/aspeed_smc: Convert mem ops to read/write_with_attrs for error handling
Posted by Cédric Le Goater 1 week, 5 days ago
Error conditions (invalid flash mode, unwritable flash, invalid data
FIFO offset) now return MEMTX_ERROR instead of silently succeeding or
returning undefined values.

This allows the memory subsystem to properly propagate transaction
errors to the guest, improving QEMU reliability.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3335
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/ssi/aspeed_smc.c | 58 ++++++++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 24 deletions(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index ed6cedabcb3b..b14b1cbddfc3 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -502,17 +502,18 @@ static void aspeed_smc_flash_setup(AspeedSMCFlash *fl, uint32_t addr)
     }
 }
 
-static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
+static MemTxResult aspeed_smc_flash_read(void *opaque, hwaddr addr,
+                                 uint64_t *data, unsigned size, MemTxAttrs attrs)
 {
     AspeedSMCFlash *fl = opaque;
     AspeedSMCState *s = fl->controller;
-    uint64_t ret = 0;
     int i;
 
+    *data = 0;
     switch (aspeed_smc_flash_mode(fl)) {
     case CTRL_USERMODE:
         for (i = 0; i < size; i++) {
-            ret |= (uint64_t) ssi_transfer(s->spi, 0x0) << (8 * i);
+            *data |= (uint64_t) ssi_transfer(s->spi, 0x0) << (8 * i);
         }
         break;
     case CTRL_READMODE:
@@ -521,18 +522,19 @@ static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
         aspeed_smc_flash_setup(fl, addr);
 
         for (i = 0; i < size; i++) {
-            ret |= (uint64_t) ssi_transfer(s->spi, 0x0) << (8 * i);
+            *data |= (uint64_t) ssi_transfer(s->spi, 0x0) << (8 * i);
         }
 
         aspeed_smc_flash_unselect(fl);
         break;
     default:
         aspeed_smc_error("invalid flash mode %d", aspeed_smc_flash_mode(fl));
+        return MEMTX_ERROR;
     }
 
-    trace_aspeed_smc_flash_read(fl->cs, addr, size, ret,
+    trace_aspeed_smc_flash_read(fl->cs, addr, size, *data,
                                 aspeed_smc_flash_mode(fl));
-    return ret;
+    return MEMTX_OK;
 }
 
 /*
@@ -633,8 +635,8 @@ static bool aspeed_smc_do_snoop(AspeedSMCFlash *fl,  uint64_t data,
     return false;
 }
 
-static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
-                                   unsigned size)
+static MemTxResult aspeed_smc_flash_write(void *opaque, hwaddr addr,
+                                   uint64_t data, unsigned size, MemTxAttrs attrs)
 {
     AspeedSMCFlash *fl = opaque;
     AspeedSMCState *s = fl->controller;
@@ -645,7 +647,7 @@ static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
 
     if (!aspeed_smc_is_writable(fl)) {
         aspeed_smc_error("flash is not writable at 0x%" HWADDR_PRIx, addr);
-        return;
+        return MEMTX_ERROR;
     }
 
     switch (aspeed_smc_flash_mode(fl)) {
@@ -670,12 +672,15 @@ static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
         break;
     default:
         aspeed_smc_error("invalid flash mode %d", aspeed_smc_flash_mode(fl));
+        return MEMTX_ERROR;
     }
+
+    return MEMTX_OK;
 }
 
 static const MemoryRegionOps aspeed_smc_flash_ops = {
-    .read = aspeed_smc_flash_read,
-    .write = aspeed_smc_flash_write,
+    .read_with_attrs = aspeed_smc_flash_read,
+    .write_with_attrs = aspeed_smc_flash_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
     .valid = {
         .min_access_size = 1,
@@ -763,7 +768,8 @@ static void aspeed_smc_reset(DeviceState *d)
     s->snoop_dummies = 0;
 }
 
-static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
+static MemTxResult aspeed_smc_read(void *opaque, hwaddr addr, uint64_t *data,
+                                   unsigned int size, MemTxAttrs attrs)
 {
     AspeedSMCState *s = ASPEED_SMC(opaque);
     AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(opaque);
@@ -792,7 +798,7 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
 
         trace_aspeed_smc_read(addr << 2, size, s->regs[addr]);
 
-        return s->regs[addr];
+        *data = s->regs[addr];
     } else if (aspeed_smc_has_data_fifo(asc) && addr >= R_DATA_FIFO) {
         cs = asc->data_fifo_offset_to_cs(s, addr << 2);
         if (cs >= 0) {
@@ -801,15 +807,17 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
              * The flash address is provided by the SPI command/address cycles,
              * the MMIO addr parameter is ignored.
              */
-            return aspeed_smc_flash_read(&s->flashes[cs], 0, size);
+            return aspeed_smc_flash_read(&s->flashes[cs], 0, data, size, attrs);
         }
         aspeed_smc_error("Invalid data fifo offset %" HWADDR_PRIx, addr << 2);
-        return -1;
+        *data = -1;
+        return MEMTX_ERROR;
     } else {
         qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
                       __func__, addr);
-        return -1;
+        *data = -1;
     }
+    return MEMTX_OK;
 }
 
 static uint8_t aspeed_smc_hclk_divisor(uint8_t hclk_mask)
@@ -1130,8 +1138,8 @@ static void aspeed_2600_smc_dma_ctrl(AspeedSMCState *s, uint32_t dma_ctrl)
     s->regs[R_DMA_CTRL] &= ~(DMA_CTRL_REQUEST | DMA_CTRL_GRANT);
 }
 
-static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
-                             unsigned int size)
+static MemTxResult aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
+                                    unsigned int size, MemTxAttrs attrs)
 {
     AspeedSMCState *s = ASPEED_SMC(opaque);
     AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
@@ -1186,19 +1194,21 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
              * The flash address is provided by the SPI command/address cycles,
              * the MMIO addr parameter is ignored.
              */
-            return aspeed_smc_flash_write(&s->flashes[cs], 0, data, size);
+            return aspeed_smc_flash_write(&s->flashes[cs], 0, data, size,
+                                          attrs);
         }
         aspeed_smc_error("Invalid data fifo offset %" HWADDR_PRIx, addr << 2);
+        return MEMTX_ERROR;
     } else {
         qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
                       __func__, addr);
-        return;
     }
+    return MEMTX_OK;
 }
 
 static const MemoryRegionOps aspeed_smc_ops = {
-    .read = aspeed_smc_read,
-    .write = aspeed_smc_write,
+    .read_with_attrs = aspeed_smc_read,
+    .write_with_attrs = aspeed_smc_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
@@ -2073,8 +2083,8 @@ static const uint32_t aspeed_2700_fmc_resets[ASPEED_SMC_R_MAX] = {
 };
 
 static const MemoryRegionOps aspeed_2700_smc_flash_ops = {
-    .read = aspeed_smc_flash_read,
-    .write = aspeed_smc_flash_write,
+    .read_with_attrs = aspeed_smc_flash_read,
+    .write_with_attrs = aspeed_smc_flash_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
     .valid = {
         .min_access_size = 1,
-- 
2.53.0


Re: [PATCH 1/3] hw/ssi/aspeed_smc: Convert mem ops to read/write_with_attrs for error handling
Posted by Cédric Le Goater 1 week, 4 days ago
On 3/22/26 22:57, Cédric Le Goater wrote:
> Error conditions (invalid flash mode, unwritable flash, invalid data
> FIFO offset) now return MEMTX_ERROR instead of silently succeeding or
> returning undefined values.

The invalid FIFO offset is not upstream yet, only in my aspeed-11.0
branch including :

   https://lore.kernel.org/all/20260224065556.3847942-14-jamin_lin@aspeedtech.com/

I will resend a v2 with the updated patch.

Thanks,

C.

> This allows the memory subsystem to properly propagate transaction
> errors to the guest, improving QEMU reliability.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3335
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   hw/ssi/aspeed_smc.c | 58 ++++++++++++++++++++++++++-------------------
>   1 file changed, 34 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index ed6cedabcb3b..b14b1cbddfc3 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -502,17 +502,18 @@ static void aspeed_smc_flash_setup(AspeedSMCFlash *fl, uint32_t addr)
>       }
>   }
>   
> -static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
> +static MemTxResult aspeed_smc_flash_read(void *opaque, hwaddr addr,
> +                                 uint64_t *data, unsigned size, MemTxAttrs attrs)
>   {
>       AspeedSMCFlash *fl = opaque;
>       AspeedSMCState *s = fl->controller;
> -    uint64_t ret = 0;
>       int i;
>   
> +    *data = 0;
>       switch (aspeed_smc_flash_mode(fl)) {
>       case CTRL_USERMODE:
>           for (i = 0; i < size; i++) {
> -            ret |= (uint64_t) ssi_transfer(s->spi, 0x0) << (8 * i);
> +            *data |= (uint64_t) ssi_transfer(s->spi, 0x0) << (8 * i);
>           }
>           break;
>       case CTRL_READMODE:
> @@ -521,18 +522,19 @@ static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
>           aspeed_smc_flash_setup(fl, addr);
>   
>           for (i = 0; i < size; i++) {
> -            ret |= (uint64_t) ssi_transfer(s->spi, 0x0) << (8 * i);
> +            *data |= (uint64_t) ssi_transfer(s->spi, 0x0) << (8 * i);
>           }
>   
>           aspeed_smc_flash_unselect(fl);
>           break;
>       default:
>           aspeed_smc_error("invalid flash mode %d", aspeed_smc_flash_mode(fl));
> +        return MEMTX_ERROR;
>       }
>   
> -    trace_aspeed_smc_flash_read(fl->cs, addr, size, ret,
> +    trace_aspeed_smc_flash_read(fl->cs, addr, size, *data,
>                                   aspeed_smc_flash_mode(fl));
> -    return ret;
> +    return MEMTX_OK;
>   }
>   
>   /*
> @@ -633,8 +635,8 @@ static bool aspeed_smc_do_snoop(AspeedSMCFlash *fl,  uint64_t data,
>       return false;
>   }
>   
> -static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
> -                                   unsigned size)
> +static MemTxResult aspeed_smc_flash_write(void *opaque, hwaddr addr,
> +                                   uint64_t data, unsigned size, MemTxAttrs attrs)
>   {
>       AspeedSMCFlash *fl = opaque;
>       AspeedSMCState *s = fl->controller;
> @@ -645,7 +647,7 @@ static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
>   
>       if (!aspeed_smc_is_writable(fl)) {
>           aspeed_smc_error("flash is not writable at 0x%" HWADDR_PRIx, addr);
> -        return;
> +        return MEMTX_ERROR;
>       }
>   
>       switch (aspeed_smc_flash_mode(fl)) {
> @@ -670,12 +672,15 @@ static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
>           break;
>       default:
>           aspeed_smc_error("invalid flash mode %d", aspeed_smc_flash_mode(fl));
> +        return MEMTX_ERROR;
>       }
> +
> +    return MEMTX_OK;
>   }
>   
>   static const MemoryRegionOps aspeed_smc_flash_ops = {
> -    .read = aspeed_smc_flash_read,
> -    .write = aspeed_smc_flash_write,
> +    .read_with_attrs = aspeed_smc_flash_read,
> +    .write_with_attrs = aspeed_smc_flash_write,
>       .endianness = DEVICE_LITTLE_ENDIAN,
>       .valid = {
>           .min_access_size = 1,
> @@ -763,7 +768,8 @@ static void aspeed_smc_reset(DeviceState *d)
>       s->snoop_dummies = 0;
>   }
>   
> -static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
> +static MemTxResult aspeed_smc_read(void *opaque, hwaddr addr, uint64_t *data,
> +                                   unsigned int size, MemTxAttrs attrs)
>   {
>       AspeedSMCState *s = ASPEED_SMC(opaque);
>       AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(opaque);
> @@ -792,7 +798,7 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
>   
>           trace_aspeed_smc_read(addr << 2, size, s->regs[addr]);
>   
> -        return s->regs[addr];
> +        *data = s->regs[addr];
>       } else if (aspeed_smc_has_data_fifo(asc) && addr >= R_DATA_FIFO) {
>           cs = asc->data_fifo_offset_to_cs(s, addr << 2);
>           if (cs >= 0) {
> @@ -801,15 +807,17 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
>                * The flash address is provided by the SPI command/address cycles,
>                * the MMIO addr parameter is ignored.
>                */
> -            return aspeed_smc_flash_read(&s->flashes[cs], 0, size);
> +            return aspeed_smc_flash_read(&s->flashes[cs], 0, data, size, attrs);
>           }
>           aspeed_smc_error("Invalid data fifo offset %" HWADDR_PRIx, addr << 2);
> -        return -1;
> +        *data = -1;
> +        return MEMTX_ERROR;
>       } else {
>           qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
>                         __func__, addr);
> -        return -1;
> +        *data = -1;
>       }
> +    return MEMTX_OK;
>   }
>   
>   static uint8_t aspeed_smc_hclk_divisor(uint8_t hclk_mask)
> @@ -1130,8 +1138,8 @@ static void aspeed_2600_smc_dma_ctrl(AspeedSMCState *s, uint32_t dma_ctrl)
>       s->regs[R_DMA_CTRL] &= ~(DMA_CTRL_REQUEST | DMA_CTRL_GRANT);
>   }
>   
> -static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
> -                             unsigned int size)
> +static MemTxResult aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
> +                                    unsigned int size, MemTxAttrs attrs)
>   {
>       AspeedSMCState *s = ASPEED_SMC(opaque);
>       AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
> @@ -1186,19 +1194,21 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
>                * The flash address is provided by the SPI command/address cycles,
>                * the MMIO addr parameter is ignored.
>                */
> -            return aspeed_smc_flash_write(&s->flashes[cs], 0, data, size);
> +            return aspeed_smc_flash_write(&s->flashes[cs], 0, data, size,
> +                                          attrs);
>           }
>           aspeed_smc_error("Invalid data fifo offset %" HWADDR_PRIx, addr << 2);
> +        return MEMTX_ERROR;
>       } else {
>           qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
>                         __func__, addr);
> -        return;
>       }
> +    return MEMTX_OK;
>   }
>   
>   static const MemoryRegionOps aspeed_smc_ops = {
> -    .read = aspeed_smc_read,
> -    .write = aspeed_smc_write,
> +    .read_with_attrs = aspeed_smc_read,
> +    .write_with_attrs = aspeed_smc_write,
>       .endianness = DEVICE_LITTLE_ENDIAN,
>   };
>   
> @@ -2073,8 +2083,8 @@ static const uint32_t aspeed_2700_fmc_resets[ASPEED_SMC_R_MAX] = {
>   };
>   
>   static const MemoryRegionOps aspeed_2700_smc_flash_ops = {
> -    .read = aspeed_smc_flash_read,
> -    .write = aspeed_smc_flash_write,
> +    .read_with_attrs = aspeed_smc_flash_read,
> +    .write_with_attrs = aspeed_smc_flash_write,
>       .endianness = DEVICE_LITTLE_ENDIAN,
>       .valid = {
>           .min_access_size = 1,


RE: [PATCH 1/3] hw/ssi/aspeed_smc: Convert mem ops to read/write_with_attrs for error handling
Posted by Jamin Lin 1 week, 4 days ago
> -----Original Message-----
> From: Cédric Le Goater <clg@redhat.com>
> Sent: Monday, March 23, 2026 5:58 AM
> To: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org; Peter Maydell <peter.maydell@linaro.org>; Jamin
> Lin <jamin_lin@aspeedtech.com>; Kane Chen <kane_chen@aspeedtech.com>;
> Cédric Le Goater <clg@redhat.com>
> Subject: [PATCH 1/3] hw/ssi/aspeed_smc: Convert mem ops to
> read/write_with_attrs for error handling
> 
> Error conditions (invalid flash mode, unwritable flash, invalid data FIFO offset)
> now return MEMTX_ERROR instead of silently succeeding or returning
> undefined values.
> 
> This allows the memory subsystem to properly propagate transaction errors to
> the guest, improving QEMU reliability.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3335
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>  hw/ssi/aspeed_smc.c | 58 ++++++++++++++++++++++++++-------------------
>  1 file changed, 34 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index
> ed6cedabcb3b..b14b1cbddfc3 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -502,17 +502,18 @@ static void
> aspeed_smc_flash_setup(AspeedSMCFlash *fl, uint32_t addr)
>      }
>  }
> 
> -static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned
> size)
> +static MemTxResult aspeed_smc_flash_read(void *opaque, hwaddr addr,
> +                                 uint64_t *data, unsigned size,
> +MemTxAttrs attrs)
>  {
>      AspeedSMCFlash *fl = opaque;
>      AspeedSMCState *s = fl->controller;
> -    uint64_t ret = 0;
>      int i;
> 
> +    *data = 0;
>      switch (aspeed_smc_flash_mode(fl)) {
>      case CTRL_USERMODE:
>          for (i = 0; i < size; i++) {
> -            ret |= (uint64_t) ssi_transfer(s->spi, 0x0) << (8 * i);
> +            *data |= (uint64_t) ssi_transfer(s->spi, 0x0) << (8 * i);
>          }
>          break;
>      case CTRL_READMODE:
> @@ -521,18 +522,19 @@ static uint64_t aspeed_smc_flash_read(void
> *opaque, hwaddr addr, unsigned size)
>          aspeed_smc_flash_setup(fl, addr);
> 
>          for (i = 0; i < size; i++) {
> -            ret |= (uint64_t) ssi_transfer(s->spi, 0x0) << (8 * i);
> +            *data |= (uint64_t) ssi_transfer(s->spi, 0x0) << (8 * i);
>          }
> 
>          aspeed_smc_flash_unselect(fl);
>          break;
>      default:
>          aspeed_smc_error("invalid flash mode %d",
> aspeed_smc_flash_mode(fl));
> +        return MEMTX_ERROR;
>      }
> 
> -    trace_aspeed_smc_flash_read(fl->cs, addr, size, ret,
> +    trace_aspeed_smc_flash_read(fl->cs, addr, size, *data,
>                                  aspeed_smc_flash_mode(fl));
> -    return ret;
> +    return MEMTX_OK;
>  }
> 
>  /*
> @@ -633,8 +635,8 @@ static bool aspeed_smc_do_snoop(AspeedSMCFlash
> *fl,  uint64_t data,
>      return false;
>  }
> 
> -static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t
> data,
> -                                   unsigned size)
> +static MemTxResult aspeed_smc_flash_write(void *opaque, hwaddr addr,
> +                                   uint64_t data, unsigned size,
> +MemTxAttrs attrs)
>  {
>      AspeedSMCFlash *fl = opaque;
>      AspeedSMCState *s = fl->controller; @@ -645,7 +647,7 @@ static void
> aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
> 
>      if (!aspeed_smc_is_writable(fl)) {
>          aspeed_smc_error("flash is not writable at 0x%" HWADDR_PRIx,
> addr);
> -        return;
> +        return MEMTX_ERROR;
>      }
> 
>      switch (aspeed_smc_flash_mode(fl)) { @@ -670,12 +672,15 @@ static
> void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
>          break;
>      default:
>          aspeed_smc_error("invalid flash mode %d",
> aspeed_smc_flash_mode(fl));
> +        return MEMTX_ERROR;
>      }
> +
> +    return MEMTX_OK;
>  }
> 
>  static const MemoryRegionOps aspeed_smc_flash_ops = {
> -    .read = aspeed_smc_flash_read,
> -    .write = aspeed_smc_flash_write,
> +    .read_with_attrs = aspeed_smc_flash_read,
> +    .write_with_attrs = aspeed_smc_flash_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,
>      .valid = {
>          .min_access_size = 1,
> @@ -763,7 +768,8 @@ static void aspeed_smc_reset(DeviceState *d)
>      s->snoop_dummies = 0;
>  }
> 
> -static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int
> size)
> +static MemTxResult aspeed_smc_read(void *opaque, hwaddr addr, uint64_t
> *data,
> +                                   unsigned int size, MemTxAttrs
> attrs)
>  {
>      AspeedSMCState *s = ASPEED_SMC(opaque);
>      AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(opaque); @@ -792,7
> +798,7 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr,
> unsigned int size)
> 
>          trace_aspeed_smc_read(addr << 2, size, s->regs[addr]);
> 
> -        return s->regs[addr];
> +        *data = s->regs[addr];
>      } else if (aspeed_smc_has_data_fifo(asc) && addr >= R_DATA_FIFO) {
>          cs = asc->data_fifo_offset_to_cs(s, addr << 2);
>          if (cs >= 0) {
> @@ -801,15 +807,17 @@ static uint64_t aspeed_smc_read(void *opaque,
> hwaddr addr, unsigned int size)
>               * The flash address is provided by the SPI command/address
> cycles,
>               * the MMIO addr parameter is ignored.
>               */
> -            return aspeed_smc_flash_read(&s->flashes[cs], 0, size);
> +            return aspeed_smc_flash_read(&s->flashes[cs], 0, data,
> + size, attrs);
>          }
>          aspeed_smc_error("Invalid data fifo offset %" HWADDR_PRIx, addr
> << 2);
> -        return -1;
> +        *data = -1;
> +        return MEMTX_ERROR;
>      } else {
>          qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%"
> HWADDR_PRIx "\n",
>                        __func__, addr);
> -        return -1;
> +        *data = -1;
>      }
> +    return MEMTX_OK;
>  }
> 
>  static uint8_t aspeed_smc_hclk_divisor(uint8_t hclk_mask) @@ -1130,8
> +1138,8 @@ static void aspeed_2600_smc_dma_ctrl(AspeedSMCState *s,
> uint32_t dma_ctrl)
>      s->regs[R_DMA_CTRL] &= ~(DMA_CTRL_REQUEST |
> DMA_CTRL_GRANT);  }
> 
> -static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
> -                             unsigned int size)
> +static MemTxResult aspeed_smc_write(void *opaque, hwaddr addr, uint64_t
> data,
> +                                    unsigned int size, MemTxAttrs
> +attrs)
>  {
>      AspeedSMCState *s = ASPEED_SMC(opaque);
>      AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s); @@ -1186,19
> +1194,21 @@ static void aspeed_smc_write(void *opaque, hwaddr addr,
> uint64_t data,
>               * The flash address is provided by the SPI command/address
> cycles,
>               * the MMIO addr parameter is ignored.
>               */
> -            return aspeed_smc_flash_write(&s->flashes[cs], 0, data, size);
> +            return aspeed_smc_flash_write(&s->flashes[cs], 0, data, size,
> +                                          attrs);
>          }
>          aspeed_smc_error("Invalid data fifo offset %" HWADDR_PRIx, addr
> << 2);
> +        return MEMTX_ERROR;
>      } else {
>          qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%"
> HWADDR_PRIx "\n",
>                        __func__, addr);
> -        return;
>      }
> +    return MEMTX_OK;
>  }
> 
>  static const MemoryRegionOps aspeed_smc_ops = {
> -    .read = aspeed_smc_read,
> -    .write = aspeed_smc_write,
> +    .read_with_attrs = aspeed_smc_read,
> +    .write_with_attrs = aspeed_smc_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,  };
> 
> @@ -2073,8 +2083,8 @@ static const uint32_t
> aspeed_2700_fmc_resets[ASPEED_SMC_R_MAX] = {  };
> 
>  static const MemoryRegionOps aspeed_2700_smc_flash_ops = {
> -    .read = aspeed_smc_flash_read,
> -    .write = aspeed_smc_flash_write,
> +    .read_with_attrs = aspeed_smc_flash_read,
> +    .write_with_attrs = aspeed_smc_flash_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,
>      .valid = {
>          .min_access_size = 1,
> --
> 2.53.0

Reviewed-by: Jamin Lin <jamin_lin@aspeedtech.com>

Thanks,
Jamin