[PATCH v1 0/1] hw/misc/aspeed_sbc: Implement OTP memory and controller

[PATCH v1 0/1] hw/misc/aspeed_sbc: Implement OTP memory and controller

[Kane-Chen-AS via]

This patch introduces part of the Secure Boot Controller device,
which consists of several sub-components, including an OTP memory,
OTP controller, cryptographic engine, and boot controller.

In this version, the implementation includes the OTP memory and its
controller. The OTP memory can be programmed from within the guest
OS via a software utility.

Kane-Chen-AS (1):
  hw/misc/aspeed_sbc: Implement OTP memory and controller

 hw/misc/aspeed_sbc.c         | 304 +++++++++++++++++++++++++++++++++++
 include/hw/misc/aspeed_sbc.h |  14 ++
 2 files changed, 318 insertions(+)

-- 
2.43.0

[PATCH v1 1/1] hw/misc/aspeed_sbc: Implement OTP memory and controller

[Kane-Chen-AS via]

This patch adds the OTP memory and its controller as part of the
Secure Boot Controller (SBC) device model. The OTP memory content is
persisted to a file named 'otpmem', which is created if it does not
already exist.

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

diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c
index e4a6bd1581..5d77fd45d7 100644
--- a/hw/misc/aspeed_sbc.c
+++ b/hw/misc/aspeed_sbc.c
@@ -15,9 +15,15 @@
 #include "hw/misc/aspeed_sbc.h"
 #include "qapi/error.h"
 #include "migration/vmstate.h"
+#include "system/block-backend.h"
+#include "qobject/qdict.h"
 
 #define R_PROT          (0x000 / 4)
+#define R_CMD           (0x004 / 4)
+#define R_ADDR          (0x010 / 4)
 #define R_STATUS        (0x014 / 4)
+#define R_CAMP1         (0x020 / 4)
+#define R_CAMP2         (0x024 / 4)
 #define R_QSR           (0x040 / 4)
 
 /* R_STATUS */
@@ -41,6 +47,18 @@
 #define QSR_RSA_MASK           (0x3 << 12)
 #define QSR_HASH_MASK          (0x3 << 10)
 
+#define OTP_FILE_PATH "otpmem"
+
+#define BLK_VALID(s) \
+    do { \
+        if (s->blk == NULL) { \
+            qemu_log_mask(LOG_GUEST_ERROR, \
+                          "%s: blk is not initialized\n", \
+                          __func__); \
+            return; \
+        } \
+    } while (0)
+
 static uint64_t aspeed_sbc_read(void *opaque, hwaddr addr, unsigned int size)
 {
     AspeedSBCState *s = ASPEED_SBC(opaque);
@@ -57,6 +75,196 @@ static uint64_t aspeed_sbc_read(void *opaque, hwaddr addr, unsigned int size)
     return s->regs[addr];
 }
 
+static void aspeed_sbc_otpmem_read(void *opaque)
+{
+    AspeedSBCState *s = ASPEED_SBC(opaque);
+    uint32_t otp_addr, data, otp_offset;
+    bool is_data = false;
+
+    BLK_VALID(s);
+    otp_addr = s->regs[R_ADDR];
+    if (otp_addr < OTP_DATA_DWORD_COUNT) {
+        is_data = true;
+    } else if (otp_addr >= OTP_TOTAL_DWORD_COUNT) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Invalid OTP addr 0x%x\n",
+                      __func__, otp_addr);
+        return;
+    }
+    otp_offset = otp_addr << 2;
+
+    if (blk_pread(s->blk, (int64_t)otp_offset, sizeof(data), &data, 0) < 0) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Failed to read data 0x%x\n",
+                      __func__, otp_offset);
+        return;
+    }
+    s->regs[R_CAMP1] = data;
+
+    if (is_data) {
+        if (blk_pread(s->blk, (int64_t)otp_offset + 4,
+                      sizeof(data), &data, 0) < 0) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: Failed to read data 0x%x\n",
+                          __func__, otp_offset);
+            return;
+        }
+        s->regs[R_CAMP2] = data;
+    }
+}
+
+static bool check_bit_conditions(uint32_t otp_addr,
+                                 uint32_t value, uint32_t prog_bit)
+{
+    uint32_t programed_bits, pass;
+    bool is_odd = otp_addr & 1;
+
+    if (is_odd) {
+        programed_bits = ~value & prog_bit;
+    } else {
+        programed_bits = value & (~prog_bit);
+    }
+
+    pass = value ^ (~prog_bit);
+
+    if (programed_bits) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Found programed bits in addr %x\n",
+                      __func__, otp_addr);
+        for (int i = 0; i < 32; ++i) {
+            if (programed_bits & (1U << i)) {
+                qemu_log_mask(LOG_GUEST_ERROR,
+                              "  Programed bit %d\n",
+                              i);
+            }
+        }
+    }
+
+    return pass != 0;
+}
+
+static bool program_otp_data(void *opaque, uint32_t otp_addr,
+                             uint32_t prog_bit, uint32_t *value)
+{
+    AspeedSBCState *s = ASPEED_SBC(opaque);
+    bool is_odd = otp_addr & 1;
+    uint32_t otp_offset = otp_addr << 2;
+
+    if (blk_pread(s->blk, (int64_t)otp_offset,
+                  sizeof(uint32_t), value, 0) < 0) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Failed to read data 0x%x\n",
+                      __func__, otp_offset);
+        return false;
+    }
+
+    if (check_bit_conditions(otp_addr, *value, prog_bit) == false) {
+        return false;
+    }
+
+    if (is_odd) {
+        *value &= ~prog_bit;
+    } else {
+        *value |= ~prog_bit;
+    }
+
+    return true;
+}
+
+static void mr_handler(uint32_t otp_addr, uint32_t data)
+{
+    switch (otp_addr) {
+    case MODE_REGISTER:
+    case MODE_REGISTER_A:
+    case MODE_REGISTER_B:
+        /* HW behavior, do nothing here */
+        break;
+    default:
+    qemu_log_mask(LOG_GUEST_ERROR,
+                  "%s: Unsupported address 0x%x\n",
+                  __func__, otp_addr);
+        return;
+    }
+}
+
+static void aspeed_sbc_otpmem_write(void *opaque)
+{
+    AspeedSBCState *s = ASPEED_SBC(opaque);
+    uint32_t otp_addr, data;
+
+    otp_addr = s->regs[R_ADDR];
+    data = s->regs[R_CAMP1];
+
+    if (otp_addr == 0) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: ignore write program bit request\n",
+                      __func__);
+    } else if (otp_addr >= MODE_REGISTER) {
+        mr_handler(otp_addr, data);
+    } else {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Unhandled OTP write address 0x%x\n",
+                      __func__, otp_addr);
+    }
+}
+
+static void aspeed_sbc_otpmem_prog(void *opaque)
+{
+    AspeedSBCState *s = ASPEED_SBC(opaque);
+    uint32_t otp_addr, value, otp_offset;
+
+    BLK_VALID(s);
+    otp_addr = s->regs[R_ADDR];
+    if (otp_addr >= OTP_TOTAL_DWORD_COUNT) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Invalid OTP addr 0x%x\n",
+                      __func__, otp_addr);
+        return;
+    }
+
+    otp_offset = otp_addr << 2;
+    if (program_otp_data(opaque, otp_addr,
+                         s->regs[R_CAMP1], &value) == false) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                       "%s: Failed to program data 0x%x to 0x%x\n",
+                      __func__, s->regs[R_CAMP1], otp_offset);
+        return;
+    }
+
+    if (blk_pwrite(s->blk, (int64_t)otp_offset,
+                   sizeof(value), &value, 0) < 0) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Failed to write data 0x%x to 0x%x\n",
+                      __func__, value, otp_offset);
+    }
+}
+
+static void aspeed_sbc_handle_command(void *opaque, uint32_t cmd)
+{
+    AspeedSBCState *s = ASPEED_SBC(opaque);
+
+    s->regs[R_STATUS] &= ~(OTP_MEM_IDLE | OTP_IDLE);
+
+    switch (cmd) {
+    case READ_CMD:
+        aspeed_sbc_otpmem_read(s);
+        break;
+    case WRITE_CMD:
+        aspeed_sbc_otpmem_write(s);
+        break;
+    case PROG_CMD:
+        aspeed_sbc_otpmem_prog(s);
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Unknown command 0x%x\n",
+                      __func__, cmd);
+        break;
+    }
+
+    s->regs[R_STATUS] |= (OTP_MEM_IDLE | OTP_IDLE);
+}
+
 static void aspeed_sbc_write(void *opaque, hwaddr addr, uint64_t data,
                               unsigned int size)
 {
@@ -78,6 +286,9 @@ static void aspeed_sbc_write(void *opaque, hwaddr addr, uint64_t data,
                       "%s: write to read only register 0x%" HWADDR_PRIx "\n",
                       __func__, addr << 2);
         return;
+    case R_CMD:
+        aspeed_sbc_handle_command(opaque, data);
+        return;
     default:
         break;
     }
@@ -113,6 +324,92 @@ static void aspeed_sbc_reset(DeviceState *dev)
     }
 
     s->regs[R_QSR] = s->signing_settings;
+
+    if (!s->blk) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: blk not initialized!\n",
+                      __func__);
+        return;
+    }
+}
+
+static BlockBackend *init_otpmem(int64_t size_bytes)
+{
+    Error *local_err = NULL;
+    BlockDriverState *bs = NULL;
+    BlockBackend *blk = NULL;
+    bool image_created = false;
+    QDict *options;
+    uint32_t i, odd_def = 0xffffffff, even_def = 0, *def;
+
+    if (!g_file_test(OTP_FILE_PATH, G_FILE_TEST_EXISTS)) {
+        bdrv_img_create(OTP_FILE_PATH, "raw", NULL, NULL,
+                        NULL, size_bytes, 0, true, &local_err);
+        if (local_err) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: Failed to create image %s: %s\n",
+                          __func__, OTP_FILE_PATH,
+                          error_get_pretty(local_err));
+            error_free(local_err);
+            return NULL;
+        }
+        image_created = true;
+    }
+
+    blk = blk_new(qemu_get_aio_context(),
+                  BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
+                  0);
+    if (!blk) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Failed to create BlockBackend\n",
+                      __func__);
+        return NULL;
+    }
+
+    options =  qdict_new();
+    qdict_put_str(options, "driver", "raw");
+    bs = bdrv_open(OTP_FILE_PATH, NULL, options, BDRV_O_RDWR, &local_err);
+    if (local_err) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Failed to create OTP memory, err = %s\n",
+                      __func__, error_get_pretty(local_err));
+        blk_unref(blk);
+        error_free(local_err);
+        return NULL;
+    }
+
+    blk_insert_bs(blk, bs, &local_err);
+    if (local_err) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Failed to insert OTP memory to SBC, err = %s\n",
+                      __func__, error_get_pretty(local_err));
+        bdrv_unref(bs);
+        blk_unref(blk);
+        error_free(local_err);
+        return NULL;
+    }
+    bdrv_unref(bs);
+
+    if (image_created) {
+        /* init otp memory data */
+        for (i = 0; i < OTP_TOTAL_DWORD_COUNT; i++) {
+            if (i & 1) {
+                def = &odd_def;
+            } else {
+                def = &even_def;
+            }
+
+            if (blk_pwrite(blk, i << 2, sizeof(uint32_t), def, 0) < 0) {
+                qemu_log_mask(LOG_GUEST_ERROR,
+                              "%s: Failed to init OTP memory file\n",
+                              __func__);
+                blk_unref(blk);
+                return NULL;
+            }
+        }
+    }
+
+    return blk;
 }
 
 static void aspeed_sbc_realize(DeviceState *dev, Error **errp)
@@ -124,6 +421,13 @@ static void aspeed_sbc_realize(DeviceState *dev, Error **errp)
             TYPE_ASPEED_SBC, 0x1000);
 
     sysbus_init_mmio(sbd, &s->iomem);
+
+    s->blk = init_otpmem(OTP_FILE_SIZE);
+    if (s->blk == NULL) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Failed to attach otpmem\n",
+                      __func__);
+    }
 }
 
 static const VMStateDescription vmstate_aspeed_sbc = {
diff --git a/include/hw/misc/aspeed_sbc.h b/include/hw/misc/aspeed_sbc.h
index 405e6782b9..fbdef86a63 100644
--- a/include/hw/misc/aspeed_sbc.h
+++ b/include/hw/misc/aspeed_sbc.h
@@ -27,6 +27,18 @@ OBJECT_DECLARE_TYPE(AspeedSBCState, AspeedSBCClass, ASPEED_SBC)
 #define QSR_SHA384                  (0x2 << 10)
 #define QSR_SHA512                  (0x3 << 10)
 
+#define READ_CMD                    (0x23b1e361)
+#define WRITE_CMD                   (0x23b1e362)
+#define PROG_CMD                    (0x23b1e364)
+
+#define OTP_DATA_DWORD_COUNT        (0x800)
+#define OTP_TOTAL_DWORD_COUNT       (0x1000)
+#define OTP_FILE_SIZE               (OTP_TOTAL_DWORD_COUNT * sizeof(uint32_t))
+
+#define MODE_REGISTER               (0x1000)
+#define MODE_REGISTER_A             (0x3000)
+#define MODE_REGISTER_B             (0x5000)
+
 struct AspeedSBCState {
     SysBusDevice parent;
 
@@ -36,6 +48,8 @@ struct AspeedSBCState {
     MemoryRegion iomem;
 
     uint32_t regs[ASPEED_SBC_NR_REGS];
+
+    BlockBackend *blk;
 };
 
 struct AspeedSBCClass {
-- 
2.43.0

Re: [PATCH v1 0/1] hw/misc/aspeed_sbc: Implement OTP memory and controller

[Cédric Le Goater]

Hello,

On 4/2/25 11:14, Kane-Chen-AS wrote:
> This patch introduces part of the Secure Boot Controller device,
> which consists of several sub-components, including an OTP memory,
> OTP controller, cryptographic engine, and boot controller.
> 
> In this version, the implementation includes the OTP memory and its
> controller. The OTP memory can be programmed from within the guest
> OS via a software utility.


What is the OTP memory ? An external flash device or built-in SRAM ?
If the latter, I suggest using an allocated buffer under the SBC model
and avoid the complexity of the BlockBackend implementation and
the definition of a drive on the command line for it. The
proposal is bypassing a lot of QEMU layers for this purpose.

Is the support the same for ast1030 and ast2600 SoC?

Thanks,

C.


> 
> Kane-Chen-AS (1):
>    hw/misc/aspeed_sbc: Implement OTP memory and controller
> 
>   hw/misc/aspeed_sbc.c         | 304 +++++++++++++++++++++++++++++++++++
>   include/hw/misc/aspeed_sbc.h |  14 ++
>   2 files changed, 318 insertions(+)
> 

Re: [PATCH v1 0/1] hw/misc/aspeed_sbc: Implement OTP memory and controller

[Philippe Mathieu-Daudé]

+qemu-block@

On 4/4/25 14:06, Cédric Le Goater wrote:
> Hello,
> 
> On 4/2/25 11:14, Kane-Chen-AS wrote:
>> This patch introduces part of the Secure Boot Controller device,
>> which consists of several sub-components, including an OTP memory,
>> OTP controller, cryptographic engine, and boot controller.
>>
>> In this version, the implementation includes the OTP memory and its
>> controller. The OTP memory can be programmed from within the guest
>> OS via a software utility.
> 
> 
> What is the OTP memory ? An external flash device or built-in SRAM ?
> If the latter, I suggest using an allocated buffer under the SBC model
> and avoid the complexity of the BlockBackend implementation and
> the definition of a drive on the command line for it. The
> proposal is bypassing a lot of QEMU layers for this purpose.

More of the former, a built-in eFuse behaving more like flash. So using
block backend for the storage seems correct to me. However I don't think
the implementation belongs to hw/misc/aspeed_sbc; ideally we'd have some
abstract (or interface) implementation in hw/block/otp.c -- with methods
such program_otp_data() --, completed by hw/block/aspeed_otc.c.

Current patch might be good enough to start with IMHO.

Regards,

Phil.

Re: [PATCH v1 0/1] hw/misc/aspeed_sbc: Implement OTP memory and controller

[Cédric Le Goater]

On 4/4/25 15:00, Philippe Mathieu-Daudé wrote:
> +qemu-block@
> 
> On 4/4/25 14:06, Cédric Le Goater wrote:
>> Hello,
>>
>> On 4/2/25 11:14, Kane-Chen-AS wrote:
>>> This patch introduces part of the Secure Boot Controller device,
>>> which consists of several sub-components, including an OTP memory,
>>> OTP controller, cryptographic engine, and boot controller.
>>>
>>> In this version, the implementation includes the OTP memory and its
>>> controller. The OTP memory can be programmed from within the guest
>>> OS via a software utility.
>>
>>
>> What is the OTP memory ? An external flash device or built-in SRAM ?
>> If the latter, I suggest using an allocated buffer under the SBC model
>> and avoid the complexity of the BlockBackend implementation and
>> the definition of a drive on the command line for it. The
>> proposal is bypassing a lot of QEMU layers for this purpose.
> 
> More of the former, a built-in eFuse behaving more like flash. So using
> block backend for the storage seems correct to me. 

How would you define the drive backend on the command line ?

> However I don't think
> the implementation belongs to hw/misc/aspeed_sbc; ideally we'd have some
> abstract (or interface) implementation in hw/block/otp.c -- with methods
> such program_otp_data() --, completed by hw/block/aspeed_otc.c.

I was imagining more something like NPCM7xxOTPState or BCM2835OTPState
and not SiFiveUOTPState.

> Current patch might be good enough to start with IMHO.

Have you looked at the next patch and how the backend is handled ?
I will let the block people Ack this patch in that case. It's
beyond my skills.

Thanks,

C.

RE: [PATCH v1 0/1] hw/misc/aspeed_sbc: Implement OTP memory and controller

[Kane Chen]

Hi Cédric/Philippe,

OTP (One-Time Programmable) memory is a type of non-volatile memory
in which each bit can be programmed only once. It is typically used
to store critical and permanent information, such as the chip ID and
secure boot keys. The structure and behavior of OTP memory are
consistent across both the AST1030 and AST2600 platforms.

As Philippe pointed out, this proposal models the OTP memory as a
flash device and utilizes a block backend for persistent storage. In
contrast, existing implementations such as NPCM7xxOTPState,
BCM2835OTPState, and SiFiveUOTPState expose OTP memory via MMIO and
always initialize it in a blank state. The goal of this design is to
allow the guest system to boot with a pre-configured OTP memory
state. To support this, the OTP memory is backed by a file,
simulating persistent flash behavior.

The OTP memory access flow is as follows:
1. The guest issues a read or write OTP command to the Secure Boot
   Controller (SBC)
2. The SBC triggers the corresponding operation in the OTP controller
3. The SBC returns the result to the guest

Since the guest interacts with OTP memory exclusively through the
SBC, the OTP logic is implemented within aspeed_sbc.c.

If there are existing architectural guidelines or design patterns
that should be followed for modeling OTP devices, I would greatly
appreciate your feedback. I am happy to revise the implementation
accordingly and submit updated patches for further review.

Thanks for your comments and review.

Best Regards,
Kane

> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Friday, April 4, 2025 9:54 PM
> To: Philippe Mathieu-Daudé <philmd@linaro.org>; 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>; qemu-block <qemu-block@nongnu.org>
> Cc: Troy Lee <troy_lee@aspeedtech.com>
> Subject: Re: [PATCH v1 0/1] hw/misc/aspeed_sbc: Implement OTP memory and
> controller
> 
> On 4/4/25 15:00, Philippe Mathieu-Daudé wrote:
> > +qemu-block@
> >
> > On 4/4/25 14:06, Cédric Le Goater wrote:
> >> Hello,
> >>
> >> On 4/2/25 11:14, Kane-Chen-AS wrote:
> >>> This patch introduces part of the Secure Boot Controller device,
> >>> which consists of several sub-components, including an OTP memory,
> >>> OTP controller, cryptographic engine, and boot controller.
> >>>
> >>> In this version, the implementation includes the OTP memory and its
> >>> controller. The OTP memory can be programmed from within the guest
> >>> OS via a software utility.
> >>
> >>
> >> What is the OTP memory ? An external flash device or built-in SRAM ?
> >> If the latter, I suggest using an allocated buffer under the SBC
> >> model and avoid the complexity of the BlockBackend implementation and
> >> the definition of a drive on the command line for it. The proposal is
> >> bypassing a lot of QEMU layers for this purpose.
> >
> > More of the former, a built-in eFuse behaving more like flash. So
> > using block backend for the storage seems correct to me.
> 
> How would you define the drive backend on the command line ?
> 
> > However I don't think
> > the implementation belongs to hw/misc/aspeed_sbc; ideally we'd have
> > some abstract (or interface) implementation in hw/block/otp.c -- with
> > methods such program_otp_data() --, completed by hw/block/aspeed_otc.c.
> 
> I was imagining more something like NPCM7xxOTPState or BCM2835OTPState
> and not SiFiveUOTPState.
> 
> > Current patch might be good enough to start with IMHO.
> 
> Have you looked at the next patch and how the backend is handled ?
> I will let the block people Ack this patch in that case. It's beyond my skills.
> 
> Thanks,
> 
> C.
> 

Re: [PATCH v1 0/1] hw/misc/aspeed_sbc: Implement OTP memory and controller

[Cédric Le Goater]

Hello Kane,

+ Markus (for ebc29e1beab0 implementation)

On 4/7/25 09:33, Kane Chen wrote:
> Hi Cédric/Philippe,
> 
> OTP (One-Time Programmable) memory is a type of non-volatile memory
> in which each bit can be programmed only once. It is typically used
> to store critical and permanent information, such as the chip ID and
> secure boot keys. The structure and behavior of OTP memory are
> consistent across both the AST1030 and AST2600 platforms.
> 
> As Philippe pointed out, this proposal models the OTP memory as a
> flash device and utilizes a block backend for persistent storage. In
> contrast, existing implementations such as NPCM7xxOTPState,
> BCM2835OTPState, and SiFiveUOTPState expose OTP memory via MMIO and
> always initialize it in a blank state. 

AFAIU, Aspeed SBC is also MMIO based or is there another device,
an eeprom, accessible through an external bus ? How is it
implemented in HW ?

> The goal of this design is to
> allow the guest system to boot with a pre-configured OTP memory
> state. 

Yes. This is a valid request. It's not the first time we've had
this kind of requests. The initial content of EEPROM devices are
an example and some machines, like the rainier, have a lot.

If the device can be defined on the command line, like would be
an EEPROM device attached to an I2C bus or a flash device attached
to a SPI bus, we can use a 'drive' property. Something like :

   qemu-system-arm -M ast2600-evb \
       -blockdev node-name=fmc0,driver=file,filename=/path/to/fmc0.img \
       -device mx66u51235f,bus=ssi.0,cs=0x0,drive=fmc0 \
       -blockdev node-name=fmc1,driver=file,filename=/path/to/fmc1.img \
       -device mx66u51235f,bus=ssi.0,cs=0x1,drive=fmc1 \
       -blockdev node-name=spi1,driver=file,filename=/path/to/spi1.img \
       -device mx66u51235f,cs=0x0,bus=ssi.1,drive=spi1 \
       ...

However, the Aspeed SBC device is a platform device and it makes
things more complex : it can not be created on the command line,
it is directly created by the machine and the soc and passing
device properties to specify a blockdev it is not possible :

   qemu-system-arm -M ast2600-evb \
       -blockdev node-name=otpmem,driver=file,filename=/path/to/otpmem.img \
       -device aspeed-sbc,drive=otpmem \
       ...


> To support this, the OTP memory is backed by a file,
> simulating persistent flash behavior.

The idea is good but the implementation is problematic.

     +static BlockBackend *init_otpmem(int64_t size_bytes)
     +{
     +    Error *local_err = NULL;
     +    BlockDriverState *bs = NULL;
     +    BlockBackend *blk = NULL;
     +    bool image_created = false;
     +    QDict *options;
     +    uint32_t i, odd_def = 0xffffffff, even_def = 0, *def;
     +
     +    if (!g_file_test(OTP_FILE_PATH, G_FILE_TEST_EXISTS)) {
     +        bdrv_img_create(OTP_FILE_PATH, "raw", NULL, NULL,
     +                        NULL, size_bytes, 0, true, &local_err);
     +        if (local_err) {
     +            qemu_log_mask(LOG_GUEST_ERROR,
     +                          "%s: Failed to create image %s: %s\n",
     +                          __func__, OTP_FILE_PATH,
     +                          error_get_pretty(local_err));
     +            error_free(local_err);
     +            return NULL;
     +        }
     +        image_created = true;
     +    }
     +
     +    blk = blk_new(qemu_get_aio_context(),
     +                  BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
     +                  0);
     +    if (!blk) {
     +        qemu_log_mask(LOG_GUEST_ERROR,
     +                      "%s: Failed to create BlockBackend\n",
     +                      __func__);
     +        return NULL;
     +    }
     +
     +    options =  qdict_new();
     +    qdict_put_str(options, "driver", "raw");
     +    bs = bdrv_open(OTP_FILE_PATH, NULL, options, BDRV_O_RDWR, &local_err);
     +    if (local_err) {
     +        qemu_log_mask(LOG_GUEST_ERROR,
     +                      "%s: Failed to create OTP memory, err = %s\n",
     +                      __func__, error_get_pretty(local_err));
     +        blk_unref(blk);
     +        error_free(local_err);
     +        return NULL;
     +    }
     +
     +    blk_insert_bs(blk, bs, &local_err);
     +    if (local_err) {
     +        qemu_log_mask(LOG_GUEST_ERROR,
     +                      "%s: Failed to insert OTP memory to SBC, err = %s\n",
     +                      __func__, error_get_pretty(local_err));
     +        bdrv_unref(bs);
     +        blk_unref(blk);
     +        error_free(local_err);
     +        return NULL;
     +    }
     +    bdrv_unref(bs);
     ...

IMO, this is low level block code that a device model shouldn't have
to deal with. A 'drive' should be used instead. Now, if the qemu-block
maintainers are OK with it, we need their approval.

  
> The OTP memory access flow is as follows:
> 1. The guest issues a read or write OTP command to the Secure Boot
>     Controller (SBC)
> 2. The SBC triggers the corresponding operation in the OTP controller
> 3. The SBC returns the result to the guest
> 
> Since the guest interacts with OTP memory exclusively through the
> SBC, the OTP logic is implemented within aspeed_sbc.c.
> 
> If there are existing architectural guidelines or design patterns
> that should be followed for modeling OTP devices, I would greatly
> appreciate your feedback. I am happy to revise the implementation
> accordingly and submit updated patches for further review.

Adding a 'drive' property to the aspeed-sbc model shouldn't change
too much the proposal and it will remove the init_otpmem() routine,
which is problematic.

Then, we need to find a way to set the 'drive' property of the
aspeed-sbc model. I suggest using the same method of the edk2 flash
devices of the q35 machine. See ebc29e1beab0. Setting a machine
option would set the drive. Something like :

   qemu-system-arm -M ast2600-evb,otpmem=otpmem-drive \
       -blockdev node-name=otpmem,driver=file,filename=/path/to/otpmem.img \
       ...

This machine option would only be defined for machine types needing
it.

Thanks,

C.





> Thanks for your comments and review.
> 
> Best Regards,
> Kane
> 
>> -----Original Message-----
>> From: Cédric Le Goater <clg@kaod.org>
>> Sent: Friday, April 4, 2025 9:54 PM
>> To: Philippe Mathieu-Daudé <philmd@linaro.org>; 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>; qemu-block <qemu-block@nongnu.org>
>> Cc: Troy Lee <troy_lee@aspeedtech.com>
>> Subject: Re: [PATCH v1 0/1] hw/misc/aspeed_sbc: Implement OTP memory and
>> controller
>>
>> On 4/4/25 15:00, Philippe Mathieu-Daudé wrote:
>>> +qemu-block@
>>>
>>> On 4/4/25 14:06, Cédric Le Goater wrote:
>>>> Hello,
>>>>
>>>> On 4/2/25 11:14, Kane-Chen-AS wrote:
>>>>> This patch introduces part of the Secure Boot Controller device,
>>>>> which consists of several sub-components, including an OTP memory,
>>>>> OTP controller, cryptographic engine, and boot controller.
>>>>>
>>>>> In this version, the implementation includes the OTP memory and its
>>>>> controller. The OTP memory can be programmed from within the guest
>>>>> OS via a software utility.
>>>>
>>>>
>>>> What is the OTP memory ? An external flash device or built-in SRAM ?
>>>> If the latter, I suggest using an allocated buffer under the SBC
>>>> model and avoid the complexity of the BlockBackend implementation and
>>>> the definition of a drive on the command line for it. The proposal is
>>>> bypassing a lot of QEMU layers for this purpose.
>>>
>>> More of the former, a built-in eFuse behaving more like flash. So
>>> using block backend for the storage seems correct to me.
>>
>> How would you define the drive backend on the command line ?
>>
>>> However I don't think
>>> the implementation belongs to hw/misc/aspeed_sbc; ideally we'd have
>>> some abstract (or interface) implementation in hw/block/otp.c -- with
>>> methods such program_otp_data() --, completed by hw/block/aspeed_otc.c.
>>
>> I was imagining more something like NPCM7xxOTPState or BCM2835OTPState
>> and not SiFiveUOTPState.
>>
>>> Current patch might be good enough to start with IMHO.
>>
>> Have you looked at the next patch and how the backend is handled ?
>> I will let the block people Ack this patch in that case. It's beyond my skills.
>>
>> Thanks,
>>
>> C.
>>
> 

RE: [PATCH v1 0/1] hw/misc/aspeed_sbc: Implement OTP memory and controller

[Kane Chen]

Hi Cédric,

Thank you for your comments and suggestions. I’ll start by reviewing
the changes in commit ebc29e1beab0. At the moment, I don’t have
another example of a device that is accessed through an external bus,
but I’ll look into it and see what I can find.

Best Regards,
Kane

> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Monday, April 7, 2025 5:55 PM
> To: Kane Chen <kane_chen@aspeedtech.com>; Philippe Mathieu-Daudé
> <philmd@linaro.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>; qemu-block <qemu-block@nongnu.org>; Markus
> Armbruster <armbru@redhat.com>
> Cc: Troy Lee <troy_lee@aspeedtech.com>
> Subject: Re: [PATCH v1 0/1] hw/misc/aspeed_sbc: Implement OTP memory and
> controller
> 
> Hello Kane,
> 
> + Markus (for ebc29e1beab0 implementation)
> 
> On 4/7/25 09:33, Kane Chen wrote:
> > Hi Cédric/Philippe,
> >
> > OTP (One-Time Programmable) memory is a type of non-volatile memory in
> > which each bit can be programmed only once. It is typically used to
> > store critical and permanent information, such as the chip ID and
> > secure boot keys. The structure and behavior of OTP memory are
> > consistent across both the AST1030 and AST2600 platforms.
> >
> > As Philippe pointed out, this proposal models the OTP memory as a
> > flash device and utilizes a block backend for persistent storage. In
> > contrast, existing implementations such as NPCM7xxOTPState,
> > BCM2835OTPState, and SiFiveUOTPState expose OTP memory via MMIO and
> > always initialize it in a blank state.
> 
> AFAIU, Aspeed SBC is also MMIO based or is there another device, an eeprom,
> accessible through an external bus ? How is it implemented in HW ?
> 
> > The goal of this design is to
> > allow the guest system to boot with a pre-configured OTP memory state.
> 
> Yes. This is a valid request. It's not the first time we've had this kind of requests.
> The initial content of EEPROM devices are an example and some machines,
> like the rainier, have a lot.
> 
> If the device can be defined on the command line, like would be an EEPROM
> device attached to an I2C bus or a flash device attached to a SPI bus, we can
> use a 'drive' property. Something like :
> 
>    qemu-system-arm -M ast2600-evb \
>        -blockdev node-name=fmc0,driver=file,filename=/path/to/fmc0.img \
>        -device mx66u51235f,bus=ssi.0,cs=0x0,drive=fmc0 \
>        -blockdev node-name=fmc1,driver=file,filename=/path/to/fmc1.img \
>        -device mx66u51235f,bus=ssi.0,cs=0x1,drive=fmc1 \
>        -blockdev node-name=spi1,driver=file,filename=/path/to/spi1.img \
>        -device mx66u51235f,cs=0x0,bus=ssi.1,drive=spi1 \
>        ...
> 
> However, the Aspeed SBC device is a platform device and it makes things more
> complex : it can not be created on the command line, it is directly created by
> the machine and the soc and passing device properties to specify a blockdev it
> is not possible :
> 
>    qemu-system-arm -M ast2600-evb \
>        -blockdev
> node-name=otpmem,driver=file,filename=/path/to/otpmem.img \
>        -device aspeed-sbc,drive=otpmem \
>        ...
> 
> 
> > To support this, the OTP memory is backed by a file, simulating
> > persistent flash behavior.
> 
> The idea is good but the implementation is problematic.
> 
>      +static BlockBackend *init_otpmem(int64_t size_bytes)
>      +{
>      +    Error *local_err = NULL;
>      +    BlockDriverState *bs = NULL;
>      +    BlockBackend *blk = NULL;
>      +    bool image_created = false;
>      +    QDict *options;
>      +    uint32_t i, odd_def = 0xffffffff, even_def = 0, *def;
>      +
>      +    if (!g_file_test(OTP_FILE_PATH, G_FILE_TEST_EXISTS)) {
>      +        bdrv_img_create(OTP_FILE_PATH, "raw", NULL, NULL,
>      +                        NULL, size_bytes, 0, true, &local_err);
>      +        if (local_err) {
>      +            qemu_log_mask(LOG_GUEST_ERROR,
>      +                          "%s: Failed to create image %s: %s\n",
>      +                          __func__, OTP_FILE_PATH,
>      +                          error_get_pretty(local_err));
>      +            error_free(local_err);
>      +            return NULL;
>      +        }
>      +        image_created = true;
>      +    }
>      +
>      +    blk = blk_new(qemu_get_aio_context(),
>      +                  BLK_PERM_CONSISTENT_READ |
> BLK_PERM_WRITE,
>      +                  0);
>      +    if (!blk) {
>      +        qemu_log_mask(LOG_GUEST_ERROR,
>      +                      "%s: Failed to create BlockBackend\n",
>      +                      __func__);
>      +        return NULL;
>      +    }
>      +
>      +    options =  qdict_new();
>      +    qdict_put_str(options, "driver", "raw");
>      +    bs = bdrv_open(OTP_FILE_PATH, NULL, options, BDRV_O_RDWR,
> &local_err);
>      +    if (local_err) {
>      +        qemu_log_mask(LOG_GUEST_ERROR,
>      +                      "%s: Failed to create OTP memory, err =
> %s\n",
>      +                      __func__, error_get_pretty(local_err));
>      +        blk_unref(blk);
>      +        error_free(local_err);
>      +        return NULL;
>      +    }
>      +
>      +    blk_insert_bs(blk, bs, &local_err);
>      +    if (local_err) {
>      +        qemu_log_mask(LOG_GUEST_ERROR,
>      +                      "%s: Failed to insert OTP memory to SBC,
> err = %s\n",
>      +                      __func__, error_get_pretty(local_err));
>      +        bdrv_unref(bs);
>      +        blk_unref(blk);
>      +        error_free(local_err);
>      +        return NULL;
>      +    }
>      +    bdrv_unref(bs);
>      ...
> 
> IMO, this is low level block code that a device model shouldn't have to deal
> with. A 'drive' should be used instead. Now, if the qemu-block maintainers are
> OK with it, we need their approval.
> 
> 
> > The OTP memory access flow is as follows:
> > 1. The guest issues a read or write OTP command to the Secure Boot
> >     Controller (SBC)
> > 2. The SBC triggers the corresponding operation in the OTP controller
> > 3. The SBC returns the result to the guest
> >
> > Since the guest interacts with OTP memory exclusively through the SBC,
> > the OTP logic is implemented within aspeed_sbc.c.
> >
> > If there are existing architectural guidelines or design patterns that
> > should be followed for modeling OTP devices, I would greatly
> > appreciate your feedback. I am happy to revise the implementation
> > accordingly and submit updated patches for further review.
> 
> Adding a 'drive' property to the aspeed-sbc model shouldn't change too much
> the proposal and it will remove the init_otpmem() routine, which is
> problematic.
> 
> Then, we need to find a way to set the 'drive' property of the aspeed-sbc model.
> I suggest using the same method of the edk2 flash devices of the q35 machine.
> See ebc29e1beab0. Setting a machine option would set the drive. Something
> like :
> 
>    qemu-system-arm -M ast2600-evb,otpmem=otpmem-drive \
>        -blockdev
> node-name=otpmem,driver=file,filename=/path/to/otpmem.img \
>        ...
> 
> This machine option would only be defined for machine types needing it.
> 
> Thanks,
> 
> C.
> 
> 
> 
> 
> 
> > Thanks for your comments and review.
> >
> > Best Regards,
> > Kane
> >
> >> -----Original Message-----
> >> From: Cédric Le Goater <clg@kaod.org>
> >> Sent: Friday, April 4, 2025 9:54 PM
> >> To: Philippe Mathieu-Daudé <philmd@linaro.org>; 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>; qemu-block <qemu-block@nongnu.org>
> >> Cc: Troy Lee <troy_lee@aspeedtech.com>
> >> Subject: Re: [PATCH v1 0/1] hw/misc/aspeed_sbc: Implement OTP memory
> >> and controller
> >>
> >> On 4/4/25 15:00, Philippe Mathieu-Daudé wrote:
> >>> +qemu-block@
> >>>
> >>> On 4/4/25 14:06, Cédric Le Goater wrote:
> >>>> Hello,
> >>>>
> >>>> On 4/2/25 11:14, Kane-Chen-AS wrote:
> >>>>> This patch introduces part of the Secure Boot Controller device,
> >>>>> which consists of several sub-components, including an OTP memory,
> >>>>> OTP controller, cryptographic engine, and boot controller.
> >>>>>
> >>>>> In this version, the implementation includes the OTP memory and
> >>>>> its controller. The OTP memory can be programmed from within the
> >>>>> guest OS via a software utility.
> >>>>
> >>>>
> >>>> What is the OTP memory ? An external flash device or built-in SRAM ?
> >>>> If the latter, I suggest using an allocated buffer under the SBC
> >>>> model and avoid the complexity of the BlockBackend implementation
> >>>> and the definition of a drive on the command line for it. The
> >>>> proposal is bypassing a lot of QEMU layers for this purpose.
> >>>
> >>> More of the former, a built-in eFuse behaving more like flash. So
> >>> using block backend for the storage seems correct to me.
> >>
> >> How would you define the drive backend on the command line ?
> >>
> >>> However I don't think
> >>> the implementation belongs to hw/misc/aspeed_sbc; ideally we'd have
> >>> some abstract (or interface) implementation in hw/block/otp.c --
> >>> with methods such program_otp_data() --, completed by
> hw/block/aspeed_otc.c.
> >>
> >> I was imagining more something like NPCM7xxOTPState or
> >> BCM2835OTPState and not SiFiveUOTPState.
> >>
> >>> Current patch might be good enough to start with IMHO.
> >>
> >> Have you looked at the next patch and how the backend is handled ?
> >> I will let the block people Ack this patch in that case. It's beyond my skills.
> >>
> >> Thanks,
> >>
> >> C.
> >>
> >

Configuring onboard devices, in particular memory contents (was: [PATCH v1 0/1] hw/misc/aspeed_sbc: Implement OTP memory and controller)

[Markus Armbruster]

Cédric Le Goater <clg@kaod.org> writes:

> Hello Kane,
>
> + Markus (for ebc29e1beab0 implementation)
>
> On 4/7/25 09:33, Kane Chen wrote:
>> Hi Cédric/Philippe,
>> OTP (One-Time Programmable) memory is a type of non-volatile memory
>> in which each bit can be programmed only once. It is typically used
>> to store critical and permanent information, such as the chip ID and
>> secure boot keys. The structure and behavior of OTP memory are
>> consistent across both the AST1030 and AST2600 platforms.
>> As Philippe pointed out, this proposal models the OTP memory as a
>> flash device and utilizes a block backend for persistent storage. In
>> contrast, existing implementations such as NPCM7xxOTPState,
>> BCM2835OTPState, and SiFiveUOTPState expose OTP memory via MMIO and
>> always initialize it in a blank state. 
>
> AFAIU, Aspeed SBC is also MMIO based or is there another device,
> an eeprom, accessible through an external bus ? How is it
> implemented in HW ?
>
>> The goal of this design is to
>> allow the guest system to boot with a pre-configured OTP memory
>> state. 
>
> Yes. This is a valid request. It's not the first time we've had
> this kind of requests. The initial content of EEPROM devices are
> an example and some machines, like the rainier, have a lot.
>
> If the device can be defined on the command line, like would be
> an EEPROM device attached to an I2C bus or a flash device attached
> to a SPI bus, we can use a 'drive' property. Something like :
>
>   qemu-system-arm -M ast2600-evb \
>       -blockdev node-name=fmc0,driver=file,filename=/path/to/fmc0.img \
>       -device mx66u51235f,bus=ssi.0,cs=0x0,drive=fmc0 \
>       -blockdev node-name=fmc1,driver=file,filename=/path/to/fmc1.img \
>       -device mx66u51235f,bus=ssi.0,cs=0x1,drive=fmc1 \
>       -blockdev node-name=spi1,driver=file,filename=/path/to/spi1.img \
>       -device mx66u51235f,cs=0x0,bus=ssi.1,drive=spi1 \
>       ...
>
> However, the Aspeed SBC device is a platform device and it makes
> things more complex : it can not be created on the command line,
> it is directly created by the machine and the soc and passing
> device properties to specify a blockdev it is not possible :
>
>   qemu-system-arm -M ast2600-evb \
>       -blockdev node-name=otpmem,driver=file,filename=/path/to/otpmem.img \
>       -device aspeed-sbc,drive=otpmem \
>       ...

Configuring onboard devices is an old problem, and so far we have failed
at solving it adequately.

-device / device_add let you configure the new device in a general way,
but these work only for device the user creates, not for devices the
board creates automatically.

We have a bunch of ad hoc and mostly ancient ways to configure them, but
they're all limited.  For example:

* A number of old command line options, such as -drive, -serial, -net
  nic, create device backends and additionally deposit configuration in
  some global table the board may elect to use however it sees fit.  The
  intended use is to create frontends connected to these backends.

  Some boards error out when they can't honor something in the table.
  Others silently ignore parts of the table, or all of it.  Bad UI.

  Device configuration the table doesn't support is not accessible this
  way.  If you extend the table (and the associated option) to provide
  access to some device-specific configuration, all the other devices
  will silently ignore the new configuration bits.  Again, bad UI.

  There's another serious issue with block devices: -drive is obsolete
  for configurating complex block backends.  But its replacement
  -blockdev is for backend configuration only.  If you use -blockdev,
  you can't add to the table.

* Command line option -global lets you change property defaults.  This
  can be used to configure an onboard device as long as it is the only
  such device in the system.  Limited use, and also bad UI.

A modern attempt at a solution is to have machine properties alias
properties of onboard devices, so you can specify them with -machine.
For instance, a few machines expose the "drive" property of two onboard
pflash devices as machine properties "pflash0" and "pflash1".

Commits

    e0561e60f170 (hw/arm/virt: Support firmware configuration with -blockdev)
    ebc29e1beab0 (pc: Support firmware configuration with -blockdev) 

explain this in a lot more detail in their commit messages.

Sadly, this solution does not scale.  Adding alias properties to the
machine object is work, sometimes a lot of work (evidence: the two
commits above).  There are simply too many onboard devices with too many
properties to all manually alias.

Of course, even an insufficiently general / scalable solution like this
one can work well enough for specific cases.

>> To support this, the OTP memory is backed by a file,
>> simulating persistent flash behavior.
>
> The idea is good but the implementation is problematic.
>
>     +static BlockBackend *init_otpmem(int64_t size_bytes)
>     +{
>     +    Error *local_err = NULL;
>     +    BlockDriverState *bs = NULL;
>     +    BlockBackend *blk = NULL;
>     +    bool image_created = false;
>     +    QDict *options;
>     +    uint32_t i, odd_def = 0xffffffff, even_def = 0, *def;
>     +
>     +    if (!g_file_test(OTP_FILE_PATH, G_FILE_TEST_EXISTS)) {
>     +        bdrv_img_create(OTP_FILE_PATH, "raw", NULL, NULL,
>     +                        NULL, size_bytes, 0, true, &local_err);
>     +        if (local_err) {
>     +            qemu_log_mask(LOG_GUEST_ERROR,
>     +                          "%s: Failed to create image %s: %s\n",
>     +                          __func__, OTP_FILE_PATH,
>     +                          error_get_pretty(local_err));
>     +            error_free(local_err);
>     +            return NULL;
>     +        }
>     +        image_created = true;
>     +    }
>     +
>     +    blk = blk_new(qemu_get_aio_context(),
>     +                  BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
>     +                  0);
>     +    if (!blk) {
>     +        qemu_log_mask(LOG_GUEST_ERROR,
>     +                      "%s: Failed to create BlockBackend\n",
>     +                      __func__);
>     +        return NULL;
>     +    }
>     +
>     +    options =  qdict_new();
>     +    qdict_put_str(options, "driver", "raw");
>     +    bs = bdrv_open(OTP_FILE_PATH, NULL, options, BDRV_O_RDWR, &local_err);
>     +    if (local_err) {
>     +        qemu_log_mask(LOG_GUEST_ERROR,
>     +                      "%s: Failed to create OTP memory, err = %s\n",
>     +                      __func__, error_get_pretty(local_err));
>     +        blk_unref(blk);
>     +        error_free(local_err);
>     +        return NULL;
>     +    }
>     +
>     +    blk_insert_bs(blk, bs, &local_err);
>     +    if (local_err) {
>     +        qemu_log_mask(LOG_GUEST_ERROR,
>     +                      "%s: Failed to insert OTP memory to SBC, err = %s\n",
>     +                      __func__, error_get_pretty(local_err));
>     +        bdrv_unref(bs);
>     +        blk_unref(blk);
>     +        error_free(local_err);
>     +        return NULL;
>     +    }
>     +    bdrv_unref(bs);
>     ...
>
> IMO, this is low level block code that a device model shouldn't have
> to deal with. A 'drive' should be used instead. Now, if the qemu-block
> maintainers are OK with it, we need their approval.

Using block backends to specify the contents of a memory device is a bit
of a hack.  However, it's the hacky solution we use, and until we have a
better solution, new code is well advised to stick to the same hacky
solution we use in existing code.

Why is it a bit of a hack?  Well, memory isn't a block device.  For
read-only memory, all we want from the block device is slurping in some
image in its entirety.  We're not interesting in reading parts, or
writing at all.  For writable memory, we are interested in writing, but
there's often a awkward translation to block device blocks.

>  > The OTP memory access flow is as follows:
>> 1. The guest issues a read or write OTP command to the Secure Boot
>>     Controller (SBC)
>> 2. The SBC triggers the corresponding operation in the OTP controller
>> 3. The SBC returns the result to the guest
>> Since the guest interacts with OTP memory exclusively through the
>> SBC, the OTP logic is implemented within aspeed_sbc.c.
>> If there are existing architectural guidelines or design patterns
>> that should be followed for modeling OTP devices, I would greatly
>> appreciate your feedback. I am happy to revise the implementation
>> accordingly and submit updated patches for further review.
>
> Adding a 'drive' property to the aspeed-sbc model shouldn't change
> too much the proposal and it will remove the init_otpmem() routine,
> which is problematic.
>
> Then, we need to find a way to set the 'drive' property of the
> aspeed-sbc model. I suggest using the same method of the edk2 flash
> devices of the q35 machine. See ebc29e1beab0. Setting a machine
> option would set the drive. Something like :
>
>   qemu-system-arm -M ast2600-evb,otpmem=otpmem-drive \
>       -blockdev node-name=otpmem,driver=file,filename=/path/to/otpmem.img \
>       ...
>
> This machine option would only be defined for machine types needing
> it.

I don't love this solution, but it's what we use elsewhere.  I think
Cédric is right.

RE: Configuring onboard devices, in particular memory contents (was: [PATCH v1 0/1] hw/misc/aspeed_sbc: Implement OTP memory and controller)

[Kane Chen]

Hi Markus,

Thank you for the background information.

Since the OTP device is part of the Secure Boot Controller (SBC), I plan to register it in the global table. I believe this will simplify usage.

Meanwhile, based on Philippe's comment, I’m working on `aspeed_otp.c` to handle low-level OTP operations. This approach should help decouple SBC and OTP functionalities.

Once testing is complete, I will submit a separate patch for further review.

Best regards,
Kane
> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Tuesday, April 8, 2025 7:39 PM
> To: Cédric Le Goater <clg@kaod.org>
> Cc: Kane Chen <kane_chen@aspeedtech.com>; Philippe Mathieu-Daudé
> <philmd@linaro.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>; qemu-block <qemu-block@nongnu.org>; Troy Lee
> <troy_lee@aspeedtech.com>
> Subject: Configuring onboard devices, in particular memory contents (was:
> [PATCH v1 0/1] hw/misc/aspeed_sbc: Implement OTP memory and controller)
> 
> Cédric Le Goater <clg@kaod.org> writes:
> 
> > Hello Kane,
> >
> > + Markus (for ebc29e1beab0 implementation)
> >
> > On 4/7/25 09:33, Kane Chen wrote:
> >> Hi Cédric/Philippe,
> >> OTP (One-Time Programmable) memory is a type of non-volatile memory
> >> in which each bit can be programmed only once. It is typically used
> >> to store critical and permanent information, such as the chip ID and
> >> secure boot keys. The structure and behavior of OTP memory are
> >> consistent across both the AST1030 and AST2600 platforms.
> >> As Philippe pointed out, this proposal models the OTP memory as a
> >> flash device and utilizes a block backend for persistent storage. In
> >> contrast, existing implementations such as NPCM7xxOTPState,
> >> BCM2835OTPState, and SiFiveUOTPState expose OTP memory via MMIO
> and
> >> always initialize it in a blank state.
> >
> > AFAIU, Aspeed SBC is also MMIO based or is there another device, an
> > eeprom, accessible through an external bus ? How is it implemented in
> > HW ?
> >
> >> The goal of this design is to
> >> allow the guest system to boot with a pre-configured OTP memory
> >> state.
> >
> > Yes. This is a valid request. It's not the first time we've had this
> > kind of requests. The initial content of EEPROM devices are an example
> > and some machines, like the rainier, have a lot.
> >
> > If the device can be defined on the command line, like would be an
> > EEPROM device attached to an I2C bus or a flash device attached to a
> > SPI bus, we can use a 'drive' property. Something like :
> >
> >   qemu-system-arm -M ast2600-evb \
> >       -blockdev node-name=fmc0,driver=file,filename=/path/to/fmc0.img
> \
> >       -device mx66u51235f,bus=ssi.0,cs=0x0,drive=fmc0 \
> >       -blockdev node-name=fmc1,driver=file,filename=/path/to/fmc1.img
> \
> >       -device mx66u51235f,bus=ssi.0,cs=0x1,drive=fmc1 \
> >       -blockdev node-name=spi1,driver=file,filename=/path/to/spi1.img \
> >       -device mx66u51235f,cs=0x0,bus=ssi.1,drive=spi1 \
> >       ...
> >
> > However, the Aspeed SBC device is a platform device and it makes
> > things more complex : it can not be created on the command line, it is
> > directly created by the machine and the soc and passing device
> > properties to specify a blockdev it is not possible :
> >
> >   qemu-system-arm -M ast2600-evb \
> >       -blockdev
> node-name=otpmem,driver=file,filename=/path/to/otpmem.img \
> >       -device aspeed-sbc,drive=otpmem \
> >       ...
> 
> Configuring onboard devices is an old problem, and so far we have failed at
> solving it adequately.
> 
> -device / device_add let you configure the new device in a general way, but
> these work only for device the user creates, not for devices the board creates
> automatically.
> 
> We have a bunch of ad hoc and mostly ancient ways to configure them, but
> they're all limited.  For example:
> 
> * A number of old command line options, such as -drive, -serial, -net
>   nic, create device backends and additionally deposit configuration in
>   some global table the board may elect to use however it sees fit.  The
>   intended use is to create frontends connected to these backends.
> 
>   Some boards error out when they can't honor something in the table.
>   Others silently ignore parts of the table, or all of it.  Bad UI.
> 
>   Device configuration the table doesn't support is not accessible this
>   way.  If you extend the table (and the associated option) to provide
>   access to some device-specific configuration, all the other devices
>   will silently ignore the new configuration bits.  Again, bad UI.
> 
>   There's another serious issue with block devices: -drive is obsolete
>   for configurating complex block backends.  But its replacement
>   -blockdev is for backend configuration only.  If you use -blockdev,
>   you can't add to the table.
> 
> * Command line option -global lets you change property defaults.  This
>   can be used to configure an onboard device as long as it is the only
>   such device in the system.  Limited use, and also bad UI.
> 
> A modern attempt at a solution is to have machine properties alias properties
> of onboard devices, so you can specify them with -machine.
> For instance, a few machines expose the "drive" property of two onboard
> pflash devices as machine properties "pflash0" and "pflash1".
> 
> Commits
> 
>     e0561e60f170 (hw/arm/virt: Support firmware configuration with
> -blockdev)
>     ebc29e1beab0 (pc: Support firmware configuration with -blockdev)
> 
> explain this in a lot more detail in their commit messages.
> 
> Sadly, this solution does not scale.  Adding alias properties to the machine
> object is work, sometimes a lot of work (evidence: the two commits above).
> There are simply too many onboard devices with too many properties to all
> manually alias.
> 
> Of course, even an insufficiently general / scalable solution like this one can
> work well enough for specific cases.
> 
> >> To support this, the OTP memory is backed by a file, simulating
> >> persistent flash behavior.
> >
> > The idea is good but the implementation is problematic.
> >
> >     +static BlockBackend *init_otpmem(int64_t size_bytes)
> >     +{
> >     +    Error *local_err = NULL;
> >     +    BlockDriverState *bs = NULL;
> >     +    BlockBackend *blk = NULL;
> >     +    bool image_created = false;
> >     +    QDict *options;
> >     +    uint32_t i, odd_def = 0xffffffff, even_def = 0, *def;
> >     +
> >     +    if (!g_file_test(OTP_FILE_PATH, G_FILE_TEST_EXISTS)) {
> >     +        bdrv_img_create(OTP_FILE_PATH, "raw", NULL, NULL,
> >     +                        NULL, size_bytes, 0, true, &local_err);
> >     +        if (local_err) {
> >     +            qemu_log_mask(LOG_GUEST_ERROR,
> >     +                          "%s: Failed to create image %s: %s\n",
> >     +                          __func__, OTP_FILE_PATH,
> >     +                          error_get_pretty(local_err));
> >     +            error_free(local_err);
> >     +            return NULL;
> >     +        }
> >     +        image_created = true;
> >     +    }
> >     +
> >     +    blk = blk_new(qemu_get_aio_context(),
> >     +                  BLK_PERM_CONSISTENT_READ |
> BLK_PERM_WRITE,
> >     +                  0);
> >     +    if (!blk) {
> >     +        qemu_log_mask(LOG_GUEST_ERROR,
> >     +                      "%s: Failed to create BlockBackend\n",
> >     +                      __func__);
> >     +        return NULL;
> >     +    }
> >     +
> >     +    options =  qdict_new();
> >     +    qdict_put_str(options, "driver", "raw");
> >     +    bs = bdrv_open(OTP_FILE_PATH, NULL, options, BDRV_O_RDWR,
> &local_err);
> >     +    if (local_err) {
> >     +        qemu_log_mask(LOG_GUEST_ERROR,
> >     +                      "%s: Failed to create OTP memory, err =
> %s\n",
> >     +                      __func__, error_get_pretty(local_err));
> >     +        blk_unref(blk);
> >     +        error_free(local_err);
> >     +        return NULL;
> >     +    }
> >     +
> >     +    blk_insert_bs(blk, bs, &local_err);
> >     +    if (local_err) {
> >     +        qemu_log_mask(LOG_GUEST_ERROR,
> >     +                      "%s: Failed to insert OTP memory to SBC,
> err = %s\n",
> >     +                      __func__, error_get_pretty(local_err));
> >     +        bdrv_unref(bs);
> >     +        blk_unref(blk);
> >     +        error_free(local_err);
> >     +        return NULL;
> >     +    }
> >     +    bdrv_unref(bs);
> >     ...
> >
> > IMO, this is low level block code that a device model shouldn't have
> > to deal with. A 'drive' should be used instead. Now, if the qemu-block
> > maintainers are OK with it, we need their approval.
> 
> Using block backends to specify the contents of a memory device is a bit of a
> hack.  However, it's the hacky solution we use, and until we have a better
> solution, new code is well advised to stick to the same hacky solution we use in
> existing code.
> 
> Why is it a bit of a hack?  Well, memory isn't a block device.  For read-only
> memory, all we want from the block device is slurping in some image in its
> entirety.  We're not interesting in reading parts, or writing at all.  For
> writable memory, we are interested in writing, but there's often a awkward
> translation to block device blocks.
> 
> >  > The OTP memory access flow is as follows:
> >> 1. The guest issues a read or write OTP command to the Secure Boot
> >>     Controller (SBC)
> >> 2. The SBC triggers the corresponding operation in the OTP controller
> >> 3. The SBC returns the result to the guest Since the guest interacts
> >> with OTP memory exclusively through the SBC, the OTP logic is
> >> implemented within aspeed_sbc.c.
> >> If there are existing architectural guidelines or design patterns
> >> that should be followed for modeling OTP devices, I would greatly
> >> appreciate your feedback. I am happy to revise the implementation
> >> accordingly and submit updated patches for further review.
> >
> > Adding a 'drive' property to the aspeed-sbc model shouldn't change too
> > much the proposal and it will remove the init_otpmem() routine, which
> > is problematic.
> >
> > Then, we need to find a way to set the 'drive' property of the
> > aspeed-sbc model. I suggest using the same method of the edk2 flash
> > devices of the q35 machine. See ebc29e1beab0. Setting a machine option
> > would set the drive. Something like :
> >
> >   qemu-system-arm -M ast2600-evb,otpmem=otpmem-drive \
> >       -blockdev
> node-name=otpmem,driver=file,filename=/path/to/otpmem.img \
> >       ...
> >
> > This machine option would only be defined for machine types needing
> > it.
> 
> I don't love this solution, but it's what we use elsewhere.  I think Cédric is
> right.

Re: Configuring onboard devices, in particular memory contents

[Cédric Le Goater]

On 4/11/25 11:08, Kane Chen wrote:
> Hi Markus,
> 
> Thank you for the background information.
> 
> Since the OTP device is part of the Secure Boot Controller (SBC), I plan to register it in the global table. I believe this will simplify usage.
> 
> Meanwhile, based on Philippe's comment, I’m working on `aspeed_otp.c` to handle low-level OTP operations. 
>
> This approach should help decouple SBC and OTP functionalities.

AFAUI, I think Philippe meant something more like a generic OPT block
model, hw/block/otp.c, which would implement the low level storage
primitives of an OTP device. This model would then be used by the
Aspeed SBC model, providing the SW/HW interface, and possibly also
used by other models, such as the NPCM7xxOTPState, BCM2835OTPState,
and SiFiveUOTPState models.

Anyhow, separating the layers is a good idea :
   - OTP like device
   - Aspeed SBC interface
   - machine/SoC interface to define the backend.
   - and a doc update !
   - possibly a (functional) test case


Thanks,

C.



> 
> Once testing is complete, I will submit a separate patch for further review.
> 
> Best regards,
> Kane
>> -----Original Message-----
>> From: Markus Armbruster <armbru@redhat.com>
>> Sent: Tuesday, April 8, 2025 7:39 PM
>> To: Cédric Le Goater <clg@kaod.org>
>> Cc: Kane Chen <kane_chen@aspeedtech.com>; Philippe Mathieu-Daudé
>> <philmd@linaro.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>; qemu-block <qemu-block@nongnu.org>; Troy Lee
>> <troy_lee@aspeedtech.com>
>> Subject: Configuring onboard devices, in particular memory contents (was:
>> [PATCH v1 0/1] hw/misc/aspeed_sbc: Implement OTP memory and controller)
>>
>> Cédric Le Goater <clg@kaod.org> writes:
>>
>>> Hello Kane,
>>>
>>> + Markus (for ebc29e1beab0 implementation)
>>>
>>> On 4/7/25 09:33, Kane Chen wrote:
>>>> Hi Cédric/Philippe,
>>>> OTP (One-Time Programmable) memory is a type of non-volatile memory
>>>> in which each bit can be programmed only once. It is typically used
>>>> to store critical and permanent information, such as the chip ID and
>>>> secure boot keys. The structure and behavior of OTP memory are
>>>> consistent across both the AST1030 and AST2600 platforms.
>>>> As Philippe pointed out, this proposal models the OTP memory as a
>>>> flash device and utilizes a block backend for persistent storage. In
>>>> contrast, existing implementations such as NPCM7xxOTPState,
>>>> BCM2835OTPState, and SiFiveUOTPState expose OTP memory via MMIO
>> and
>>>> always initialize it in a blank state.
>>>
>>> AFAIU, Aspeed SBC is also MMIO based or is there another device, an
>>> eeprom, accessible through an external bus ? How is it implemented in
>>> HW ?
>>>
>>>> The goal of this design is to
>>>> allow the guest system to boot with a pre-configured OTP memory
>>>> state.
>>>
>>> Yes. This is a valid request. It's not the first time we've had this
>>> kind of requests. The initial content of EEPROM devices are an example
>>> and some machines, like the rainier, have a lot.
>>>
>>> If the device can be defined on the command line, like would be an
>>> EEPROM device attached to an I2C bus or a flash device attached to a
>>> SPI bus, we can use a 'drive' property. Something like :
>>>
>>>    qemu-system-arm -M ast2600-evb \
>>>        -blockdev node-name=fmc0,driver=file,filename=/path/to/fmc0.img
>> \
>>>        -device mx66u51235f,bus=ssi.0,cs=0x0,drive=fmc0 \
>>>        -blockdev node-name=fmc1,driver=file,filename=/path/to/fmc1.img
>> \
>>>        -device mx66u51235f,bus=ssi.0,cs=0x1,drive=fmc1 \
>>>        -blockdev node-name=spi1,driver=file,filename=/path/to/spi1.img \
>>>        -device mx66u51235f,cs=0x0,bus=ssi.1,drive=spi1 \
>>>        ...
>>>
>>> However, the Aspeed SBC device is a platform device and it makes
>>> things more complex : it can not be created on the command line, it is
>>> directly created by the machine and the soc and passing device
>>> properties to specify a blockdev it is not possible :
>>>
>>>    qemu-system-arm -M ast2600-evb \
>>>        -blockdev
>> node-name=otpmem,driver=file,filename=/path/to/otpmem.img \
>>>        -device aspeed-sbc,drive=otpmem \
>>>        ...
>>
>> Configuring onboard devices is an old problem, and so far we have failed at
>> solving it adequately.
>>
>> -device / device_add let you configure the new device in a general way, but
>> these work only for device the user creates, not for devices the board creates
>> automatically.
>>
>> We have a bunch of ad hoc and mostly ancient ways to configure them, but
>> they're all limited.  For example:
>>
>> * A number of old command line options, such as -drive, -serial, -net
>>    nic, create device backends and additionally deposit configuration in
>>    some global table the board may elect to use however it sees fit.  The
>>    intended use is to create frontends connected to these backends.
>>
>>    Some boards error out when they can't honor something in the table.
>>    Others silently ignore parts of the table, or all of it.  Bad UI.
>>
>>    Device configuration the table doesn't support is not accessible this
>>    way.  If you extend the table (and the associated option) to provide
>>    access to some device-specific configuration, all the other devices
>>    will silently ignore the new configuration bits.  Again, bad UI.
>>
>>    There's another serious issue with block devices: -drive is obsolete
>>    for configurating complex block backends.  But its replacement
>>    -blockdev is for backend configuration only.  If you use -blockdev,
>>    you can't add to the table.
>>
>> * Command line option -global lets you change property defaults.  This
>>    can be used to configure an onboard device as long as it is the only
>>    such device in the system.  Limited use, and also bad UI.
>>
>> A modern attempt at a solution is to have machine properties alias properties
>> of onboard devices, so you can specify them with -machine.
>> For instance, a few machines expose the "drive" property of two onboard
>> pflash devices as machine properties "pflash0" and "pflash1".
>>
>> Commits
>>
>>      e0561e60f170 (hw/arm/virt: Support firmware configuration with
>> -blockdev)
>>      ebc29e1beab0 (pc: Support firmware configuration with -blockdev)
>>
>> explain this in a lot more detail in their commit messages.
>>
>> Sadly, this solution does not scale.  Adding alias properties to the machine
>> object is work, sometimes a lot of work (evidence: the two commits above).
>> There are simply too many onboard devices with too many properties to all
>> manually alias.
>>
>> Of course, even an insufficiently general / scalable solution like this one can
>> work well enough for specific cases.
>>
>>>> To support this, the OTP memory is backed by a file, simulating
>>>> persistent flash behavior.
>>>
>>> The idea is good but the implementation is problematic.
>>>
>>>      +static BlockBackend *init_otpmem(int64_t size_bytes)
>>>      +{
>>>      +    Error *local_err = NULL;
>>>      +    BlockDriverState *bs = NULL;
>>>      +    BlockBackend *blk = NULL;
>>>      +    bool image_created = false;
>>>      +    QDict *options;
>>>      +    uint32_t i, odd_def = 0xffffffff, even_def = 0, *def;
>>>      +
>>>      +    if (!g_file_test(OTP_FILE_PATH, G_FILE_TEST_EXISTS)) {
>>>      +        bdrv_img_create(OTP_FILE_PATH, "raw", NULL, NULL,
>>>      +                        NULL, size_bytes, 0, true, &local_err);
>>>      +        if (local_err) {
>>>      +            qemu_log_mask(LOG_GUEST_ERROR,
>>>      +                          "%s: Failed to create image %s: %s\n",
>>>      +                          __func__, OTP_FILE_PATH,
>>>      +                          error_get_pretty(local_err));
>>>      +            error_free(local_err);
>>>      +            return NULL;
>>>      +        }
>>>      +        image_created = true;
>>>      +    }
>>>      +
>>>      +    blk = blk_new(qemu_get_aio_context(),
>>>      +                  BLK_PERM_CONSISTENT_READ |
>> BLK_PERM_WRITE,
>>>      +                  0);
>>>      +    if (!blk) {
>>>      +        qemu_log_mask(LOG_GUEST_ERROR,
>>>      +                      "%s: Failed to create BlockBackend\n",
>>>      +                      __func__);
>>>      +        return NULL;
>>>      +    }
>>>      +
>>>      +    options =  qdict_new();
>>>      +    qdict_put_str(options, "driver", "raw");
>>>      +    bs = bdrv_open(OTP_FILE_PATH, NULL, options, BDRV_O_RDWR,
>> &local_err);
>>>      +    if (local_err) {
>>>      +        qemu_log_mask(LOG_GUEST_ERROR,
>>>      +                      "%s: Failed to create OTP memory, err =
>> %s\n",
>>>      +                      __func__, error_get_pretty(local_err));
>>>      +        blk_unref(blk);
>>>      +        error_free(local_err);
>>>      +        return NULL;
>>>      +    }
>>>      +
>>>      +    blk_insert_bs(blk, bs, &local_err);
>>>      +    if (local_err) {
>>>      +        qemu_log_mask(LOG_GUEST_ERROR,
>>>      +                      "%s: Failed to insert OTP memory to SBC,
>> err = %s\n",
>>>      +                      __func__, error_get_pretty(local_err));
>>>      +        bdrv_unref(bs);
>>>      +        blk_unref(blk);
>>>      +        error_free(local_err);
>>>      +        return NULL;
>>>      +    }
>>>      +    bdrv_unref(bs);
>>>      ...
>>>
>>> IMO, this is low level block code that a device model shouldn't have
>>> to deal with. A 'drive' should be used instead. Now, if the qemu-block
>>> maintainers are OK with it, we need their approval.
>>
>> Using block backends to specify the contents of a memory device is a bit of a
>> hack.  However, it's the hacky solution we use, and until we have a better
>> solution, new code is well advised to stick to the same hacky solution we use in
>> existing code.
>>
>> Why is it a bit of a hack?  Well, memory isn't a block device.  For read-only
>> memory, all we want from the block device is slurping in some image in its
>> entirety.  We're not interesting in reading parts, or writing at all.  For
>> writable memory, we are interested in writing, but there's often a awkward
>> translation to block device blocks.
>>
>>>   > The OTP memory access flow is as follows:
>>>> 1. The guest issues a read or write OTP command to the Secure Boot
>>>>      Controller (SBC)
>>>> 2. The SBC triggers the corresponding operation in the OTP controller
>>>> 3. The SBC returns the result to the guest Since the guest interacts
>>>> with OTP memory exclusively through the SBC, the OTP logic is
>>>> implemented within aspeed_sbc.c.
>>>> If there are existing architectural guidelines or design patterns
>>>> that should be followed for modeling OTP devices, I would greatly
>>>> appreciate your feedback. I am happy to revise the implementation
>>>> accordingly and submit updated patches for further review.
>>>
>>> Adding a 'drive' property to the aspeed-sbc model shouldn't change too
>>> much the proposal and it will remove the init_otpmem() routine, which
>>> is problematic.
>>>
>>> Then, we need to find a way to set the 'drive' property of the
>>> aspeed-sbc model. I suggest using the same method of the edk2 flash
>>> devices of the q35 machine. See ebc29e1beab0. Setting a machine option
>>> would set the drive. Something like :
>>>
>>>    qemu-system-arm -M ast2600-evb,otpmem=otpmem-drive \
>>>        -blockdev
>> node-name=otpmem,driver=file,filename=/path/to/otpmem.img \
>>>        ...
>>>
>>> This machine option would only be defined for machine types needing
>>> it.
>>
>> I don't love this solution, but it's what we use elsewhere.  I think Cédric is
>> right.
>