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/[email protected]/

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 <[email protected]>
---
  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,


Reply via email to