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