[PATCH v2 0/3] aspeed: Improve error handling and fix DMA issues

[PATCH v2 0/3] aspeed: Improve error handling and fix DMA issues

[Cédric Le Goater]

Hello,

This series addresses several error handling issues in Aspeed device
models that could lead to guest-triggerable denial of service or
incorrect behavior.

The first two patches improve DMA error handling by properly
propagating memory transaction errors to the guest. The aspeed_smc
patch converts memory operations to use read/write_with_attrs to
return MEMTX_ERROR on invalid conditions, while the ftgmac100 patch
checks DMA operation return values.

The third patch fixes an incorrect assertion in the aspeed_i2c model
that could be triggered when firmware uses the RX_BUF_LEN_W1T bit to
program DMA length fields separately, which is valid per the Aspeed
datasheet.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3335
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3315

Thanks,

C. 

Changes in v2:

- v1 was taking into account patch : 
  https://lore.kernel.org/all/20260224065556.3847942-14-jamin_lin@aspeedtech.com
  Rebased on upstream 

Cédric Le Goater (3):
  hw/ssi/aspeed_smc: Convert mem ops to read/write_with_attrs for error
    handling
  ftgmac100: Improve DMA error handling
  hw/i2c/aspeed_i2c: Remove assert

 hw/i2c/aspeed_i2c.c |  1 -
 hw/net/ftgmac100.c  | 10 +++++++--
 hw/ssi/aspeed_smc.c | 49 ++++++++++++++++++++++++++-------------------
 3 files changed, 36 insertions(+), 24 deletions(-)

-- 
2.53.0

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

[Cédric Le Goater]

Error conditions (invalid flash mode, unwritable flash) 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
Reviewed-by: Jamin Lin <jamin_lin@aspeedtech.com>
Link: https://lore.kernel.org/qemu-devel/20260322215732.387383-2-clg@redhat.com
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/ssi/aspeed_smc.c | 49 ++++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 21 deletions(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index b9d5ecba2929..f0deeea996c3 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -493,17 +493,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:
@@ -512,18 +513,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;
 }
 
 /*
@@ -624,8 +626,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;
@@ -636,7 +638,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)) {
@@ -661,12 +663,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,
@@ -754,7 +759,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);
@@ -782,12 +788,13 @@ 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 {
         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)
@@ -1108,8 +1115,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);
@@ -1159,13 +1166,13 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
     } 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,
 };
 
@@ -2007,8 +2014,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

[PATCH v2 2/3] ftgmac100: Improve DMA error handling

[Cédric Le Goater]

Currently, DMA memory operation errors in the ftgmac100 model are not
all tested and this can lead to a guest-triggerable denial of service
as described in https://gitlab.com/qemu-project/qemu/-/work_items/3335.

To fix this, check the return value of ftgmac100_write_bd() in the TX
path and exit the TX loop on error to prevent further processing. In
the event of a DMA error, also set FTGMAC100_INT_AHB_ERR interrupt
flag as appropriate.

The FTGMAC100_INT_AHB_ERR interrupt status bit only applies to the
AST2400 SoC; on newer Aspeed SoCs, it is a reserved bit.
Nevertheless, since it is supported by the Linux driver and it should
be safe to use in the QEMU implementation across all SoCs.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3335
Reviewed-by: Jamin Lin <jamin_lin@aspeedtech.com>
Link: https://lore.kernel.org/qemu-devel/20260322215732.387383-3-clg@redhat.com
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/net/ftgmac100.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index d29f7dcd171b..2f05bba11d01 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -624,7 +624,10 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint64_t tx_ring,
         bd.des0 &= ~FTGMAC100_TXDES0_TXDMA_OWN;
 
         /* Write back the modified descriptor.  */
-        ftgmac100_write_bd(&bd, addr);
+        if (ftgmac100_write_bd(&bd, addr)) {
+            s->isr |= FTGMAC100_INT_AHB_ERR;
+            break;
+        }
         /* Advance to the next descriptor.  */
         if (bd.des0 & s->txdes0_edotr) {
             addr = tx_ring;
@@ -1134,7 +1137,10 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
             bd.des0 |= flags | FTGMAC100_RXDES0_LRS;
             s->isr |= FTGMAC100_INT_RPKT_BUF;
         }
-        ftgmac100_write_bd(&bd, addr);
+        if (ftgmac100_write_bd(&bd, addr)) {
+            s->isr |= FTGMAC100_INT_AHB_ERR;
+            break;
+        }
         if (bd.des0 & s->rxdes0_edorr) {
             addr = s->rx_ring;
         } else {
-- 
2.53.0

[PATCH v2 3/3] hw/i2c/aspeed_i2c: Remove assert

[Cédric Le Goater]

According to the Aspeed datasheet, the RX_BUF_LEN_W1T and
TX_BUF_LEN_W1T bits of the A_I2CS_DMA_LEN (0x2c) register allow
firmware to program the TX and RX DMA length (TX_BUF_LEN and
RX_BUF_LEN fields of the same register) separately without the need to
read/modify/write the value.  If RX_BUF_LEN_W1T and TX_BUF_LEN_W1T
bits are 0, then both TX and RX DMA length will be written.

When setting the RX_BUF_LEN field, the TX_BUF_LEN field being set is
not an invalid condition. Remove the assert.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3315
Reviewed-by: Jamin Lin <jamin_lin@aspeedtech.com>
Link: https://lore.kernel.org/qemu-devel/20260322215732.387383-4-clg@redhat.com
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/i2c/aspeed_i2c.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index ad6342bec0d4..5d18f8d49ea4 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -780,7 +780,6 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
         bus->regs[R_I2CS_DMA_RX_ADDR] = value;
         break;
     case A_I2CS_DMA_LEN:
-        assert(FIELD_EX32(value, I2CS_DMA_LEN, TX_BUF_LEN) == 0);
         if (FIELD_EX32(value, I2CS_DMA_LEN, RX_BUF_LEN_W1T)) {
             ARRAY_FIELD_DP32(bus->regs, I2CS_DMA_LEN, RX_BUF_LEN,
                              FIELD_EX32(value, I2CS_DMA_LEN, RX_BUF_LEN));
-- 
2.53.0

Re: [PATCH v2 0/3] aspeed: Improve error handling and fix DMA issues

[Cédric Le Goater]

On 3/23/26 13:55, Cédric Le Goater wrote:
> Hello,
> 
> This series addresses several error handling issues in Aspeed device
> models that could lead to guest-triggerable denial of service or
> incorrect behavior.
> 
> The first two patches improve DMA error handling by properly
> propagating memory transaction errors to the guest. The aspeed_smc
> patch converts memory operations to use read/write_with_attrs to
> return MEMTX_ERROR on invalid conditions, while the ftgmac100 patch
> checks DMA operation return values.
> 
> The third patch fixes an incorrect assertion in the aspeed_i2c model
> that could be triggered when firmware uses the RX_BUF_LEN_W1T bit to
> program DMA length fields separately, which is valid per the Aspeed
> datasheet.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3335
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3315
> 
> Thanks,
> 
> C.
> 
> Changes in v2:
> 
> - v1 was taking into account patch :
>    https://lore.kernel.org/all/20260224065556.3847942-14-jamin_lin@aspeedtech.com
>    Rebased on upstream
> 
> Cédric Le Goater (3):
>    hw/ssi/aspeed_smc: Convert mem ops to read/write_with_attrs for error
>      handling
>    ftgmac100: Improve DMA error handling
>    hw/i2c/aspeed_i2c: Remove assert
> 
>   hw/i2c/aspeed_i2c.c |  1 -
>   hw/net/ftgmac100.c  | 10 +++++++--
>   hw/ssi/aspeed_smc.c | 49 ++++++++++++++++++++++++++-------------------
>   3 files changed, 36 insertions(+), 24 deletions(-)
> 


Applied to aspeed-next.

Thanks,

C.

Re: [PATCH v2 2/3] ftgmac100: Improve DMA error handling

[Michael Tokarev]

On 23.03.2026 15:55, Cédric Le Goater wrote:
> Currently, DMA memory operation errors in the ftgmac100 model are not
> all tested and this can lead to a guest-triggerable denial of service
> as described in https://gitlab.com/qemu-project/qemu/-/work_items/3335.
> 
> To fix this, check the return value of ftgmac100_write_bd() in the TX
> path and exit the TX loop on error to prevent further processing. In
> the event of a DMA error, also set FTGMAC100_INT_AHB_ERR interrupt
> flag as appropriate.
> 
> The FTGMAC100_INT_AHB_ERR interrupt status bit only applies to the
> AST2400 SoC; on newer Aspeed SoCs, it is a reserved bit.
> Nevertheless, since it is supported by the Linux driver and it should
> be safe to use in the QEMU implementation across all SoCs.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3335
> Reviewed-by: Jamin Lin <jamin_lin@aspeedtech.com>
> Link: https://lore.kernel.org/qemu-devel/20260322215732.387383-3-clg@redhat.com
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>

Shouldn't this be picked up for current qemu stable series?

Thanks,

/mjt

Re: [PATCH v2 2/3] ftgmac100: Improve DMA error handling

[Cédric Le Goater]

Hello Michael,

On 3/24/26 15:45, Michael Tokarev wrote:
> On 23.03.2026 15:55, Cédric Le Goater wrote:
>> Currently, DMA memory operation errors in the ftgmac100 model are not
>> all tested and this can lead to a guest-triggerable denial of service
>> as described in https://gitlab.com/qemu-project/qemu/-/work_items/3335.
>>
>> To fix this, check the return value of ftgmac100_write_bd() in the TX
>> path and exit the TX loop on error to prevent further processing. In
>> the event of a DMA error, also set FTGMAC100_INT_AHB_ERR interrupt
>> flag as appropriate.
>>
>> The FTGMAC100_INT_AHB_ERR interrupt status bit only applies to the
>> AST2400 SoC; on newer Aspeed SoCs, it is a reserved bit.
>> Nevertheless, since it is supported by the Linux driver and it should
>> be safe to use in the QEMU implementation across all SoCs.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3335
>> Reviewed-by: Jamin Lin <jamin_lin@aspeedtech.com>
>> Link: https://lore.kernel.org/qemu-devel/20260322215732.387383-3-clg@redhat.com
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> 
> Shouldn't this be picked up for current qemu stable series?

It should be safe to do so.

The overall fix for the reported issue needs :

   https://lore.kernel.org/qemu-devel/20260324124131.1053711-5-clg@redhat.com/T/#u

Thanks,

C.