1. Patchew
  2. QEMU
  3. View series

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

Tan Siewert posted 1 patch 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20250612144052.22478-1-tan@siewert.io
Maintainers: "Cédric Le Goater" <clg@kaod.org>, Peter Maydell <peter.maydell@linaro.org>, Steven Lee <steven_lee@aspeedtech.com>, Troy Lee <leetroy@gmail.com>, Jamin Lin <jamin_lin@aspeedtech.com>, Andrew Jeffery <andrew@codeconstruct.com.au>, Joel Stanley <joel@jms.id.au>
There is a newer version of this series
hw/misc/aspeed_scu.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
[PATCH v2] hw/misc/aspeed_scu: Handle AST2600 protection key registers correctly
Posted by Tan Siewert 8 months ago 
The AST2600 SCU has two protection key registers (0x00 and 0x10) that
both need to be unlocked. Each must be unlocked individually, but
locking one will lock both.

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 <tan@siewert.io>
---
V2:
  - Fix protection key register check to be an OR instead of AND
  - Add missing return if SCU is locked (like for AST2500)

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

diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index 4930e00fed..4dcfe8f7b4 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,7 @@ 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 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 +732,27 @@ 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;
+    case AST2600_PROT_KEY2:
+        /*
+         * Writing a value other than the protection key will lock
+         * both protection registers, but unlocking must be done
+         * to each protection register individually.
+         */
+        if (data != ASPEED_SCU_PROT_KEY) {
+            s->regs[AST2600_PROT_KEY] = 0;
+            s->regs[AST2600_PROT_KEY2] = 0;
+        } else {
+            s->regs[reg] = (data == ASPEED_SCU_PROT_KEY) ? 1 : 0;
+        }
         return;
     case AST2600_HW_STRAP1:
     case AST2600_HW_STRAP2:
-- 
2.49.0
RE: [PATCH v2] hw/misc/aspeed_scu: Handle AST2600 protection key registers correctly
Posted by Jamin Lin 8 months ago 
Hi Tan,

> Subject: [PATCH v2] hw/misc/aspeed_scu: Handle AST2600 protection key
> registers correctly
> 
> The AST2600 SCU has two protection key registers (0x00 and 0x10) that both
> need to be unlocked. Each must be unlocked individually, but locking one will
> lock both.
> 
> 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 <tan@siewert.io>
> ---
> V2:
>   - Fix protection key register check to be an OR instead of AND
>   - Add missing return if SCU is locked (like for AST2500)
> 
>  hw/misc/aspeed_scu.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c index
> 4930e00fed..4dcfe8f7b4 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,7 @@ 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 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 +732,27 @@ 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;
> +    case AST2600_PROT_KEY2:
> +        /*
> +         * Writing a value other than the protection key will lock
> +         * both protection registers, but unlocking must be done
> +         * to each protection register individually.
> +         */
> +        if (data != ASPEED_SCU_PROT_KEY) {
> +            s->regs[AST2600_PROT_KEY] = 0;
> +            s->regs[AST2600_PROT_KEY2] = 0;
> +        } else {
> +            s->regs[reg] = (data == ASPEED_SCU_PROT_KEY) ? 1 : 0;
               It can set 1 directly.
               s->regs[reg] = 1;
> +        }
>          return;
According to the datasheet description: it seems your changes do not match the actual hardware behavior.
Protection Key
This register is designed to protect SCU registers from unpredictable updates,
especially when ARM CPU is out of control.
The password of the protection key is 0x1688A8A8.
Unlock SCU registers: Write 0x1688A8A8 to this register
Lock SCU registers: Write others value to this register
Only firmware can lock the SCU registers, other softwares (ex. system
BIOS/driver) can not do this to prevent disturbing the operation of firmware.
When this register is unlocked, the read back value of this register is 0x00000001.
When this register is locked, the read back value of this register is 0x00000000.
Writing to SCU000 can be seen on both SCU000 and SCU010.
Writing to SCU010 is only on SCU010 itself. SCU000 does not change.

I The following results were tested on the AST2600 EVB.
Could you please verify the QEMU behavior?

Writing to SCU000 can be seen on both SCU000 and SCU010.
Writing to SCU010 is only on SCU010 itself. SCU000 does not change.

1. locked status
ast# md 1e6e2000
1e6e2000: 00000000                               ....
ast# md 1e6e2010
1e6e2010: 00000000

unlocked protection key
mw 1e6e2000 1688A8A8

both key and key1 unlock
ast# md 1e6e2000
1e6e2000: 00000001                               ....
ast# md 1e6e2010
1e6e2010: 0000000

lock protection key
ast# mw 1e6e2000 abcd

both key and key1 unlock
ast# md 1e6e2000
1e6e2000: 00000000                               ....
ast# md 1e6e2010
1e6e2010: 00000000

unlocked protection key1
ast# mw 1e6e2010 1688A8A8
ast# md 1e6e2000

Only protection key 1 unlock
1e6e2000: 00000000                               ....
ast# md 1e6e2010
1e6e2010: 00000001

Thanks-Jamin

>      case AST2600_HW_STRAP1:
>      case AST2600_HW_STRAP2:
> --
> 2.49.0
RE: [PATCH v2] hw/misc/aspeed_scu: Handle AST2600 protection key registers correctly
Posted by Jamin Lin 8 months ago 
> registers correctly
> 
> Hi Tan,
> 
> > Subject: [PATCH v2] hw/misc/aspeed_scu: Handle AST2600 protection key
> > registers correctly
> >
> > The AST2600 SCU has two protection key registers (0x00 and 0x10) that
> > both need to be unlocked. Each must be unlocked individually, but
> > locking one will lock both.
> >
> > 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 <tan@siewert.io>
> > ---
> > V2:
> >   - Fix protection key register check to be an OR instead of AND
> >   - Add missing return if SCU is locked (like for AST2500)
> >
> >  hw/misc/aspeed_scu.c | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c index
> > 4930e00fed..4dcfe8f7b4 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,7 @@ 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 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 +732,27 @@
> 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;
> > +    case AST2600_PROT_KEY2:
> > +        /*
> > +         * Writing a value other than the protection key will lock
> > +         * both protection registers, but unlocking must be done
> > +         * to each protection register individually.
> > +         */
> > +        if (data != ASPEED_SCU_PROT_KEY) {
> > +            s->regs[AST2600_PROT_KEY] = 0;
> > +            s->regs[AST2600_PROT_KEY2] = 0;
> > +        } else {
> > +            s->regs[reg] = (data == ASPEED_SCU_PROT_KEY) ? 1 : 0;
>                It can set 1 directly.
>                s->regs[reg] = 1;
> > +        }
> >          return;
> According to the datasheet description: it seems your changes do not match
> the actual hardware behavior.
> Protection Key
> This register is designed to protect SCU registers from unpredictable updates,
> especially when ARM CPU is out of control.
> The password of the protection key is 0x1688A8A8.
> Unlock SCU registers: Write 0x1688A8A8 to this register Lock SCU registers:
> Write others value to this register Only firmware can lock the SCU registers,
> other softwares (ex. system
> BIOS/driver) can not do this to prevent disturbing the operation of firmware.
> When this register is unlocked, the read back value of this register is
> 0x00000001.
> When this register is locked, the read back value of this register is 0x00000000.
> Writing to SCU000 can be seen on both SCU000 and SCU010.
> Writing to SCU010 is only on SCU010 itself. SCU000 does not change.
> 
> I The following results were tested on the AST2600 EVB.
The following results were tested on the AST2600 EVB.
Sorry for typo

> Could you please verify the QEMU behavior?
> 
> Writing to SCU000 can be seen on both SCU000 and SCU010.
> Writing to SCU010 is only on SCU010 itself. SCU000 does not change.
> 
> 1. locked status
> ast# md 1e6e2000
> 1e6e2000: 00000000                               ....
> ast# md 1e6e2010
> 1e6e2010: 00000000
> 
> unlocked protection key
> mw 1e6e2000 1688A8A8
> 
> both key and key1 unlock
> ast# md 1e6e2000
> 1e6e2000: 00000001                               ....
> ast# md 1e6e2010
> 1e6e2010: 0000000
> 
Sorry for typo
1e6e2010: 00000001

> lock protection key
> ast# mw 1e6e2000 abcd
> 
> both key and key1 unlock
> ast# md 1e6e2000
> 1e6e2000: 00000000                               ....
> ast# md 1e6e2010
> 1e6e2010: 00000000
> 
> unlocked protection key1
> ast# mw 1e6e2010 1688A8A8
> ast# md 1e6e2000
> 
> Only protection key 1 unlock
> 1e6e2000: 00000000                               ....
> ast# md 1e6e2010
> 1e6e2010: 00000001
> 
> Thanks-Jamin
> 
> >      case AST2600_HW_STRAP1:
> >      case AST2600_HW_STRAP2:
> > --
> > 2.49.0
>
Re: [PATCH v2] hw/misc/aspeed_scu: Handle AST2600 protection key registers correctly
Posted by Tan Siewert 8 months ago 
Many Thanks for your review, Jamin!

On 13.06.25 05:01, Jamin Lin wrote:
>> According to the datasheet description: it seems your changes do not match
>> the actual hardware behavior.
>> Protection Key
>> This register is designed to protect SCU registers from unpredictable updates,
>> especially when ARM CPU is out of control.
>> The password of the protection key is 0x1688A8A8.
>> Unlock SCU registers: Write 0x1688A8A8 to this register Lock SCU registers:
>> Write others value to this register Only firmware can lock the SCU registers,
>> other softwares (ex. system
>> BIOS/driver) can not do this to prevent disturbing the operation of firmware.
>> When this register is unlocked, the read back value of this register is
>> 0x00000001.
>> When this register is locked, the read back value of this register is 0x00000000.
>> Writing to SCU000 can be seen on both SCU000 and SCU010.
>> Writing to SCU010 is only on SCU010 itself. SCU000 does not change.
>>

Interesting. I tried something similar but apparently did not enough 
research on my AST2600-A3 card (or messed smth else up during research).

You're right, (un-)locking SCU000 will (un-)lock SCU010 too, but not 
vice versa.

>> Could you please verify the QEMU behavior?

Right now, locking either SCU000 or SCU010 will be reflected on both 
registers, and unlocking must be always done separately.

I guess it's time for a v3 then.

Cheers,
Tan

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.