* [PATCH v4] hw/misc/aspeed_scu: Handle AST2600 protection key registers correctly
@ 2025-06-19 8:53 Tan Siewert
2025-06-19 11:58 ` Cédric Le Goater
0 siblings, 1 reply; 5+ messages in thread
From: Tan Siewert @ 2025-06-19 8:53 UTC (permalink / raw)
To: qemu-devel
Cc: Tan Siewert, Cédric Le Goater, Peter Maydell, Steven Lee,
Troy Lee, Jamin Lin, Andrew Jeffery, Joel Stanley, qemu-arm
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>
---
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:
--
2.49.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v4] hw/misc/aspeed_scu: Handle AST2600 protection key registers correctly
2025-06-19 8:53 [PATCH v4] hw/misc/aspeed_scu: Handle AST2600 protection key registers correctly Tan Siewert
@ 2025-06-19 11:58 ` Cédric Le Goater
2025-06-20 1:23 ` Jamin Lin
0 siblings, 1 reply; 5+ messages in thread
From: Cédric Le Goater @ 2025-06-19 11:58 UTC (permalink / raw)
To: Tan Siewert, qemu-devel
Cc: Peter Maydell, Steven Lee, Troy Lee, Jamin Lin, Andrew Jeffery,
Joel Stanley, qemu-arm
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:
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH v4] hw/misc/aspeed_scu: Handle AST2600 protection key registers correctly
2025-06-19 11:58 ` Cédric Le Goater
@ 2025-06-20 1:23 ` Jamin Lin
2025-06-27 10:16 ` Tan Siewert
0 siblings, 1 reply; 5+ messages in thread
From: Jamin Lin @ 2025-06-20 1:23 UTC (permalink / raw)
To: Cédric Le Goater, Tan Siewert, qemu-devel@nongnu.org
Cc: Peter Maydell, Steven Lee, Troy Lee, Andrew Jeffery, Joel Stanley,
qemu-arm@nongnu.org
> 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 <tan@siewert.io>
>
> Could you please resend your R-b ?
>
> Thanks,
>
> C.
>
>
Reviewed-by: Jamin Lin <jamin_lin@aspeedtech.com>
Thanks-Jamin
>
> > ---
> > 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:
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v4] hw/misc/aspeed_scu: Handle AST2600 protection key registers correctly
2025-06-20 1:23 ` Jamin Lin
@ 2025-06-27 10:16 ` Tan Siewert
2025-06-27 11:32 ` Cédric Le Goater
0 siblings, 1 reply; 5+ messages in thread
From: Tan Siewert @ 2025-06-27 10:16 UTC (permalink / raw)
To: Jamin Lin, Cédric Le Goater, qemu-devel@nongnu.org
Cc: Peter Maydell, Steven Lee, Troy Lee, Andrew Jeffery, Joel Stanley,
qemu-arm@nongnu.org
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 <tan@siewert.io>
>>
>> Could you please resend your R-b ?
>>
>> Thanks,
>>
>> C.
>>
>>
>
> Reviewed-by: Jamin Lin <jamin_lin@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:
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v4] hw/misc/aspeed_scu: Handle AST2600 protection key registers correctly
2025-06-27 10:16 ` Tan Siewert
@ 2025-06-27 11:32 ` Cédric Le Goater
0 siblings, 0 replies; 5+ messages in thread
From: Cédric Le Goater @ 2025-06-27 11:32 UTC (permalink / raw)
To: Tan Siewert, Jamin Lin, qemu-devel@nongnu.org
Cc: Peter Maydell, Steven Lee, Troy Lee, Andrew Jeffery, Joel Stanley,
qemu-arm@nongnu.org
On 6/27/25 12:16, Tan Siewert wrote:
> 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 <tan@siewert.io>
>>>
>>> Could you please resend your R-b ?
>>>
>>> Thanks,
>>>
>>> C.
>>>
>>>
>>
>> Reviewed-by: Jamin Lin <jamin_lin@aspeedtech.com>
>>
>> Thanks-Jamin
>
> Cédric,
>
> Do you think this patch could be applied to aspeed-next?
I did a week ago.
https://github.com/legoater/qemu/commit/7b27524a27a3e835b2e6b725e1778950015cbb63
Thanks,
C.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-06-27 11:32 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-19 8:53 [PATCH v4] hw/misc/aspeed_scu: Handle AST2600 protection key registers correctly Tan Siewert
2025-06-19 11:58 ` Cédric Le Goater
2025-06-20 1:23 ` Jamin Lin
2025-06-27 10:16 ` Tan Siewert
2025-06-27 11:32 ` Cédric Le Goater
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).