Re: [PATCH v4] hw/misc/aspeed_scu: Handle AST2600 protection key registers correctly

[Tan Siewert]

On 20.06.25 03:23, Jamin Lin wrote:

Subject: Re: [PATCH v4] hw/misc/aspeed_scu: Handle AST2600 protection key
registers correctly

Jamin,

On 6/19/25 10:53, Tan Siewert wrote:

The AST2600 SCU has two protection key registers (0x00 and 0x10) that
both need to be unlocked. (Un-)locking 0x00 modifies both protection
key registers, while modifying 0x10 only modifies itself.

This commit updates the SCU write logic to reject writes unless both
protection key registers are unlocked, matching the behaviour of real
hardware.

Signed-off-by: Tan Siewert <t...@siewert.io>

Could you please resend your R-b ?

Thanks,

C.

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

Thanks-Jamin

Cédric,

Do you think this patch could be applied to aspeed-next?

Thanks,
Tan

---
V4:
    - Fix mis-understanding of or operator in lock check [Tan]
    - Move SCU protection data variable outside of switch case [Cedric]
    - Fix u32 -> uint32_t mistake (now bool as same result) [Cedric]

   hw/misc/aspeed_scu.c | 16 ++++++++++++++--
   1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c index
4930e00fed..39832cd861 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -91,6 +91,7 @@
   #define BMC_DEV_ID           TO_REG(0x1A4)

   #define AST2600_PROT_KEY          TO_REG(0x00)
+#define AST2600_PROT_KEY2         TO_REG(0x10)
   #define AST2600_SILICON_REV       TO_REG(0x04)
   #define AST2600_SILICON_REV2      TO_REG(0x14)
   #define AST2600_SYS_RST_CTRL      TO_REG(0x40)
@@ -722,6 +723,8 @@ static void aspeed_ast2600_scu_write(void *opaque,

hwaddr offset,

       int reg = TO_REG(offset);
       /* Truncate here so bitwise operations below behave as expected */
       uint32_t data = data64;
+    bool prot_data_state = data == ASPEED_SCU_PROT_KEY;
+    bool unlocked = s->regs[AST2600_PROT_KEY] &&
+ s->regs[AST2600_PROT_KEY2];

       if (reg >= ASPEED_AST2600_SCU_NR_REGS) {
           qemu_log_mask(LOG_GUEST_ERROR, @@ -730,15 +733,24

@@ static

void aspeed_ast2600_scu_write(void *opaque, hwaddr offset,
           return;
       }

-    if (reg > PROT_KEY && !s->regs[PROT_KEY]) {
+    if ((reg != AST2600_PROT_KEY && reg != AST2600_PROT_KEY2) &&
+ !unlocked) {
           qemu_log_mask(LOG_GUEST_ERROR, "%s: SCU is locked!\n",
__func__);
+        return;
       }

       trace_aspeed_scu_write(offset, size, data);

       switch (reg) {
       case AST2600_PROT_KEY:
-        s->regs[reg] = (data == ASPEED_SCU_PROT_KEY) ? 1 : 0;
+        /*
+         * Writing a value to SCU000 will modify both protection
+         * registers to each protection register individually.
+         */
+        s->regs[AST2600_PROT_KEY] = prot_data_state;
+        s->regs[AST2600_PROT_KEY2] = prot_data_state;
+        return;
+    case AST2600_PROT_KEY2:
+        s->regs[AST2600_PROT_KEY2] = prot_data_state;
           return;
       case AST2600_HW_STRAP1:
       case AST2600_HW_STRAP2: