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

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.

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

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.

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

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.

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.

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.
> 

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.
> 
>