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