public inbox for qemu-devel@nongnu.org
 help / color / mirror / Atom feed
* [PULL 0/5] aspeed queue
@ 2026-03-24 12:41 Cédric Le Goater
  2026-03-24 12:41 ` [PULL 1/5] hw/i2c/aspeed: fix lost interrupts on back-to-back commands Cédric Le Goater
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Cédric Le Goater @ 2026-03-24 12:41 UTC (permalink / raw)
  To: qemu-arm, qemu-devel; +Cc: Cédric Le Goater

The following changes since commit 750ed363ebfb108082b40bc532b20564bf00f913:

  Merge tag 'hw-misc-20260323' of https://github.com/philmd/qemu into staging (2026-03-23 16:58:15 +0000)

are available in the Git repository at:

  https://github.com/legoater/qemu/ tags/pull-aspeed-20260324

for you to fetch changes up to 5fca367b89de2d11631cb27a90a8c5431d004f58:

  hw/i2c/aspeed_i2c: Remove assert (2026-03-24 11:19:40 +0100)

----------------------------------------------------------------
aspeed queue:

* Rework Aspeed SMC mem ops to improve error handling
* Fix race in Aspeed I2C model
* Disable kernel crypto self-tests in AST2700 boot tests

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

Jamin Lin (2):
      MAINTAINERS: Add Kane Chen as reviewer for Aspeed machines
      tests/functional/aarch64/test_aspeed: Disable kernel crypto self-tests in AST2700 boot tests

Jithu Joseph (1):
      hw/i2c/aspeed: fix lost interrupts on back-to-back commands

 MAINTAINERS                                       |  1 +
 include/hw/i2c/aspeed_i2c.h                       |  1 +
 hw/i2c/aspeed_i2c.c                               | 47 +++++++++++++++++++++-
 hw/ssi/aspeed_smc.c                               | 49 +++++++++++++----------
 tests/functional/aarch64/test_aspeed_ast2700a1.py | 11 ++++-
 tests/functional/aarch64/test_aspeed_ast2700a2.py | 11 ++++-
 tests/functional/aarch64/test_aspeed_ast2700fc.py |  9 ++++-
 7 files changed, 101 insertions(+), 28 deletions(-)



^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PULL 1/5] hw/i2c/aspeed: fix lost interrupts on back-to-back commands
  2026-03-24 12:41 [PULL 0/5] aspeed queue Cédric Le Goater
@ 2026-03-24 12:41 ` Cédric Le Goater
  2026-03-24 12:41 ` [PULL 2/5] MAINTAINERS: Add Kane Chen as reviewer for Aspeed machines Cédric Le Goater
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Cédric Le Goater @ 2026-03-24 12:41 UTC (permalink / raw)
  To: qemu-arm, qemu-devel; +Cc: Jithu Joseph, Cédric Le Goater

From: Jithu Joseph <jithu.joseph@oss.qualcomm.com>

QEMU executes I2C commands synchronously inside the CMD register write
handler. On real hardware each command takes time on the bus, so the
ISR can clear the previous interrupt status before the next completion
arrives. In QEMU, when the guest ISR handles a TX_ACK and immediately
issues the next command by writing to CMD, that command completes
instantly — before the ISR returns to W1C-clear the first TX_ACK.
Since the bit is already set, setting it again is a no-op. The ISR
then clears it, wiping both completions at once. No interrupt fires
for the second command and the driver stalls.

This affects any multi-step I2C transaction: register reads, SMBus
word reads, and PMBus device probes all fail ("Error: Read failed"
from i2cget, -ETIMEDOUT from kernel drivers).

The issue is exposed when the guest kernel includes commit "i2c:
aspeed: Acknowledge Tx done with and without ACK irq late" [1] which
defers W1C acknowledgment of TX_ACK until after the ISR has issued
the next command. This means the old TX_ACK is still set when the
next command completes synchronously, and the subsequent W1C wipes
both completions at once.

The trace below shows `i2cget -y 15 0x50 0x00` (read EEPROM register
0x00) failing without the fix. The first START+TX sets TX_ACK. The
ISR handles it and issues a second TX to send the register address.
That TX completes synchronously while TX_ACK is still set:

  aspeed_i2c_bus_cmd cmd=0x3 start|tx| intr=0x0    # START+TX, clean
  aspeed_i2c_bus_raise_interrupt intr=0x1 ack|      # TX_ACK set
  aspeed_i2c_bus_read  0x10: 0x1                    # ISR reads TX_ACK
  aspeed_i2c_bus_write 0x14: 0x2                    # ISR issues TX cmd
  aspeed_i2c_bus_cmd cmd=0x400002 tx| intr=0x1      # TX runs, TX_ACK already set!
  aspeed_i2c_bus_raise_interrupt intr=0x1 ack|      # re-set is no-op
  aspeed_i2c_bus_write 0x10: 0x1                    # ISR W1C clears TX_ACK
  aspeed_i2c_bus_read  0x10: 0x0                    # LOST — both ACKs wiped

The driver sees INTR_STS=0 and never proceeds to the read phase.

Fix this by tracking interrupt bits that collide with already-pending
bits. Before calling aspeed_i2c_bus_handle_cmd(), save and clear
INTR_STS so that only freshly set bits are visible after the call.
Any overlap between the old and new bits is saved in pending_intr_sts.
When the ISR later W1C-clears the old bits, re-apply the saved
pending bits so the ISR sees them on its next loop iteration.

With the fix, the same operation completes successfully:

  aspeed_i2c_bus_cmd cmd=0x3 start|tx| intr=0x0    # START+TX, clean
  aspeed_i2c_bus_raise_interrupt intr=0x1 ack|      # TX_ACK set
  aspeed_i2c_bus_read  0x10: 0x1                    # ISR reads TX_ACK
  aspeed_i2c_bus_write 0x14: 0x2                    # ISR issues TX cmd
  aspeed_i2c_bus_cmd cmd=0x400002 tx| intr=0x0      # INTR_STS cleared first
  aspeed_i2c_bus_raise_interrupt intr=0x1 ack|      # TX_ACK freshly set
  aspeed_i2c_bus_write 0x10: 0x1                    # ISR W1C clears TX_ACK
  aspeed_i2c_bus_read  0x10: 0x1                    # RE-DELIVERED from pending
  aspeed_i2c_bus_write 0x14: 0x1b                   # ISR proceeds: START+RX
  aspeed_i2c_bus_cmd cmd=0x40001b start|tx|rx|last| # read phase completes
  i2c_recv recv(addr:0x50) data:0x00                # data received

[1] https://lore.kernel.org/all/20231211102217.2436294-3-quan@os.amperecomputing.com/

Signed-off-by: Jithu Joseph <jithu.joseph@oss.qualcomm.com>
Fixes: 1602001195dc ("i2c: add aspeed i2c controller")
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Link: https://lore.kernel.org/qemu-devel/20260311023712.2730185-1-jithu.joseph@oss.qualcomm.com
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 include/hw/i2c/aspeed_i2c.h |  1 +
 hw/i2c/aspeed_i2c.c         | 46 +++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 53a9dba71b07..d42cb4865aa5 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -256,6 +256,7 @@ struct AspeedI2CBus {
     qemu_irq irq;
 
     uint32_t regs[ASPEED_I2C_NEW_NUM_REG];
+    uint32_t pending_intr_sts;
     uint8_t pool[ASPEED_I2C_BUS_POOL_SIZE];
     uint64_t dma_dram_offset;
 };
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 8022938f3478..ad6342bec0d4 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -628,6 +628,8 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
     bool handle_rx;
     bool w1t;
+    uint32_t old_intr;
+    uint32_t cmd_intr;
 
     trace_aspeed_i2c_bus_write(bus->id, offset, size, value);
 
@@ -665,6 +667,17 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
             break;
         }
         bus->regs[R_I2CM_INTR_STS] &= ~(value & 0xf007f07f);
+        /*
+         * Re-apply interrupts lost due to synchronous command completion.
+         * When a command completes instantly during an MMIO write, the new
+         * interrupt status bits collide with already-pending bits. After
+         * the ISR clears them, re-apply the saved bits so the ISR can
+         * process the new completion.
+         */
+        if (bus->pending_intr_sts) {
+            bus->regs[R_I2CM_INTR_STS] |= bus->pending_intr_sts;
+            bus->pending_intr_sts = 0;
+        }
         if (!bus->regs[R_I2CM_INTR_STS]) {
             bus->controller->intr_status &= ~(1 << bus->id);
             qemu_irq_lower(aic->bus_get_irq(bus));
@@ -708,7 +721,17 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
             bus->regs[R_I2CM_CMD] = value;
         }
 
+        old_intr = bus->regs[R_I2CM_INTR_STS];
+        bus->regs[R_I2CM_INTR_STS] = 0;
         aspeed_i2c_bus_handle_cmd(bus, value);
+        /*
+         * cmd_intr has only the bits handle_cmd freshly set.
+         * Overlap with old_intr means the same bit was re-fired
+         * and would be lost when the ISR W1C-clears the old one.
+         */
+        cmd_intr = bus->regs[R_I2CM_INTR_STS];
+        bus->regs[R_I2CM_INTR_STS] = cmd_intr | old_intr;
+        bus->pending_intr_sts |= old_intr & cmd_intr;
         aspeed_i2c_bus_raise_interrupt(bus);
         break;
     case A_I2CM_DMA_TX_ADDR:
@@ -845,6 +868,8 @@ static void aspeed_i2c_bus_old_write(AspeedI2CBus *bus, hwaddr offset,
 {
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
     bool handle_rx;
+    uint32_t old_intr;
+    uint32_t cmd_intr;
 
     trace_aspeed_i2c_bus_write(bus->id, offset, size, value);
 
@@ -868,6 +893,17 @@ static void aspeed_i2c_bus_old_write(AspeedI2CBus *bus, hwaddr offset,
         handle_rx = SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CD_INTR_STS, RX_DONE)
                     && SHARED_FIELD_EX32(value, RX_DONE);
         bus->regs[R_I2CD_INTR_STS] &= ~(value & 0x7FFF);
+        /*
+         * Re-apply interrupts lost due to synchronous command completion.
+         * When a command completes instantly during an MMIO write, the new
+         * interrupt status bits collide with already-pending bits. After
+         * the ISR clears them, re-apply the saved bits so the ISR can
+         * process the new completion.
+         */
+        if (bus->pending_intr_sts) {
+            bus->regs[R_I2CD_INTR_STS] |= bus->pending_intr_sts;
+            bus->pending_intr_sts = 0;
+        }
         if (!bus->regs[R_I2CD_INTR_STS]) {
             bus->controller->intr_status &= ~(1 << bus->id);
             qemu_irq_lower(aic->bus_get_irq(bus));
@@ -915,7 +951,17 @@ static void aspeed_i2c_bus_old_write(AspeedI2CBus *bus, hwaddr offset,
         bus->regs[R_I2CD_CMD] &= ~0xFFFF;
         bus->regs[R_I2CD_CMD] |= value & 0xFFFF;
 
+        old_intr = bus->regs[R_I2CD_INTR_STS];
+        bus->regs[R_I2CD_INTR_STS] = 0;
         aspeed_i2c_bus_handle_cmd(bus, value);
+        /*
+         * cmd_intr has only the bits handle_cmd freshly set.
+         * Overlap with old_intr means the same bit was re-fired
+         * and would be lost when the ISR W1C-clears the old one.
+         */
+        cmd_intr = bus->regs[R_I2CD_INTR_STS];
+        bus->regs[R_I2CD_INTR_STS] = cmd_intr | old_intr;
+        bus->pending_intr_sts |= old_intr & cmd_intr;
         aspeed_i2c_bus_raise_interrupt(bus);
         break;
     case A_I2CD_DMA_ADDR:
-- 
2.53.0



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PULL 2/5] MAINTAINERS: Add Kane Chen as reviewer for Aspeed machines
  2026-03-24 12:41 [PULL 0/5] aspeed queue Cédric Le Goater
  2026-03-24 12:41 ` [PULL 1/5] hw/i2c/aspeed: fix lost interrupts on back-to-back commands Cédric Le Goater
@ 2026-03-24 12:41 ` Cédric Le Goater
  2026-03-24 12:41 ` [PULL 3/5] tests/functional/aarch64/test_aspeed: Disable kernel crypto self-tests in AST2700 boot tests Cédric Le Goater
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Cédric Le Goater @ 2026-03-24 12:41 UTC (permalink / raw)
  To: qemu-arm, qemu-devel; +Cc: Jamin Lin, Kane Chen, Cédric Le Goater

From: Jamin Lin <jamin_lin@aspeedtech.com>

Signed-off-by: Kane Chen <kane_chen@aspeedtech.com>
Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Link: https://lore.kernel.org/qemu-devel/20260316070347.3079299-1-jamin_lin@aspeedtech.com
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 25fb621c30a4..8d837677353a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1247,6 +1247,7 @@ M: Peter Maydell <peter.maydell@linaro.org>
 R: Steven Lee <steven_lee@aspeedtech.com>
 R: Troy Lee <leetroy@gmail.com>
 R: Jamin Lin <jamin_lin@aspeedtech.com>
+R: Kane Chen <kane_chen@aspeedtech.com>
 R: Andrew Jeffery <andrew@codeconstruct.com.au>
 R: Joel Stanley <joel@jms.id.au>
 L: qemu-arm@nongnu.org
-- 
2.53.0



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PULL 3/5] tests/functional/aarch64/test_aspeed: Disable kernel crypto self-tests in AST2700 boot tests
  2026-03-24 12:41 [PULL 0/5] aspeed queue Cédric Le Goater
  2026-03-24 12:41 ` [PULL 1/5] hw/i2c/aspeed: fix lost interrupts on back-to-back commands Cédric Le Goater
  2026-03-24 12:41 ` [PULL 2/5] MAINTAINERS: Add Kane Chen as reviewer for Aspeed machines Cédric Le Goater
@ 2026-03-24 12:41 ` Cédric Le Goater
  2026-03-24 12:41 ` [PULL 4/5] hw/ssi/aspeed_smc: Convert mem ops to read/write_with_attrs for error handling Cédric Le Goater
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Cédric Le Goater @ 2026-03-24 12:41 UTC (permalink / raw)
  To: qemu-arm, qemu-devel; +Cc: Jamin Lin, Cédric Le Goater

From: Jamin Lin <jamin_lin@aspeedtech.com>

Disable the kernel crypto self-tests in the AST2700 functional tests by
appending "cryptomgr.notests=1" to the U-Boot bootargs before booting
the kernel.

The ASPEED SDK enables crypto self-tests during kernel startup to
validate the hardware crypto engine. However, the current QEMU
implementation of the AST2700 HACE/crypto engine is still incomplete.
As a result, the kernel crypto self-tests trigger multiple warnings
during boot when running under QEMU.

Typical examples observed in the kernel log include failures for
several cipher modes such as DES/TDES/AES in ECB/CBC/CTR modes:

alg: self-tests for ctr(des) using aspeed-ctr-des failed (rc=-22)
alg: self-tests for ecb(des3_ede) using aspeed-ecb-tdes failed (rc=-22)
alg: self-tests for cbc(aes) using aspeed-cbc-aes failed (rc=-22)
...

To reduce noise in the functional test logs, the tests now append
the following parameter to the kernel bootargs:

  cryptomgr.notests=1

This disables the kernel crypto self-tests when running the functional
tests under QEMU.

For validating the HACE implementation, we should instead rely on the
dedicated QEMU unit tests located in:

  tests/qtest/ast2700-hace-test.c

Once the QEMU implementation of the ASPEED HACE/crypto model has
progressed further and supports the missing crypto modes, we can
reassess whether enabling the kernel crypto self-tests again in the
functional tests is appropriate.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Link: https://lore.kernel.org/qemu-devel/20260316081549.1279841-1-jamin_lin@aspeedtech.com
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 tests/functional/aarch64/test_aspeed_ast2700a1.py | 11 +++++++++--
 tests/functional/aarch64/test_aspeed_ast2700a2.py | 11 +++++++++--
 tests/functional/aarch64/test_aspeed_ast2700fc.py |  9 +++++++--
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/tests/functional/aarch64/test_aspeed_ast2700a1.py b/tests/functional/aarch64/test_aspeed_ast2700a1.py
index 5c0c4b0ed50f..b0c08854daa9 100755
--- a/tests/functional/aarch64/test_aspeed_ast2700a1.py
+++ b/tests/functional/aarch64/test_aspeed_ast2700a1.py
@@ -51,9 +51,11 @@ def verify_vbootrom_firmware_flow(self):
         wait_for_console_pattern(self, 'pass')
         wait_for_console_pattern(self, 'Jumping to BL31 (Trusted Firmware-A)')
 
+    def disable_kernel_crypto_selftest(self):
+         exec_command_and_wait_for_pattern(self,
+            'setenv bootargs "${bootargs} cryptomgr.notests=1"', '=>')
+
     def enable_ast2700_pcie2(self):
-        wait_for_console_pattern(self, 'Hit any key to stop autoboot')
-        exec_command_and_wait_for_pattern(self, '\012', '=>')
         exec_command_and_wait_for_pattern(self,
             'cp 100420000 403000000 900000', '=>')
         exec_command_and_wait_for_pattern(self,
@@ -67,8 +69,13 @@ def enable_ast2700_pcie2(self):
 
     def verify_openbmc_boot_start(self, enable_pcie=True):
         wait_for_console_pattern(self, 'U-Boot 2023.10')
+        wait_for_console_pattern(self, 'Hit any key to stop autoboot')
+        exec_command_and_wait_for_pattern(self, '\012', '=>')
+        self.disable_kernel_crypto_selftest()
         if enable_pcie:
             self.enable_ast2700_pcie2()
+        else:
+            exec_command(self, 'boot')
         wait_for_console_pattern(self, 'Linux version ')
 
     def verify_openbmc_boot_and_login(self, name, enable_pcie=True):
diff --git a/tests/functional/aarch64/test_aspeed_ast2700a2.py b/tests/functional/aarch64/test_aspeed_ast2700a2.py
index cc62a915b538..ed414999f4fa 100755
--- a/tests/functional/aarch64/test_aspeed_ast2700a2.py
+++ b/tests/functional/aarch64/test_aspeed_ast2700a2.py
@@ -51,9 +51,11 @@ def verify_vbootrom_firmware_flow(self):
         wait_for_console_pattern(self, 'pass')
         wait_for_console_pattern(self, 'Jumping to BL31 (Trusted Firmware-A)')
 
+    def disable_kernel_crypto_selftest(self):
+         exec_command_and_wait_for_pattern(self,
+            'setenv bootargs "${bootargs} cryptomgr.notests=1"', '=>')
+
     def enable_ast2700_pcie2(self):
-        wait_for_console_pattern(self, 'Hit any key to stop autoboot')
-        exec_command_and_wait_for_pattern(self, '\012', '=>')
         exec_command_and_wait_for_pattern(self,
             'cp 100420000 403000000 900000', '=>')
         exec_command_and_wait_for_pattern(self,
@@ -67,8 +69,13 @@ def enable_ast2700_pcie2(self):
 
     def verify_openbmc_boot_start(self, enable_pcie=True):
         wait_for_console_pattern(self, 'U-Boot 2023.10')
+        wait_for_console_pattern(self, 'Hit any key to stop autoboot')
+        exec_command_and_wait_for_pattern(self, '\012', '=>')
+        self.disable_kernel_crypto_selftest()
         if enable_pcie:
             self.enable_ast2700_pcie2()
+        else:
+            exec_command(self, 'boot')
         wait_for_console_pattern(self, 'Linux version ')
 
     def verify_openbmc_boot_and_login(self, name, enable_pcie=True):
diff --git a/tests/functional/aarch64/test_aspeed_ast2700fc.py b/tests/functional/aarch64/test_aspeed_ast2700fc.py
index f68f40a1bfaf..df889134edaa 100755
--- a/tests/functional/aarch64/test_aspeed_ast2700fc.py
+++ b/tests/functional/aarch64/test_aspeed_ast2700fc.py
@@ -27,9 +27,11 @@ def do_test_aarch64_aspeed_sdk_start(self, image):
 
         self.vm.launch()
 
+    def disable_kernel_crypto_selftest(self):
+         exec_command_and_wait_for_pattern(self,
+            'setenv bootargs "${bootargs} cryptomgr.notests=1"', '=>')
+
     def enable_ast2700_pcie2(self):
-        wait_for_console_pattern(self, 'Hit any key to stop autoboot')
-        exec_command_and_wait_for_pattern(self, '\012', '=>')
         exec_command_and_wait_for_pattern(self,
             'cp 100420000 403000000 900000', '=>')
         exec_command_and_wait_for_pattern(self,
@@ -43,6 +45,9 @@ def enable_ast2700_pcie2(self):
 
     def verify_openbmc_boot_and_login(self, name):
         wait_for_console_pattern(self, 'U-Boot 2023.10')
+        wait_for_console_pattern(self, 'Hit any key to stop autoboot')
+        exec_command_and_wait_for_pattern(self, '\012', '=>')
+        self.disable_kernel_crypto_selftest()
         self.enable_ast2700_pcie2()
         wait_for_console_pattern(self, 'Starting kernel ...')
 
-- 
2.53.0



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PULL 4/5] hw/ssi/aspeed_smc: Convert mem ops to read/write_with_attrs for error handling
  2026-03-24 12:41 [PULL 0/5] aspeed queue Cédric Le Goater
                   ` (2 preceding siblings ...)
  2026-03-24 12:41 ` [PULL 3/5] tests/functional/aarch64/test_aspeed: Disable kernel crypto self-tests in AST2700 boot tests Cédric Le Goater
@ 2026-03-24 12:41 ` Cédric Le Goater
  2026-03-24 12:41 ` [PULL 5/5] hw/i2c/aspeed_i2c: Remove assert Cédric Le Goater
  2026-03-24 18:35 ` [PULL 0/5] aspeed queue Peter Maydell
  5 siblings, 0 replies; 7+ messages in thread
From: Cédric Le Goater @ 2026-03-24 12:41 UTC (permalink / raw)
  To: qemu-arm, qemu-devel; +Cc: Cédric Le Goater, Jamin Lin

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/20260323125545.577653-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



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PULL 5/5] hw/i2c/aspeed_i2c: Remove assert
  2026-03-24 12:41 [PULL 0/5] aspeed queue Cédric Le Goater
                   ` (3 preceding siblings ...)
  2026-03-24 12:41 ` [PULL 4/5] hw/ssi/aspeed_smc: Convert mem ops to read/write_with_attrs for error handling Cédric Le Goater
@ 2026-03-24 12:41 ` Cédric Le Goater
  2026-03-24 18:35 ` [PULL 0/5] aspeed queue Peter Maydell
  5 siblings, 0 replies; 7+ messages in thread
From: Cédric Le Goater @ 2026-03-24 12:41 UTC (permalink / raw)
  To: qemu-arm, qemu-devel; +Cc: Cédric Le Goater, Jamin Lin

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/20260323125545.577653-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



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PULL 0/5] aspeed queue
  2026-03-24 12:41 [PULL 0/5] aspeed queue Cédric Le Goater
                   ` (4 preceding siblings ...)
  2026-03-24 12:41 ` [PULL 5/5] hw/i2c/aspeed_i2c: Remove assert Cédric Le Goater
@ 2026-03-24 18:35 ` Peter Maydell
  5 siblings, 0 replies; 7+ messages in thread
From: Peter Maydell @ 2026-03-24 18:35 UTC (permalink / raw)
  To: Cédric Le Goater; +Cc: qemu-arm, qemu-devel

On Tue, 24 Mar 2026 at 12:41, Cédric Le Goater <clg@redhat.com> wrote:
>
> The following changes since commit 750ed363ebfb108082b40bc532b20564bf00f913:
>
>   Merge tag 'hw-misc-20260323' of https://github.com/philmd/qemu into staging (2026-03-23 16:58:15 +0000)
>
> are available in the Git repository at:
>
>   https://github.com/legoater/qemu/ tags/pull-aspeed-20260324
>
> for you to fetch changes up to 5fca367b89de2d11631cb27a90a8c5431d004f58:
>
>   hw/i2c/aspeed_i2c: Remove assert (2026-03-24 11:19:40 +0100)
>
> ----------------------------------------------------------------
> aspeed queue:
>
> * Rework Aspeed SMC mem ops to improve error handling
> * Fix race in Aspeed I2C model
> * Disable kernel crypto self-tests in AST2700 boot tests
>



Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/11.0
for any user-visible changes.

-- PMM


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-03-24 18:36 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-24 12:41 [PULL 0/5] aspeed queue Cédric Le Goater
2026-03-24 12:41 ` [PULL 1/5] hw/i2c/aspeed: fix lost interrupts on back-to-back commands Cédric Le Goater
2026-03-24 12:41 ` [PULL 2/5] MAINTAINERS: Add Kane Chen as reviewer for Aspeed machines Cédric Le Goater
2026-03-24 12:41 ` [PULL 3/5] tests/functional/aarch64/test_aspeed: Disable kernel crypto self-tests in AST2700 boot tests Cédric Le Goater
2026-03-24 12:41 ` [PULL 4/5] hw/ssi/aspeed_smc: Convert mem ops to read/write_with_attrs for error handling Cédric Le Goater
2026-03-24 12:41 ` [PULL 5/5] hw/i2c/aspeed_i2c: Remove assert Cédric Le Goater
2026-03-24 18:35 ` [PULL 0/5] aspeed queue Peter Maydell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox