* [PATCH v3 0/3] hw/misc/aspeed_otp: Introduce OTP memory and integrate with SBC
@ 2025-04-23 2:56 ` Kane Chen via
0 siblings, 0 replies; 21+ messages in thread
From: Kane Chen via @ 2025-04-23 2:56 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Jamin Lin, Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: troy_lee, Kane-Chen-AS
From: Kane-Chen-AS <kane_chen@aspeedtech.com>
Dear reviewers,
This patch series introduces a new model for the ASPEED OTP (One-Time
Programmable) memory and integrates it with the ASPEED Secure Boot
Controller (SBC) and SoC models such as AST1030 and AST2600.
The OTP memory is implemented as a QEMU device (`aspeed.otpmem`) and is
designed to simulate fuse-like behavior using a block backend. Programming
is restricted by irreversible bit-flip rules (e.g., 0->1 or 1->0 depending
on address parity). The memory content is persistent and can be passed via
`-drive id=otpmem`.
The SBC device (`aspeed.sbc`) accesses the OTP device through a QOM link
property. This decouples OTP handling from the SBC logic and allows
future extensibility such as secure configuration or key provisioning.
The SoC integration connects the OTP memory to the SBC and provides
device-tree-style infrastructure for future firmware use.
Any feedback or suggestions are appreciated!
Thanks,
Kane
---
ChangeLog
---------
v3:
- Minor fixes and refinements on top of v2 content
v2:
- Separated the OTP memory into a standalone QEMU device (`aspeed.otpmem`)
- Replaced ad-hoc file handling with standard `-drive` backend integration
- Linked OTP to SBC via QOM property for cleaner abstraction
- Improved memory bounds checking and irreversible programming logic
v1:
- Embedded OTP logic in the SBC model and created the backing file internally.
---
Kane-Chen-AS (3):
hw/misc/aspeed_otp: Add Aspeed OTP memory device model
hw/misc/aspeed_sbc: Connect Aspeed OTP memory device to SBC controller
hw/arm: Integrate Aspeed OTP memory into AST10x0 and AST2600 SoCs
hw/arm/aspeed_ast10x0.c | 19 +++
hw/arm/aspeed_ast2600.c | 19 +++
hw/misc/aspeed_otpmem.c | 211 ++++++++++++++++++++++++++++++++
hw/misc/aspeed_sbc.c | 146 ++++++++++++++++++++++
hw/misc/meson.build | 1 +
include/hw/arm/aspeed_soc.h | 2 +
include/hw/misc/aspeed_otpmem.h | 40 ++++++
include/hw/misc/aspeed_sbc.h | 15 +++
8 files changed, 453 insertions(+)
create mode 100644 hw/misc/aspeed_otpmem.c
create mode 100644 include/hw/misc/aspeed_otpmem.h
--
2.43.0
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 0/3] hw/misc/aspeed_otp: Introduce OTP memory and integrate with SBC
@ 2025-04-23 2:56 ` Kane Chen via
0 siblings, 0 replies; 21+ messages in thread
From: Kane Chen via @ 2025-04-23 2:56 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Jamin Lin, Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: troy_lee, Kane-Chen-AS
From: Kane-Chen-AS <kane_chen@aspeedtech.com>
Dear reviewers,
This patch series introduces a new model for the ASPEED OTP (One-Time
Programmable) memory and integrates it with the ASPEED Secure Boot
Controller (SBC) and SoC models such as AST1030 and AST2600.
The OTP memory is implemented as a QEMU device (`aspeed.otpmem`) and is
designed to simulate fuse-like behavior using a block backend. Programming
is restricted by irreversible bit-flip rules (e.g., 0->1 or 1->0 depending
on address parity). The memory content is persistent and can be passed via
`-drive id=otpmem`.
The SBC device (`aspeed.sbc`) accesses the OTP device through a QOM link
property. This decouples OTP handling from the SBC logic and allows
future extensibility such as secure configuration or key provisioning.
The SoC integration connects the OTP memory to the SBC and provides
device-tree-style infrastructure for future firmware use.
Any feedback or suggestions are appreciated!
Thanks,
Kane
---
ChangeLog
---------
v3:
- Minor fixes and refinements on top of v2 content
v2:
- Separated the OTP memory into a standalone QEMU device (`aspeed.otpmem`)
- Replaced ad-hoc file handling with standard `-drive` backend integration
- Linked OTP to SBC via QOM property for cleaner abstraction
- Improved memory bounds checking and irreversible programming logic
v1:
- Embedded OTP logic in the SBC model and created the backing file internally.
---
Kane-Chen-AS (3):
hw/misc/aspeed_otp: Add Aspeed OTP memory device model
hw/misc/aspeed_sbc: Connect Aspeed OTP memory device to SBC controller
hw/arm: Integrate Aspeed OTP memory into AST10x0 and AST2600 SoCs
hw/arm/aspeed_ast10x0.c | 19 +++
hw/arm/aspeed_ast2600.c | 19 +++
hw/misc/aspeed_otpmem.c | 211 ++++++++++++++++++++++++++++++++
hw/misc/aspeed_sbc.c | 146 ++++++++++++++++++++++
hw/misc/meson.build | 1 +
include/hw/arm/aspeed_soc.h | 2 +
include/hw/misc/aspeed_otpmem.h | 40 ++++++
include/hw/misc/aspeed_sbc.h | 15 +++
8 files changed, 453 insertions(+)
create mode 100644 hw/misc/aspeed_otpmem.c
create mode 100644 include/hw/misc/aspeed_otpmem.h
--
2.43.0
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 1/3] hw/misc/aspeed_otp: Add Aspeed OTP memory device model
2025-04-23 2:56 ` Kane Chen via
@ 2025-04-23 2:56 ` Kane Chen via
-1 siblings, 0 replies; 21+ messages in thread
From: Kane Chen via @ 2025-04-23 2:56 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Jamin Lin, Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: troy_lee, Kane-Chen-AS
From: Kane-Chen-AS <kane_chen@aspeedtech.com>
This introduces a new model for the ASPEED OTP (One-Time Programmable)
memory. The device is implemented as a `SysBusDevice` and provides an
abstracted interface for OTP read, write (program), and default value
initialization.
OTP content is backed by a block device and supports QEMU’s drive
infrastructure via the "drive" property.
Features:
- Enforces irreversible bit programming logic (0->1 or 1->0)
- Provides interface for SoC/secure controller integration
- Validates bounds and bit-level constraints
- Uses QEMU error handling conventions and logging
Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
---
hw/misc/aspeed_otpmem.c | 211 ++++++++++++++++++++++++++++++++
hw/misc/meson.build | 1 +
include/hw/misc/aspeed_otpmem.h | 40 ++++++
3 files changed, 252 insertions(+)
create mode 100644 hw/misc/aspeed_otpmem.c
create mode 100644 include/hw/misc/aspeed_otpmem.h
diff --git a/hw/misc/aspeed_otpmem.c b/hw/misc/aspeed_otpmem.c
new file mode 100644
index 0000000000..4f8f2827f7
--- /dev/null
+++ b/hw/misc/aspeed_otpmem.c
@@ -0,0 +1,211 @@
+/*
+ * ASPEED OTP (One-Time Programmable) memory
+ *
+ * Copyright (C) 2025 Aspeed
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/block/block.h"
+#include "hw/block/flash.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
+#include "system/block-backend.h"
+#include "qemu/log.h"
+#include "qemu/option.h"
+#include "hw/sysbus.h"
+#include "qemu/error-report.h"
+#include "hw/misc/aspeed_otpmem.h"
+
+static const Property aspeed_otpmem_properties[] = {
+ DEFINE_PROP_DRIVE("drive", AspeedOTPMemState, blk),
+};
+
+static void aspeed_otpmem_read(void *opaque, uint32_t addr,
+ uint32_t *out, Error **errp)
+{
+ AspeedOTPMemState *otp = ASPEED_OTPMEM(opaque);
+
+ assert(otp->blk);
+
+ if (out == NULL) {
+ error_setg(errp, "out is NULL");
+ return;
+ }
+
+ if (addr > (otp->max_size - 4)) {
+ error_setg(errp, "OTP memory 0x%x is exceeded", addr);
+ return;
+ }
+
+ if (blk_pread(otp->blk, (int64_t)addr, sizeof(uint32_t), out, 0) < 0) {
+ error_setg(errp, "Failed to read data 0x%x", addr);
+ return;
+ }
+ return;
+}
+
+static bool valid_program_data(uint32_t otp_addr,
+ uint32_t value, uint32_t prog_bit)
+{
+ uint32_t programmed_bits, has_programmable_bits;
+ bool is_odd = otp_addr & 1;
+
+ /*
+ * prog_bit uses 0s to indicate target bits to program:
+ * - if OTP word is even-indexed, programmed bits flip 0->1
+ * - if odd, bits flip 1->0
+ * Bit programming is one-way only and irreversible.
+ */
+ if (is_odd) {
+ programmed_bits = ~value & prog_bit;
+ } else {
+ programmed_bits = value & (~prog_bit);
+ }
+
+ /* If there is some bit can be programed, to accept the request */
+ has_programmable_bits = value ^ (~prog_bit);
+
+ if (programmed_bits) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Found programmed bits in addr %x\n",
+ __func__, otp_addr);
+ for (int i = 0; i < 32; ++i) {
+ if (programmed_bits & (1U << i)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ " Programmed bit %d\n",
+ i);
+ }
+ }
+ }
+
+ return has_programmable_bits != 0;
+}
+
+static bool program_otpmem_data(void *opaque, uint32_t otp_addr,
+ uint32_t prog_bit, uint32_t *value)
+{
+ AspeedOTPMemState *s = ASPEED_OTPMEM(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 (!valid_program_data(otp_addr, *value, prog_bit)) {
+ return false;
+ }
+
+ if (is_odd) {
+ *value &= ~prog_bit;
+ } else {
+ *value |= ~prog_bit;
+ }
+
+ return true;
+}
+
+static void aspeed_otpmem_prog(void *s, uint32_t otp_addr,
+ uint32_t data, Error **errp)
+{
+ AspeedOTPMemState *otp = ASPEED_OTPMEM(s);
+ uint32_t otp_offset, value;
+
+ assert(otp->blk);
+
+ if (otp_addr > (otp->max_size >> 2)) {
+ error_setg(errp, "OTP memory 0x%x is exceeded", otp_addr);
+ return;
+ }
+
+ otp_offset = otp_addr << 2;
+ if (!program_otpmem_data(s, otp_addr, data, &value)) {
+ error_setg(errp, "Failed to program data");
+ return;
+ }
+
+ if (blk_pwrite(otp->blk, (int64_t)otp_offset,
+ sizeof(value), &value, 0) < 0) {
+ error_setg(errp, "Failed to write data");
+ }
+
+ return;
+}
+
+static void aspeed_otpmem_set_default(void *s, uint32_t otp_offset,
+ uint32_t data, Error **errp)
+{
+ AspeedOTPMemState *otp = ASPEED_OTPMEM(s);
+
+ if ((otp_offset + 4) > otp->max_size) {
+ error_setg(errp, "OTP memory 0x%x is exceeded", otp_offset);
+ return;
+ }
+
+ if (blk_pwrite(otp->blk, (int64_t)otp_offset,
+ sizeof(data), &data, 0) < 0) {
+ error_setg(errp, "Failed to write data");
+ }
+ return;
+}
+
+static AspeedOTPMemOps aspeed_otpmem_ops = {
+ .read = aspeed_otpmem_read,
+ .prog = aspeed_otpmem_prog,
+ .set_default_value = aspeed_otpmem_set_default
+};
+
+static void aspeed_otpmem_realize(DeviceState *dev, Error **errp)
+{
+ AspeedOTPMemState *s = ASPEED_OTPMEM(dev);
+
+ if (!s->blk) {
+ error_setg(&error_fatal, "OTP memory is not initialized");
+ return;
+ }
+
+ s->max_size = blk_getlength(s->blk);
+ if (s->max_size < 0 || (s->max_size % 4)) {
+ error_setg(&error_fatal,
+ "Unexpected OTP memory size: %" PRId64 "",
+ s->max_size);
+ return;
+ }
+
+ s->ops = &aspeed_otpmem_ops;
+
+ return;
+}
+
+static void aspeed_otpmem_system_reset(DeviceState *dev)
+{
+ return;
+}
+
+static void aspeed_otpmem_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ device_class_set_legacy_reset(dc, aspeed_otpmem_system_reset);
+ dc->realize = aspeed_otpmem_realize;
+ device_class_set_props(dc, aspeed_otpmem_properties);
+
+}
+
+static const TypeInfo aspeed_otpmem_types[] = {
+ {
+ .name = TYPE_ASPEED_OTPMEM,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(AspeedOTPMemState),
+ .class_init = aspeed_otpmem_class_init,
+ },
+};
+
+DEFINE_TYPES(aspeed_otpmem_types)
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 6d47de482c..ed1eaaa2ad 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -136,6 +136,7 @@ system_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files(
'aspeed_sbc.c',
'aspeed_sdmc.c',
'aspeed_xdma.c',
+ 'aspeed_otpmem.c',
'aspeed_peci.c',
'aspeed_sli.c'))
diff --git a/include/hw/misc/aspeed_otpmem.h b/include/hw/misc/aspeed_otpmem.h
new file mode 100644
index 0000000000..11e2de70b6
--- /dev/null
+++ b/include/hw/misc/aspeed_otpmem.h
@@ -0,0 +1,40 @@
+/*
+ * ASPEED OTP (One-Time Programmable) memory
+ *
+ * Copyright (C) 2025 Aspeed
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ */
+
+#ifndef ASPEED_OTPMMEM_H
+#define ASPEED_OTPMMEM_H
+
+#include "hw/sysbus.h"
+#include "qapi/error.h"
+
+#define TYPE_ASPEED_OTPMEM "aspeed.otpmem"
+#define ASPEED_OTPMEM_DRIVE "otpmem"
+
+#define ASPEED_OTPMEM(obj) OBJECT_CHECK(AspeedOTPMemState, (obj), \
+ TYPE_ASPEED_OTPMEM)
+
+typedef struct AspeedOTPMemOps {
+ void (*read)(void *s, uint32_t addr, uint32_t *out, Error **errp);
+ void (*prog)(void *s, uint32_t addr, uint32_t data, Error **errp);
+ void (*set_default_value)(void *s, uint32_t otp_offset,
+ uint32_t data, Error **errp);
+} AspeedOTPMemOps;
+
+typedef struct AspeedOTPMemState {
+ SysBusDevice parent_obj;
+
+ MemoryRegion mmio;
+ BlockBackend *blk;
+ int64_t max_size;
+
+ AspeedOTPMemOps *ops;
+} AspeedOTPMemState;
+
+#endif /* ASPEED_OTPMMEM_H */
+
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 1/3] hw/misc/aspeed_otp: Add Aspeed OTP memory device model
@ 2025-04-23 2:56 ` Kane Chen via
0 siblings, 0 replies; 21+ messages in thread
From: Kane Chen via @ 2025-04-23 2:56 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Jamin Lin, Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: troy_lee, Kane-Chen-AS
From: Kane-Chen-AS <kane_chen@aspeedtech.com>
This introduces a new model for the ASPEED OTP (One-Time Programmable)
memory. The device is implemented as a `SysBusDevice` and provides an
abstracted interface for OTP read, write (program), and default value
initialization.
OTP content is backed by a block device and supports QEMU’s drive
infrastructure via the "drive" property.
Features:
- Enforces irreversible bit programming logic (0->1 or 1->0)
- Provides interface for SoC/secure controller integration
- Validates bounds and bit-level constraints
- Uses QEMU error handling conventions and logging
Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
---
hw/misc/aspeed_otpmem.c | 211 ++++++++++++++++++++++++++++++++
hw/misc/meson.build | 1 +
include/hw/misc/aspeed_otpmem.h | 40 ++++++
3 files changed, 252 insertions(+)
create mode 100644 hw/misc/aspeed_otpmem.c
create mode 100644 include/hw/misc/aspeed_otpmem.h
diff --git a/hw/misc/aspeed_otpmem.c b/hw/misc/aspeed_otpmem.c
new file mode 100644
index 0000000000..4f8f2827f7
--- /dev/null
+++ b/hw/misc/aspeed_otpmem.c
@@ -0,0 +1,211 @@
+/*
+ * ASPEED OTP (One-Time Programmable) memory
+ *
+ * Copyright (C) 2025 Aspeed
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/block/block.h"
+#include "hw/block/flash.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
+#include "system/block-backend.h"
+#include "qemu/log.h"
+#include "qemu/option.h"
+#include "hw/sysbus.h"
+#include "qemu/error-report.h"
+#include "hw/misc/aspeed_otpmem.h"
+
+static const Property aspeed_otpmem_properties[] = {
+ DEFINE_PROP_DRIVE("drive", AspeedOTPMemState, blk),
+};
+
+static void aspeed_otpmem_read(void *opaque, uint32_t addr,
+ uint32_t *out, Error **errp)
+{
+ AspeedOTPMemState *otp = ASPEED_OTPMEM(opaque);
+
+ assert(otp->blk);
+
+ if (out == NULL) {
+ error_setg(errp, "out is NULL");
+ return;
+ }
+
+ if (addr > (otp->max_size - 4)) {
+ error_setg(errp, "OTP memory 0x%x is exceeded", addr);
+ return;
+ }
+
+ if (blk_pread(otp->blk, (int64_t)addr, sizeof(uint32_t), out, 0) < 0) {
+ error_setg(errp, "Failed to read data 0x%x", addr);
+ return;
+ }
+ return;
+}
+
+static bool valid_program_data(uint32_t otp_addr,
+ uint32_t value, uint32_t prog_bit)
+{
+ uint32_t programmed_bits, has_programmable_bits;
+ bool is_odd = otp_addr & 1;
+
+ /*
+ * prog_bit uses 0s to indicate target bits to program:
+ * - if OTP word is even-indexed, programmed bits flip 0->1
+ * - if odd, bits flip 1->0
+ * Bit programming is one-way only and irreversible.
+ */
+ if (is_odd) {
+ programmed_bits = ~value & prog_bit;
+ } else {
+ programmed_bits = value & (~prog_bit);
+ }
+
+ /* If there is some bit can be programed, to accept the request */
+ has_programmable_bits = value ^ (~prog_bit);
+
+ if (programmed_bits) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Found programmed bits in addr %x\n",
+ __func__, otp_addr);
+ for (int i = 0; i < 32; ++i) {
+ if (programmed_bits & (1U << i)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ " Programmed bit %d\n",
+ i);
+ }
+ }
+ }
+
+ return has_programmable_bits != 0;
+}
+
+static bool program_otpmem_data(void *opaque, uint32_t otp_addr,
+ uint32_t prog_bit, uint32_t *value)
+{
+ AspeedOTPMemState *s = ASPEED_OTPMEM(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 (!valid_program_data(otp_addr, *value, prog_bit)) {
+ return false;
+ }
+
+ if (is_odd) {
+ *value &= ~prog_bit;
+ } else {
+ *value |= ~prog_bit;
+ }
+
+ return true;
+}
+
+static void aspeed_otpmem_prog(void *s, uint32_t otp_addr,
+ uint32_t data, Error **errp)
+{
+ AspeedOTPMemState *otp = ASPEED_OTPMEM(s);
+ uint32_t otp_offset, value;
+
+ assert(otp->blk);
+
+ if (otp_addr > (otp->max_size >> 2)) {
+ error_setg(errp, "OTP memory 0x%x is exceeded", otp_addr);
+ return;
+ }
+
+ otp_offset = otp_addr << 2;
+ if (!program_otpmem_data(s, otp_addr, data, &value)) {
+ error_setg(errp, "Failed to program data");
+ return;
+ }
+
+ if (blk_pwrite(otp->blk, (int64_t)otp_offset,
+ sizeof(value), &value, 0) < 0) {
+ error_setg(errp, "Failed to write data");
+ }
+
+ return;
+}
+
+static void aspeed_otpmem_set_default(void *s, uint32_t otp_offset,
+ uint32_t data, Error **errp)
+{
+ AspeedOTPMemState *otp = ASPEED_OTPMEM(s);
+
+ if ((otp_offset + 4) > otp->max_size) {
+ error_setg(errp, "OTP memory 0x%x is exceeded", otp_offset);
+ return;
+ }
+
+ if (blk_pwrite(otp->blk, (int64_t)otp_offset,
+ sizeof(data), &data, 0) < 0) {
+ error_setg(errp, "Failed to write data");
+ }
+ return;
+}
+
+static AspeedOTPMemOps aspeed_otpmem_ops = {
+ .read = aspeed_otpmem_read,
+ .prog = aspeed_otpmem_prog,
+ .set_default_value = aspeed_otpmem_set_default
+};
+
+static void aspeed_otpmem_realize(DeviceState *dev, Error **errp)
+{
+ AspeedOTPMemState *s = ASPEED_OTPMEM(dev);
+
+ if (!s->blk) {
+ error_setg(&error_fatal, "OTP memory is not initialized");
+ return;
+ }
+
+ s->max_size = blk_getlength(s->blk);
+ if (s->max_size < 0 || (s->max_size % 4)) {
+ error_setg(&error_fatal,
+ "Unexpected OTP memory size: %" PRId64 "",
+ s->max_size);
+ return;
+ }
+
+ s->ops = &aspeed_otpmem_ops;
+
+ return;
+}
+
+static void aspeed_otpmem_system_reset(DeviceState *dev)
+{
+ return;
+}
+
+static void aspeed_otpmem_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ device_class_set_legacy_reset(dc, aspeed_otpmem_system_reset);
+ dc->realize = aspeed_otpmem_realize;
+ device_class_set_props(dc, aspeed_otpmem_properties);
+
+}
+
+static const TypeInfo aspeed_otpmem_types[] = {
+ {
+ .name = TYPE_ASPEED_OTPMEM,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(AspeedOTPMemState),
+ .class_init = aspeed_otpmem_class_init,
+ },
+};
+
+DEFINE_TYPES(aspeed_otpmem_types)
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 6d47de482c..ed1eaaa2ad 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -136,6 +136,7 @@ system_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files(
'aspeed_sbc.c',
'aspeed_sdmc.c',
'aspeed_xdma.c',
+ 'aspeed_otpmem.c',
'aspeed_peci.c',
'aspeed_sli.c'))
diff --git a/include/hw/misc/aspeed_otpmem.h b/include/hw/misc/aspeed_otpmem.h
new file mode 100644
index 0000000000..11e2de70b6
--- /dev/null
+++ b/include/hw/misc/aspeed_otpmem.h
@@ -0,0 +1,40 @@
+/*
+ * ASPEED OTP (One-Time Programmable) memory
+ *
+ * Copyright (C) 2025 Aspeed
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ */
+
+#ifndef ASPEED_OTPMMEM_H
+#define ASPEED_OTPMMEM_H
+
+#include "hw/sysbus.h"
+#include "qapi/error.h"
+
+#define TYPE_ASPEED_OTPMEM "aspeed.otpmem"
+#define ASPEED_OTPMEM_DRIVE "otpmem"
+
+#define ASPEED_OTPMEM(obj) OBJECT_CHECK(AspeedOTPMemState, (obj), \
+ TYPE_ASPEED_OTPMEM)
+
+typedef struct AspeedOTPMemOps {
+ void (*read)(void *s, uint32_t addr, uint32_t *out, Error **errp);
+ void (*prog)(void *s, uint32_t addr, uint32_t data, Error **errp);
+ void (*set_default_value)(void *s, uint32_t otp_offset,
+ uint32_t data, Error **errp);
+} AspeedOTPMemOps;
+
+typedef struct AspeedOTPMemState {
+ SysBusDevice parent_obj;
+
+ MemoryRegion mmio;
+ BlockBackend *blk;
+ int64_t max_size;
+
+ AspeedOTPMemOps *ops;
+} AspeedOTPMemState;
+
+#endif /* ASPEED_OTPMMEM_H */
+
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 2/3] hw/misc/aspeed_sbc: Connect Aspeed OTP memory device to SBC controller
2025-04-23 2:56 ` Kane Chen via
@ 2025-04-23 2:56 ` Kane Chen via
-1 siblings, 0 replies; 21+ messages in thread
From: Kane Chen via @ 2025-04-23 2:56 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Jamin Lin, Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: troy_lee, Kane-Chen-AS
From: Kane-Chen-AS <kane_chen@aspeedtech.com>
This patch integrates the `aspeed.otpmem` device with the ASPEED
Secure Boot Controller (SBC). The SBC now accepts an OTP backend via
a QOM link property ("otpmem"), enabling internal access to OTP content
for controller-specific logic.
This connection provides the foundation for future enhancements
involving fuse storage, device configuration, or secure manufacturing
data provisioning.
Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
---
hw/misc/aspeed_sbc.c | 146 +++++++++++++++++++++++++++++++++++
include/hw/misc/aspeed_sbc.h | 15 ++++
2 files changed, 161 insertions(+)
diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c
index e4a6bd1581..f0ce7bbdf0 100644
--- a/hw/misc/aspeed_sbc.c
+++ b/hw/misc/aspeed_sbc.c
@@ -17,7 +17,11 @@
#include "migration/vmstate.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 */
@@ -57,6 +61,143 @@ 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;
+ Error *local_err = NULL;
+
+ assert(s->otpmem);
+
+ 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;
+
+ s->otpmem->ops->read(s->otpmem, otp_offset, &data, &local_err);
+ if (local_err) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Failed to read data 0x%x, %s\n",
+ __func__, otp_offset,
+ error_get_pretty(local_err));
+ error_free(local_err);
+ return;
+ }
+ s->regs[R_CAMP1] = data;
+
+ if (is_data) {
+ s->otpmem->ops->read(s->otpmem, otp_offset + 4, &data, &local_err);
+ if (local_err) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Failed to read data 0x%x, %s\n",
+ __func__, otp_offset,
+ error_get_pretty(local_err));
+ error_free(local_err);
+ return;
+ }
+ s->regs[R_CAMP2] = data;
+ }
+}
+
+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;
+ Error *local_err = NULL;
+
+ assert(s->otpmem);
+
+ otp_addr = s->regs[R_ADDR];
+ value = s->regs[R_CAMP1];
+ if (otp_addr >= OTP_TOTAL_DWORD_COUNT) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Invalid OTP addr 0x%x\n",
+ __func__, otp_addr);
+ return;
+ }
+
+ s->otpmem->ops->prog(s->otpmem, otp_addr, value, &local_err);
+ if (local_err) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Failed to program data 0x%x to 0x%x, %s\n",
+ __func__, value, otp_addr,
+ error_get_pretty(local_err));
+ error_free(local_err);
+ return;
+ }
+}
+
+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 +219,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;
}
@@ -139,6 +283,8 @@ 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("otpmem", AspeedSBCState, otpmem,
+ TYPE_ASPEED_OTPMEM, AspeedOTPMemState *),
};
static void aspeed_sbc_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/misc/aspeed_sbc.h b/include/hw/misc/aspeed_sbc.h
index 405e6782b9..8ae59d977e 100644
--- a/include/hw/misc/aspeed_sbc.h
+++ b/include/hw/misc/aspeed_sbc.h
@@ -10,6 +10,7 @@
#define ASPEED_SBC_H
#include "hw/sysbus.h"
+#include "hw/misc/aspeed_otpmem.h"
#define TYPE_ASPEED_SBC "aspeed.sbc"
#define TYPE_ASPEED_AST2600_SBC TYPE_ASPEED_SBC "-ast2600"
@@ -27,6 +28,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 +49,8 @@ struct AspeedSBCState {
MemoryRegion iomem;
uint32_t regs[ASPEED_SBC_NR_REGS];
+
+ AspeedOTPMemState *otpmem;
};
struct AspeedSBCClass {
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 2/3] hw/misc/aspeed_sbc: Connect Aspeed OTP memory device to SBC controller
@ 2025-04-23 2:56 ` Kane Chen via
0 siblings, 0 replies; 21+ messages in thread
From: Kane Chen via @ 2025-04-23 2:56 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Jamin Lin, Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: troy_lee, Kane-Chen-AS
From: Kane-Chen-AS <kane_chen@aspeedtech.com>
This patch integrates the `aspeed.otpmem` device with the ASPEED
Secure Boot Controller (SBC). The SBC now accepts an OTP backend via
a QOM link property ("otpmem"), enabling internal access to OTP content
for controller-specific logic.
This connection provides the foundation for future enhancements
involving fuse storage, device configuration, or secure manufacturing
data provisioning.
Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
---
hw/misc/aspeed_sbc.c | 146 +++++++++++++++++++++++++++++++++++
include/hw/misc/aspeed_sbc.h | 15 ++++
2 files changed, 161 insertions(+)
diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c
index e4a6bd1581..f0ce7bbdf0 100644
--- a/hw/misc/aspeed_sbc.c
+++ b/hw/misc/aspeed_sbc.c
@@ -17,7 +17,11 @@
#include "migration/vmstate.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 */
@@ -57,6 +61,143 @@ 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;
+ Error *local_err = NULL;
+
+ assert(s->otpmem);
+
+ 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;
+
+ s->otpmem->ops->read(s->otpmem, otp_offset, &data, &local_err);
+ if (local_err) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Failed to read data 0x%x, %s\n",
+ __func__, otp_offset,
+ error_get_pretty(local_err));
+ error_free(local_err);
+ return;
+ }
+ s->regs[R_CAMP1] = data;
+
+ if (is_data) {
+ s->otpmem->ops->read(s->otpmem, otp_offset + 4, &data, &local_err);
+ if (local_err) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Failed to read data 0x%x, %s\n",
+ __func__, otp_offset,
+ error_get_pretty(local_err));
+ error_free(local_err);
+ return;
+ }
+ s->regs[R_CAMP2] = data;
+ }
+}
+
+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;
+ Error *local_err = NULL;
+
+ assert(s->otpmem);
+
+ otp_addr = s->regs[R_ADDR];
+ value = s->regs[R_CAMP1];
+ if (otp_addr >= OTP_TOTAL_DWORD_COUNT) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Invalid OTP addr 0x%x\n",
+ __func__, otp_addr);
+ return;
+ }
+
+ s->otpmem->ops->prog(s->otpmem, otp_addr, value, &local_err);
+ if (local_err) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Failed to program data 0x%x to 0x%x, %s\n",
+ __func__, value, otp_addr,
+ error_get_pretty(local_err));
+ error_free(local_err);
+ return;
+ }
+}
+
+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 +219,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;
}
@@ -139,6 +283,8 @@ 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("otpmem", AspeedSBCState, otpmem,
+ TYPE_ASPEED_OTPMEM, AspeedOTPMemState *),
};
static void aspeed_sbc_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/misc/aspeed_sbc.h b/include/hw/misc/aspeed_sbc.h
index 405e6782b9..8ae59d977e 100644
--- a/include/hw/misc/aspeed_sbc.h
+++ b/include/hw/misc/aspeed_sbc.h
@@ -10,6 +10,7 @@
#define ASPEED_SBC_H
#include "hw/sysbus.h"
+#include "hw/misc/aspeed_otpmem.h"
#define TYPE_ASPEED_SBC "aspeed.sbc"
#define TYPE_ASPEED_AST2600_SBC TYPE_ASPEED_SBC "-ast2600"
@@ -27,6 +28,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 +49,8 @@ struct AspeedSBCState {
MemoryRegion iomem;
uint32_t regs[ASPEED_SBC_NR_REGS];
+
+ AspeedOTPMemState *otpmem;
};
struct AspeedSBCClass {
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 3/3] hw/arm: Integrate Aspeed OTP memory into AST10x0 and AST2600 SoCs
2025-04-23 2:56 ` Kane Chen via
@ 2025-04-23 2:56 ` Kane Chen via
-1 siblings, 0 replies; 21+ messages in thread
From: Kane Chen via @ 2025-04-23 2:56 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Jamin Lin, Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: troy_lee, Kane-Chen-AS
From: Kane-Chen-AS <kane_chen@aspeedtech.com>
This patch wires up the OTP memory device (`aspeed.otpmem`) into the
AST1030 and AST2600 SoC models. The device is initialized, attached
to a backing block drive (`-drive id=otpmem`) and linked to the SBC
controller via a QOM link.
The default OTP memory image can be generated using the following
command.
```bash
for i in $(seq 1 2048); do
printf '\x00\x00\x00\x00\xff\xff\xff\xff'
done > otpmem.img
```
To load the OTP memory image into the guest, use:
```bash
./qemu-system-arm \
-drive id=otpmem,file=otpmem.img,if=none,format=raw \
...
```
Note: Do not use the -snapshot option, or OTP data writes will not
persist to the image file.
Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
---
hw/arm/aspeed_ast10x0.c | 19 +++++++++++++++++++
hw/arm/aspeed_ast2600.c | 19 +++++++++++++++++++
include/hw/arm/aspeed_soc.h | 2 ++
3 files changed, 40 insertions(+)
diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
index ec329f4991..eaa70feb9f 100644
--- a/hw/arm/aspeed_ast10x0.c
+++ b/hw/arm/aspeed_ast10x0.c
@@ -15,6 +15,7 @@
#include "system/system.h"
#include "hw/qdev-clock.h"
#include "hw/misc/unimp.h"
+#include "system/block-backend-global-state.h"
#include "hw/arm/aspeed_soc.h"
#define ASPEED_SOC_IOMEM_SIZE 0x00200000
@@ -156,6 +157,8 @@ static void aspeed_soc_ast1030_init(Object *obj)
object_initialize_child(obj, "sbc", &s->sbc, TYPE_ASPEED_SBC);
+ object_initialize_child(obj, "otpmem", &s->otpmem, TYPE_ASPEED_OTPMEM);
+
for (i = 0; i < sc->wdts_num; i++) {
snprintf(typename, sizeof(typename), "aspeed.wdt-%s", socname);
object_initialize_child(obj, "wdt[*]", &s->wdt[i], typename);
@@ -194,6 +197,7 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
Error *err = NULL;
int i;
g_autofree char *sram_name = NULL;
+ BlockBackend *blk;
if (!clock_has_source(s->sysclk)) {
error_setg(errp, "sysclk clock must be wired up by the board code");
@@ -359,6 +363,21 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
ASPEED_SMC_GET_CLASS(&s->spi[i])->flash_window_base);
}
+ /* OTP memory */
+ blk = blk_by_name(ASPEED_OTPMEM_DRIVE);
+ if (blk) {
+ blk_set_perm(blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
+ 0, &error_fatal);
+ qdev_prop_set_drive(DEVICE(&s->otpmem), "drive", blk);
+
+ if (!sysbus_realize(SYS_BUS_DEVICE(&s->otpmem), errp)) {
+ return;
+ }
+ /* Assign OTP memory to SBC */
+ object_property_set_link(OBJECT(&s->sbc), "otpmem",
+ OBJECT(&s->otpmem), &error_abort);
+ }
+
/* Secure Boot Controller */
if (!sysbus_realize(SYS_BUS_DEVICE(&s->sbc), errp)) {
return;
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 1f994ba26c..9fe3eeeb0e 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -10,6 +10,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
#include "hw/misc/unimp.h"
+#include "system/block-backend-global-state.h"
#include "hw/arm/aspeed_soc.h"
#include "qemu/module.h"
#include "qemu/error-report.h"
@@ -263,6 +264,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
object_initialize_child(obj, "sbc", &s->sbc, TYPE_ASPEED_SBC);
+ object_initialize_child(obj, "otpmem", &s->otpmem, TYPE_ASPEED_OTPMEM);
+
object_initialize_child(obj, "iomem", &s->iomem, TYPE_UNIMPLEMENTED_DEVICE);
object_initialize_child(obj, "video", &s->video, TYPE_UNIMPLEMENTED_DEVICE);
object_initialize_child(obj, "dpmcu", &s->dpmcu, TYPE_UNIMPLEMENTED_DEVICE);
@@ -293,6 +296,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
qemu_irq irq;
g_autofree char *sram_name = NULL;
+ BlockBackend *blk;
/* Default boot region (SPI memory or ROMs) */
memory_region_init(&s->spi_boot_container, OBJECT(s),
@@ -628,6 +632,21 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
sysbus_connect_irq(SYS_BUS_DEVICE(&s->i3c.devices[i]), 0, irq);
}
+ /* OTP memory */
+ blk = blk_by_name(ASPEED_OTPMEM_DRIVE);
+ if (blk) {
+ blk_set_perm(blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
+ 0, &error_fatal);
+ qdev_prop_set_drive(DEVICE(&s->otpmem), "drive", blk);
+
+ if (!sysbus_realize(SYS_BUS_DEVICE(&s->otpmem), errp)) {
+ return;
+ }
+ /* Assign OTP memory to SBC */
+ object_property_set_link(OBJECT(&s->sbc), "otpmem",
+ OBJECT(&s->otpmem), &error_abort);
+ }
+
/* Secure Boot Controller */
if (!sysbus_realize(SYS_BUS_DEVICE(&s->sbc), errp)) {
return;
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index f069d17d16..2d15c6047a 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -36,6 +36,7 @@
#include "hw/usb/hcd-ehci.h"
#include "qom/object.h"
#include "hw/misc/aspeed_lpc.h"
+#include "hw/misc/aspeed_otpmem.h"
#include "hw/misc/unimp.h"
#include "hw/misc/aspeed_peci.h"
#include "hw/fsi/aspeed_apb2opb.h"
@@ -73,6 +74,7 @@ struct AspeedSoCState {
AspeedSMCState spi[ASPEED_SPIS_NUM];
EHCISysBusState ehci[ASPEED_EHCIS_NUM];
AspeedSBCState sbc;
+ AspeedOTPMemState otpmem;
AspeedSLIState sli;
AspeedSLIState sliio;
MemoryRegion secsram;
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 3/3] hw/arm: Integrate Aspeed OTP memory into AST10x0 and AST2600 SoCs
@ 2025-04-23 2:56 ` Kane Chen via
0 siblings, 0 replies; 21+ messages in thread
From: Kane Chen via @ 2025-04-23 2:56 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Jamin Lin, Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: troy_lee, Kane-Chen-AS
From: Kane-Chen-AS <kane_chen@aspeedtech.com>
This patch wires up the OTP memory device (`aspeed.otpmem`) into the
AST1030 and AST2600 SoC models. The device is initialized, attached
to a backing block drive (`-drive id=otpmem`) and linked to the SBC
controller via a QOM link.
The default OTP memory image can be generated using the following
command.
```bash
for i in $(seq 1 2048); do
printf '\x00\x00\x00\x00\xff\xff\xff\xff'
done > otpmem.img
```
To load the OTP memory image into the guest, use:
```bash
./qemu-system-arm \
-drive id=otpmem,file=otpmem.img,if=none,format=raw \
...
```
Note: Do not use the -snapshot option, or OTP data writes will not
persist to the image file.
Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
---
hw/arm/aspeed_ast10x0.c | 19 +++++++++++++++++++
hw/arm/aspeed_ast2600.c | 19 +++++++++++++++++++
include/hw/arm/aspeed_soc.h | 2 ++
3 files changed, 40 insertions(+)
diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
index ec329f4991..eaa70feb9f 100644
--- a/hw/arm/aspeed_ast10x0.c
+++ b/hw/arm/aspeed_ast10x0.c
@@ -15,6 +15,7 @@
#include "system/system.h"
#include "hw/qdev-clock.h"
#include "hw/misc/unimp.h"
+#include "system/block-backend-global-state.h"
#include "hw/arm/aspeed_soc.h"
#define ASPEED_SOC_IOMEM_SIZE 0x00200000
@@ -156,6 +157,8 @@ static void aspeed_soc_ast1030_init(Object *obj)
object_initialize_child(obj, "sbc", &s->sbc, TYPE_ASPEED_SBC);
+ object_initialize_child(obj, "otpmem", &s->otpmem, TYPE_ASPEED_OTPMEM);
+
for (i = 0; i < sc->wdts_num; i++) {
snprintf(typename, sizeof(typename), "aspeed.wdt-%s", socname);
object_initialize_child(obj, "wdt[*]", &s->wdt[i], typename);
@@ -194,6 +197,7 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
Error *err = NULL;
int i;
g_autofree char *sram_name = NULL;
+ BlockBackend *blk;
if (!clock_has_source(s->sysclk)) {
error_setg(errp, "sysclk clock must be wired up by the board code");
@@ -359,6 +363,21 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
ASPEED_SMC_GET_CLASS(&s->spi[i])->flash_window_base);
}
+ /* OTP memory */
+ blk = blk_by_name(ASPEED_OTPMEM_DRIVE);
+ if (blk) {
+ blk_set_perm(blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
+ 0, &error_fatal);
+ qdev_prop_set_drive(DEVICE(&s->otpmem), "drive", blk);
+
+ if (!sysbus_realize(SYS_BUS_DEVICE(&s->otpmem), errp)) {
+ return;
+ }
+ /* Assign OTP memory to SBC */
+ object_property_set_link(OBJECT(&s->sbc), "otpmem",
+ OBJECT(&s->otpmem), &error_abort);
+ }
+
/* Secure Boot Controller */
if (!sysbus_realize(SYS_BUS_DEVICE(&s->sbc), errp)) {
return;
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 1f994ba26c..9fe3eeeb0e 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -10,6 +10,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
#include "hw/misc/unimp.h"
+#include "system/block-backend-global-state.h"
#include "hw/arm/aspeed_soc.h"
#include "qemu/module.h"
#include "qemu/error-report.h"
@@ -263,6 +264,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
object_initialize_child(obj, "sbc", &s->sbc, TYPE_ASPEED_SBC);
+ object_initialize_child(obj, "otpmem", &s->otpmem, TYPE_ASPEED_OTPMEM);
+
object_initialize_child(obj, "iomem", &s->iomem, TYPE_UNIMPLEMENTED_DEVICE);
object_initialize_child(obj, "video", &s->video, TYPE_UNIMPLEMENTED_DEVICE);
object_initialize_child(obj, "dpmcu", &s->dpmcu, TYPE_UNIMPLEMENTED_DEVICE);
@@ -293,6 +296,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
qemu_irq irq;
g_autofree char *sram_name = NULL;
+ BlockBackend *blk;
/* Default boot region (SPI memory or ROMs) */
memory_region_init(&s->spi_boot_container, OBJECT(s),
@@ -628,6 +632,21 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
sysbus_connect_irq(SYS_BUS_DEVICE(&s->i3c.devices[i]), 0, irq);
}
+ /* OTP memory */
+ blk = blk_by_name(ASPEED_OTPMEM_DRIVE);
+ if (blk) {
+ blk_set_perm(blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
+ 0, &error_fatal);
+ qdev_prop_set_drive(DEVICE(&s->otpmem), "drive", blk);
+
+ if (!sysbus_realize(SYS_BUS_DEVICE(&s->otpmem), errp)) {
+ return;
+ }
+ /* Assign OTP memory to SBC */
+ object_property_set_link(OBJECT(&s->sbc), "otpmem",
+ OBJECT(&s->otpmem), &error_abort);
+ }
+
/* Secure Boot Controller */
if (!sysbus_realize(SYS_BUS_DEVICE(&s->sbc), errp)) {
return;
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index f069d17d16..2d15c6047a 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -36,6 +36,7 @@
#include "hw/usb/hcd-ehci.h"
#include "qom/object.h"
#include "hw/misc/aspeed_lpc.h"
+#include "hw/misc/aspeed_otpmem.h"
#include "hw/misc/unimp.h"
#include "hw/misc/aspeed_peci.h"
#include "hw/fsi/aspeed_apb2opb.h"
@@ -73,6 +74,7 @@ struct AspeedSoCState {
AspeedSMCState spi[ASPEED_SPIS_NUM];
EHCISysBusState ehci[ASPEED_EHCIS_NUM];
AspeedSBCState sbc;
+ AspeedOTPMemState otpmem;
AspeedSLIState sli;
AspeedSLIState sliio;
MemoryRegion secsram;
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v3 1/3] hw/misc/aspeed_otp: Add Aspeed OTP memory device model
2025-04-23 2:56 ` Kane Chen via
(?)
@ 2025-04-28 7:06 ` Cédric Le Goater
2025-04-28 10:24 ` Kane Chen
-1 siblings, 1 reply; 21+ messages in thread
From: Cédric Le Goater @ 2025-04-28 7:06 UTC (permalink / raw)
To: Kane Chen, Peter Maydell, Steven Lee, Troy Lee, Jamin Lin,
Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: troy_lee
On 4/23/25 04:56, Kane Chen wrote:
> From: Kane-Chen-AS <kane_chen@aspeedtech.com>
>
> This introduces a new model for the ASPEED OTP (One-Time Programmable)
> memory. The device is implemented as a `SysBusDevice` and provides an
> abstracted interface for OTP read, write (program), and default value
> initialization.
>
> OTP content is backed by a block device and supports QEMU’s drive
> infrastructure via the "drive" property.
>
> Features:
> - Enforces irreversible bit programming logic (0->1 or 1->0)
> - Provides interface for SoC/secure controller integration
> - Validates bounds and bit-level constraints
> - Uses QEMU error handling conventions and logging
>
> Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> ---
> hw/misc/aspeed_otpmem.c | 211 ++++++++++++++++++++++++++++++++
> hw/misc/meson.build | 1 +
> include/hw/misc/aspeed_otpmem.h | 40 ++++++
> 3 files changed, 252 insertions(+)
> create mode 100644 hw/misc/aspeed_otpmem.c
> create mode 100644 include/hw/misc/aspeed_otpmem.h
>
> diff --git a/hw/misc/aspeed_otpmem.c b/hw/misc/aspeed_otpmem.c
> new file mode 100644
> index 0000000000..4f8f2827f7
> --- /dev/null
> +++ b/hw/misc/aspeed_otpmem.c
> @@ -0,0 +1,211 @@
> +/*
> + * ASPEED OTP (One-Time Programmable) memory
> + *
> + * Copyright (C) 2025 Aspeed
> + *
> + * This code is licensed under the GPL version 2 or later. See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/block/block.h"
> +#include "hw/block/flash.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/qdev-properties-system.h"
> +#include "system/block-backend.h"
> +#include "qemu/log.h"
> +#include "qemu/option.h"
> +#include "hw/sysbus.h"
> +#include "qemu/error-report.h"
> +#include "hw/misc/aspeed_otpmem.h"
> +
> +static const Property aspeed_otpmem_properties[] = {
> + DEFINE_PROP_DRIVE("drive", AspeedOTPMemState, blk),
> +};
Usually the 'Property' array is defined just before the class_init
routine. Please move it there.
> +static void aspeed_otpmem_read(void *opaque, uint32_t addr,
> + uint32_t *out, Error **errp)
hmm, that's not a MemoryRegion handler. Why not ? I will check in the
following patches
> +{
> + AspeedOTPMemState *otp = ASPEED_OTPMEM(opaque);
> +
> + assert(otp->blk);
This check shouldn't be needed if the backend id initialized in the
realize routine.
> + if (out == NULL) {
> + error_setg(errp, "out is NULL");
> + return;
> + }
> +
> + if (addr > (otp->max_size - 4)) {
Why a MemoryRegion, no need for this check.
> + error_setg(errp, "OTP memory 0x%x is exceeded", addr);
> + return;
> + }
> +
> + if (blk_pread(otp->blk, (int64_t)addr, sizeof(uint32_t), out, 0) < 0) {
> + error_setg(errp, "Failed to read data 0x%x", addr);
> + return;
> + }
> + return;
> +}
> +
> +static bool valid_program_data(uint32_t otp_addr,
> + uint32_t value, uint32_t prog_bit)
> +{
> + uint32_t programmed_bits, has_programmable_bits;
> + bool is_odd = otp_addr & 1;
> +
> + /*
> + * prog_bit uses 0s to indicate target bits to program:
> + * - if OTP word is even-indexed, programmed bits flip 0->1
> + * - if odd, bits flip 1->0
> + * Bit programming is one-way only and irreversible.
> + */
> + if (is_odd) {
> + programmed_bits = ~value & prog_bit;
> + } else {
> + programmed_bits = value & (~prog_bit);
> + }
> +
> + /* If there is some bit can be programed, to accept the request */
> + has_programmable_bits = value ^ (~prog_bit);
> +
> + if (programmed_bits) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Found programmed bits in addr %x\n",
> + __func__, otp_addr);
> + for (int i = 0; i < 32; ++i) {
> + if (programmed_bits & (1U << i)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + " Programmed bit %d\n",
> + i);
> + }
> + }
> + }
> +
> + return has_programmable_bits != 0;
> +}
> +
> +static bool program_otpmem_data(void *opaque, uint32_t otp_addr,
> + uint32_t prog_bit, uint32_t *value)
> +{
> + AspeedOTPMemState *s = ASPEED_OTPMEM(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 (!valid_program_data(otp_addr, *value, prog_bit)) {
> + return false;
> + }
> +
> + if (is_odd) {
> + *value &= ~prog_bit;
> + } else {
> + *value |= ~prog_bit;
> + }
> +
> + return true;
> +}
> +
> +static void aspeed_otpmem_prog(void *s, uint32_t otp_addr,
> + uint32_t data, Error **errp)
> +{
> + AspeedOTPMemState *otp = ASPEED_OTPMEM(s);
> + uint32_t otp_offset, value;
> +
> + assert(otp->blk);
> +
> + if (otp_addr > (otp->max_size >> 2)) {
> + error_setg(errp, "OTP memory 0x%x is exceeded", otp_addr);
> + return;
> + }
> +
> + otp_offset = otp_addr << 2;
> + if (!program_otpmem_data(s, otp_addr, data, &value)) {
> + error_setg(errp, "Failed to program data");
> + return;
> + }
> +
> + if (blk_pwrite(otp->blk, (int64_t)otp_offset,
> + sizeof(value), &value, 0) < 0) {
> + error_setg(errp, "Failed to write data");
> + }
> +
> + return;
> +}
> +
> +static void aspeed_otpmem_set_default(void *s, uint32_t otp_offset,
> + uint32_t data, Error **errp)
> +{
> + AspeedOTPMemState *otp = ASPEED_OTPMEM(s);
> +
> + if ((otp_offset + 4) > otp->max_size) {
> + error_setg(errp, "OTP memory 0x%x is exceeded", otp_offset);
> + return;
> + }
> +
> + if (blk_pwrite(otp->blk, (int64_t)otp_offset,
> + sizeof(data), &data, 0) < 0) {
> + error_setg(errp, "Failed to write data");
> + }
> + return;
> +}
> +
> +static AspeedOTPMemOps aspeed_otpmem_ops = {
> + .read = aspeed_otpmem_read,
> + .prog = aspeed_otpmem_prog,
> + .set_default_value = aspeed_otpmem_set_default
> +};
> +
> +static void aspeed_otpmem_realize(DeviceState *dev, Error **errp)
> +{
> + AspeedOTPMemState *s = ASPEED_OTPMEM(dev);
> +
> + if (!s->blk) {
> + error_setg(&error_fatal, "OTP memory is not initialized");
> + return;
> + }
> +
> + s->max_size = blk_getlength(s->blk);
> + if (s->max_size < 0 || (s->max_size % 4)) {
> + error_setg(&error_fatal,
> + "Unexpected OTP memory size: %" PRId64 "",
> + s->max_size);
> + return;
> + }
> +
> + s->ops = &aspeed_otpmem_ops;
You should consider using an AddressSpace for the OTP transactions.
> + return;
> +}
> +
> +static void aspeed_otpmem_system_reset(DeviceState *dev)
> +{
> + return;
> +}
> +
Reset is empty. Please remove.
> +static void aspeed_otpmem_class_init(ObjectClass *klass, void *data)> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + device_class_set_legacy_reset(dc, aspeed_otpmem_system_reset);
> + dc->realize = aspeed_otpmem_realize;
> + device_class_set_props(dc, aspeed_otpmem_properties);
> +
> +}
> +
> +static const TypeInfo aspeed_otpmem_types[] = {
> + {
> + .name = TYPE_ASPEED_OTPMEM,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(AspeedOTPMemState),
> + .class_init = aspeed_otpmem_class_init,
> + },
> +};
> +
> +DEFINE_TYPES(aspeed_otpmem_types)
> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index 6d47de482c..ed1eaaa2ad 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -136,6 +136,7 @@ system_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files(
> 'aspeed_sbc.c',
> 'aspeed_sdmc.c',
> 'aspeed_xdma.c',
> + 'aspeed_otpmem.c',
> 'aspeed_peci.c',
> 'aspeed_sli.c'))
>
> diff --git a/include/hw/misc/aspeed_otpmem.h b/include/hw/misc/aspeed_otpmem.h
> new file mode 100644
> index 0000000000..11e2de70b6
> --- /dev/null
> +++ b/include/hw/misc/aspeed_otpmem.h
> @@ -0,0 +1,40 @@
> +/*
> + * ASPEED OTP (One-Time Programmable) memory
> + *
> + * Copyright (C) 2025 Aspeed
> + *
> + * This code is licensed under the GPL version 2 or later. See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#ifndef ASPEED_OTPMMEM_H
> +#define ASPEED_OTPMMEM_H
> +
> +#include "hw/sysbus.h"
> +#include "qapi/error.h"
> +
> +#define TYPE_ASPEED_OTPMEM "aspeed.otpmem"
> +#define ASPEED_OTPMEM_DRIVE "otpmem"
This ASPEED_OTPMEM_DRIVE definition looks wrong to me. What is it for ?
Thanks,
C.
> +
> +#define ASPEED_OTPMEM(obj) OBJECT_CHECK(AspeedOTPMemState, (obj), \
> + TYPE_ASPEED_OTPMEM)
> +
> +typedef struct AspeedOTPMemOps {
> + void (*read)(void *s, uint32_t addr, uint32_t *out, Error **errp);
> + void (*prog)(void *s, uint32_t addr, uint32_t data, Error **errp);
> + void (*set_default_value)(void *s, uint32_t otp_offset,
> + uint32_t data, Error **errp);
> +} AspeedOTPMemOps;
> +
> +typedef struct AspeedOTPMemState {
> + SysBusDevice parent_obj;
> +
> + MemoryRegion mmio;
> + BlockBackend *blk;
> + int64_t max_size;
> +
> + AspeedOTPMemOps *ops;
> +} AspeedOTPMemState;
> +
> +#endif /* ASPEED_OTPMMEM_H */
> +
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 2/3] hw/misc/aspeed_sbc: Connect Aspeed OTP memory device to SBC controller
2025-04-23 2:56 ` Kane Chen via
(?)
@ 2025-04-28 7:20 ` Cédric Le Goater
2025-04-28 10:25 ` Kane Chen
-1 siblings, 1 reply; 21+ messages in thread
From: Cédric Le Goater @ 2025-04-28 7:20 UTC (permalink / raw)
To: Kane Chen, Peter Maydell, Steven Lee, Troy Lee, Jamin Lin,
Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: troy_lee
On 4/23/25 04:56, Kane Chen wrote:
> From: Kane-Chen-AS <kane_chen@aspeedtech.com>
>
> This patch integrates the `aspeed.otpmem` device with the ASPEED
> Secure Boot Controller (SBC). The SBC now accepts an OTP backend via
> a QOM link property ("otpmem"), enabling internal access to OTP content
> for controller-specific logic.
>
> This connection provides the foundation for future enhancements
> involving fuse storage, device configuration, or secure manufacturing
> data provisioning.
>
> Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> ---
> hw/misc/aspeed_sbc.c | 146 +++++++++++++++++++++++++++++++++++
> include/hw/misc/aspeed_sbc.h | 15 ++++
> 2 files changed, 161 insertions(+)
>
> diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c
> index e4a6bd1581..f0ce7bbdf0 100644
> --- a/hw/misc/aspeed_sbc.c
> +++ b/hw/misc/aspeed_sbc.c
> @@ -17,7 +17,11 @@
> #include "migration/vmstate.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 */
> @@ -57,6 +61,143 @@ 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)
You could improve this prototype. How about :
bool aspeed_sbc_otpmem_read(AspeedSBCState *s, uint32_t otp_addr, Error *errp)
same below.
> +{
> + AspeedSBCState *s = ASPEED_SBC(opaque);
> + uint32_t otp_addr, data, otp_offset;
> + bool is_data = false;
> + Error *local_err = NULL;
> +
> + assert(s->otpmem);
> +
> + 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;
> +
> + s->otpmem->ops->read(s->otpmem, otp_offset, &data, &local_err);
> + if (local_err) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Failed to read data 0x%x, %s\n",
> + __func__, otp_offset,
> + error_get_pretty(local_err));
> + error_free(local_err);
> + return;> + }
Please use an AddressSpace. See aspeed_smc for an example.
> + s->regs[R_CAMP1] = data;
> +
> + if (is_data) {
> + s->otpmem->ops->read(s->otpmem, otp_offset + 4, &data, &local_err);
> + if (local_err) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Failed to read data 0x%x, %s\n",
> + __func__, otp_offset,
> + error_get_pretty(local_err));
> + error_free(local_err);
> + return;
> + }
> + s->regs[R_CAMP2] = data;
> + }
> +}
> +
> +static void mr_handler(uint32_t otp_addr, uint32_t data)
data is unused
> +{
> + 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,
alignment issue.
> + "%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;
> + Error *local_err = NULL;
> +
> + assert(s->otpmem);
> +
> + otp_addr = s->regs[R_ADDR];
> + value = s->regs[R_CAMP1];
> + if (otp_addr >= OTP_TOTAL_DWORD_COUNT) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Invalid OTP addr 0x%x\n",
> + __func__, otp_addr);
> + return;
> + }
> +
> + s->otpmem->ops->prog(s->otpmem, otp_addr, value, &local_err);
> + if (local_err) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Failed to program data 0x%x to 0x%x, %s\n",
> + __func__, value, otp_addr,
> + error_get_pretty(local_err));
> + error_free(local_err);
> + return;
> + }
> +}
> +
> +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 +219,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;
> }
> @@ -139,6 +283,8 @@ 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("otpmem", AspeedSBCState, otpmem,
> + TYPE_ASPEED_OTPMEM, AspeedOTPMemState *),
hmm, no.
Instead AspeedOTPMemState should be a child object of AspeedSBCState,
only created and realized for ast2600/1030 Soc. This means you will
probably need a class attribute in AspeedSBCClass.
> };
>
> static void aspeed_sbc_class_init(ObjectClass *klass, void *data)
> diff --git a/include/hw/misc/aspeed_sbc.h b/include/hw/misc/aspeed_sbc.h
> index 405e6782b9..8ae59d977e 100644
> --- a/include/hw/misc/aspeed_sbc.h
> +++ b/include/hw/misc/aspeed_sbc.h
> @@ -10,6 +10,7 @@
> #define ASPEED_SBC_H
>
> #include "hw/sysbus.h"
> +#include "hw/misc/aspeed_otpmem.h"
>
> #define TYPE_ASPEED_SBC "aspeed.sbc"
> #define TYPE_ASPEED_AST2600_SBC TYPE_ASPEED_SBC "-ast2600"
> @@ -27,6 +28,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)
> +
These define belong to the implementation : aspeed_sbc.c.
Thanks,
C.
> struct AspeedSBCState {
> SysBusDevice parent;
>
> @@ -36,6 +49,8 @@ struct AspeedSBCState {
> MemoryRegion iomem;
>
> uint32_t regs[ASPEED_SBC_NR_REGS];
> +
> + AspeedOTPMemState *otpmem;
> };
>
> struct AspeedSBCClass {
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 3/3] hw/arm: Integrate Aspeed OTP memory into AST10x0 and AST2600 SoCs
2025-04-23 2:56 ` Kane Chen via
(?)
@ 2025-04-28 7:41 ` Cédric Le Goater
2025-04-28 10:26 ` Kane Chen
-1 siblings, 1 reply; 21+ messages in thread
From: Cédric Le Goater @ 2025-04-28 7:41 UTC (permalink / raw)
To: Kane Chen, Peter Maydell, Steven Lee, Troy Lee, Jamin Lin,
Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: troy_lee
On 4/23/25 04:56, Kane Chen wrote:
> From: Kane-Chen-AS <kane_chen@aspeedtech.com>
>
> This patch wires up the OTP memory device (`aspeed.otpmem`) into the
> AST1030 and AST2600 SoC models. The device is initialized, attached
> to a backing block drive (`-drive id=otpmem`) and linked to the SBC
> controller via a QOM link.
>
> The default OTP memory image can be generated using the following
> command.
> ```bash
> for i in $(seq 1 2048); do
> printf '\x00\x00\x00\x00\xff\xff\xff\xff'
> done > otpmem.img
> ```
>
> To load the OTP memory image into the guest, use:
> ```bash
> ./qemu-system-arm \
> -drive id=otpmem,file=otpmem.img,if=none,format=raw \
> ...
> ```
I thought we were going to implement the same method of the edk2 flash
devices of the q35 machine. Setting a machine option would set the drive :
qemu-system-arm -M ast2600-evb,otpmem=otpmem-drive \
-blockdev node-name=otpmem,driver=file,filename=/path/to/otpmem.img \
...
Which is not what is proposed below.
> Note: Do not use the -snapshot option, or OTP data writes will not
> persist to the image file.
>
> Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> ---
> hw/arm/aspeed_ast10x0.c | 19 +++++++++++++++++++
> hw/arm/aspeed_ast2600.c | 19 +++++++++++++++++++
> include/hw/arm/aspeed_soc.h | 2 ++
> 3 files changed, 40 insertions(+)
>
> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
> index ec329f4991..eaa70feb9f 100644
> --- a/hw/arm/aspeed_ast10x0.c
> +++ b/hw/arm/aspeed_ast10x0.c
> @@ -15,6 +15,7 @@
> #include "system/system.h"
> #include "hw/qdev-clock.h"
> #include "hw/misc/unimp.h"
> +#include "system/block-backend-global-state.h"
> #include "hw/arm/aspeed_soc.h"
>
> #define ASPEED_SOC_IOMEM_SIZE 0x00200000
> @@ -156,6 +157,8 @@ static void aspeed_soc_ast1030_init(Object *obj)
>
> object_initialize_child(obj, "sbc", &s->sbc, TYPE_ASPEED_SBC);
>
> + object_initialize_child(obj, "otpmem", &s->otpmem, TYPE_ASPEED_OTPMEM);
> +
This belongs to AspeedSBC. See below.
> for (i = 0; i < sc->wdts_num; i++) {
> snprintf(typename, sizeof(typename), "aspeed.wdt-%s", socname);
> object_initialize_child(obj, "wdt[*]", &s->wdt[i], typename);
> @@ -194,6 +197,7 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
> Error *err = NULL;
> int i;
> g_autofree char *sram_name = NULL;
> + BlockBackend *blk;
>
> if (!clock_has_source(s->sysclk)) {
> error_setg(errp, "sysclk clock must be wired up by the board code");
> @@ -359,6 +363,21 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
> ASPEED_SMC_GET_CLASS(&s->spi[i])->flash_window_base);
> }
>
> + /* OTP memory */
> + blk = blk_by_name(ASPEED_OTPMEM_DRIVE);
> + if (blk) {
> + blk_set_perm(blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
> + 0, &error_fatal);
> + qdev_prop_set_drive(DEVICE(&s->otpmem), "drive", blk);
> +
> + if (!sysbus_realize(SYS_BUS_DEVICE(&s->otpmem), errp)) {
> + return;
> + }
> + /* Assign OTP memory to SBC */
> + object_property_set_link(OBJECT(&s->sbc), "otpmem",
> + OBJECT(&s->otpmem), &error_abort);
> + }
> +
The "optmem" machine option should be pointing to "drive" option of
the AspeedOTPMemState object in this object hierarchy :
/machine (ast2600-evb-machine)
/soc (ast2600-a3)
/sbc (aspeed.sbc)
/aspeed.sbc[0] (memory-region)
/optmem (aspeed.otpmem) <- move otpmem there
This will require using object_property_add_alias() in 2 or 3 levels.
object_property_add_alias(OBJECT(parent), "optmem"
OBJECT(child), "drive", &error_abort)
Please try that instead and let's see the result.
Thanks,
C.
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH v3 1/3] hw/misc/aspeed_otp: Add Aspeed OTP memory device model
2025-04-28 7:06 ` Cédric Le Goater
@ 2025-04-28 10:24 ` Kane Chen
0 siblings, 0 replies; 21+ messages in thread
From: Kane Chen @ 2025-04-28 10:24 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Jamin Lin, Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: Troy Lee
Hi Cédric,
> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Monday, April 28, 2025 3:06 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 v3 1/3] hw/misc/aspeed_otp: Add Aspeed OTP memory
> device model
>
> On 4/23/25 04:56, Kane Chen wrote:
> > From: Kane-Chen-AS <kane_chen@aspeedtech.com>
> >
> > This introduces a new model for the ASPEED OTP (One-Time Programmable)
> > memory. The device is implemented as a `SysBusDevice` and provides an
> > abstracted interface for OTP read, write (program), and default value
> > initialization.
> >
> > OTP content is backed by a block device and supports QEMU’s drive
> > infrastructure via the "drive" property.
> >
> > Features:
> > - Enforces irreversible bit programming logic (0->1 or 1->0)
> > - Provides interface for SoC/secure controller integration
> > - Validates bounds and bit-level constraints
> > - Uses QEMU error handling conventions and logging
> >
> > Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> > ---
> > hw/misc/aspeed_otpmem.c | 211
> ++++++++++++++++++++++++++++++++
> > hw/misc/meson.build | 1 +
> > include/hw/misc/aspeed_otpmem.h | 40 ++++++
> > 3 files changed, 252 insertions(+)
> > create mode 100644 hw/misc/aspeed_otpmem.c
> > create mode 100644 include/hw/misc/aspeed_otpmem.h
> >
> > diff --git a/hw/misc/aspeed_otpmem.c b/hw/misc/aspeed_otpmem.c new
> > file mode 100644 index 0000000000..4f8f2827f7
> > --- /dev/null
> > +++ b/hw/misc/aspeed_otpmem.c
> > @@ -0,0 +1,211 @@
> > +/*
> > + * ASPEED OTP (One-Time Programmable) memory
> > + *
> > + * Copyright (C) 2025 Aspeed
> > + *
> > + * This code is licensed under the GPL version 2 or later. See
> > + * the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "hw/block/block.h"
> > +#include "hw/block/flash.h"
> > +#include "hw/qdev-properties.h"
> > +#include "hw/qdev-properties-system.h"
> > +#include "system/block-backend.h"
> > +#include "qemu/log.h"
> > +#include "qemu/option.h"
> > +#include "hw/sysbus.h"
> > +#include "qemu/error-report.h"
> > +#include "hw/misc/aspeed_otpmem.h"
> > +
> > +static const Property aspeed_otpmem_properties[] = {
> > + DEFINE_PROP_DRIVE("drive", AspeedOTPMemState, blk), };
>
> Usually the 'Property' array is defined just before the class_init routine. Please
> move it there.
>
I will move the Property array to just before the class_init routine
in the next version of the patch.
> > +static void aspeed_otpmem_read(void *opaque, uint32_t addr,
> > + uint32_t *out, Error **errp)
>
> hmm, that's not a MemoryRegion handler. Why not ? I will check in the
> following patches
>
In the AST1030 and AST2600, the OTP memory is accessed through the SBC
(Secure Boot Controller) and cannot be accessed directly. Therefore,
I did not implement a MemoryRegion handler. If you have any concerns
regarding this design, please let me know.
> > +{
> > + AspeedOTPMemState *otp = ASPEED_OTPMEM(opaque);
> > +
> > + assert(otp->blk);
>
> This check shouldn't be needed if the backend id initialized in the realize
> routine.
>
> > + if (out == NULL) {
> > + error_setg(errp, "out is NULL");
> > + return;
> > + }
> > +
> > + if (addr > (otp->max_size - 4)) {
>
> Why a MemoryRegion, no need for this check.
I will remove the assert and the address boundary check code segments
you pointed out in the next version of the patch.
>
> > + error_setg(errp, "OTP memory 0x%x is exceeded", addr);
> > + return;
> > + }
> > +
> > + if (blk_pread(otp->blk, (int64_t)addr, sizeof(uint32_t), out, 0) < 0) {
> > + error_setg(errp, "Failed to read data 0x%x", addr);
> > + return;
> > + }
> > + return;
> > +}
> > +
> > +static bool valid_program_data(uint32_t otp_addr,
> > + uint32_t value, uint32_t prog_bit) {
> > + uint32_t programmed_bits, has_programmable_bits;
> > + bool is_odd = otp_addr & 1;
> > +
> > + /*
> > + * prog_bit uses 0s to indicate target bits to program:
> > + * - if OTP word is even-indexed, programmed bits flip 0->1
> > + * - if odd, bits flip 1->0
> > + * Bit programming is one-way only and irreversible.
> > + */
> > + if (is_odd) {
> > + programmed_bits = ~value & prog_bit;
> > + } else {
> > + programmed_bits = value & (~prog_bit);
> > + }
> > +
> > + /* If there is some bit can be programed, to accept the request */
> > + has_programmable_bits = value ^ (~prog_bit);
> > +
> > + if (programmed_bits) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: Found programmed bits in addr %x\n",
> > + __func__, otp_addr);
> > + for (int i = 0; i < 32; ++i) {
> > + if (programmed_bits & (1U << i)) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + " Programmed bit %d\n",
> > + i);
> > + }
> > + }
> > + }
> > +
> > + return has_programmable_bits != 0; }
> > +
> > +static bool program_otpmem_data(void *opaque, uint32_t otp_addr,
> > + uint32_t prog_bit, uint32_t *value) {
> > + AspeedOTPMemState *s = ASPEED_OTPMEM(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 (!valid_program_data(otp_addr, *value, prog_bit)) {
> > + return false;
> > + }
> > +
> > + if (is_odd) {
> > + *value &= ~prog_bit;
> > + } else {
> > + *value |= ~prog_bit;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +static void aspeed_otpmem_prog(void *s, uint32_t otp_addr,
> > + uint32_t data, Error **errp) {
> > + AspeedOTPMemState *otp = ASPEED_OTPMEM(s);
> > + uint32_t otp_offset, value;
> > +
> > + assert(otp->blk);
> > +
> > + if (otp_addr > (otp->max_size >> 2)) {
> > + error_setg(errp, "OTP memory 0x%x is exceeded", otp_addr);
> > + return;
> > + }
> > +
> > + otp_offset = otp_addr << 2;
> > + if (!program_otpmem_data(s, otp_addr, data, &value)) {
> > + error_setg(errp, "Failed to program data");
> > + return;
> > + }
> > +
> > + if (blk_pwrite(otp->blk, (int64_t)otp_offset,
> > + sizeof(value), &value, 0) < 0) {
> > + error_setg(errp, "Failed to write data");
> > + }
> > +
> > + return;
> > +}
> > +
> > +static void aspeed_otpmem_set_default(void *s, uint32_t otp_offset,
> > + uint32_t data, Error **errp)
> {
> > + AspeedOTPMemState *otp = ASPEED_OTPMEM(s);
> > +
> > + if ((otp_offset + 4) > otp->max_size) {
> > + error_setg(errp, "OTP memory 0x%x is exceeded", otp_offset);
> > + return;
> > + }
> > +
> > + if (blk_pwrite(otp->blk, (int64_t)otp_offset,
> > + sizeof(data), &data, 0) < 0) {
> > + error_setg(errp, "Failed to write data");
> > + }
> > + return;
> > +}
> > +
> > +static AspeedOTPMemOps aspeed_otpmem_ops = {
> > + .read = aspeed_otpmem_read,
> > + .prog = aspeed_otpmem_prog,
> > + .set_default_value = aspeed_otpmem_set_default };
> > +
> > +static void aspeed_otpmem_realize(DeviceState *dev, Error **errp) {
> > + AspeedOTPMemState *s = ASPEED_OTPMEM(dev);
> > +
> > + if (!s->blk) {
> > + error_setg(&error_fatal, "OTP memory is not initialized");
> > + return;
> > + }
> > +
> > + s->max_size = blk_getlength(s->blk);
> > + if (s->max_size < 0 || (s->max_size % 4)) {
> > + error_setg(&error_fatal,
> > + "Unexpected OTP memory size: %" PRId64 "",
> > + s->max_size);
> > + return;
> > + }
> > +
> > + s->ops = &aspeed_otpmem_ops;
>
> You should consider using an AddressSpace for the OTP transactions.
>
Regarding the use of ops function pointers: the OTP memory behaves
similarly to an efuse device (like xlnx-efuse.c), but with different
read/write rules. Additionally, the access behaviors differ across
different ASPEED SoCs (e.g., AST2700 vs AST2600/AST1030), so I chose
a flexible ops-based design for potential future extension. I will
also evaluate whether using an AddressSpace would fit the requirements
better.
> > + return;
> > +}
> > +
> > +static void aspeed_otpmem_system_reset(DeviceState *dev) {
> > + return;
> > +}
> > +
>
> Reset is empty. Please remove.
I will remove the empty reset function in the next patch as suggested.
>
>
> > +static void aspeed_otpmem_class_init(ObjectClass *klass, void *data)> +{
> > + DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > + device_class_set_legacy_reset(dc, aspeed_otpmem_system_reset);
> > + dc->realize = aspeed_otpmem_realize;
> > + device_class_set_props(dc, aspeed_otpmem_properties);
> > +
> > +}
> > +
> > +static const TypeInfo aspeed_otpmem_types[] = {
> > + {
> > + .name = TYPE_ASPEED_OTPMEM,
> > + .parent = TYPE_SYS_BUS_DEVICE,
> > + .instance_size = sizeof(AspeedOTPMemState),
> > + .class_init = aspeed_otpmem_class_init,
> > + },
> > +};
> > +
> > +DEFINE_TYPES(aspeed_otpmem_types)
> > diff --git a/hw/misc/meson.build b/hw/misc/meson.build index
> > 6d47de482c..ed1eaaa2ad 100644
> > --- a/hw/misc/meson.build
> > +++ b/hw/misc/meson.build
> > @@ -136,6 +136,7 @@ system_ss.add(when: 'CONFIG_ASPEED_SOC', if_true:
> files(
> > 'aspeed_sbc.c',
> > 'aspeed_sdmc.c',
> > 'aspeed_xdma.c',
> > + 'aspeed_otpmem.c',
> > 'aspeed_peci.c',
> > 'aspeed_sli.c'))
> >
> > diff --git a/include/hw/misc/aspeed_otpmem.h
> > b/include/hw/misc/aspeed_otpmem.h new file mode 100644 index
> > 0000000000..11e2de70b6
> > --- /dev/null
> > +++ b/include/hw/misc/aspeed_otpmem.h
> > @@ -0,0 +1,40 @@
> > +/*
> > + * ASPEED OTP (One-Time Programmable) memory
> > + *
> > + * Copyright (C) 2025 Aspeed
> > + *
> > + * This code is licensed under the GPL version 2 or later. See
> > + * the COPYING file in the top-level directory.
> > + */
> > +
> > +#ifndef ASPEED_OTPMMEM_H
> > +#define ASPEED_OTPMMEM_H
> > +
> > +#include "hw/sysbus.h"
> > +#include "qapi/error.h"
> > +
> > +#define TYPE_ASPEED_OTPMEM "aspeed.otpmem"
> > +#define ASPEED_OTPMEM_DRIVE "otpmem"
>
> This ASPEED_OTPMEM_DRIVE definition looks wrong to me. What is it for ?
>
The ASPEED_OTPMEM_DRIVE macro is actually used in a later patch
(patch 3: "hw/arm: Integrate Aspeed OTP memory into AST10x0 and AST2600 SoCs").
I will move this macro definition to the appropriate patch file.
> Thanks,
>
> C.
>
>
> > +
> > +#define ASPEED_OTPMEM(obj) OBJECT_CHECK(AspeedOTPMemState, (obj),
> \
> > + TYPE_ASPEED_OTPMEM)
> > +
> > +typedef struct AspeedOTPMemOps {
> > + void (*read)(void *s, uint32_t addr, uint32_t *out, Error **errp);
> > + void (*prog)(void *s, uint32_t addr, uint32_t data, Error **errp);
> > + void (*set_default_value)(void *s, uint32_t otp_offset,
> > + uint32_t data, Error **errp); }
> > +AspeedOTPMemOps;
> > +
> > +typedef struct AspeedOTPMemState {
> > + SysBusDevice parent_obj;
> > +
> > + MemoryRegion mmio;
> > + BlockBackend *blk;
> > + int64_t max_size;
> > +
> > + AspeedOTPMemOps *ops;
> > +} AspeedOTPMemState;
> > +
> > +#endif /* ASPEED_OTPMMEM_H */
> > +
Thanks again for your comments and review.
Best Regards,
Kane
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH v3 2/3] hw/misc/aspeed_sbc: Connect Aspeed OTP memory device to SBC controller
2025-04-28 7:20 ` Cédric Le Goater
@ 2025-04-28 10:25 ` Kane Chen
2025-05-08 9:46 ` Kane Chen
0 siblings, 1 reply; 21+ messages in thread
From: Kane Chen @ 2025-04-28 10:25 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Jamin Lin, Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: Troy Lee
Hi Cédric,
> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Monday, April 28, 2025 3:21 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 v3 2/3] hw/misc/aspeed_sbc: Connect Aspeed OTP
> memory device to SBC controller
>
> On 4/23/25 04:56, Kane Chen wrote:
> > From: Kane-Chen-AS <kane_chen@aspeedtech.com>
> >
> > This patch integrates the `aspeed.otpmem` device with the ASPEED
> > Secure Boot Controller (SBC). The SBC now accepts an OTP backend via a
> > QOM link property ("otpmem"), enabling internal access to OTP content
> > for controller-specific logic.
> >
> > This connection provides the foundation for future enhancements
> > involving fuse storage, device configuration, or secure manufacturing
> > data provisioning.
> >
> > Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> > ---
> > hw/misc/aspeed_sbc.c | 146
> +++++++++++++++++++++++++++++++++++
> > include/hw/misc/aspeed_sbc.h | 15 ++++
> > 2 files changed, 161 insertions(+)
> >
> > diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c index
> > e4a6bd1581..f0ce7bbdf0 100644
> > --- a/hw/misc/aspeed_sbc.c
> > +++ b/hw/misc/aspeed_sbc.c
> > @@ -17,7 +17,11 @@
> > #include "migration/vmstate.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 */
> > @@ -57,6 +61,143 @@ 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)
>
> You could improve this prototype. How about :
>
> bool aspeed_sbc_otpmem_read(AspeedSBCState *s, uint32_t otp_addr, Error
> *errp)
>
> same below.
Sure. I will update the function prototype in the next patch, as you suggested.
>
> > +{
> > + AspeedSBCState *s = ASPEED_SBC(opaque);
> > + uint32_t otp_addr, data, otp_offset;
> > + bool is_data = false;
> > + Error *local_err = NULL;
> > +
> > + assert(s->otpmem);
> > +
> > + 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;
> > +
> > + s->otpmem->ops->read(s->otpmem, otp_offset, &data, &local_err);
> > + if (local_err) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: Failed to read data 0x%x, %s\n",
> > + __func__, otp_offset,
> > + error_get_pretty(local_err));
> > + error_free(local_err);
> > + return;> + }
>
> Please use an AddressSpace. See aspeed_smc for an example.
>
I will rework the code to use an AddressSpace for OTP memory access.
Thanks for pointing me to the aspeed_smc example — it’s very helpful.
> > + s->regs[R_CAMP1] = data;
> > +
> > + if (is_data) {
> > + s->otpmem->ops->read(s->otpmem, otp_offset + 4, &data,
> &local_err);
> > + if (local_err) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: Failed to read data 0x%x, %s\n",
> > + __func__, otp_offset,
> > + error_get_pretty(local_err));
> > + error_free(local_err);
> > + return;
> > + }
> > + s->regs[R_CAMP2] = data;
> > + }
> > +}
> > +
> > +static void mr_handler(uint32_t otp_addr, uint32_t data)
>
> data is unused
I will remove it in the next patch.
>
> > +{
> > + 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,
>
> alignment issue.
>
I will adjust it in the next patch.
> > + "%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;
> > + Error *local_err = NULL;
> > +
> > + assert(s->otpmem);
> > +
> > + otp_addr = s->regs[R_ADDR];
> > + value = s->regs[R_CAMP1];
> > + if (otp_addr >= OTP_TOTAL_DWORD_COUNT) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: Invalid OTP addr 0x%x\n",
> > + __func__, otp_addr);
> > + return;
> > + }
> > +
> > + s->otpmem->ops->prog(s->otpmem, otp_addr, value, &local_err);
> > + if (local_err) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: Failed to program data 0x%x to 0x%x,
> %s\n",
> > + __func__, value, otp_addr,
> > + error_get_pretty(local_err));
> > + error_free(local_err);
> > + return;
> > + }
> > +}
> > +
> > +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 +219,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;
> > }
> > @@ -139,6 +283,8 @@ 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("otpmem", AspeedSBCState, otpmem,
> > + TYPE_ASPEED_OTPMEM, AspeedOTPMemState
> *),
>
> hmm, no.
>
> Instead AspeedOTPMemState should be a child object of AspeedSBCState, only
> created and realized for ast2600/1030 Soc. This means you will probably
> need a class attribute in AspeedSBCClass.
I will remove the DEFINE_PROP_LINK("otpmem", ...) approach and instead
manage AspeedOTPMemState as a child object of AspeedSBCState.
>
>
>
> > };
> >
> > static void aspeed_sbc_class_init(ObjectClass *klass, void *data)
> > diff --git a/include/hw/misc/aspeed_sbc.h
> > b/include/hw/misc/aspeed_sbc.h index 405e6782b9..8ae59d977e 100644
> > --- a/include/hw/misc/aspeed_sbc.h
> > +++ b/include/hw/misc/aspeed_sbc.h
> > @@ -10,6 +10,7 @@
> > #define ASPEED_SBC_H
> >
> > #include "hw/sysbus.h"
> > +#include "hw/misc/aspeed_otpmem.h"
> >
> > #define TYPE_ASPEED_SBC "aspeed.sbc"
> > #define TYPE_ASPEED_AST2600_SBC TYPE_ASPEED_SBC "-ast2600"
> > @@ -27,6 +28,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)
> > +
>
> These define belong to the implementation : aspeed_sbc.c.
>
I will move them to aspeed_sbc.c.
>
> Thanks,
>
> C.
>
>
> > struct AspeedSBCState {
> > SysBusDevice parent;
> >
> > @@ -36,6 +49,8 @@ struct AspeedSBCState {
> > MemoryRegion iomem;
> >
> > uint32_t regs[ASPEED_SBC_NR_REGS];
> > +
> > + AspeedOTPMemState *otpmem;
> > };
> >
> > struct AspeedSBCClass {
Thanks again for your comments and review.
Best Regards,
Kane
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH v3 3/3] hw/arm: Integrate Aspeed OTP memory into AST10x0 and AST2600 SoCs
2025-04-28 7:41 ` Cédric Le Goater
@ 2025-04-28 10:26 ` Kane Chen
2025-04-28 11:00 ` Cédric Le Goater
0 siblings, 1 reply; 21+ messages in thread
From: Kane Chen @ 2025-04-28 10:26 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Jamin Lin, Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: Troy Lee
Hi Cédric,
> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Monday, April 28, 2025 3:41 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 v3 3/3] hw/arm: Integrate Aspeed OTP memory into
> AST10x0 and AST2600 SoCs
>
> On 4/23/25 04:56, Kane Chen wrote:
> > From: Kane-Chen-AS <kane_chen@aspeedtech.com>
> >
> > This patch wires up the OTP memory device (`aspeed.otpmem`) into the
> > AST1030 and AST2600 SoC models. The device is initialized, attached to
> > a backing block drive (`-drive id=otpmem`) and linked to the SBC
> > controller via a QOM link.
> >
> > The default OTP memory image can be generated using the following
> > command.
> > ```bash
> > for i in $(seq 1 2048); do
> > printf '\x00\x00\x00\x00\xff\xff\xff\xff'
> > done > otpmem.img
> > ```
> >
> > To load the OTP memory image into the guest, use:
> > ```bash
> > ./qemu-system-arm \
> > -drive id=otpmem,file=otpmem.img,if=none,format=raw \
> > ...
> > ```
>
> I thought we were going to implement the same method of the edk2 flash
> devices of the q35 machine. Setting a machine option would set the drive :
>
> qemu-system-arm -M ast2600-evb,otpmem=otpmem-drive \
> -blockdev
> node-name=otpmem,driver=file,filename=/path/to/otpmem.img \
> ...
>
> Which is not what is proposed below.
>
I understand that using a machine option (e.g., -M ast2600-evb,otpmem=xxx)
to specify the OTP memory drive is similar to the modeling used for
flash devices in the Q35 machine. However, in the real ASPEED hardware,
the OTP memory is physically part of the Secure Boot Controller (SBC)
and is not designed to be removable or swappable. Allowing users to
specify the OTP memory through a machine option might imply otherwise,
which could be misleading compared to the actual hardware behavior.
That said, if maintaining consistency with QEMU’s device modeling
principles (as done for flash devices) is preferred over strict
hardware modeling fidelity, I am willing to adjust the implementation
accordingly.
Could you please confirm if you still prefer following the edk2 flash
model for OTP memory, despite the slight mismatch with hardware
behavior? Once confirmed, I will proceed with updating the object
hierarchy and the machine properties as discussed.
> > Note: Do not use the -snapshot option, or OTP data writes will not
> > persist to the image file.
> >
> > Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> > ---
> > hw/arm/aspeed_ast10x0.c | 19 +++++++++++++++++++
> > hw/arm/aspeed_ast2600.c | 19 +++++++++++++++++++
> > include/hw/arm/aspeed_soc.h | 2 ++
> > 3 files changed, 40 insertions(+)
> >
> > diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c index
> > ec329f4991..eaa70feb9f 100644
> > --- a/hw/arm/aspeed_ast10x0.c
> > +++ b/hw/arm/aspeed_ast10x0.c
> > @@ -15,6 +15,7 @@
> > #include "system/system.h"
> > #include "hw/qdev-clock.h"
> > #include "hw/misc/unimp.h"
> > +#include "system/block-backend-global-state.h"
> > #include "hw/arm/aspeed_soc.h"
> >
> > #define ASPEED_SOC_IOMEM_SIZE 0x00200000 @@ -156,6 +157,8 @@
> static
> > void aspeed_soc_ast1030_init(Object *obj)
> >
> > object_initialize_child(obj, "sbc", &s->sbc, TYPE_ASPEED_SBC);
> >
> > + object_initialize_child(obj, "otpmem", &s->otpmem,
> > + TYPE_ASPEED_OTPMEM);
> > +
>
> This belongs to AspeedSBC. See below.
>
> > for (i = 0; i < sc->wdts_num; i++) {
> > snprintf(typename, sizeof(typename), "aspeed.wdt-%s",
> socname);
> > object_initialize_child(obj, "wdt[*]", &s->wdt[i],
> > typename); @@ -194,6 +197,7 @@ static void
> aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
> > Error *err = NULL;
> > int i;
> > g_autofree char *sram_name = NULL;
> > + BlockBackend *blk;
> >
> > if (!clock_has_source(s->sysclk)) {
> > error_setg(errp, "sysclk clock must be wired up by the board
> > code"); @@ -359,6 +363,21 @@ static void
> aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
> >
> ASPEED_SMC_GET_CLASS(&s->spi[i])->flash_window_base);
> > }
> >
> > + /* OTP memory */
> > + blk = blk_by_name(ASPEED_OTPMEM_DRIVE);
> > + if (blk) {
> > + blk_set_perm(blk, BLK_PERM_CONSISTENT_READ |
> BLK_PERM_WRITE,
> > + 0, &error_fatal);
> > + qdev_prop_set_drive(DEVICE(&s->otpmem), "drive", blk);
> > +
> > + if (!sysbus_realize(SYS_BUS_DEVICE(&s->otpmem), errp)) {
> > + return;
> > + }
> > + /* Assign OTP memory to SBC */
> > + object_property_set_link(OBJECT(&s->sbc), "otpmem",
> > + OBJECT(&s->otpmem),
> &error_abort);
> > + }
> > +
>
> The "optmem" machine option should be pointing to "drive" option of the
> AspeedOTPMemState object in this object hierarchy :
>
> /machine (ast2600-evb-machine)
> /soc (ast2600-a3)
> /sbc (aspeed.sbc)
> /aspeed.sbc[0] (memory-region)
> /optmem (aspeed.otpmem) <- move otpmem there
>
> This will require using object_property_add_alias() in 2 or 3 levels.
>
> object_property_add_alias(OBJECT(parent), "optmem"
> OBJECT(child), "drive", &error_abort)
>
> Please try that instead and let's see the result.
>
I will move the otpmem as a child as the aspeed.sbc. For the "optmem" machine option,
please help confirm which one is the expected behavior.
> Thanks,
>
> C.
>
>
Thanks again for your review and helpful guidance.
Best Regards,
Kane
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 3/3] hw/arm: Integrate Aspeed OTP memory into AST10x0 and AST2600 SoCs
2025-04-28 10:26 ` Kane Chen
@ 2025-04-28 11:00 ` Cédric Le Goater
2025-04-28 11:39 ` Cédric Le Goater
2025-04-29 2:59 ` Kane Chen
0 siblings, 2 replies; 21+ messages in thread
From: Cédric Le Goater @ 2025-04-28 11:00 UTC (permalink / raw)
To: Kane Chen, Peter Maydell, Steven Lee, Troy Lee, Jamin Lin,
Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: Troy Lee
Hello,
> I understand that using a machine option (e.g., -M ast2600-evb,otpmem=xxx)
> to specify the OTP memory drive is similar to the modeling used for
> flash devices in the Q35 machine. However, in the real ASPEED hardware,
> the OTP memory is physically part of the Secure Boot Controller (SBC)
So this argument is a good reason to let the Aspeed SBC model own the
otpmem model and not the SoC. It fits better HW design.
> and is not designed to be removable or swappable.
Yes. Then, in that case, you should provide a static array of uin8t_t
defined at reset, which was my first suggestion. But you said you
wanted to be able to change the initial values. I am bit lost in what
you want to achieve. Please explain.
If you want to be able to change the initial values, you need to take
into account the QEMU user interface in the design. Being able to define
the otpmem backend using a blockdev is better for the long term support.
'-drive' is a poor interface we would like to remove. What would happen
if another device of the machine needed a format=raw drive ? how would
the drives be assigned ? depending on the command line ordering like we
do for mtd drives ? :/
Anyhow, wiring the block backend to the device of the machine is
Let's first start by defining the basic model.
> Allowing users to
> specify the OTP memory through a machine option might imply otherwise,
> which could be misleading compared to the actual hardware behavior.
I don't understand your point here. Putting the otpmem model under
SBC fits better HW design. Please explain.
> That said, if maintaining consistency with QEMU’s device modeling
> principles (as done for flash devices) is preferred over strict
> hardware modeling fidelity, I am willing to adjust the implementation
> accordingly.
QEMU is an emulator. We try to avoid modeling shortcuts, but for
usability and complexity reasons, we sometimes do.
>
> Could you please confirm if you still prefer following the edk2 flash
> model for OTP memory, despite the slight mismatch with hardware
> behavior?
AFAIUI, the current proposal is not matching HW. Please explain the
mismatch.
Thanks,
C.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 3/3] hw/arm: Integrate Aspeed OTP memory into AST10x0 and AST2600 SoCs
2025-04-28 11:00 ` Cédric Le Goater
@ 2025-04-28 11:39 ` Cédric Le Goater
2025-04-29 2:59 ` Kane Chen
1 sibling, 0 replies; 21+ messages in thread
From: Cédric Le Goater @ 2025-04-28 11:39 UTC (permalink / raw)
To: Kane Chen, Peter Maydell, Steven Lee, Troy Lee, Jamin Lin,
Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: Troy Lee
On 4/28/25 13:00, Cédric Le Goater wrote:
> Hello,
>
>> I understand that using a machine option (e.g., -M ast2600-evb,otpmem=xxx)
>> to specify the OTP memory drive is similar to the modeling used for
>> flash devices in the Q35 machine. However, in the real ASPEED hardware,
>> the OTP memory is physically part of the Secure Boot Controller (SBC)
>
> So this argument is a good reason to let the Aspeed SBC model own the
> otpmem model and not the SoC. It fits better HW design.
>
>> and is not designed to be removable or swappable.
>
> Yes. Then, in that case, you should provide a static array of uin8t_t
> defined at reset, which was my first suggestion. But you said you
> wanted to be able to change the initial values. I am bit lost in what
> you want to achieve. Please explain.
>
> If you want to be able to change the initial values, you need to take
> into account the QEMU user interface in the design. Being able to define
> the otpmem backend using a blockdev is better for the long term support.
> '-drive' is a poor interface we would like to remove. What would happen
> if another device of the machine needed a format=raw drive ? how would
> the drives be assigned ? depending on the command line ordering like we
> do for mtd drives ? :/
>
> Anyhow, wiring the block backend to the device of the machine is
missing "another topic." Sorry about that. I changed my keyboard to a
an ergonomic one this monday and it is a struggle.
> Let's first start by defining the basic model.
>
>> Allowing users to
>> specify the OTP memory through a machine option might imply otherwise,
>> which could be misleading compared to the actual hardware behavior.
> I don't understand your point here. Putting the otpmem model under
> SBC fits better HW design. Please explain.
>
>> That said, if maintaining consistency with QEMU’s device modeling
>> principles (as done for flash devices) is preferred over strict
>> hardware modeling fidelity, I am willing to adjust the implementation
>> accordingly.
>
> QEMU is an emulator. We try to avoid modeling shortcuts, but for
> usability and complexity reasons, we sometimes do.
>
>>
>> Could you please confirm if you still prefer following the edk2 flash
>> model for OTP memory, despite the slight mismatch with hardware
>> behavior?
>
> AFAIUI, the current proposal is not matching HW. Please explain the
> mismatch.
>
> Thanks,
>
> C.
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH v3 3/3] hw/arm: Integrate Aspeed OTP memory into AST10x0 and AST2600 SoCs
2025-04-28 11:00 ` Cédric Le Goater
2025-04-28 11:39 ` Cédric Le Goater
@ 2025-04-29 2:59 ` Kane Chen
2025-04-29 9:05 ` Cédric Le Goater
1 sibling, 1 reply; 21+ messages in thread
From: Kane Chen @ 2025-04-29 2:59 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Jamin Lin, Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: Troy Lee
Hi Cédric,
I may have misunderstood the otpmem machine option. Please correct me if I am wrong.
> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Monday, April 28, 2025 7:01 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 v3 3/3] hw/arm: Integrate Aspeed OTP memory into
> AST10x0 and AST2600 SoCs
>
> Hello,
>
> > I understand that using a machine option (e.g., -M
> > ast2600-evb,otpmem=xxx) to specify the OTP memory drive is similar to
> > the modeling used for flash devices in the Q35 machine. However, in
> > the real ASPEED hardware, the OTP memory is physically part of the
> > Secure Boot Controller (SBC)
>
> So this argument is a good reason to let the Aspeed SBC model own the
> otpmem model and not the SoC. It fits better HW design.
>
> > and is not designed to be removable or swappable.
>
> Yes. Then, in that case, you should provide a static array of uin8t_t defined at
> reset, which was my first suggestion. But you said you wanted to be able to
> change the initial values. I am bit lost in what you want to achieve. Please
> explain.
In the HW design, the OTP memory can be used to store the chip ID,
secure settings, or the key for secure boot.
The initial value I want to set is something like the chip ID, which
is a fixed value for each chip and should not change after reset.
Therefore, I provided an API to set default values, so that these values
can be configured more easily during the SoC or SBC realize stage.
>
> If you want to be able to change the initial values, you need to take into
> account the QEMU user interface in the design. Being able to define the
> otpmem backend using a blockdev is better for the long term support.
> '-drive' is a poor interface we would like to remove. What would happen if
> another device of the machine needed a format=raw drive ? how would the
> drives be assigned ? depending on the command line ordering like we do for
> mtd drives ? :/
>
> Anyhow, wiring the block backend to the device of the machine is Let's first
> start by defining the basic model.
I will move the backend to use blockdev.
I previously thought that -drive internally created a blockdev backend,
but I understand now that it also introduces other issues. Thanks for
the clarification.
>
> > Allowing users to
> > specify the OTP memory through a machine option might imply otherwise,
> > which could be misleading compared to the actual hardware behavior.
> I don't understand your point here. Putting the otpmem model under SBC fits
> better HW design. Please explain.
>
> > That said, if maintaining consistency with QEMU’s device modeling
> > principles (as done for flash devices) is preferred over strict
> > hardware modeling fidelity, I am willing to adjust the implementation
> > accordingly.
>
> QEMU is an emulator. We try to avoid modeling shortcuts, but for usability and
> complexity reasons, we sometimes do.
>
> >
> > Could you please confirm if you still prefer following the edk2 flash
> > model for OTP memory, despite the slight mismatch with hardware
> > behavior?
>
> AFAIUI, the current proposal is not matching HW. Please explain the mismatch.
The Secure Boot Controller (SBC) includes some components like OTP
memory, crypto engine, boot controller, and so on. All components
within the SBC are fixed and cannot be changed. If we allow an otpmem
machine option, it may imply that different types or sizes of OTP
memory models are supported, such as:
* Different size: -M ast2600-evb,otpmem=otpmem-64k-drive
* Different model: -M ast2600-evb,otpmem=flash-drive
However, in the real hardware design, there is only one fixed model
for the OTP memory. Based on this, I am not sure if introducing an
otpmem machine option is appropriate.
> Thanks,
>
> C.
>
Best Regards,
Kane
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 3/3] hw/arm: Integrate Aspeed OTP memory into AST10x0 and AST2600 SoCs
2025-04-29 2:59 ` Kane Chen
@ 2025-04-29 9:05 ` Cédric Le Goater
2025-04-30 10:30 ` Kane Chen
0 siblings, 1 reply; 21+ messages in thread
From: Cédric Le Goater @ 2025-04-29 9:05 UTC (permalink / raw)
To: Kane Chen, Peter Maydell, Steven Lee, Troy Lee, Jamin Lin,
Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: Troy Lee
Hello Kane,
[ ... ]
> The Secure Boot Controller (SBC) includes some components like OTP
> memory, crypto engine, boot controller, and so on. All components
> within the SBC are fixed and cannot be changed. If we allow an otpmem
> machine option, it may imply that different types or sizes of OTP
> memory models are supported, such as:
>
> * Different size: -M ast2600-evb,otpmem=otpmem-64k-drive
> * Different model: -M ast2600-evb,otpmem=flash-drive
The optmem model should check the size and fail to realize in that
case. This would stop the machine before reset. This is a common
pattern in QEMU. See m25p80_realize().
Also, I think we would like the machine to start even if there is no
block backend. Please check how m25p80 models that behavior.
Thanks,
C.
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH v3 3/3] hw/arm: Integrate Aspeed OTP memory into AST10x0 and AST2600 SoCs
2025-04-29 9:05 ` Cédric Le Goater
@ 2025-04-30 10:30 ` Kane Chen
2025-04-30 11:50 ` Cédric Le Goater
0 siblings, 1 reply; 21+ messages in thread
From: Kane Chen @ 2025-04-30 10:30 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Jamin Lin, Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: Troy Lee
Hi Cédric,
Currently, the OTP memory is not supported, but the guest firmware
still attempts to access it, even though no functionality is
available.
To handle this case, I will silently discard those operations when no
backend is present, so that the machine can still boot and run without
error.
If you would prefer to have a message logged, please let me know.
Best Regards,
Kane
> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Tuesday, April 29, 2025 5:06 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 v3 3/3] hw/arm: Integrate Aspeed OTP memory into
> AST10x0 and AST2600 SoCs
>
> Hello Kane,
>
> [ ... ]
>
> > The Secure Boot Controller (SBC) includes some components like OTP
> > memory, crypto engine, boot controller, and so on. All components
> > within the SBC are fixed and cannot be changed. If we allow an otpmem
> > machine option, it may imply that different types or sizes of OTP
> > memory models are supported, such as:
> >
> > * Different size: -M ast2600-evb,otpmem=otpmem-64k-drive
> > * Different model: -M ast2600-evb,otpmem=flash-drive
>
> The optmem model should check the size and fail to realize in that case. This
> would stop the machine before reset. This is a common pattern in QEMU. See
> m25p80_realize().
>
> Also, I think we would like the machine to start even if there is no block
> backend. Please check how m25p80 models that behavior.
>
> Thanks,
>
> C.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 3/3] hw/arm: Integrate Aspeed OTP memory into AST10x0 and AST2600 SoCs
2025-04-30 10:30 ` Kane Chen
@ 2025-04-30 11:50 ` Cédric Le Goater
0 siblings, 0 replies; 21+ messages in thread
From: Cédric Le Goater @ 2025-04-30 11:50 UTC (permalink / raw)
To: Kane Chen, Peter Maydell, Steven Lee, Troy Lee, Jamin Lin,
Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs,
open list:All patches CC here
Cc: Troy Lee
On 4/30/25 12:30, Kane Chen wrote:
> Hi Cédric,
>
> Currently, the OTP memory is not supported, but the guest firmware
> still attempts to access it, even though no functionality is
> available.
>
> To handle this case, I will silently discard those operations when no
> backend is present, so that the machine can still boot and run without
> error.
>
> If you would prefer to have a message logged, please let me know.
Please add trace events for this case. There are always useful.
Thanks,
C.
>
> Best Regards,
> Kane
>> -----Original Message-----
>> From: Cédric Le Goater <clg@kaod.org>
>> Sent: Tuesday, April 29, 2025 5:06 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 v3 3/3] hw/arm: Integrate Aspeed OTP memory into
>> AST10x0 and AST2600 SoCs
>>
>> Hello Kane,
>>
>> [ ... ]
>>
>>> The Secure Boot Controller (SBC) includes some components like OTP
>>> memory, crypto engine, boot controller, and so on. All components
>>> within the SBC are fixed and cannot be changed. If we allow an otpmem
>>> machine option, it may imply that different types or sizes of OTP
>>> memory models are supported, such as:
>>>
>>> * Different size: -M ast2600-evb,otpmem=otpmem-64k-drive
>>> * Different model: -M ast2600-evb,otpmem=flash-drive
>>
>> The optmem model should check the size and fail to realize in that case. This
>> would stop the machine before reset. This is a common pattern in QEMU. See
>> m25p80_realize().
>>
>> Also, I think we would like the machine to start even if there is no block
>> backend. Please check how m25p80 models that behavior.
>>
>> Thanks,
>>
>> C.
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH v3 2/3] hw/misc/aspeed_sbc: Connect Aspeed OTP memory device to SBC controller
2025-04-28 10:25 ` Kane Chen
@ 2025-05-08 9:46 ` Kane Chen
0 siblings, 0 replies; 21+ messages in thread
From: Kane Chen @ 2025-05-08 9:46 UTC (permalink / raw)
To: Kane Chen, Cédric Le Goater, Peter Maydell, Steven Lee,
Troy Lee, Jamin Lin, Andrew Jeffery, Joel Stanley,
open list:ASPEED BMCs, open list:All patches CC here
Cc: Troy Lee
Hi Cédric,
> > > +{
> > > + AspeedSBCState *s = ASPEED_SBC(opaque);
> > > + uint32_t otp_addr, data, otp_offset;
> > > + bool is_data = false;
> > > + Error *local_err = NULL;
> > > +
> > > + assert(s->otpmem);
> > > +
> > > + 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;
> > > +
> > > + s->otpmem->ops->read(s->otpmem, otp_offset, &data, &local_err);
> > > + if (local_err) {
> > > + qemu_log_mask(LOG_GUEST_ERROR,
> > > + "%s: Failed to read data 0x%x, %s\n",
> > > + __func__, otp_offset,
> > > + error_get_pretty(local_err));
> > > + error_free(local_err);
> > > + return;> + }
> >
> > Please use an AddressSpace. See aspeed_smc for an example.
> >
> I will rework the code to use an AddressSpace for OTP memory access.
> Thanks for pointing me to the aspeed_smc example — it’s very helpful.
Sorry for bringing up this topic again. While working with AddressSpace,
I found that I couldn't directly use address_space_read() or
address_space_write() in the SBC code, because the OTP memory enforces
more specific access rules.
For example, some regions of the OTP memory may be configured as
write-only, meaning the guest is not allowed to read from them. To
support this behavior, I plan to implement custom read/write functions
in the OTP memory device. These functions will act as wrappers around
the AddressSpace APIs and enforce additional checks before allowing
memory accesses from the SBC.
These additional checks will be introduced in a separate patch to keep
the changes organized and easier to review.
Do you have any concerns about this approach?
Best Regards,
Kane
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2025-05-08 9:47 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-23 2:56 [PATCH v3 0/3] hw/misc/aspeed_otp: Introduce OTP memory and integrate with SBC Kane Chen via
2025-04-23 2:56 ` Kane Chen via
2025-04-23 2:56 ` [PATCH v3 1/3] hw/misc/aspeed_otp: Add Aspeed OTP memory device model Kane Chen via
2025-04-23 2:56 ` Kane Chen via
2025-04-28 7:06 ` Cédric Le Goater
2025-04-28 10:24 ` Kane Chen
2025-04-23 2:56 ` [PATCH v3 2/3] hw/misc/aspeed_sbc: Connect Aspeed OTP memory device to SBC controller Kane Chen via
2025-04-23 2:56 ` Kane Chen via
2025-04-28 7:20 ` Cédric Le Goater
2025-04-28 10:25 ` Kane Chen
2025-05-08 9:46 ` Kane Chen
2025-04-23 2:56 ` [PATCH v3 3/3] hw/arm: Integrate Aspeed OTP memory into AST10x0 and AST2600 SoCs Kane Chen via
2025-04-23 2:56 ` Kane Chen via
2025-04-28 7:41 ` Cédric Le Goater
2025-04-28 10:26 ` Kane Chen
2025-04-28 11:00 ` Cédric Le Goater
2025-04-28 11:39 ` Cédric Le Goater
2025-04-29 2:59 ` Kane Chen
2025-04-29 9:05 ` Cédric Le Goater
2025-04-30 10:30 ` Kane Chen
2025-04-30 11:50 ` Cédric Le Goater
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.