Hi Cédric,
Thank you for your comments and suggestions. I’ll start by reviewing
the changes in commit ebc29e1beab0. At the moment, I don’t have
another example of a device that is accessed through an external bus,
but I’ll look into it and see what I can find.
Best Regards,
Kane
> -----Original Message-----
> From: Cédric Le Goater <c...@kaod.org>
> Sent: Monday, April 7, 2025 5:55 PM
> To: 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-devel@nongnu.org>; qemu-block <qemu-bl...@nongnu.org>; Markus
> Armbruster <arm...@redhat.com>
> Cc: Troy Lee <troy_...@aspeedtech.com>
> Subject: Re: [PATCH v1 0/1] hw/misc/aspeed_sbc: Implement OTP memory and
> controller
>
> 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 \
> ...
>
>
> > 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.
>
>
> > 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.
>
> Thanks,
>
> C.
>
>
>
>
>
> > Thanks for your comments and review.
> >
> > Best Regards,
> > Kane
> >
> >> -----Original Message-----
> >> From: Cédric Le Goater <c...@kaod.org>
> >> Sent: Friday, April 4, 2025 9:54 PM
> >> To: Philippe Mathieu-Daudé <phi...@linaro.org>; Kane Chen
> >> <kane_c...@aspeedtech.com>; 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>; qemu-block <qemu-bl...@nongnu.org>
> >> Cc: Troy Lee <troy_...@aspeedtech.com>
> >> Subject: Re: [PATCH v1 0/1] hw/misc/aspeed_sbc: Implement OTP memory
> >> and controller
> >>
> >> On 4/4/25 15:00, Philippe Mathieu-Daudé wrote:
> >>> +qemu-block@
> >>>
> >>> On 4/4/25 14:06, Cédric Le Goater wrote:
> >>>> Hello,
> >>>>
> >>>> On 4/2/25 11:14, Kane-Chen-AS wrote:
> >>>>> This patch introduces part of the Secure Boot Controller device,
> >>>>> which consists of several sub-components, including an OTP memory,
> >>>>> OTP controller, cryptographic engine, and boot controller.
> >>>>>
> >>>>> In this version, the implementation includes the OTP memory and
> >>>>> its controller. The OTP memory can be programmed from within the
> >>>>> guest OS via a software utility.
> >>>>
> >>>>
> >>>> What is the OTP memory ? An external flash device or built-in SRAM ?
> >>>> If the latter, I suggest using an allocated buffer under the SBC
> >>>> model and avoid the complexity of the BlockBackend implementation
> >>>> and the definition of a drive on the command line for it. The
> >>>> proposal is bypassing a lot of QEMU layers for this purpose.
> >>>
> >>> More of the former, a built-in eFuse behaving more like flash. So
> >>> using block backend for the storage seems correct to me.
> >>
> >> How would you define the drive backend on the command line ?
> >>
> >>> However I don't think
> >>> the implementation belongs to hw/misc/aspeed_sbc; ideally we'd have
> >>> some abstract (or interface) implementation in hw/block/otp.c --
> >>> with methods such program_otp_data() --, completed by
> hw/block/aspeed_otc.c.
> >>
> >> I was imagining more something like NPCM7xxOTPState or
> >> BCM2835OTPState and not SiFiveUOTPState.
> >>
> >>> Current patch might be good enough to start with IMHO.
> >>
> >> Have you looked at the next patch and how the backend is handled ?
> >> I will let the block people Ack this patch in that case. It's beyond my
> >> skills.
> >>
> >> Thanks,
> >>
> >> C.
> >>
> >
