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.


Reply via email to