[PATCH v1 0/2] hw/misc/aspeed_sbc: Fix ABR and secure boot state reporting

[PATCH v1 0/2] hw/misc/aspeed_sbc: Fix ABR and secure boot state reporting

[Kane Chen]

The ASPEED Secure Boot Controller (SBC) model currently derives its
eMMC ABR (Alternate Boot Recovery) and secure boot enable bits from
fixed "emmc-abr"/"signing-settings" machine properties. The logic
that reflects these properties into R_STATUS is broken, so neither
bit can ever actually be reported as enabled, and for secure boot
this also leaves R_STATUS and R_QSR inconsistent with each other.

This series replaces both properties with logic that derives the ABR
and secure boot state directly from the relevant OTP configuration
straps, matching how the real hardware determines these settings.

Any feedback or suggestions are appreciated!

Kane-Chen-AS (2):
  hw/misc/aspeed_sbc: Derive ABR state from OTP config straps
  hw/misc/aspeed_sbc: Derive secure boot state from OTP config straps

 include/hw/misc/aspeed_sbc.h |  3 --
 hw/misc/aspeed_sbc.c         | 69 +++++++++++++++++++++++++++++-------
 2 files changed, 56 insertions(+), 16 deletions(-)

-- 
2.43.0

[PATCH v1 1/2] hw/misc/aspeed_sbc: Derive ABR state from OTP config straps

[Kane Chen]

The eMMC ABR (Alternate Boot Recovery) enable state was controlled by
a fixed "emmc-abr" machine property, and the logic reflecting it in
R_STATUS was broken so ABR could never actually be reported as
enabled.

Instead of relying on the property, read the ABR strap value directly
from the OTP configuration space and derive the enable state from it,
matching how the real hardware determines ABR. The now-unused
"emmc-abr" property is removed.

Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
---
 include/hw/misc/aspeed_sbc.h |  1 -
 hw/misc/aspeed_sbc.c         | 35 ++++++++++++++++++++++++++++++++---
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/include/hw/misc/aspeed_sbc.h b/include/hw/misc/aspeed_sbc.h
index 07c7c22a86..7152497b2a 100644
--- a/include/hw/misc/aspeed_sbc.h
+++ b/include/hw/misc/aspeed_sbc.h
@@ -32,7 +32,6 @@ OBJECT_DECLARE_TYPE(AspeedSBCState, AspeedSBCClass, ASPEED_SBC)
 struct AspeedSBCState {
     SysBusDevice parent;
 
-    bool emmc_abr;
     uint32_t signing_settings;
 
     MemoryRegion iomem;
diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c
index 1dfcf14e5b..5d4da39d30 100644
--- a/hw/misc/aspeed_sbc.c
+++ b/hw/misc/aspeed_sbc.c
@@ -60,6 +60,9 @@
 #define MODE_REGISTER_A             (0x3000)
 #define MODE_REGISTER_B             (0x5000)
 
+/* OTP Address */
+#define OTP_CFG0                (0x800)
+
 static uint64_t aspeed_sbc_read(void *opaque, hwaddr addr, unsigned int size)
 {
     AspeedSBCState *s = ASPEED_SBC(opaque);
@@ -261,17 +264,44 @@ static const MemoryRegionOps aspeed_sbc_ops = {
     },
 };
 
+static bool aspeed_get_abr_state(AspeedSBCState *s)
+{
+    uint32_t value;
+    int i;
+    bool enable = false;
+    int config_offset;
+
+    /*
+     * ABR is a strap setting, and each strap setting consists of six
+     * sub-values. Read all sub-values to retrieve the latest setting.
+     */
+    for (i = 17; i < 28; i += 2) {
+        config_offset = OTP_CFG0;
+        config_offset |= (i / 8) * 0x200;
+        config_offset |= (i % 8) * 0x2;
+
+        aspeed_sbc_otp_read(s, config_offset);
+        value = s->regs[R_CAMP1];
+        enable ^= (value >> 11) & 0x1;
+    }
+
+    return enable;
+}
+
 static void aspeed_sbc_reset_hold(Object *obj, ResetType type)
 {
     AspeedSBCState *s = ASPEED_SBC(obj);
+    bool abr;
 
     memset(s->regs, 0, sizeof(s->regs));
 
+    abr = aspeed_get_abr_state(s);
+
     /* Set secure boot enabled with RSA4096_SHA256 and enable eMMC ABR */
     s->regs[R_STATUS] = OTP_IDLE | OTP_MEM_IDLE;
 
-    if (s->emmc_abr) {
-        s->regs[R_STATUS] &= ABR_EN;
+    if (abr) {
+        s->regs[R_STATUS] |= ABR_EN;
     }
 
     if (s->signing_settings) {
@@ -323,7 +353,6 @@ static const VMStateDescription vmstate_aspeed_sbc = {
 };
 
 static const Property aspeed_sbc_properties[] = {
-    DEFINE_PROP_BOOL("emmc-abr", AspeedSBCState, emmc_abr, 0),
     DEFINE_PROP_UINT32("signing-settings", AspeedSBCState, signing_settings, 0),
 };
 
-- 
2.43.0

[PATCH v1 2/2] hw/misc/aspeed_sbc: Derive secure boot state from OTP config straps

[Kane Chen]

The secure boot enable bit in R_STATUS and the R_QSR signing settings
were both driven by a fixed "signing-settings" machine property, but
the two ended up inconsistent: the logic guarding the R_STATUS enable
bit was broken and never actually set it, while R_QSR still reported
the configured signing settings, so a machine could show secure boot
configured in R_QSR while R_STATUS said it was disabled.

Instead of relying on the property, read the relevant OTP
configuration bits directly and derive both the R_STATUS secure boot
enable state and the QSR value from them, matching how the real
hardware determines these settings. The now-unused "signing-settings"
property is removed.

Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
---
 include/hw/misc/aspeed_sbc.h |  2 --
 hw/misc/aspeed_sbc.c         | 34 ++++++++++++++++++++++++----------
 2 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/include/hw/misc/aspeed_sbc.h b/include/hw/misc/aspeed_sbc.h
index 7152497b2a..474922cbd2 100644
--- a/include/hw/misc/aspeed_sbc.h
+++ b/include/hw/misc/aspeed_sbc.h
@@ -32,8 +32,6 @@ OBJECT_DECLARE_TYPE(AspeedSBCState, AspeedSBCClass, ASPEED_SBC)
 struct AspeedSBCState {
     SysBusDevice parent;
 
-    uint32_t signing_settings;
-
     MemoryRegion iomem;
 
     uint32_t regs[ASPEED_SBC_NR_REGS];
diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c
index 5d4da39d30..ce03f717a9 100644
--- a/hw/misc/aspeed_sbc.c
+++ b/hw/misc/aspeed_sbc.c
@@ -63,6 +63,19 @@
 /* OTP Address */
 #define OTP_CFG0                (0x800)
 
+static bool aspeed_sbc_otp_read(AspeedSBCState *s, uint32_t otp_addr);
+
+static uint32_t aspeed_otp_read_cfg0(AspeedSBCState *s)
+{
+    uint32_t value = 0;
+
+    if (aspeed_sbc_otp_read(s, OTP_CFG0)) {
+        value = s->regs[R_CAMP1];
+    }
+
+    return value;
+}
+
 static uint64_t aspeed_sbc_read(void *opaque, hwaddr addr, unsigned int size)
 {
     AspeedSBCState *s = ASPEED_SBC(opaque);
@@ -76,7 +89,12 @@ static uint64_t aspeed_sbc_read(void *opaque, hwaddr addr, unsigned int size)
         return 0;
     }
 
-    return s->regs[addr];
+    switch (addr) {
+    case R_QSR:
+        return aspeed_otp_read_cfg0(s);
+    default:
+        return s->regs[addr];
+    }
 }
 
 static bool aspeed_sbc_otp_read(AspeedSBCState *s,
@@ -291,6 +309,7 @@ static bool aspeed_get_abr_state(AspeedSBCState *s)
 static void aspeed_sbc_reset_hold(Object *obj, ResetType type)
 {
     AspeedSBCState *s = ASPEED_SBC(obj);
+    uint32_t value;
     bool abr;
 
     memset(s->regs, 0, sizeof(s->regs));
@@ -304,11 +323,11 @@ static void aspeed_sbc_reset_hold(Object *obj, ResetType type)
         s->regs[R_STATUS] |= ABR_EN;
     }
 
-    if (s->signing_settings) {
-        s->regs[R_STATUS] &= SECURE_BOOT_EN;
-    }
+    value = aspeed_otp_read_cfg0(s);
 
-    s->regs[R_QSR] = s->signing_settings;
+    if (value & BIT(1)) {
+        s->regs[R_STATUS] |= SECURE_BOOT_EN;
+    }
 }
 
 static void aspeed_sbc_instance_init(Object *obj)
@@ -352,10 +371,6 @@ static const VMStateDescription vmstate_aspeed_sbc = {
     }
 };
 
-static const Property aspeed_sbc_properties[] = {
-    DEFINE_PROP_UINT32("signing-settings", AspeedSBCState, signing_settings, 0),
-};
-
 static void aspeed_sbc_class_init(ObjectClass *klass, const void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -364,7 +379,6 @@ static void aspeed_sbc_class_init(ObjectClass *klass, const void *data)
     dc->realize = aspeed_sbc_realize;
     rc->phases.hold = aspeed_sbc_reset_hold;
     dc->vmsd = &vmstate_aspeed_sbc;
-    device_class_set_props(dc, aspeed_sbc_properties);
 }
 
 
-- 
2.43.0

RE: [PATCH v1 1/2] hw/misc/aspeed_sbc: Derive ABR state from OTP config straps

[Jamin Lin]

> Subject: [PATCH v1 1/2] hw/misc/aspeed_sbc: Derive ABR state from OTP
> config straps
> 
> The eMMC ABR (Alternate Boot Recovery) enable state was controlled by a
> fixed "emmc-abr" machine property, and the logic reflecting it in R_STATUS
> was broken so ABR could never actually be reported as enabled.
> 
> Instead of relying on the property, read the ABR strap value directly from the
> OTP configuration space and derive the enable state from it, matching how the
> real hardware determines ABR. The now-unused "emmc-abr" property is
> removed.
> 
> Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> ---
>  include/hw/misc/aspeed_sbc.h |  1 -
>  hw/misc/aspeed_sbc.c         | 35
> ++++++++++++++++++++++++++++++++---
>  2 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/misc/aspeed_sbc.h b/include/hw/misc/aspeed_sbc.h
> index 07c7c22a86..7152497b2a 100644
> --- a/include/hw/misc/aspeed_sbc.h
> +++ b/include/hw/misc/aspeed_sbc.h
> @@ -32,7 +32,6 @@ OBJECT_DECLARE_TYPE(AspeedSBCState,
> AspeedSBCClass, ASPEED_SBC)  struct AspeedSBCState {
>      SysBusDevice parent;
> 
> -    bool emmc_abr;
>      uint32_t signing_settings;
> 
>      MemoryRegion iomem;
> diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c index
> 1dfcf14e5b..5d4da39d30 100644
> --- a/hw/misc/aspeed_sbc.c
> +++ b/hw/misc/aspeed_sbc.c
> @@ -60,6 +60,9 @@
>  #define MODE_REGISTER_A             (0x3000)
>  #define MODE_REGISTER_B             (0x5000)
> 
> +/* OTP Address */
> +#define OTP_CFG0                (0x800)
> +
>  static uint64_t aspeed_sbc_read(void *opaque, hwaddr addr, unsigned int size)
> {
>      AspeedSBCState *s = ASPEED_SBC(opaque); @@ -261,17 +264,44 @@
> static const MemoryRegionOps aspeed_sbc_ops = {
>      },
>  };
> 
> +static bool aspeed_get_abr_state(AspeedSBCState *s) {
> +    uint32_t value;
> +    int i;
> +    bool enable = false;
> +    int config_offset;
> +
> +    /*
> +     * ABR is a strap setting, and each strap setting consists of six
> +     * sub-values. Read all sub-values to retrieve the latest setting.
> +     */
> +    for (i = 17; i < 28; i += 2) {
> +        config_offset = OTP_CFG0;
> +        config_offset |= (i / 8) * 0x200;
> +        config_offset |= (i % 8) * 0x2;
> +
> +        aspeed_sbc_otp_read(s, config_offset);
> +        value = s->regs[R_CAMP1];
> +        enable ^= (value >> 11) & 0x1;
> +    }
> +
> +    return enable;
> +}
> +
>  static void aspeed_sbc_reset_hold(Object *obj, ResetType type)  {
>      AspeedSBCState *s = ASPEED_SBC(obj);
> +    bool abr;
> 
>      memset(s->regs, 0, sizeof(s->regs));
> 
> +    abr = aspeed_get_abr_state(s);
> +
>      /* Set secure boot enabled with RSA4096_SHA256 and enable eMMC
> ABR */
>      s->regs[R_STATUS] = OTP_IDLE | OTP_MEM_IDLE;
> 
> -    if (s->emmc_abr) {
> -        s->regs[R_STATUS] &= ABR_EN;
> +    if (abr) {
> +        s->regs[R_STATUS] |= ABR_EN;
>      }
> 
>      if (s->signing_settings) {
> @@ -323,7 +353,6 @@ static const VMStateDescription vmstate_aspeed_sbc
> = {  };
> 
>  static const Property aspeed_sbc_properties[] = {
> -    DEFINE_PROP_BOOL("emmc-abr", AspeedSBCState, emmc_abr, 0),
>      DEFINE_PROP_UINT32("signing-settings", AspeedSBCState,
> signing_settings, 0),  };
> 
> --
> 2.43.0

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

RE: [PATCH v1 2/2] hw/misc/aspeed_sbc: Derive secure boot state from OTP config straps

[Jamin Lin]

> -----Original Message-----
> From: Kane Chen <kane_chen@aspeedtech.com>
> Sent: Monday, August 31, 2026 1:37 PM
> To: 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>; open
> list:ASPEED BMCs <qemu-arm@nongnu.org>; open list:All patches CC here
> <qemu-devel@nongnu.org>
> Cc: Troy Lee <troy_lee@aspeedtech.com>; Kane Chen
> <kane_chen@aspeedtech.com>
> Subject: [PATCH v1 2/2] hw/misc/aspeed_sbc: Derive secure boot state from
> OTP config straps
> 
> The secure boot enable bit in R_STATUS and the R_QSR signing settings were
> both driven by a fixed "signing-settings" machine property, but the two ended
> up inconsistent: the logic guarding the R_STATUS enable bit was broken and
> never actually set it, while R_QSR still reported the configured signing settings,
> so a machine could show secure boot configured in R_QSR while R_STATUS
> said it was disabled.
> 
> Instead of relying on the property, read the relevant OTP configuration bits
> directly and derive both the R_STATUS secure boot enable state and the QSR
> value from them, matching how the real hardware determines these settings.
> The now-unused "signing-settings"
> property is removed.
> 
> Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> ---
>  include/hw/misc/aspeed_sbc.h |  2 --
>  hw/misc/aspeed_sbc.c         | 34 ++++++++++++++++++++++++----------
>  2 files changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/include/hw/misc/aspeed_sbc.h b/include/hw/misc/aspeed_sbc.h
> index 7152497b2a..474922cbd2 100644
> --- a/include/hw/misc/aspeed_sbc.h
> +++ b/include/hw/misc/aspeed_sbc.h
> @@ -32,8 +32,6 @@ OBJECT_DECLARE_TYPE(AspeedSBCState,
> AspeedSBCClass, ASPEED_SBC)  struct AspeedSBCState {
>      SysBusDevice parent;
> 
> -    uint32_t signing_settings;
> -
>      MemoryRegion iomem;
> 
>      uint32_t regs[ASPEED_SBC_NR_REGS];
> diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c index
> 5d4da39d30..ce03f717a9 100644
> --- a/hw/misc/aspeed_sbc.c
> +++ b/hw/misc/aspeed_sbc.c
> @@ -63,6 +63,19 @@
>  /* OTP Address */
>  #define OTP_CFG0                (0x800)
> 
> +static bool aspeed_sbc_otp_read(AspeedSBCState *s, uint32_t otp_addr);
> +
> +static uint32_t aspeed_otp_read_cfg0(AspeedSBCState *s) {
> +    uint32_t value = 0;
> +
> +    if (aspeed_sbc_otp_read(s, OTP_CFG0)) {
> +        value = s->regs[R_CAMP1];
> +    }
> +
> +    return value;
> +}
> +
>  static uint64_t aspeed_sbc_read(void *opaque, hwaddr addr, unsigned int size)
> {
>      AspeedSBCState *s = ASPEED_SBC(opaque); @@ -76,7 +89,12 @@ static
> uint64_t aspeed_sbc_read(void *opaque, hwaddr addr, unsigned int size)
>          return 0;
>      }
> 
> -    return s->regs[addr];
> +    switch (addr) {
> +    case R_QSR:
> +        return aspeed_otp_read_cfg0(s);
> +    default:
> +        return s->regs[addr];
> +    }
>  }
> 
>  static bool aspeed_sbc_otp_read(AspeedSBCState *s, @@ -291,6 +309,7 @@
> static bool aspeed_get_abr_state(AspeedSBCState *s)  static void
> aspeed_sbc_reset_hold(Object *obj, ResetType type)  {
>      AspeedSBCState *s = ASPEED_SBC(obj);
> +    uint32_t value;
>      bool abr;
> 
>      memset(s->regs, 0, sizeof(s->regs)); @@ -304,11 +323,11 @@ static void
> aspeed_sbc_reset_hold(Object *obj, ResetType type)
>          s->regs[R_STATUS] |= ABR_EN;
>      }
> 
> -    if (s->signing_settings) {
> -        s->regs[R_STATUS] &= SECURE_BOOT_EN;
> -    }
> +    value = aspeed_otp_read_cfg0(s);
> 
> -    s->regs[R_QSR] = s->signing_settings;
> +    if (value & BIT(1)) {
> +        s->regs[R_STATUS] |= SECURE_BOOT_EN;
> +    }
>  }
> 
>  static void aspeed_sbc_instance_init(Object *obj) @@ -352,10 +371,6 @@
> static const VMStateDescription vmstate_aspeed_sbc = {
>      }
>  };
> 
> -static const Property aspeed_sbc_properties[] = {
> -    DEFINE_PROP_UINT32("signing-settings", AspeedSBCState,
> signing_settings, 0),
> -};
> -
>  static void aspeed_sbc_class_init(ObjectClass *klass, const void *data)  {
>      DeviceClass *dc = DEVICE_CLASS(klass); @@ -364,7 +379,6 @@ static
> void aspeed_sbc_class_init(ObjectClass *klass, const void *data)
>      dc->realize = aspeed_sbc_realize;
>      rc->phases.hold = aspeed_sbc_reset_hold;
>      dc->vmsd = &vmstate_aspeed_sbc;
> -    device_class_set_props(dc, aspeed_sbc_properties);
>  }
> 
> 
> --
> 2.43.0

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

Re: [PATCH v1 1/2] hw/misc/aspeed_sbc: Derive ABR state from OTP config straps

[Cédric Le Goater]

On 8/31/26 07:37, Kane Chen wrote:
> The eMMC ABR (Alternate Boot Recovery) enable state was controlled by
> a fixed "emmc-abr" machine property, and the logic reflecting it in
> R_STATUS was broken so ABR could never actually be reported as
> enabled.
> 
> Instead of relying on the property, read the ABR strap value directly
> from the OTP configuration space and derive the enable state from it,
> matching how the real hardware determines ABR. The now-unused
> "emmc-abr" property is removed.

This is an ABI breakage. It was never used and broken. I guess we are fine.

> Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> ---
>   include/hw/misc/aspeed_sbc.h |  1 -
>   hw/misc/aspeed_sbc.c         | 35 ++++++++++++++++++++++++++++++++---
>   2 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/misc/aspeed_sbc.h b/include/hw/misc/aspeed_sbc.h
> index 07c7c22a86..7152497b2a 100644
> --- a/include/hw/misc/aspeed_sbc.h
> +++ b/include/hw/misc/aspeed_sbc.h
> @@ -32,7 +32,6 @@ OBJECT_DECLARE_TYPE(AspeedSBCState, AspeedSBCClass, ASPEED_SBC)
>   struct AspeedSBCState {
>       SysBusDevice parent;
>   
> -    bool emmc_abr;
>       uint32_t signing_settings;
>   
>       MemoryRegion iomem;
> diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c
> index 1dfcf14e5b..5d4da39d30 100644
> --- a/hw/misc/aspeed_sbc.c
> +++ b/hw/misc/aspeed_sbc.c
> @@ -60,6 +60,9 @@
>   #define MODE_REGISTER_A             (0x3000)
>   #define MODE_REGISTER_B             (0x5000)
>   
> +/* OTP Address */
> +#define OTP_CFG0                (0x800)
> +

I think that several OTP defines describing the OTP address space
would be better placed in aspeed_otp.h :

     #define OTP_MEMORY_SIZE 0x4000

     #define OTP_DATA_DWORD_COUNT        (0x800)
     #define OTP_TOTAL_DWORD_COUNT       (0x1000)

     /* OTP Address */
     #define OTP_CFG0                (0x800)

Please explain the various regions.

also, does this test makes sense :

     if (otp_addr >= OTP_TOTAL_DWORD_COUNT) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "Invalid OTP addr 0x%x\n",
                       otp_addr);
         return false;
     }

since the transaction on the address_space should fail. Or it could be

Please rework aspeed_sbc_otp_read() which duplicate the same code twice.


   static bool aspeed_otp_read_raw(AspeedSBCState *s, uint32_t otp_addr,
                                   uint32_t *value)
   {
       uint32_t otp_offset = otp_addr << 2;

       if (address_space_read(&s->otp.as, otp_offset, MEMTXATTRS_UNSPECIFIED,
                              value, sizeof(*value)) != MEMTX_OK) {
           qemu_log_mask(LOG_GUEST_ERROR,
                         "Failed to read OTP memory, addr = %x\n", otp_addr);
           return false;
       }

       return true;
   }
>   static uint64_t aspeed_sbc_read(void *opaque, hwaddr addr, unsigned int size)
>   {
>       AspeedSBCState *s = ASPEED_SBC(opaque);
> @@ -261,17 +264,44 @@ static const MemoryRegionOps aspeed_sbc_ops = {
>       },
>   };
>   
> +static bool aspeed_get_abr_state(AspeedSBCState *s)

aspeed_sbc_otp_get_abr_state()

Shouln't this be done in the OP model instead.

> +{
> +    uint32_t value;
> +    int i;
> +    bool enable = false;
> +    int config_offset;
> +
> +    /*
> +     * ABR is a strap setting, and each strap setting consists of six
> +     * sub-values. Read all sub-values to retrieve the latest setting.
> +     */
> +    for (i = 17; i < 28; i += 2) {

sigh. What are these magic values ... ?  Please explain.

> +        config_offset = OTP_CFG0; 
> +        config_offset |= (i / 8) * 0x200;
> +        config_offset |= (i % 8) * 0x2;
> +
> +        aspeed_sbc_otp_read(s, config_offset);

May be introduce a aspeed_sbc_otp_read_config() helper.

> +        value = s->regs[R_CAMP1];
> +        enable ^= (value >> 11) & 0x1;
> +    }
> +
> +    return enable;
> +}
> +
>   static void aspeed_sbc_reset_hold(Object *obj, ResetType type)
>   {
>       AspeedSBCState *s = ASPEED_SBC(obj);
> +    bool abr;
>   
>       memset(s->regs, 0, sizeof(s->regs));
>   
> +    abr = aspeed_get_abr_state(s);

This creates an implicit ordering dependency: works because OTP is
a QOM child of the SBC, so its reset runs first.

So, can't we do that at OTP realize time instead ? OTP content is
non-volatile and doesn't change across resets, so reading it once
at realize time is sufficient.

> +
>       /* Set secure boot enabled with RSA4096_SHA256 and enable eMMC ABR */
>       s->regs[R_STATUS] = OTP_IDLE | OTP_MEM_IDLE;
>   
> -    if (s->emmc_abr) {
> -        s->regs[R_STATUS] &= ABR_EN;
> +    if (abr) {
> +        s->regs[R_STATUS] |= ABR_EN;
>       }
>   
>       if (s->signing_settings) {
> @@ -323,7 +353,6 @@ static const VMStateDescription vmstate_aspeed_sbc = {
>   };
>   
>   static const Property aspeed_sbc_properties[] = {
> -    DEFINE_PROP_BOOL("emmc-abr", AspeedSBCState, emmc_abr, 0),
>       DEFINE_PROP_UINT32("signing-settings", AspeedSBCState, signing_settings, 0),
>   };
>   

Re: [PATCH v1 2/2] hw/misc/aspeed_sbc: Derive secure boot state from OTP config straps

[Cédric Le Goater]

On 8/31/26 07:37, Kane Chen wrote:
> The secure boot enable bit in R_STATUS and the R_QSR signing settings
> were both driven by a fixed "signing-settings" machine property, but
> the two ended up inconsistent: the logic guarding the R_STATUS enable
> bit was broken and never actually set it, while R_QSR still reported
> the configured signing settings, so a machine could show secure boot
> configured in R_QSR while R_STATUS said it was disabled.
> 
> Instead of relying on the property, read the relevant OTP
> configuration bits directly and derive both the R_STATUS secure boot
> enable state and the QSR value from them, matching how the real
> hardware determines these settings. The now-unused "signing-settings"
> property is removed.
> 
> Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> ---
>   include/hw/misc/aspeed_sbc.h |  2 --
>   hw/misc/aspeed_sbc.c         | 34 ++++++++++++++++++++++++----------
>   2 files changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/include/hw/misc/aspeed_sbc.h b/include/hw/misc/aspeed_sbc.h
> index 7152497b2a..474922cbd2 100644
> --- a/include/hw/misc/aspeed_sbc.h
> +++ b/include/hw/misc/aspeed_sbc.h
> @@ -32,8 +32,6 @@ OBJECT_DECLARE_TYPE(AspeedSBCState, AspeedSBCClass, ASPEED_SBC)
>   struct AspeedSBCState {
>       SysBusDevice parent;
>   
> -    uint32_t signing_settings;
> -
>       MemoryRegion iomem;
>   
>       uint32_t regs[ASPEED_SBC_NR_REGS];
> diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c
> index 5d4da39d30..ce03f717a9 100644
> --- a/hw/misc/aspeed_sbc.c
> +++ b/hw/misc/aspeed_sbc.c
> @@ -63,6 +63,19 @@
>   /* OTP Address */
>   #define OTP_CFG0                (0x800)
>   
> +static bool aspeed_sbc_otp_read(AspeedSBCState *s, uint32_t otp_addr);
> +
> +static uint32_t aspeed_otp_read_cfg0(AspeedSBCState *s)
> +{
> +    uint32_t value = 0;
> +
> +    if (aspeed_sbc_otp_read(s, OTP_CFG0)) {

This call is clobbering s->regs[R_CAMP1]. Let's see how we can improve
patch 1 first.

Thanks,

C.


> +        value = s->regs[R_CAMP1];
> +    }
>> +    return value;> +}
> +
>   static uint64_t aspeed_sbc_read(void *opaque, hwaddr addr, unsigned int size)
>   {
>       AspeedSBCState *s = ASPEED_SBC(opaque);
> @@ -76,7 +89,12 @@ static uint64_t aspeed_sbc_read(void *opaque, hwaddr addr, unsigned int size)
>           return 0;
>       }
>   
> -    return s->regs[addr];
> +    switch (addr) {
> +    case R_QSR:
> +        return aspeed_otp_read_cfg0(s);
> +    default:
> +        return s->regs[addr];
> +    }
>   }
>   
>   static bool aspeed_sbc_otp_read(AspeedSBCState *s,
> @@ -291,6 +309,7 @@ static bool aspeed_get_abr_state(AspeedSBCState *s)
>   static void aspeed_sbc_reset_hold(Object *obj, ResetType type)
>   {
>       AspeedSBCState *s = ASPEED_SBC(obj);
> +    uint32_t value;
>       bool abr;
>   
>       memset(s->regs, 0, sizeof(s->regs));
> @@ -304,11 +323,11 @@ static void aspeed_sbc_reset_hold(Object *obj, ResetType type)
>           s->regs[R_STATUS] |= ABR_EN;
>       }
>   
> -    if (s->signing_settings) {
> -        s->regs[R_STATUS] &= SECURE_BOOT_EN;
> -    }
> +    value = aspeed_otp_read_cfg0(s);
>   
> -    s->regs[R_QSR] = s->signing_settings;
> +    if (value & BIT(1)) {
> +        s->regs[R_STATUS] |= SECURE_BOOT_EN;
> +    }
>   }
>   
>   static void aspeed_sbc_instance_init(Object *obj)
> @@ -352,10 +371,6 @@ static const VMStateDescription vmstate_aspeed_sbc = {
>       }
>   };
>   
> -static const Property aspeed_sbc_properties[] = {
> -    DEFINE_PROP_UINT32("signing-settings", AspeedSBCState, signing_settings, 0),
> -};
> -
>   static void aspeed_sbc_class_init(ObjectClass *klass, const void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -364,7 +379,6 @@ static void aspeed_sbc_class_init(ObjectClass *klass, const void *data)
>       dc->realize = aspeed_sbc_realize;
>       rc->phases.hold = aspeed_sbc_reset_hold;
>       dc->vmsd = &vmstate_aspeed_sbc;
> -    device_class_set_props(dc, aspeed_sbc_properties);
>   }
>   
>   

RE: [PATCH v1 1/2] hw/misc/aspeed_sbc: Derive ABR state from OTP config straps

[Kane Chen]

> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Monday, August 31, 2026 7:10 PM
> To: Kane Chen <kane_chen@aspeedtech.com>; 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>; open
> list:ASPEED BMCs <qemu-arm@nongnu.org>; open list:All patches CC here
> <qemu-devel@nongnu.org>
> Cc: Troy Lee <troy_lee@aspeedtech.com>
> Subject: Re: [PATCH v1 1/2] hw/misc/aspeed_sbc: Derive ABR state from OTP
> config straps
> 
> On 8/31/26 07:37, Kane Chen wrote:
> > The eMMC ABR (Alternate Boot Recovery) enable state was controlled by
> > a fixed "emmc-abr" machine property, and the logic reflecting it in
> > R_STATUS was broken so ABR could never actually be reported as
> > enabled.
> >
> > Instead of relying on the property, read the ABR strap value directly
> > from the OTP configuration space and derive the enable state from it,
> > matching how the real hardware determines ABR. The now-unused
> > "emmc-abr" property is removed.
> 
> This is an ABI breakage. It was never used and broken. I guess we are fine.
> 
> > Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> > ---
> >   include/hw/misc/aspeed_sbc.h |  1 -
> >   hw/misc/aspeed_sbc.c         | 35
> ++++++++++++++++++++++++++++++++---
> >   2 files changed, 32 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/hw/misc/aspeed_sbc.h
> > b/include/hw/misc/aspeed_sbc.h index 07c7c22a86..7152497b2a 100644
> > --- a/include/hw/misc/aspeed_sbc.h
> > +++ b/include/hw/misc/aspeed_sbc.h
> > @@ -32,7 +32,6 @@ OBJECT_DECLARE_TYPE(AspeedSBCState,
> AspeedSBCClass, ASPEED_SBC)
> >   struct AspeedSBCState {
> >       SysBusDevice parent;
> >
> > -    bool emmc_abr;
> >       uint32_t signing_settings;
> >
> >       MemoryRegion iomem;
> > diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c index
> > 1dfcf14e5b..5d4da39d30 100644
> > --- a/hw/misc/aspeed_sbc.c
> > +++ b/hw/misc/aspeed_sbc.c
> > @@ -60,6 +60,9 @@
> >   #define MODE_REGISTER_A             (0x3000)
> >   #define MODE_REGISTER_B             (0x5000)
> >
> > +/* OTP Address */
> > +#define OTP_CFG0                (0x800)
> > +
> 
> I think that several OTP defines describing the OTP address space would be
> better placed in aspeed_otp.h :
> 
>      #define OTP_MEMORY_SIZE 0x4000
> 
>      #define OTP_DATA_DWORD_COUNT        (0x800)
>      #define OTP_TOTAL_DWORD_COUNT       (0x1000)
> 
>      /* OTP Address */
>      #define OTP_CFG0                (0x800)
> 
> Please explain the various regions.
> 
> also, does this test makes sense :
> 
>      if (otp_addr >= OTP_TOTAL_DWORD_COUNT) {
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "Invalid OTP addr 0x%x\n",
>                        otp_addr);
>          return false;
>      }
> 
> since the transaction on the address_space should fail. Or it could be
> 
> Please rework aspeed_sbc_otp_read() which duplicate the same code twice.
> 
> 
>    static bool aspeed_otp_read_raw(AspeedSBCState *s, uint32_t otp_addr,
>                                    uint32_t *value)
>    {
>        uint32_t otp_offset = otp_addr << 2;
> 
>        if (address_space_read(&s->otp.as, otp_offset,
> MEMTXATTRS_UNSPECIFIED,
>                               value, sizeof(*value)) != MEMTX_OK) {
>            qemu_log_mask(LOG_GUEST_ERROR,
>                          "Failed to read OTP memory, addr = %x\n",
> otp_addr);
>            return false;
>        }
> 
>        return true;
>    }
> >   static uint64_t aspeed_sbc_read(void *opaque, hwaddr addr, unsigned int
> size)
> >   {
> >       AspeedSBCState *s = ASPEED_SBC(opaque); @@ -261,17 +264,44
> @@
> > static const MemoryRegionOps aspeed_sbc_ops = {
> >       },
> >   };
> >
> > +static bool aspeed_get_abr_state(AspeedSBCState *s)
> 
> aspeed_sbc_otp_get_abr_state()
> 
> Shouln't this be done in the OP model instead.
> 
> > +{
> > +    uint32_t value;
> > +    int i;
> > +    bool enable = false;
> > +    int config_offset;
> > +
> > +    /*
> > +     * ABR is a strap setting, and each strap setting consists of six
> > +     * sub-values. Read all sub-values to retrieve the latest setting.
> > +     */
> > +    for (i = 17; i < 28; i += 2) {
> 
> sigh. What are these magic values ... ?  Please explain.
> 
> > +        config_offset = OTP_CFG0;
> > +        config_offset |= (i / 8) * 0x200;
> > +        config_offset |= (i % 8) * 0x2;
> > +
> > +        aspeed_sbc_otp_read(s, config_offset);
> 
> May be introduce a aspeed_sbc_otp_read_config() helper.
> 
> > +        value = s->regs[R_CAMP1];
> > +        enable ^= (value >> 11) & 0x1;
> > +    }
> > +
> > +    return enable;
> > +}
> > +
> >   static void aspeed_sbc_reset_hold(Object *obj, ResetType type)
> >   {
> >       AspeedSBCState *s = ASPEED_SBC(obj);
> > +    bool abr;
> >
> >       memset(s->regs, 0, sizeof(s->regs));
> >
> > +    abr = aspeed_get_abr_state(s);
> 
> This creates an implicit ordering dependency: works because OTP is a QOM
> child of the SBC, so its reset runs first.
> 
> So, can't we do that at OTP realize time instead ? OTP content is non-volatile
> and doesn't change across resets, so reading it once at realize time is sufficient.
> 
> > +
> >       /* Set secure boot enabled with RSA4096_SHA256 and enable eMMC
> ABR */
> >       s->regs[R_STATUS] = OTP_IDLE | OTP_MEM_IDLE;
> >
> > -    if (s->emmc_abr) {
> > -        s->regs[R_STATUS] &= ABR_EN;
> > +    if (abr) {
> > +        s->regs[R_STATUS] |= ABR_EN;
> >       }
> >
> >       if (s->signing_settings) {
> > @@ -323,7 +353,6 @@ static const VMStateDescription
> vmstate_aspeed_sbc = {
> >   };
> >
> >   static const Property aspeed_sbc_properties[] = {
> > -    DEFINE_PROP_BOOL("emmc-abr", AspeedSBCState, emmc_abr, 0),
> >       DEFINE_PROP_UINT32("signing-settings", AspeedSBCState,
> signing_settings, 0),
> >   };
> >
Hi Cédric,

Thanks for your review and comments. I will address the comments
regarding the OTP address definitions, boundary checks, and code
cleanup.

Regarding the interaction between the SBC and OTP models, I would
like to clarify the expected behavior before making further changes.

On real hardware, some OTP settings are reflected in SBC registers.
For example, the eMMC ABR enable state and secure boot enable state
are reflected in the SBC STATUS register.

However, the OTP configuration can also be programmed at runtime.
Therefore, reading the OTP configuration only once at realize/reset time
may leave the SBC STATUS register out of sync if the OTP
configuration is changed afterward.

I see two possible approaches to keep the SBC STATUS register
synchronized with the OTP configuration:

Update the SBC STATUS register at reset and whenever the relevant
OTP configuration is programmed. This would keep the cached STATUS
value synchronized with the latest OTP configuration.
Derive the relevant SBC STATUS bits directly from the OTP
configuration whenever the STATUS register is read. This avoids
maintaining a cached copy of the OTP-derived state.

Could you please advise which approach would be preferred?

Best Regards,
Kane

Re: [PATCH v1 1/2] hw/misc/aspeed_sbc: Derive ABR state from OTP config straps

[Cédric Le Goater]

Hello,

> However, the OTP configuration can also be programmed at runtime.
> Therefore, reading the OTP configuration only once at realize/reset time
> may leave the SBC STATUS register out of sync if the OTP
> configuration is changed afterward.
> 
> I see two possible approaches to keep the SBC STATUS register
> synchronized with the OTP configuration:
> 
> Update the SBC STATUS register at reset and whenever the relevant
> OTP configuration is programmed. This would keep the cached STATUS
> value synchronized with the latest OTP configuration.

Caching values is always a problem. Unless performance is impacted,
computing is better.

> Derive the relevant SBC STATUS bits directly from the OTP
> configuration whenever the STATUS register is read. This avoids
> maintaining a cached copy of the OTP-derived state.

yes something like :

   static uint32_t aspeed_otp_read_cfg0(AspeedSBCState *s)
  {
@@ -110,6 +111,23 @@ static uint64_t aspeed_sbc_read(void *op
      }
  
      switch (addr) {
+    case R_STATUS: {
+        uint32_t val = s->regs[R_STATUS];
+
+        if (aspeed_get_abr_state(s)) {
+            val |= ABR_EN;
+        } else {
+            val &= ~ABR_EN;
+        }

is better IMO. The SBC status register is a reflection of the OTP
fuse state. You could ask he HW designers for info on how the state
is maintained.

Thanks,

C.

RE: [PATCH v1 1/2] hw/misc/aspeed_sbc: Derive ABR state from OTP config straps

[Kane Chen]

> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Thursday, September 3, 2026 3:23 PM
> To: Kane Chen <kane_chen@aspeedtech.com>; 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>; open
> list:ASPEED BMCs <qemu-arm@nongnu.org>; open list:All patches CC here
> <qemu-devel@nongnu.org>
> Cc: Troy Lee <troy_lee@aspeedtech.com>
> Subject: Re: [PATCH v1 1/2] hw/misc/aspeed_sbc: Derive ABR state from OTP
> config straps
> 
> Hello,
> 
> > However, the OTP configuration can also be programmed at runtime.
> > Therefore, reading the OTP configuration only once at realize/reset
> > time may leave the SBC STATUS register out of sync if the OTP
> > configuration is changed afterward.
> >
> > I see two possible approaches to keep the SBC STATUS register
> > synchronized with the OTP configuration:
> >
> > Update the SBC STATUS register at reset and whenever the relevant OTP
> > configuration is programmed. This would keep the cached STATUS value
> > synchronized with the latest OTP configuration.
> 
> Caching values is always a problem. Unless performance is impacted,
> computing is better.
> 
> > Derive the relevant SBC STATUS bits directly from the OTP
> > configuration whenever the STATUS register is read. This avoids
> > maintaining a cached copy of the OTP-derived state.
> 
> yes something like :
> 
>    static uint32_t aspeed_otp_read_cfg0(AspeedSBCState *s)
>   {
> @@ -110,6 +111,23 @@ static uint64_t aspeed_sbc_read(void *op
>       }
> 
>       switch (addr) {
> +    case R_STATUS: {
> +        uint32_t val = s->regs[R_STATUS];
> +
> +        if (aspeed_get_abr_state(s)) {
> +            val |= ABR_EN;
> +        } else {
> +            val &= ~ABR_EN;
> +        }
> 
> is better IMO. The SBC status register is a reflection of the OTP fuse state. You
> could ask he HW designers for info on how the state is maintained.
> 
> Thanks,
> 
> C.

Hi Cédric,

Thanks for your comments. I will update the code to read the OTP status
dynamically at runtime.

Best Regards,
Kane