Hi Bernhard,
Thank you for your suggestion. I will update the code to use assert()
in the next version of the patch. If you have any concerns about the
other code segments, please feel free to let me know.
Best Regards,
Kane
> -----Original Message-----
> From: Bernhard Beschow <shen...@gmail.com>
> Sent: Thursday, April 17, 2025 6:06 PM
> To: Kane Chen <kane_c...@aspeedtech.com>; Kane Chen via
> <qemu-devel@nongnu.org>; Cédric Le Goater <c...@kaod.org>; Peter Maydell
> <peter.mayd...@linaro.org>; Steven Lee <steven_...@aspeedtech.com>; Troy
> Lee <leet...@gmail.com>; Jamin Lin <jamin_...@aspeedtech.com>; Andrew
> Jeffery <and...@codeconstruct.com.au>; Joel Stanley <j...@jms.id.au>; open
> list:ASPEED BMCs <qemu-...@nongnu.org>; open list:All patches CC here
> <qemu-devel@nongnu.org>
> Cc: Troy Lee <troy_...@aspeedtech.com>; Kane Chen
> <kane_c...@aspeedtech.com>
> Subject: Re: [PATCH v2 1/3] hw/misc/aspeed_otp: Add Aspeed OTP memory
> device model
>
>
>
> Am 17. April 2025 03:09:53 UTC schrieb Kane Chen via
> <qemu-devel@nongnu.org>:
> >From: Kane-Chen-AS <kane_c...@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_c...@aspeedtech.com>
> >---
> > hw/misc/aspeed_otpmem.c | 217
> ++++++++++++++++++++++++++++++++
> > hw/misc/meson.build | 1 +
> > include/hw/misc/aspeed_otpmem.h | 40 ++++++
> > 3 files changed, 258 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..cf4b3aa4d2
> >--- /dev/null
> >+++ b/hw/misc/aspeed_otpmem.c
> >@@ -0,0 +1,217 @@
> >+/*
> >+ * 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);
> >+
> >+ if (!otp->blk) {
> >+ error_setg(errp, "OTP memory is not initialized");
> >+ return;
> >+ }
>
> Since the same condition is checked in realize(), this if statement is
> unreachable code. Maybe use an assert() here? There is more unreachable
> code like this in other functions.
>
> Best regards,
> Bernhard
>
> >+
> >+ 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;
> >+
> >+ if (!otp->blk) {
> >+ error_setg(errp, "OTP memory is not initialized");
> >+ return;
> >+ }
> >+
> >+ 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 */
> >+
