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

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

[Kane Chen]

The ABR and secure boot states are currently controlled by the
"emmc-abr" and "signing-settings" machine properties.

On real hardware, these states are determined by the OTP configuration
and reflected in the SBC status registers. This series updates the
model to derive the ABR and secure boot states from OTP instead, and
removes the corresponding machine properties.

The series also consolidates the OTP layout definitions and adds
helpers for reading OTP configuration and strap settings.

Any feedback or suggestions are appreciated!

Kane
---

ChangeLog
---------
v2:
- Move OTP layout definitions to aspeed_otp.h
- Add helpers for reading OTP configuration and strap settings
- Derive ABR and secure boot states using the new OTP helpers

v1:
- Initial version

---

Kane-Chen-AS (3):
  hw/nvram/aspeed_otp, hw/misc/aspeed_sbc: Consolidate OTP layout
    definitions
  hw/misc/aspeed_sbc: Derive ABR state from OTP config straps
  hw/misc/aspeed_sbc: Derive secure boot state from OTP config

 include/hw/misc/aspeed_sbc.h  |  3 --
 include/hw/nvram/aspeed_otp.h | 32 ++++++++++++
 hw/misc/aspeed_sbc.c          | 97 +++++++++++++++++------------------
 hw/nvram/aspeed_otp.c         | 63 +++++++++++++++++++++++
 4 files changed, 143 insertions(+), 52 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] hw/nvram/aspeed_otp, hw/misc/aspeed_sbc: Consolidate OTP layout definitions

[Kane Chen]

Move the OTP address-space layout definitions from aspeed_sbc.c to
aspeed_otp.h, as they describe the OTP device layout.

Add an aspeed_otp_read() helper to reduce duplicated OTP read code.

Remove the redundant OTP address bounds checks, as invalid accesses
are already handled by the OTP address space.

No functional change.

Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
---
 include/hw/nvram/aspeed_otp.h | 13 +++++++++
 hw/misc/aspeed_sbc.c          | 52 +++++++++++------------------------
 2 files changed, 29 insertions(+), 36 deletions(-)

diff --git a/include/hw/nvram/aspeed_otp.h b/include/hw/nvram/aspeed_otp.h
index 3752353860..2b6c2eaaa3 100644
--- a/include/hw/nvram/aspeed_otp.h
+++ b/include/hw/nvram/aspeed_otp.h
@@ -16,6 +16,19 @@
 #define TYPE_ASPEED_OTP "aspeed-otp"
 OBJECT_DECLARE_SIMPLE_TYPE(AspeedOTPState, ASPEED_OTP)
 
+#define OTP_MEMORY_SIZE                 0x4000
+
+/*
+ * The OTP address space is indexed by dword address and is split into
+ * two regions:
+ *
+ *  - [0, OTP_DATA_DWORD_COUNT]: the data region. Each address contains
+ *    64 bits of data.
+ *  - [OTP_DATA_DWORD_COUNT, OTP_MEMORY_SIZE / 4]: the configuration
+ *    region. Each address contains 32 bits of data.
+ */
+#define OTP_DATA_DWORD_COUNT            (0x800)
+
 typedef struct AspeedOTPState {
     DeviceState parent_obj;
 
diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c
index 5c193d9086..3402cef5c3 100644
--- a/hw/misc/aspeed_sbc.c
+++ b/hw/misc/aspeed_sbc.c
@@ -66,15 +66,11 @@
 #define QSR_RSA_MASK           (0x3 << 12)
 #define QSR_HASH_MASK          (0x3 << 10)
 
-#define OTP_MEMORY_SIZE 0x4000
 /* OTP command */
 #define SBC_OTP_CMD_READ 0x23b1e361
 #define SBC_OTP_CMD_WRITE 0x23b1e362
 #define SBC_OTP_CMD_PROG 0x23b1e364
 
-#define OTP_DATA_DWORD_COUNT        (0x800)
-#define OTP_TOTAL_DWORD_COUNT       (0x1000)
-
 /* Voltage mode */
 #define MODE_REGISTER               (0x1000)
 #define MODE_REGISTER_A             (0x3000)
@@ -96,43 +92,34 @@ static uint64_t aspeed_sbc_read(void *opaque, hwaddr addr, unsigned int size)
     return s->regs[addr];
 }
 
-static bool aspeed_sbc_otp_read(AspeedSBCState *s,
-                                   uint32_t otp_addr)
+static bool aspeed_otp_read(AspeedSBCState *s, uint32_t otp_addr,
+                             uint32_t *value)
 {
-    MemTxResult ret;
-    AspeedOTPState *otp = &s->otp;
-    uint32_t value, otp_offset;
-    bool is_data = false;
-
-    if (otp_addr < OTP_DATA_DWORD_COUNT) {
-        is_data = true;
-    } else if (otp_addr >= OTP_TOTAL_DWORD_COUNT) {
+    if (address_space_read(&s->otp.as, otp_addr << 2, MEMTXATTRS_UNSPECIFIED,
+                            value, sizeof(*value)) != MEMTX_OK) {
         qemu_log_mask(LOG_GUEST_ERROR,
-                      "Invalid OTP addr 0x%x\n",
+                      "Failed to read OTP memory, addr = %x\n",
                       otp_addr);
         return false;
     }
 
-    otp_offset = otp_addr << 2;
-    ret = address_space_read(&otp->as, otp_offset, MEMTXATTRS_UNSPECIFIED,
-                             &value, sizeof(value));
-    if (ret != MEMTX_OK) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "Failed to read OTP memory, addr = %x\n",
-                      otp_addr);
+    return true;
+}
+
+static bool aspeed_sbc_otp_read(AspeedSBCState *s,
+                                   uint32_t otp_addr)
+{
+    uint32_t value;
+    bool is_data = otp_addr < OTP_DATA_DWORD_COUNT;
+
+    if (!aspeed_otp_read(s, otp_addr, &value)) {
         return false;
     }
     s->regs[R_CAMP1] = value;
     trace_aspeed_sbc_otp_read(otp_addr, value);
 
     if (is_data) {
-        ret = address_space_read(&otp->as, otp_offset + 4,
-                                 MEMTXATTRS_UNSPECIFIED,
-                                 &value, sizeof(value));
-        if (ret != MEMTX_OK) {
-            qemu_log_mask(LOG_GUEST_ERROR,
-                          "Failed to read OTP memory, addr = %x\n",
-                          otp_addr);
+        if (!aspeed_otp_read(s, otp_addr + 1, &value)) {
             return false;
         }
         s->regs[R_CAMP2] = value;
@@ -181,13 +168,6 @@ static bool aspeed_sbc_otp_prog(AspeedSBCState *s,
     uint32_t value = s->regs[R_CAMP1];
     uint32_t otp_offset = otp_addr << 2;
 
-    if (otp_addr >= OTP_TOTAL_DWORD_COUNT) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "Invalid OTP addr 0x%x\n",
-                      otp_addr);
-        return false;
-    }
-
     ret = address_space_write(&otp->as, otp_offset, MEMTXATTRS_UNSPECIFIED,
                               &value, sizeof(value));
     if (ret != MEMTX_OK) {
-- 
2.43.0

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

[Kane Chen]

The eMMC ABR enable state is currently controlled by the "emmc-abr"
machine property instead of the OTP configuration.

Add helpers to read the OTP configuration and resolve strap values.
Use the ABR strap value to update ABR_EN when R_STATUS is read.

Remove the "emmc-abr" property as the ABR state is now derived from
OTP.

Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
---
 include/hw/misc/aspeed_sbc.h  |  1 -
 include/hw/nvram/aspeed_otp.h | 23 +++++++++++--
 hw/misc/aspeed_sbc.c          | 29 ++++++++++++----
 hw/nvram/aspeed_otp.c         | 63 +++++++++++++++++++++++++++++++++++
 4 files changed, 106 insertions(+), 10 deletions(-)

diff --git a/include/hw/misc/aspeed_sbc.h b/include/hw/misc/aspeed_sbc.h
index 756c612356..b210ffb8aa 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/include/hw/nvram/aspeed_otp.h b/include/hw/nvram/aspeed_otp.h
index 2b6c2eaaa3..f91af12ea9 100644
--- a/include/hw/nvram/aspeed_otp.h
+++ b/include/hw/nvram/aspeed_otp.h
@@ -24,11 +24,27 @@ OBJECT_DECLARE_SIMPLE_TYPE(AspeedOTPState, ASPEED_OTP)
  *
  *  - [0, OTP_DATA_DWORD_COUNT]: the data region. Each address contains
  *    64 bits of data.
- *  - [OTP_DATA_DWORD_COUNT, OTP_MEMORY_SIZE / 4]: the configuration
- *    region. Each address contains 32 bits of data.
+ *  - [OTP_DATA_DWORD_COUNT(OTP_CFG0), OTP_MEMORY_SIZE / 4]: the
+ *    configuration region. Each address contains 32 bits of data.
  */
 #define OTP_DATA_DWORD_COUNT            (0x800)
 
+/* Start of the OTP configuration/strap region. */
+#define OTP_CFG0                        (0x800)
+
+/*
+ * OTP straps are a 64-bit value packed as two 32-bit halves starting at
+ * config word OTP_STRAP_START_INDEX (OTPCFG16 and OTPCFG17). Each strap
+ * bit is stored redundantly in OTP_STRAP_COPY_NUM config words, spaced
+ * (OTP_STRAP_BIT_NUM / 32) words apart -- i.e. one word per 32-bit half,
+ * so the two halves interleave; the effective bit value is the XOR of
+ * all copies, matching how the real hardware and the ast-otp reference
+ * tool resolve straps.
+ */
+#define OTP_STRAP_START_INDEX           16
+#define OTP_STRAP_BIT_NUM               64
+#define OTP_STRAP_COPY_NUM              6
+
 typedef struct AspeedOTPState {
     DeviceState parent_obj;
 
@@ -43,4 +59,7 @@ typedef struct AspeedOTPState {
     uint8_t *storage;
 } AspeedOTPState;
 
+uint32_t aspeed_otp_read_config(AspeedOTPState *s, unsigned int cfg_word);
+bool aspeed_otp_read_strap(AspeedOTPState *s, unsigned int bit);
+
 #endif /* ASPEED_OTP_H */
diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c
index 3402cef5c3..6cec9ad3da 100644
--- a/hw/misc/aspeed_sbc.c
+++ b/hw/misc/aspeed_sbc.c
@@ -71,6 +71,9 @@
 #define SBC_OTP_CMD_WRITE 0x23b1e362
 #define SBC_OTP_CMD_PROG 0x23b1e364
 
+/* OTP strap bits */
+#define OTP_STRAP_ABR_EN        0x2b
+
 /* Voltage mode */
 #define MODE_REGISTER               (0x1000)
 #define MODE_REGISTER_A             (0x3000)
@@ -89,7 +92,21 @@ static uint64_t aspeed_sbc_read(void *opaque, hwaddr addr, unsigned int size)
         return 0;
     }
 
-    return s->regs[addr];
+    switch (addr) {
+    case R_STATUS: {
+        uint32_t val = s->regs[R_STATUS];
+
+        if (aspeed_otp_read_strap(&s->otp, OTP_STRAP_ABR_EN)) {
+            val |= ABR_EN;
+        } else {
+            val &= ~ABR_EN;
+        }
+
+        return val;
+    }
+    default:
+        return s->regs[addr];
+    }
 }
 
 static bool aspeed_otp_read(AspeedSBCState *s, uint32_t otp_addr,
@@ -388,13 +405,12 @@ static void aspeed_sbc_reset_hold(Object *obj, ResetType type)
 
     memset(s->regs, 0, sizeof(s->regs));
 
-    /* Set secure boot enabled with RSA4096_SHA256 and enable eMMC ABR */
+    /*
+     * ABR_EN is derived from OTP on every read, see aspeed_sbc_read().
+     * Set secure boot enabled with RSA4096_SHA256.
+     */
     s->regs[R_STATUS] = OTP_IDLE | OTP_MEM_IDLE;
 
-    if (s->emmc_abr) {
-        s->regs[R_STATUS] &= ABR_EN;
-    }
-
     if (s->signing_settings) {
         s->regs[R_STATUS] &= SECURE_BOOT_EN;
     }
@@ -452,7 +468,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),
     DEFINE_PROP_LINK("sram", AspeedSBCState, sram,
                      TYPE_MEMORY_REGION, MemoryRegion *),
diff --git a/hw/nvram/aspeed_otp.c b/hw/nvram/aspeed_otp.c
index 81304bda3a..127da15642 100644
--- a/hw/nvram/aspeed_otp.c
+++ b/hw/nvram/aspeed_otp.c
@@ -106,6 +106,69 @@ static void aspeed_otp_write(void *opaque, hwaddr otp_offset,
     trace_aspeed_otp_prog(otp_offset, val, value);
 }
 
+/*
+ * Each OTP configuration setting contains 32 bits of data.
+ * Configuration words are grouped in banks of 8, with banks spaced
+ * 0x200 dwords apart and words within a bank spaced 2 dwords apart:
+ *
+ *   offset(n) = OTP_CFG0 + (n / 8) * 0x200 + (n % 8) * 2
+ *
+ * Returns 0 if the OTP read fails.
+ */
+uint32_t aspeed_otp_read_config(AspeedOTPState *s, unsigned int cfg_word)
+{
+    uint32_t otp_addr = OTP_CFG0 + (cfg_word / 8) * 0x200 + (cfg_word % 8) * 2;
+    uint32_t value = 0;
+
+    if (address_space_read(&s->as, otp_addr << 2, MEMTXATTRS_UNSPECIFIED,
+                            &value, sizeof(value)) != MEMTX_OK) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Failed to read OTP config word %u\n",
+                      __func__, cfg_word);
+        return 0;
+    }
+
+    return value;
+}
+
+/*
+ * OTP straps are a 64-bit value split across two 32-bit config words,
+ * each of which is repeated OTP_STRAP_COPY_NUM times for reliability.
+ * The two words interleave starting at OTP_STRAP_START_INDEX:
+ *
+ *   cfg_word:    16   17   18   19   20   21   22   23   24   25   26   27
+ *              +----+----+----+----+----+----+----+----+----+----+----+----+
+ *              | L0 | H0 | L1 | H1 | L2 | H2 | L3 | H3 | L4 | H4 | L5 | H5 |
+ *              +----+----+----+----+----+----+----+----+----+----+----+----+
+ *
+ *   Lx = copy #x of bit 0-31, Hx = copy #x of bit 32-63
+ *   (x = 0 .. OTP_STRAP_COPY_NUM - 1)
+ *
+ * To resolve strap bit `n` (0 <= n < OTP_STRAP_BIT_NUM):
+ *   half    = n / 32   -- 0 selects the L* words, 1 selects the H* words
+ *   bit_pos = n % 32   -- bit position within the word
+ *   word(i) = OTP_STRAP_START_INDEX + half + i * (OTP_STRAP_BIT_NUM / 32)
+ *
+ * The effective value of bit `n` is the XOR of bit `bit_pos` across all
+ * OTP_STRAP_COPY_NUM copies of word(i).
+ */
+bool aspeed_otp_read_strap(AspeedOTPState *s, unsigned int bit)
+{
+    uint32_t cfg_word = OTP_STRAP_START_INDEX + bit / 32;
+    uint32_t bit_pos = bit % 32;
+    bool enable = false;
+    int i;
+
+    assert(bit < OTP_STRAP_BIT_NUM);
+
+    for (i = 0; i < OTP_STRAP_COPY_NUM; i++) {
+        enable ^= (aspeed_otp_read_config(s, cfg_word) >> bit_pos) & 0x1;
+        cfg_word += OTP_STRAP_BIT_NUM / 32;
+    }
+
+    return enable;
+}
+
 static bool aspeed_otp_init_storage(AspeedOTPState *s, Error **errp)
 {
     uint32_t *p;
-- 
2.43.0

[PATCH v2 3/3] hw/misc/aspeed_sbc: Derive secure boot state from OTP config

[Kane Chen]

The secure boot state is currently controlled by the
"signing-settings" machine property instead of the OTP configuration.

Derive SECURE_BOOT_EN and R_QSR from the OTP configuration when the
registers are read.

Remove the "signing-settings" property as the secure boot state is now
derived from OTP.

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

diff --git a/include/hw/misc/aspeed_sbc.h b/include/hw/misc/aspeed_sbc.h
index b210ffb8aa..7f3fa7062f 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 6cec9ad3da..ad4c2b4b1d 100644
--- a/hw/misc/aspeed_sbc.c
+++ b/hw/misc/aspeed_sbc.c
@@ -79,6 +79,8 @@
 #define MODE_REGISTER_A             (0x3000)
 #define MODE_REGISTER_B             (0x5000)
 
+static bool aspeed_sbc_otp_read(AspeedSBCState *s, uint32_t otp_addr);
+
 static uint64_t aspeed_sbc_read(void *opaque, hwaddr addr, unsigned int size)
 {
     AspeedSBCState *s = ASPEED_SBC(opaque);
@@ -102,8 +104,16 @@ static uint64_t aspeed_sbc_read(void *opaque, hwaddr addr, unsigned int size)
             val &= ~ABR_EN;
         }
 
+        if (aspeed_otp_read_config(&s->otp, 0) & BIT(1)) {
+            val |= SECURE_BOOT_EN;
+        } else {
+            val &= ~SECURE_BOOT_EN;
+        }
+
         return val;
     }
+    case R_QSR:
+        return aspeed_otp_read_config(&s->otp, 0);
     default:
         return s->regs[addr];
     }
@@ -406,16 +416,11 @@ static void aspeed_sbc_reset_hold(Object *obj, ResetType type)
     memset(s->regs, 0, sizeof(s->regs));
 
     /*
-     * ABR_EN is derived from OTP on every read, see aspeed_sbc_read().
-     * Set secure boot enabled with RSA4096_SHA256.
+     * ABR_EN and SECURE_BOOT_EN are derived from OTP on every read, see
+     * aspeed_sbc_read(), since the SBC status register is just a
+     * reflection of the OTP fuse state.
      */
     s->regs[R_STATUS] = OTP_IDLE | OTP_MEM_IDLE;
-
-    if (s->signing_settings) {
-        s->regs[R_STATUS] &= SECURE_BOOT_EN;
-    }
-
-    s->regs[R_QSR] = s->signing_settings;
 }
 
 static void aspeed_sbc_instance_init(Object *obj)
@@ -468,7 +473,6 @@ static const VMStateDescription vmstate_aspeed_sbc = {
 };
 
 static const Property aspeed_sbc_properties[] = {
-    DEFINE_PROP_UINT32("signing-settings", AspeedSBCState, signing_settings, 0),
     DEFINE_PROP_LINK("sram", AspeedSBCState, sram,
                      TYPE_MEMORY_REGION, MemoryRegion *),
 };
-- 
2.43.0

Re: [PATCH v2 0/3] hw/misc/aspeed_sbc: Fix ABR and secure boot state reporting

[Cédric Le Goater]

On 9/18/26 08:53, Kane Chen wrote:
> The ABR and secure boot states are currently controlled by the
> "emmc-abr" and "signing-settings" machine properties.
> 
> On real hardware, these states are determined by the OTP configuration
> and reflected in the SBC status registers. This series updates the
> model to derive the ABR and secure boot states from OTP instead, and
> removes the corresponding machine properties.
> 
> The series also consolidates the OTP layout definitions and adds
> helpers for reading OTP configuration and strap settings.
> 
> Any feedback or suggestions are appreciated!
> 
> Kane
> ---
> 
> ChangeLog
> ---------
> v2:
> - Move OTP layout definitions to aspeed_otp.h
> - Add helpers for reading OTP configuration and strap settings
> - Derive ABR and secure boot states using the new OTP helpers
> 
> v1:
> - Initial version
> 
> ---
> 
> Kane-Chen-AS (3):
>    hw/nvram/aspeed_otp, hw/misc/aspeed_sbc: Consolidate OTP layout
>      definitions
>    hw/misc/aspeed_sbc: Derive ABR state from OTP config straps
>    hw/misc/aspeed_sbc: Derive secure boot state from OTP config
> 
>   include/hw/misc/aspeed_sbc.h  |  3 --
>   include/hw/nvram/aspeed_otp.h | 32 ++++++++++++
>   hw/misc/aspeed_sbc.c          | 97 +++++++++++++++++------------------
>   hw/nvram/aspeed_otp.c         | 63 +++++++++++++++++++++++
>   4 files changed, 143 insertions(+), 52 deletions(-)
> 

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.