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

["Cédric Le Goater" <clg@kaod.org>]

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 <tan@siewert.io>

Could you please resend your R-b ?

Thanks,

C.



> ---
> 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: