Re: [PATCH] driver: rng: Do not check ARM_SMCCC_TRNG_VERSION
On Wed, Jul 10, 2024 at 12:23 AM Leo Yan wrote:
>
> As described in the document SMC Calling Convention (ARM DEN 0028 1.5 F),
> section 7 "Arm Architecture Calls", the SMC call SMCCC_ARCH_FEATURES is
> not expected to support the function ID ARM_SMCCC_TRNG_VERSION. Trusted
> Firmware-A follows up the specification in its implementation.
>
> This commit removes the invocation to avoid the failure - which is a
> wrong calling in U-boot. The later code invokes ARM_SMCCC_TRNG_VERSION
> for retrieving the TRNG version, except it can read back the version
> number, it also can be used to detect whether the TRNG is supported or
> not.
LGTM.
Reviewed-by: Weizhao Ouyang
BR,
Weizhao
>
> Signed-off-by: Leo Yan
> ---
> drivers/rng/smccc_trng.c | 4
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/rng/smccc_trng.c b/drivers/rng/smccc_trng.c
> index f59b80666b..1da1affd8e 100644
> --- a/drivers/rng/smccc_trng.c
> +++ b/drivers/rng/smccc_trng.c
> @@ -135,10 +135,6 @@ static bool smccc_trng_is_supported(void
> (*invoke_fn)(unsigned long a0, unsigned
> {
> struct arm_smccc_res res;
>
> - (*invoke_fn)(ARM_SMCCC_ARCH_FEATURES, ARM_SMCCC_TRNG_VERSION, 0, 0,
> 0, 0, 0, 0, );
> - if (res.a0 == ARM_SMCCC_RET_NOT_SUPPORTED)
> - return false;
> -
> (*invoke_fn)(ARM_SMCCC_TRNG_VERSION, 0, 0, 0, 0, 0, 0, 0, );
> if (res.a0 & BIT(31))
> return false;
> --
> 2.34.1
>
Re: [PATCH v2] efi_loader: Fix EFI_VARIABLE_APPEND_WRITE hash check
On Fri, May 10, 2024 at 5:23 PM Heinrich Schuchardt wrote:
>
> On 5/8/24 13:13, Weizhao Ouyang wrote:
> > According to UEFI v2.10 spec section 8.2.6, if a caller invokes the
> > SetVariables() service, it will produce a digest from hash(VariableName,
> > VendorGuid, Attributes, TimeStamp, DataNew_variable_content), then the
> > firmware that implements the SetVariable() service will compare the
> > digest with the result of applying the signer’s public key to the
> > signature. For EFI variable append write, efitools sign-efi-sig-list has
> > an option "-a" to add EFI_VARIABLE_APPEND_WRITE attr, and u-boot will
> > drop this attribute in efi_set_variable_int(). So if a caller uses
> > "sign-efi-sig-list -a" to create the authenticated variable, this append
> > write will fail in the u-boot due to "hash check failed".
> >
> > This patch resumes writing the EFI_VARIABLE_APPEND_WRITE attr to ensure
> > that the hash check is correct. And also update the "test_efi_secboot"
> > test case to compliance with the change.
>
> Looking at EDK II I see that VerifyTimeBasedPayloadAndUpdate() calls
> VerifyTimeBasedPayload() before invoking
> AuthServiceInternalUpdateVariableWithTimeStamp() which handles the
> append flag so this seems to match the intended behavior that you describe.
>
> Should we move the IS_ENABLED(CONFIG_EFI_SECURE_BOOT) check to
> efi_variable_authenticate() and call this function directly after
> setvariable_allowed() if
> EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS is set?
>
> This would make efi_set_variable_int() easier to read and would simplify
> the handling of the attribute flag.
Yeah, I think it's reasonable.
BR,
Weizhao
>
> The UEFI Self-Certification Test (SCT) never highlighted the issue. If a
> test is missing there,it would be a good idea to add an issue in
> https://bugzilla.tianocore.org/.
>
> Best regards
>
> Heinrich
>
> >
> > Signed-off-by: Weizhao Ouyang
> > ---
> > v2: skip attr for efi_var_mem_ins()
> > ---
> > lib/efi_loader/efi_variable.c | 6 +++---
> > test/py/tests/test_efi_secboot/conftest.py | 10 ++
> > test/py/tests/test_efi_secboot/test_authvar.py | 4 ++--
> > test/py/tests/test_efi_secboot/test_signed.py | 10 +-
> > 4 files changed, 20 insertions(+), 10 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> > index 1cc02acb3b..09651d4675 100644
> > --- a/lib/efi_loader/efi_variable.c
> > +++ b/lib/efi_loader/efi_variable.c
> > @@ -288,7 +288,6 @@ efi_status_t efi_set_variable_int(const u16
> > *variable_name,
> > /* check if a variable exists */
> > var = efi_var_mem_find(vendor, variable_name, NULL);
> > append = !!(attributes & EFI_VARIABLE_APPEND_WRITE);
> > - attributes &= ~EFI_VARIABLE_APPEND_WRITE;
> > delete = !append && (!data_size || !attributes);
> >
> > /* check attributes */
> > @@ -304,7 +303,7 @@ efi_status_t efi_set_variable_int(const u16
> > *variable_name,
> >
> > /* attributes won't be changed */
> > if (!delete &&
> > - ((ro_check && var->attr != attributes) ||
> > + ((ro_check && var->attr != (attributes &
> > ~EFI_VARIABLE_APPEND_WRITE)) ||
> >(!ro_check && ((var->attr & ~EFI_VARIABLE_READ_ONLY)
> > != (attributes &
> > ~EFI_VARIABLE_READ_ONLY) {
> > return EFI_INVALID_PARAMETER;
> > @@ -378,7 +377,8 @@ efi_status_t efi_set_variable_int(const u16
> > *variable_name,
> > for (; *old_data; ++old_data)
> > ;
> > ++old_data;
> > - ret = efi_var_mem_ins(variable_name, vendor, attributes,
> > + ret = efi_var_mem_ins(variable_name, vendor,
> > + attributes & ~EFI_VARIABLE_APPEND_WRITE,
> > var->length, old_data, data_size, data,
> > time);
> > } else {
> > diff --git a/test/py/tests/test_efi_secboot/conftest.py
> > b/test/py/tests/test_efi_secboot/conftest.py
> > index ff7ac7c810..0fa0747fc7 100644
> > --- a/test/py/tests/test_efi_secboot/conftest.py
> > +++ b/test/py/tests/test_efi_secboot/conftest.py
> > @@ -64,6 +64,12 @@ def efi_boot_env(request, u_boot_config):
> > check_call('cd %s; %
Re: [PATCH] efi_loader: capsule: detect possible ESP device path
On Wed, May 8, 2024 at 9:56 PM Ilias Apalodimas
wrote:
>
> >
> > > + *
> > > + * Get a possible efi system partition by expanding a boot option
> > > + * file path.
> > > + *
> > > + * @boot_dev The device path pointing to a boot option
> > > + * Return: The full ESP device path or NULL if fail
> > > + */
> > > +static struct efi_device_path *get_esp_from_boot_option_file_path(struct
> > > efi_device_path *boot_dev)
> > > +{
> > > + efi_status_t ret = EFI_SUCCESS;
> > > + efi_handle_t handle;
> > > + struct efi_device_path *dp = boot_dev;
> > > + struct efi_device_path *full_path = NULL;
> > > +
> > > + ret =
> > > EFI_CALL(efi_locate_device_path(_simple_file_system_protocol_guid,
> > > + ,
> > > + ));
> > > + if (ret != EFI_SUCCESS)
> > > + ret = EFI_CALL(efi_locate_device_path(_block_io_guid,
> > > , ));
> > > +
> > > + /* Expand Media Device Path */
> >
> > I don't see where the device path is expanded in this patch.
> > We already have try_load_from_media() which tries to do something
> > similar. Is anything missing from there?
>
> Looking a bit closer, we do lack calling ConnectController there, but
> all this should be added to try_load_from_media() not in a different
> path.
Yeah, I'll summarize them to the try_load_from_media().
> Also I don't think we need the Fixes tag
Okay.
BR,
Weizhao
>
> Thanks
> /Ilias
> >
> > > + if (ret == EFI_SUCCESS && EFI_DP_TYPE(dp, END, END)) {
> >
> > Can you invert the logic here and return immediately?
> > (if ret != EFI_SUCCESS ...etc )
> > return full_path;
> >
> > The indentation will go away.
> >
> > > + struct efi_device_path *temp_dp;
> > > + struct efi_block_io *block_io;
> > > + void *buffer;
> > > + efi_handle_t *simple_file_system_handle;
> > > + efi_uintn_t number_handles, index;
> > > + u32 size;
> > > + u32 temp_size;
> > > +
> > > + temp_dp = boot_dev;
> > > + ret =
> > > EFI_CALL(efi_locate_device_path(_simple_file_system_protocol_guid,
> > > + _dp,
> > > + ));
> > > + /*
> > > +* For device path pointing to simple file system, it
> > > only expands to one
> > > +* full path
> >
> > Why do we have multiple calls to efi_locate_device_path()? Aren't the
> > arguments identical?
> > Which part of edk2 did you read? Is it BmExpandMediaDevicePath()?
> >
> >
> > > +*/
> > > + if (ret == EFI_SUCCESS && EFI_DP_TYPE(temp_dp, END, END))
> > > {
> > > + if (device_is_present_and_system_part(temp_dp))
> > > + return temp_dp;
> > > + }
> > > +
> > > + /*
> > > +* For device path only pointing to the removable device
> > > handle, try to
> > > +* search all its children handles
> > > +*/
> > > + ret = EFI_CALL(efi_locate_device_path(_block_io_guid,
> > > _dp, ));
> > > + EFI_CALL(efi_connect_controller(handle, NULL, NULL,
> > > true));
> > > +
> > > + /* Align with edk2, issue a dummy read to the device to
> > > check the device change */
> > > + ret = EFI_CALL(efi_handle_protocol(handle,
> > > _block_io_guid, (void **)_io));
> > > + if (ret == EFI_SUCCESS) {
> > > + buffer = memalign(block_io->media->io_align,
> > > block_io->media->block_size);
> > > + if (buffer) {
> > > + ret =
> > > EFI_CALL(block_io->read_blocks(block_io,
> > > +
> > > block_io->media->media_id,
> > > +0,
> > > +
> > > block_io->media->block_size,
> > > +
> > > buffer));
> > > + free(buffer);
> > > + } else {
> > > + return full_path;
> > > + }
> > > + } else {
> > > + return full_path;
> > > + }
> > > +
> > > + /* detect the default boot file from removable media */
> > > + size = efi_dp_size(boot_dev) - sizeof(struct
> > > efi_device_path);
> > > + EFI_CALL(efi_locate_handle_buffer(BY_PROTOCOL,
> > > +
> > > _simple_file_system_protocol_guid,
> > > + NULL,
> > > +
Re: [PATCH] efi_loader: capsule: detect possible ESP device path
On Wed, May 8, 2024 at 9:53 PM Heinrich Schuchardt wrote:
>
> On 5/8/24 14:59, Weizhao Ouyang wrote:
> > On Wed, May 8, 2024 at 7:52 PM Heinrich Schuchardt
> > wrote:
> >>
> >> On 5/8/24 13:24, Weizhao Ouyang wrote:
> >>> When using CapsuleApp to execute on-disk update, it will choose the
> >>> first boot option as BootNext entry to perform the capsule update after
> >>> a reboot. But auto-generate boot option will ignore the logical
> >>> partition which might be an ESP, thus it could not find the capsule
> >>> file.
> >>>
> >>> Align with the EDK II, detect the possible ESP device path by expanding
> >>> the media path.
> >>
> >> Hello Wheizhoa,
> >>
> >> From the description I would not know how to reproduce the problem you
> >> face.
> >>
> >> Could you, please, provide an example (including disk image and capsule)
> >> for QEMU and describe expected and observed behavior.
> >>
> >> We should add a CI test case covering the observed bug.
> >
> > Hi Heinrich,
> >
> > Here is a brief description, I'll add a testcase in the next patch.
> >
> > background:
> > - there is an ESP in an mmc logical partition
> > - in the edk2 shell payload, running "CapsuleApp.efi -OD" to perform
> > the capsule on-disk update
> >
> > workflow:
> > - currently, u-boot will auto-generate boot option and install with
> > eliminating logical partitions. So we will have a boot variable
> > Boot that its file_path seems like: "/VenHw/eMMC(0)/eMMC(0)"
> > - then we run the efi app in the edk shell payload. The CapsuleApp
> > will find this boot entry, and it will detect that there is an ESP
> > in this mmc device. According to the on-disk update flow, it will
> > copy the capsule to the ESP, and add the Boot to the BootNext.
>
> Why do you need an EFI app? Shouldn't the operating system place the
> capsule file?
>
> What makes you expect that there is any ESP on whatever Boot points
> to? You should evaluate BootCurrent if you want to know from which boot
> entry the current application was started from.
CapsuleApp[1] is a shell application that can manually trigger
capsule update process. As one of its flow, it will install the
capsule to the ESP and update the BootNext. The purpose of this
patch is to fix the broken update process and align with edk2.
>
> > - After CapsuleApp.efi reset the device, u-boot will check
> > OsIndications and perform the on-disk update flow. Then it will
>
> U-Boot does not check OsIndications.
I mean the check_run_capsules().
[1]
https://github.com/tianocore/edk2/tree/master/MdeModulePkg/Application/CapsuleApp
BR,
Weizhao
>
> Best regards
>
> Heirnich
>
> > search for the capsule file from the boot entry indicated by the
> > BootNext. Unfortunately, its file_path is "/VenHw/eMMC(0)/eMMC(0)",
> > so the search will fail.
> > - Using this patch, u-boot will expand the Boot file path to
> > detect its ESP, so the capsule file will be found in the expanded
> > device path, eg: "/VenHw/eMMC(0)/eMMC(0)/HD(2)".
> >
> > BR,
> > Weizhao
> >
> >>
> >>>
> >>> Fixes: f86fba8adbf3 ("efi_loader: auto-generate boot option for each
> >>> blkio device")
> >>> Signed-off-by: Weizhao Ouyang
> >>> ---
> >>>include/efi_loader.h | 6 ++
> >>>lib/efi_loader/efi_boottime.c | 15 ++---
> >>>lib/efi_loader/efi_capsule.c | 110 +-
> >>>3 files changed, 118 insertions(+), 13 deletions(-)
> >>>
> >>> diff --git a/include/efi_loader.h b/include/efi_loader.h
> >>> index 9600941aa327..9d78fa936701 100644
> >>> --- a/include/efi_loader.h
> >>> +++ b/include/efi_loader.h
> >>> @@ -683,6 +683,12 @@ efi_status_t efi_protocol_open(struct efi_handler
> >>> *handler,
> >>> void **protocol_interface, void
> >>> *agent_handle,
> >>> void *controller_handle, uint32_t
> >>> attributes);
> >>>
> >>> +/* Connect drivers to controller */
> >>> +efi_status_t EFIAPI efi_connect_controller(efi_handle_t
> >>> controller_handle,
> >>> +efi_handle_t
> >>> *driver_image_handle,
> >>> +str
Re: [PATCH] efi_loader: capsule: detect possible ESP device path
Hi Ilias,
On Wed, May 8, 2024 at 9:47 PM Ilias Apalodimas
wrote:
>
> Hi Weizhao,
>
> On Wed, 8 May 2024 at 14:24, Weizhao Ouyang wrote:
> >
> > When using CapsuleApp to execute on-disk update, it will choose the
> > first boot option as BootNext entry to perform the capsule update after
> > a reboot.
>
> This is not entirely accurate. The capsule update on-disk mechanism
> will look in the ESP pointed to by BootNext or the highest priority
> BootOrder.
> But the problem you are describing isn't tied only to capsules. IIUC
> if you have a logical partition with bootXXX.efi the current code will
> ignore it as well and the automatic selection of the boot option won't
> work.
> Can you adjust the commit message on v2 and mention the scanning as an
> issue with the capsule on disk being the obvious side-effect?
I'll do.
>
> > But auto-generate boot option will ignore the logical
> > partition which might be an ESP, thus it could not find the capsule
> > file.
> >
> > Align with the EDK II, detect the possible ESP device path by expanding
> > the media path.
>
> >
> > Fixes: f86fba8adbf3 ("efi_loader: auto-generate boot option for each blkio
> > device")
> > Signed-off-by: Weizhao Ouyang
> > ---
> > include/efi_loader.h | 6 ++
> > lib/efi_loader/efi_boottime.c | 15 ++---
> > lib/efi_loader/efi_capsule.c | 110 +-
> > 3 files changed, 118 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 9600941aa327..9d78fa936701 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -683,6 +683,12 @@ efi_status_t efi_protocol_open(struct efi_handler
> > *handler,
> >void **protocol_interface, void
> > *agent_handle,
> >void *controller_handle, uint32_t
> > attributes);
> >
> > +/* Connect drivers to controller */
> > +efi_status_t EFIAPI efi_connect_controller(efi_handle_t controller_handle,
> > + efi_handle_t
> > *driver_image_handle,
> > + struct efi_device_path
> > *remain_device_path,
> > + bool recursive);
> > +
> > /* Install multiple protocol interfaces */
> > efi_status_t EFIAPI
> > efi_install_multiple_protocol_interfaces(efi_handle_t *handle, ...);
> > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > index 1951291747cd..2c86d78208b2 100644
> > --- a/lib/efi_loader/efi_boottime.c
> > +++ b/lib/efi_loader/efi_boottime.c
> > @@ -103,12 +103,6 @@ static efi_status_t EFIAPI efi_disconnect_controller(
> > efi_handle_t driver_image_handle,
> > efi_handle_t child_handle);
> >
> > -static
> > -efi_status_t EFIAPI efi_connect_controller(efi_handle_t controller_handle,
> > - efi_handle_t
> > *driver_image_handle,
> > - struct efi_device_path
> > *remain_device_path,
> > - bool recursive);
> > -
> > /* Called on every callback entry */
> > int __efi_entry_check(void)
> > {
> > @@ -3670,11 +3664,10 @@ static efi_status_t efi_connect_single_controller(
> > *
> > * Return: status code
> > */
> > -static efi_status_t EFIAPI efi_connect_controller(
> > - efi_handle_t controller_handle,
> > - efi_handle_t *driver_image_handle,
> > - struct efi_device_path *remain_device_path,
> > - bool recursive)
> > +efi_status_t EFIAPI efi_connect_controller(efi_handle_t controller_handle,
> > + efi_handle_t
> > *driver_image_handle,
> > + struct efi_device_path
> > *remain_device_path,
> > + bool recursive)
> > {
> > efi_status_t r;
> > efi_status_t ret = EFI_NOT_FOUND;
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > index de0d49ebebda..919e3cba071b 100644
> > --- a/lib/efi_loader/efi_capsule.c
> > +++ b/lib/efi_loader/efi_capsule.c
> > @@ -922,6 +922,105 @@ static bool device_is_present_and_system_part(struct
> > efi_device_path *dp)
> > retu
Re: [PATCH] efi_loader: capsule: detect possible ESP device path
Hi Dan,
Thanks for the suggestion, I'll fix them in the next patch.
BR,
Weizhao
On Wed, May 8, 2024 at 8:04 PM Dan Carpenter wrote:
>
> On Wed, May 08, 2024 at 07:24:01PM +0800, Weizhao Ouyang wrote:
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > index de0d49ebebda..919e3cba071b 100644
> > --- a/lib/efi_loader/efi_capsule.c
> > +++ b/lib/efi_loader/efi_capsule.c
> > @@ -922,6 +922,105 @@ static bool device_is_present_and_system_part(struct
> > efi_device_path *dp)
> > return true;
> > }
> >
> > +/**
> > + * get_esp_from_boot_option_file_path - get the expand device path
> > + *
> > + * Get a possible efi system partition by expanding a boot option
> > + * file path.
> > + *
> > + * @boot_dev The device path pointing to a boot option
> > + * Return: The full ESP device path or NULL if fail
> > + */
> > +static struct efi_device_path *get_esp_from_boot_option_file_path(struct
> > efi_device_path *boot_dev)
> > +{
> > + efi_status_t ret = EFI_SUCCESS;
> > + efi_handle_t handle;
> > + struct efi_device_path *dp = boot_dev;
> > + struct efi_device_path *full_path = NULL;
> > +
> > + ret =
> > EFI_CALL(efi_locate_device_path(_simple_file_system_protocol_guid,
> > + ,
> > + ));
> > + if (ret != EFI_SUCCESS)
> > + ret = EFI_CALL(efi_locate_device_path(_block_io_guid,
> > , ));
> > +
> > + /* Expand Media Device Path */
> > + if (ret == EFI_SUCCESS && EFI_DP_TYPE(dp, END, END)) {
>
> Flip this around. Always do failure handling. Never do success
> handling. full_path is always NULL. Just return that. Delete
> the variable.
>
> if (ret != EFI_SUCCESS ||
> !EFI_DP_TYPE(dp, END, END))
> return NULL;
>
>
> > + struct efi_device_path *temp_dp;
> > + struct efi_block_io *block_io;
> > + void *buffer;
> > + efi_handle_t *simple_file_system_handle;
> > + efi_uintn_t number_handles, index;
> > + u32 size;
> > + u32 temp_size;
> > +
> > + temp_dp = boot_dev;
> > + ret =
> > EFI_CALL(efi_locate_device_path(_simple_file_system_protocol_guid,
> > + _dp,
> > + ));
> > + /*
> > + * For device path pointing to simple file system, it only
> > expands to one
> > + * full path
> > + */
> > + if (ret == EFI_SUCCESS && EFI_DP_TYPE(temp_dp, END, END)) {
>
> "Always" was hyperbole. This success handling is fine.
>
> > + if (device_is_present_and_system_part(temp_dp))
> > + return temp_dp;
> > + }
> > +
> > + /*
> > + * For device path only pointing to the removable device
> > handle, try to
> > + * search all its children handles
> > + */
> > + ret = EFI_CALL(efi_locate_device_path(_block_io_guid,
> > _dp, ));
>
> ret is not used. Delete.
>
> > + EFI_CALL(efi_connect_controller(handle, NULL, NULL, true));
> > +
> > + /* Align with edk2, issue a dummy read to the device to check
> > the device change */
> > + ret = EFI_CALL(efi_handle_protocol(handle,
> > _block_io_guid, (void **)_io));
> > + if (ret == EFI_SUCCESS) {
>
> if (ret != EFI_SUCCESS)
> return NULL;
>
> > + buffer = memalign(block_io->media->io_align,
> > block_io->media->block_size);
> > + if (buffer) {
>
> if (!buffer)
> return NULL;
>
>
> > + ret = EFI_CALL(block_io->read_blocks(block_io,
> > +
> > block_io->media->media_id,
> > + 0,
> > +
> > block_io->media->block_size,
> > + buffer));
>
> Delete the unused "ret = " assignment.
>
> > +
Re: [PATCH] efi_loader: capsule: detect possible ESP device path
On Wed, May 8, 2024 at 7:52 PM Heinrich Schuchardt wrote:
>
> On 5/8/24 13:24, Weizhao Ouyang wrote:
> > When using CapsuleApp to execute on-disk update, it will choose the
> > first boot option as BootNext entry to perform the capsule update after
> > a reboot. But auto-generate boot option will ignore the logical
> > partition which might be an ESP, thus it could not find the capsule
> > file.
> >
> > Align with the EDK II, detect the possible ESP device path by expanding
> > the media path.
>
> Hello Wheizhoa,
>
> From the description I would not know how to reproduce the problem you
> face.
>
> Could you, please, provide an example (including disk image and capsule)
> for QEMU and describe expected and observed behavior.
>
> We should add a CI test case covering the observed bug.
Hi Heinrich,
Here is a brief description, I'll add a testcase in the next patch.
background:
- there is an ESP in an mmc logical partition
- in the edk2 shell payload, running "CapsuleApp.efi -OD" to perform
the capsule on-disk update
workflow:
- currently, u-boot will auto-generate boot option and install with
eliminating logical partitions. So we will have a boot variable
Boot that its file_path seems like: "/VenHw/eMMC(0)/eMMC(0)"
- then we run the efi app in the edk shell payload. The CapsuleApp
will find this boot entry, and it will detect that there is an ESP
in this mmc device. According to the on-disk update flow, it will
copy the capsule to the ESP, and add the Boot to the BootNext.
- After CapsuleApp.efi reset the device, u-boot will check
OsIndications and perform the on-disk update flow. Then it will
search for the capsule file from the boot entry indicated by the
BootNext. Unfortunately, its file_path is "/VenHw/eMMC(0)/eMMC(0)",
so the search will fail.
- Using this patch, u-boot will expand the Boot file path to
detect its ESP, so the capsule file will be found in the expanded
device path, eg: "/VenHw/eMMC(0)/eMMC(0)/HD(2)".
BR,
Weizhao
>
> >
> > Fixes: f86fba8adbf3 ("efi_loader: auto-generate boot option for each blkio
> > device")
> > Signed-off-by: Weizhao Ouyang
> > ---
> > include/efi_loader.h | 6 ++
> > lib/efi_loader/efi_boottime.c | 15 ++---
> > lib/efi_loader/efi_capsule.c | 110 +-
> > 3 files changed, 118 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 9600941aa327..9d78fa936701 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -683,6 +683,12 @@ efi_status_t efi_protocol_open(struct efi_handler
> > *handler,
> > void **protocol_interface, void *agent_handle,
> > void *controller_handle, uint32_t attributes);
> >
> > +/* Connect drivers to controller */
> > +efi_status_t EFIAPI efi_connect_controller(efi_handle_t controller_handle,
> > +efi_handle_t *driver_image_handle,
> > +struct efi_device_path
> > *remain_device_path,
> > +bool recursive);
> > +
> > /* Install multiple protocol interfaces */
> > efi_status_t EFIAPI
> > efi_install_multiple_protocol_interfaces(efi_handle_t *handle, ...);
> > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > index 1951291747cd..2c86d78208b2 100644
> > --- a/lib/efi_loader/efi_boottime.c
> > +++ b/lib/efi_loader/efi_boottime.c
> > @@ -103,12 +103,6 @@ static efi_status_t EFIAPI efi_disconnect_controller(
> > efi_handle_t driver_image_handle,
> > efi_handle_t child_handle);
> >
> > -static
> > -efi_status_t EFIAPI efi_connect_controller(efi_handle_t controller_handle,
> > -efi_handle_t *driver_image_handle,
> > -struct efi_device_path
> > *remain_device_path,
> > -bool recursive);
> > -
> > /* Called on every callback entry */
> > int __efi_entry_check(void)
> > {
> > @@ -3670,11 +3664,10 @@ static efi_status_t efi_connect_single_controller(
> >*
> >* Return: status code
> >*/
> > -static efi_status_t EFIAPI efi_connect_controller(
> > - efi_handle_t controller_handle,
> > - efi_handle_t *driver_image_handle,
> > - struct efi_device_path *remain_device_path,
> > -
[PATCH] efi_loader: capsule: detect possible ESP device path
When using CapsuleApp to execute on-disk update, it will choose the
first boot option as BootNext entry to perform the capsule update after
a reboot. But auto-generate boot option will ignore the logical
partition which might be an ESP, thus it could not find the capsule
file.
Align with the EDK II, detect the possible ESP device path by expanding
the media path.
Fixes: f86fba8adbf3 ("efi_loader: auto-generate boot option for each blkio
device")
Signed-off-by: Weizhao Ouyang
---
include/efi_loader.h | 6 ++
lib/efi_loader/efi_boottime.c | 15 ++---
lib/efi_loader/efi_capsule.c | 110 +-
3 files changed, 118 insertions(+), 13 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 9600941aa327..9d78fa936701 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -683,6 +683,12 @@ efi_status_t efi_protocol_open(struct efi_handler *handler,
void **protocol_interface, void *agent_handle,
void *controller_handle, uint32_t attributes);
+/* Connect drivers to controller */
+efi_status_t EFIAPI efi_connect_controller(efi_handle_t controller_handle,
+ efi_handle_t *driver_image_handle,
+ struct efi_device_path
*remain_device_path,
+ bool recursive);
+
/* Install multiple protocol interfaces */
efi_status_t EFIAPI
efi_install_multiple_protocol_interfaces(efi_handle_t *handle, ...);
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 1951291747cd..2c86d78208b2 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -103,12 +103,6 @@ static efi_status_t EFIAPI efi_disconnect_controller(
efi_handle_t driver_image_handle,
efi_handle_t child_handle);
-static
-efi_status_t EFIAPI efi_connect_controller(efi_handle_t controller_handle,
- efi_handle_t *driver_image_handle,
- struct efi_device_path
*remain_device_path,
- bool recursive);
-
/* Called on every callback entry */
int __efi_entry_check(void)
{
@@ -3670,11 +3664,10 @@ static efi_status_t efi_connect_single_controller(
*
* Return: status code
*/
-static efi_status_t EFIAPI efi_connect_controller(
- efi_handle_t controller_handle,
- efi_handle_t *driver_image_handle,
- struct efi_device_path *remain_device_path,
- bool recursive)
+efi_status_t EFIAPI efi_connect_controller(efi_handle_t controller_handle,
+ efi_handle_t *driver_image_handle,
+ struct efi_device_path
*remain_device_path,
+ bool recursive)
{
efi_status_t r;
efi_status_t ret = EFI_NOT_FOUND;
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index de0d49ebebda..919e3cba071b 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -922,6 +922,105 @@ static bool device_is_present_and_system_part(struct
efi_device_path *dp)
return true;
}
+/**
+ * get_esp_from_boot_option_file_path - get the expand device path
+ *
+ * Get a possible efi system partition by expanding a boot option
+ * file path.
+ *
+ * @boot_dev The device path pointing to a boot option
+ * Return: The full ESP device path or NULL if fail
+ */
+static struct efi_device_path *get_esp_from_boot_option_file_path(struct
efi_device_path *boot_dev)
+{
+ efi_status_t ret = EFI_SUCCESS;
+ efi_handle_t handle;
+ struct efi_device_path *dp = boot_dev;
+ struct efi_device_path *full_path = NULL;
+
+ ret =
EFI_CALL(efi_locate_device_path(_simple_file_system_protocol_guid,
+ ,
+ ));
+ if (ret != EFI_SUCCESS)
+ ret = EFI_CALL(efi_locate_device_path(_block_io_guid, ,
));
+
+ /* Expand Media Device Path */
+ if (ret == EFI_SUCCESS && EFI_DP_TYPE(dp, END, END)) {
+ struct efi_device_path *temp_dp;
+ struct efi_block_io *block_io;
+ void *buffer;
+ efi_handle_t *simple_file_system_handle;
+ efi_uintn_t number_handles, index;
+ u32 size;
+ u32 temp_size;
+
+ temp_dp = boot_dev;
+ ret =
EFI_CALL(efi_locate_device_path(_simple_file_system_protocol_guid,
+ _dp,
+ ));
+ /*
+* For device path pointing to simple file system
[PATCH] arm: rockchip: using generic capsule update mechanism
Currently Rockchip's capsule update mechanism only accepts capsules in
form of a mmc partition, but a generic capsule update mechanism should
be used to satisfy the universal requirements.
Signed-off-by: Weizhao Ouyang
---
arch/arm/mach-rockchip/board.c | 153 --
board/radxa/rockpi4-rk3399/rockpi4-rk3399.c | 166 +++-
2 files changed, 158 insertions(+), 161 deletions(-)
diff --git a/arch/arm/mach-rockchip/board.c b/arch/arm/mach-rockchip/board.c
index cd226844b6..73fbb3f58c 100644
--- a/arch/arm/mach-rockchip/board.c
+++ b/arch/arm/mach-rockchip/board.c
@@ -35,155 +35,6 @@
#include
#include
-#if IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) &&
IS_ENABLED(CONFIG_EFI_PARTITION)
-
-#define DFU_ALT_BUF_LENSZ_1K
-
-static struct efi_fw_image *fw_images;
-
-static bool updatable_image(struct disk_partition *info)
-{
- int i;
- bool ret = false;
- efi_guid_t image_type_guid;
-
- uuid_str_to_bin(info->type_guid, image_type_guid.b,
- UUID_STR_FORMAT_GUID);
-
- for (i = 0; i < update_info.num_images; i++) {
- if (!guidcmp(_images[i].image_type_id, _type_guid)) {
- ret = true;
- break;
- }
- }
-
- return ret;
-}
-
-static void set_image_index(struct disk_partition *info, int index)
-{
- int i;
- efi_guid_t image_type_guid;
-
- uuid_str_to_bin(info->type_guid, image_type_guid.b,
- UUID_STR_FORMAT_GUID);
-
- for (i = 0; i < update_info.num_images; i++) {
- if (!guidcmp(_images[i].image_type_id, _type_guid)) {
- fw_images[i].image_index = index;
- break;
- }
- }
-}
-
-static int get_mmc_desc(struct blk_desc **desc)
-{
- int ret;
- struct mmc *mmc;
- struct udevice *dev;
-
- /*
-* For now the firmware images are assumed to
-* be on the SD card
-*/
- ret = uclass_get_device(UCLASS_MMC, 1, );
- if (ret)
- return -1;
-
- mmc = mmc_get_mmc_dev(dev);
- if (!mmc)
- return -ENODEV;
-
- if ((ret = mmc_init(mmc)))
- return ret;
-
- *desc = mmc_get_blk_desc(mmc);
- if (!*desc)
- return -1;
-
- return 0;
-}
-
-void set_dfu_alt_info(char *interface, char *devstr)
-{
- const char *name;
- bool first = true;
- int p, len, devnum, ret;
- char buf[DFU_ALT_BUF_LEN];
- struct disk_partition info;
- struct blk_desc *desc = NULL;
-
- ret = get_mmc_desc();
- if (ret) {
- log_err("Unable to get mmc desc\n");
- return;
- }
-
- memset(buf, 0, sizeof(buf));
- name = blk_get_uclass_name(desc->uclass_id);
- devnum = desc->devnum;
- len = strlen(buf);
-
- len += snprintf(buf + len, DFU_ALT_BUF_LEN - len,
-"%s %d=", name, devnum);
-
- for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
- if (part_get_info(desc, p, ))
- continue;
-
- /* Add entry to dfu_alt_info only for updatable images */
- if (updatable_image()) {
- if (!first)
- len += snprintf(buf + len,
- DFU_ALT_BUF_LEN - len, ";");
-
- len += snprintf(buf + len, DFU_ALT_BUF_LEN - len,
- "%s%d_%s part %d %d",
- name, devnum, info.name, devnum, p);
- first = false;
- }
- }
-
- log_debug("dfu_alt_info => %s\n", buf);
- env_set("dfu_alt_info", buf);
-}
-
-__weak void rockchip_capsule_update_board_setup(void)
-{
-}
-
-static void gpt_capsule_update_setup(void)
-{
- int p, i, ret;
- struct disk_partition info;
- struct blk_desc *desc = NULL;
-
- fw_images = update_info.images;
- rockchip_capsule_update_board_setup();
-
- ret = get_mmc_desc();
- if (ret) {
- log_err("Unable to get mmc desc\n");
- return;
- }
-
- for (p = 1, i = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
- if (part_get_info(desc, p, ))
- continue;
-
- /*
-* Since we have a GPT partitioned device, the updatable
-* images could be stored in any order. Populate the
-* image_index at runtime.
-*/
- if (updatable_image()) {
- set_image_index(, i);
- i++;
- }
- }
-}
-#endif /* CONFIG_EFI_HAVE_CAPSULE_SUPPORT && CONFIG_EFI_PART
[PATCH v2] efi_loader: Fix EFI_VARIABLE_APPEND_WRITE hash check
According to UEFI v2.10 spec section 8.2.6, if a caller invokes the
SetVariables() service, it will produce a digest from hash(VariableName,
VendorGuid, Attributes, TimeStamp, DataNew_variable_content), then the
firmware that implements the SetVariable() service will compare the
digest with the result of applying the signer’s public key to the
signature. For EFI variable append write, efitools sign-efi-sig-list has
an option "-a" to add EFI_VARIABLE_APPEND_WRITE attr, and u-boot will
drop this attribute in efi_set_variable_int(). So if a caller uses
"sign-efi-sig-list -a" to create the authenticated variable, this append
write will fail in the u-boot due to "hash check failed".
This patch resumes writing the EFI_VARIABLE_APPEND_WRITE attr to ensure
that the hash check is correct. And also update the "test_efi_secboot"
test case to compliance with the change.
Signed-off-by: Weizhao Ouyang
---
v2: skip attr for efi_var_mem_ins()
---
lib/efi_loader/efi_variable.c | 6 +++---
test/py/tests/test_efi_secboot/conftest.py | 10 ++
test/py/tests/test_efi_secboot/test_authvar.py | 4 ++--
test/py/tests/test_efi_secboot/test_signed.py | 10 +-
4 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index 1cc02acb3b..09651d4675 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -288,7 +288,6 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
/* check if a variable exists */
var = efi_var_mem_find(vendor, variable_name, NULL);
append = !!(attributes & EFI_VARIABLE_APPEND_WRITE);
- attributes &= ~EFI_VARIABLE_APPEND_WRITE;
delete = !append && (!data_size || !attributes);
/* check attributes */
@@ -304,7 +303,7 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
/* attributes won't be changed */
if (!delete &&
- ((ro_check && var->attr != attributes) ||
+ ((ro_check && var->attr != (attributes &
~EFI_VARIABLE_APPEND_WRITE)) ||
(!ro_check && ((var->attr & ~EFI_VARIABLE_READ_ONLY)
!= (attributes &
~EFI_VARIABLE_READ_ONLY) {
return EFI_INVALID_PARAMETER;
@@ -378,7 +377,8 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
for (; *old_data; ++old_data)
;
++old_data;
- ret = efi_var_mem_ins(variable_name, vendor, attributes,
+ ret = efi_var_mem_ins(variable_name, vendor,
+ attributes & ~EFI_VARIABLE_APPEND_WRITE,
var->length, old_data, data_size, data,
time);
} else {
diff --git a/test/py/tests/test_efi_secboot/conftest.py
b/test/py/tests/test_efi_secboot/conftest.py
index ff7ac7c810..0fa0747fc7 100644
--- a/test/py/tests/test_efi_secboot/conftest.py
+++ b/test/py/tests/test_efi_secboot/conftest.py
@@ -64,6 +64,12 @@ def efi_boot_env(request, u_boot_config):
check_call('cd %s; %scert-to-efi-sig-list -g %s db1.crt db1.esl;
%ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key db db1.esl db1.auth'
% (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
shell=True)
+# db2 (APPEND_WRITE)
+check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj
/CN=TEST_db2/ -keyout db2.key -out db2.crt -nodes -days 365'
+ % mnt_point, shell=True)
+check_call('cd %s; %scert-to-efi-sig-list -g %s db2.crt db2.esl;
%ssign-efi-sig-list -a -c KEK.crt -k KEK.key db db2.esl db2.auth'
+ % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
+ shell=True)
# dbx (TEST_dbx certificate)
check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj
/CN=TEST_dbx/ -keyout dbx.key -out dbx.crt -nodes -days 365'
% mnt_point, shell=True)
@@ -84,6 +90,10 @@ def efi_boot_env(request, u_boot_config):
check_call('cd %s; %scert-to-efi-hash-list -g %s -s 256 db1.crt
dbx_hash1.crl; %ssign-efi-sig-list -t "2020-04-06" -c KEK.crt -k KEK.key dbx
dbx_hash1.crl dbx_hash1.auth'
% (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
shell=True)
+# dbx_hash2 (digest of TEST_db2 certificate, with APPEND_WRITE)
+check_call('cd %s; %scert-to-efi-hash-list -g %s -s 256 db2.crt
dbx_hash2.crl; %ssign-efi-sig-list -a -c KEK.crt -k KEK.key dbx dbx_hash2.crl
dbx_hash2.auth'
+ % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
+ shell=True)
# dbx_db (with TEST_db certificate)
Re: [RFC PATCH] efi_loader: Fix EFI_VARIABLE_APPEND_WRITE hash check
On Wed, Apr 10, 2024 at 8:09 PM Heinrich Schuchardt wrote: > > On 10.04.24 13:53, Weizhao Ouyang wrote: > > On Thu, Apr 4, 2024 at 1:48 AM Weizhao Ouyang wrote: > >> > >> Hi Heinrich, > >> > >> On Wed, Apr 3, 2024 at 10:54 PM Heinrich Schuchardt > >> wrote: > >>> > >>> On 27.03.24 15:06, Weizhao Ouyang wrote: > >>>> According to UEFI v2.10 spec section 8.2.6, if a caller invokes the > >>>> SetVariables() service, it will produce a digest from hash(VariableName, > >>>> VendorGuid, Attributes, TimeStamp, DataNew_variable_content), then the > >>>> firmware that implements the SetVariable() service will compare the > >>>> digest with the result of applying the signer’s public key to the > >>>> signature. For EFI variable append write, efitools sign-efi-sig-list has > >>>> an option "-a" to add EFI_VARIABLE_APPEND_WRITE attr, and u-boot will > >>>> drop this attribute in efi_set_variable_int(). So if a caller uses > >>>> "sign-efi-sig-list -a" to create the authenticated variable, this append > >>>> write will fail in the u-boot due to "hash check failed". > >>> > >>> Currently U-Boot does not allow to use EFI_VARIABLE_APPEND_WRITE for any > >>> non-existent variable, signed or not. This does not match the EDK II > >>> behavior. > >> > >> I'm not referring to the non-existent variable situation, it's a normal > >> existent variable update. > >> > >> I mean: > >> 1. process PK > >> 2. process KEK > >> 3. process a new APPEND_WRITE KEK (came from "sign-efi-sig-list -a ...", > >> so the digest and attrtibute of the KEK auth file will contain > >> APPEND_WRITE) > >> Then the hash check will fail. > >> > >>> > >>> There is a patch queued for this issue: > >>> > >>> [PATCH v2] efi_loader: fix append write behavior to non-existent variable > >>> https://lore.kernel.org/u-boot/20240402090950.3819705-1-kojima.masah...@socionext.com/ > >>> > >>> Could you, please, retest with that patch. > >>> > >>>> > >>>> This patch resumes writing the EFI_VARIABLE_APPEND_WRITE attr to ensure > >>>> that the hash check is correct. And also update the "test_efi_secboot" > >>>> test case to compliance with the change. > >>>> > >>>> Signed-off-by: Weizhao Ouyang > >>>> --- > >>>>lib/efi_loader/efi_variable.c | 3 +-- > >>>>test/py/tests/test_efi_secboot/conftest.py | 10 ++ > >>>>test/py/tests/test_efi_secboot/test_authvar.py | 4 ++-- > >>>>test/py/tests/test_efi_secboot/test_signed.py | 10 +- > >>>>4 files changed, 18 insertions(+), 9 deletions(-) > >>>> > >>>> diff --git a/lib/efi_loader/efi_variable.c > >>>> b/lib/efi_loader/efi_variable.c > >>>> index 40f7a0fb10..f41aa8f9ad 100644 > >>>> --- a/lib/efi_loader/efi_variable.c > >>>> +++ b/lib/efi_loader/efi_variable.c > >>>> @@ -259,7 +259,6 @@ efi_status_t efi_set_variable_int(const u16 > >>>> *variable_name, > >>>>/* check if a variable exists */ > >>>>var = efi_var_mem_find(vendor, variable_name, NULL); > >>>>append = !!(attributes & EFI_VARIABLE_APPEND_WRITE); > >>>> - attributes &= ~(u32)EFI_VARIABLE_APPEND_WRITE; > >>> > >>> According to the UEFI specification for GetVariable(): > >>> > >>> "The EFI_VARIABLE_APPEND_WRITE attribute will never be set in the > >>> returned Attributes bitmask parameter." > >> > >> Yes, that's why I called it a RFC patch. > >> > >>> > >>> We don't want to set EFI_VARIABLE_APPEND_WRITE in a variable when > >>> calling efi_var_mem_ins(). > >> > >> The APPEND_WRITE attribute is allowed to be set prior to invoking the > >> service (UEFI section 8.2.6). So anyway we should consider compatibility > >> with this case, along with making changes to GetVariable(). > > > > Part of the process in edk2 is: > > > > VerifyTimeBasedPayloadAndUpdate > > --> VerifyTimeBasedPayload // calculate digest and compare digest > > --> AuthServiceInternalU
[PATCH] efi_loader: using EFI_UNSUPPORTED for private authenticated variables
Improve error message for UEFI SCT tests. Signed-off-by: Weizhao Ouyang --- lib/efi_loader/efi_variable.c | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index 2951dc78c7..e6c1219a11 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -163,6 +163,7 @@ static efi_status_t efi_variable_authenticate(const u16 *variable, break; default: /* TODO: support private authenticated variables */ + ret = EFI_UNSUPPORTED; goto err; } -- 2.40.1
Re: [RFC PATCH] efi_loader: Fix EFI_VARIABLE_APPEND_WRITE hash check
On Thu, Apr 4, 2024 at 1:48 AM Weizhao Ouyang wrote: > > Hi Heinrich, > > On Wed, Apr 3, 2024 at 10:54 PM Heinrich Schuchardt > wrote: > > > > On 27.03.24 15:06, Weizhao Ouyang wrote: > > > According to UEFI v2.10 spec section 8.2.6, if a caller invokes the > > > SetVariables() service, it will produce a digest from hash(VariableName, > > > VendorGuid, Attributes, TimeStamp, DataNew_variable_content), then the > > > firmware that implements the SetVariable() service will compare the > > > digest with the result of applying the signer’s public key to the > > > signature. For EFI variable append write, efitools sign-efi-sig-list has > > > an option "-a" to add EFI_VARIABLE_APPEND_WRITE attr, and u-boot will > > > drop this attribute in efi_set_variable_int(). So if a caller uses > > > "sign-efi-sig-list -a" to create the authenticated variable, this append > > > write will fail in the u-boot due to "hash check failed". > > > > Currently U-Boot does not allow to use EFI_VARIABLE_APPEND_WRITE for any > > non-existent variable, signed or not. This does not match the EDK II > > behavior. > > I'm not referring to the non-existent variable situation, it's a normal > existent variable update. > > I mean: > 1. process PK > 2. process KEK > 3. process a new APPEND_WRITE KEK (came from "sign-efi-sig-list -a ...", >so the digest and attrtibute of the KEK auth file will contain >APPEND_WRITE) > Then the hash check will fail. > > > > > There is a patch queued for this issue: > > > > [PATCH v2] efi_loader: fix append write behavior to non-existent variable > > https://lore.kernel.org/u-boot/20240402090950.3819705-1-kojima.masah...@socionext.com/ > > > > Could you, please, retest with that patch. > > > > > > > > This patch resumes writing the EFI_VARIABLE_APPEND_WRITE attr to ensure > > > that the hash check is correct. And also update the "test_efi_secboot" > > > test case to compliance with the change. > > > > > > Signed-off-by: Weizhao Ouyang > > > --- > > > lib/efi_loader/efi_variable.c | 3 +-- > > > test/py/tests/test_efi_secboot/conftest.py | 10 ++ > > > test/py/tests/test_efi_secboot/test_authvar.py | 4 ++-- > > > test/py/tests/test_efi_secboot/test_signed.py | 10 +- > > > 4 files changed, 18 insertions(+), 9 deletions(-) > > > > > > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > > > index 40f7a0fb10..f41aa8f9ad 100644 > > > --- a/lib/efi_loader/efi_variable.c > > > +++ b/lib/efi_loader/efi_variable.c > > > @@ -259,7 +259,6 @@ efi_status_t efi_set_variable_int(const u16 > > > *variable_name, > > > /* check if a variable exists */ > > > var = efi_var_mem_find(vendor, variable_name, NULL); > > > append = !!(attributes & EFI_VARIABLE_APPEND_WRITE); > > > - attributes &= ~(u32)EFI_VARIABLE_APPEND_WRITE; > > > > According to the UEFI specification for GetVariable(): > > > > "The EFI_VARIABLE_APPEND_WRITE attribute will never be set in the > > returned Attributes bitmask parameter." > > Yes, that's why I called it a RFC patch. > > > > > We don't want to set EFI_VARIABLE_APPEND_WRITE in a variable when > > calling efi_var_mem_ins(). > > The APPEND_WRITE attribute is allowed to be set prior to invoking the > service (UEFI section 8.2.6). So anyway we should consider compatibility > with this case, along with making changes to GetVariable(). Part of the process in edk2 is: VerifyTimeBasedPayloadAndUpdate --> VerifyTimeBasedPayload // calculate digest and compare digest --> AuthServiceInternalUpdateVariableWithTimeStamp --> VariableExLibUpdateVariable --> UpdateVariable // unset the APPEND_WRITE We can align with this implementation. BR, Weizhao > > BR, > Weizhao > > > > > > delete = !append && (!data_size || !attributes); > > > > > > /* check attributes */ > > > @@ -275,7 +274,7 @@ efi_status_t efi_set_variable_int(const u16 > > > *variable_name, > > > > > > /* attributes won't be changed */ > > > if (!delete && > > > - ((ro_check && var->attr != attributes) || > > > + ((ro_check && var->attr != (attributes & > > > ~(u32)EFI_VARIABLE_APPEND_WRITE)) || > > >(!ro_c
Re: [RFC PATCH] efi_loader: Fix EFI_VARIABLE_APPEND_WRITE hash check
Hi Heinrich,
On Wed, Apr 3, 2024 at 10:54 PM Heinrich Schuchardt wrote:
>
> On 27.03.24 15:06, Weizhao Ouyang wrote:
> > According to UEFI v2.10 spec section 8.2.6, if a caller invokes the
> > SetVariables() service, it will produce a digest from hash(VariableName,
> > VendorGuid, Attributes, TimeStamp, DataNew_variable_content), then the
> > firmware that implements the SetVariable() service will compare the
> > digest with the result of applying the signer’s public key to the
> > signature. For EFI variable append write, efitools sign-efi-sig-list has
> > an option "-a" to add EFI_VARIABLE_APPEND_WRITE attr, and u-boot will
> > drop this attribute in efi_set_variable_int(). So if a caller uses
> > "sign-efi-sig-list -a" to create the authenticated variable, this append
> > write will fail in the u-boot due to "hash check failed".
>
> Currently U-Boot does not allow to use EFI_VARIABLE_APPEND_WRITE for any
> non-existent variable, signed or not. This does not match the EDK II
> behavior.
I'm not referring to the non-existent variable situation, it's a normal
existent variable update.
I mean:
1. process PK
2. process KEK
3. process a new APPEND_WRITE KEK (came from "sign-efi-sig-list -a ...",
so the digest and attrtibute of the KEK auth file will contain
APPEND_WRITE)
Then the hash check will fail.
>
> There is a patch queued for this issue:
>
> [PATCH v2] efi_loader: fix append write behavior to non-existent variable
> https://lore.kernel.org/u-boot/20240402090950.3819705-1-kojima.masah...@socionext.com/
>
> Could you, please, retest with that patch.
>
> >
> > This patch resumes writing the EFI_VARIABLE_APPEND_WRITE attr to ensure
> > that the hash check is correct. And also update the "test_efi_secboot"
> > test case to compliance with the change.
> >
> > Signed-off-by: Weizhao Ouyang
> > ---
> > lib/efi_loader/efi_variable.c | 3 +--
> > test/py/tests/test_efi_secboot/conftest.py | 10 ++
> > test/py/tests/test_efi_secboot/test_authvar.py | 4 ++--
> > test/py/tests/test_efi_secboot/test_signed.py | 10 +-
> > 4 files changed, 18 insertions(+), 9 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> > index 40f7a0fb10..f41aa8f9ad 100644
> > --- a/lib/efi_loader/efi_variable.c
> > +++ b/lib/efi_loader/efi_variable.c
> > @@ -259,7 +259,6 @@ efi_status_t efi_set_variable_int(const u16
> > *variable_name,
> > /* check if a variable exists */
> > var = efi_var_mem_find(vendor, variable_name, NULL);
> > append = !!(attributes & EFI_VARIABLE_APPEND_WRITE);
> > - attributes &= ~(u32)EFI_VARIABLE_APPEND_WRITE;
>
> According to the UEFI specification for GetVariable():
>
> "The EFI_VARIABLE_APPEND_WRITE attribute will never be set in the
> returned Attributes bitmask parameter."
Yes, that's why I called it a RFC patch.
>
> We don't want to set EFI_VARIABLE_APPEND_WRITE in a variable when
> calling efi_var_mem_ins().
The APPEND_WRITE attribute is allowed to be set prior to invoking the
service (UEFI section 8.2.6). So anyway we should consider compatibility
with this case, along with making changes to GetVariable().
BR,
Weizhao
>
> > delete = !append && (!data_size || !attributes);
> >
> > /* check attributes */
> > @@ -275,7 +274,7 @@ efi_status_t efi_set_variable_int(const u16
> > *variable_name,
> >
> > /* attributes won't be changed */
> > if (!delete &&
> > - ((ro_check && var->attr != attributes) ||
> > + ((ro_check && var->attr != (attributes &
> > ~(u32)EFI_VARIABLE_APPEND_WRITE)) ||
> >(!ro_check && ((var->attr & ~(u32)EFI_VARIABLE_READ_ONLY)
>
> The (u32) conversions are superfluous.
>
> Best regards
>
> Heinrich
>
> > != (attributes &
> > ~(u32)EFI_VARIABLE_READ_ONLY) {
> > return EFI_INVALID_PARAMETER;
> > diff --git a/test/py/tests/test_efi_secboot/conftest.py
> > b/test/py/tests/test_efi_secboot/conftest.py
> > index ff7ac7c810..0fa0747fc7 100644
> > --- a/test/py/tests/test_efi_secboot/conftest.py
> > +++ b/test/py/tests/test_efi_secboot/conftest.py
> > @@ -64,6 +64,12 @@ def efi_boot_env(request, u_boot_config):
> > check_call('cd %s; %scert-to-efi-sig-list -g %s db1.crt db1.esl;
> > %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key db db1.esl
[RFC PATCH] efi_loader: Fix EFI_VARIABLE_APPEND_WRITE hash check
According to UEFI v2.10 spec section 8.2.6, if a caller invokes the
SetVariables() service, it will produce a digest from hash(VariableName,
VendorGuid, Attributes, TimeStamp, DataNew_variable_content), then the
firmware that implements the SetVariable() service will compare the
digest with the result of applying the signer’s public key to the
signature. For EFI variable append write, efitools sign-efi-sig-list has
an option "-a" to add EFI_VARIABLE_APPEND_WRITE attr, and u-boot will
drop this attribute in efi_set_variable_int(). So if a caller uses
"sign-efi-sig-list -a" to create the authenticated variable, this append
write will fail in the u-boot due to "hash check failed".
This patch resumes writing the EFI_VARIABLE_APPEND_WRITE attr to ensure
that the hash check is correct. And also update the "test_efi_secboot"
test case to compliance with the change.
Signed-off-by: Weizhao Ouyang
---
lib/efi_loader/efi_variable.c | 3 +--
test/py/tests/test_efi_secboot/conftest.py | 10 ++
test/py/tests/test_efi_secboot/test_authvar.py | 4 ++--
test/py/tests/test_efi_secboot/test_signed.py | 10 +-
4 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index 40f7a0fb10..f41aa8f9ad 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -259,7 +259,6 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
/* check if a variable exists */
var = efi_var_mem_find(vendor, variable_name, NULL);
append = !!(attributes & EFI_VARIABLE_APPEND_WRITE);
- attributes &= ~(u32)EFI_VARIABLE_APPEND_WRITE;
delete = !append && (!data_size || !attributes);
/* check attributes */
@@ -275,7 +274,7 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
/* attributes won't be changed */
if (!delete &&
- ((ro_check && var->attr != attributes) ||
+ ((ro_check && var->attr != (attributes &
~(u32)EFI_VARIABLE_APPEND_WRITE)) ||
(!ro_check && ((var->attr & ~(u32)EFI_VARIABLE_READ_ONLY)
!= (attributes &
~(u32)EFI_VARIABLE_READ_ONLY) {
return EFI_INVALID_PARAMETER;
diff --git a/test/py/tests/test_efi_secboot/conftest.py
b/test/py/tests/test_efi_secboot/conftest.py
index ff7ac7c810..0fa0747fc7 100644
--- a/test/py/tests/test_efi_secboot/conftest.py
+++ b/test/py/tests/test_efi_secboot/conftest.py
@@ -64,6 +64,12 @@ def efi_boot_env(request, u_boot_config):
check_call('cd %s; %scert-to-efi-sig-list -g %s db1.crt db1.esl;
%ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key db db1.esl db1.auth'
% (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
shell=True)
+# db2 (APPEND_WRITE)
+check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj
/CN=TEST_db2/ -keyout db2.key -out db2.crt -nodes -days 365'
+ % mnt_point, shell=True)
+check_call('cd %s; %scert-to-efi-sig-list -g %s db2.crt db2.esl;
%ssign-efi-sig-list -a -c KEK.crt -k KEK.key db db2.esl db2.auth'
+ % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
+ shell=True)
# dbx (TEST_dbx certificate)
check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj
/CN=TEST_dbx/ -keyout dbx.key -out dbx.crt -nodes -days 365'
% mnt_point, shell=True)
@@ -84,6 +90,10 @@ def efi_boot_env(request, u_boot_config):
check_call('cd %s; %scert-to-efi-hash-list -g %s -s 256 db1.crt
dbx_hash1.crl; %ssign-efi-sig-list -t "2020-04-06" -c KEK.crt -k KEK.key dbx
dbx_hash1.crl dbx_hash1.auth'
% (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
shell=True)
+# dbx_hash2 (digest of TEST_db2 certificate, with APPEND_WRITE)
+check_call('cd %s; %scert-to-efi-hash-list -g %s -s 256 db2.crt
dbx_hash2.crl; %ssign-efi-sig-list -a -c KEK.crt -k KEK.key dbx dbx_hash2.crl
dbx_hash2.auth'
+ % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
+ shell=True)
# dbx_db (with TEST_db certificate)
check_call('cd %s; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k
KEK.key dbx db.esl dbx_db.auth'
% (mnt_point, EFITOOLS_PATH),
diff --git a/test/py/tests/test_efi_secboot/test_authvar.py
b/test/py/tests/test_efi_secboot/test_authvar.py
index f99b8270a6..d5aeb65048 100644
--- a/test/py/tests/test_efi_secboot/test_authvar.py
+++ b/test/py/tests/test_efi_secboot/test_authvar.py
@@ -183,7 +183,7 @@ class TestEfiAuthVar(object):
assert 'db:' in ''.join(output)
output = u_boot_console.run_command_list([
Re: [PATCH v2] board: rockchip: add Rockchip Toybrick TB-RK3588X board
On Fri, Mar 1, 2024 at 4:50 PM Elon Zhang wrote:
>
> TB-RK3588X board is a Rockchip Toybrick RK3588 based development board.
>
> Specification:
> Rockchip Rk3588 SoC
> 4x ARM Cortex-A76, 4x ARM Cortex-A55
> 8/16GB Memory LPDDR4x
> Mali G610MC4 GPU
> 2× MIPI-CSI0 Connector
> 1x 2Lanes PCIe3.0 Connector
> 1x SATA3.0 Connector
> 32GB eMMC Module
> 2x USB 2.0, 2x USB 3.0
> 1x HDMI Output, 1x HDMI Input
> 2x Ethernet Port
>
> Functions work normally:
> [1] USB2.0 Host
> [2] Ethernet0 with PHY RTL8211F
>
> More information can be obtained from the following websites:
> [1] https://t.rock-chips.com/en/wiki/EN/tb-rk3588x_en/index.html
> [2] http://t.rock-chips.com/
>
> Kernel commits:
> 8ffe365f8dc7 ("arm64: dts: rockchip: Add devicetree support for
> TB-RK3588X board")
> 7140387ff49d ("dt-bindings: arm: rockchip: Add Toybrick TB-RK3588X")
>
> Signed-off-by: Elon Zhang
Reviewed-by: Weizhao Ouyang
BR,
Weizhao
> ---
> Changes since v1:
> - Remove BOARD_SPECIFIC_OPTIONS in board Kconfig
> ---
> arch/arm/dts/rk3588-toybrick-x0-u-boot.dtsi | 12 +
> arch/arm/dts/rk3588-toybrick-x0.dts | 672 ++
> arch/arm/mach-rockchip/rk3588/Kconfig | 25 +
> board/rockchip/toybrick_rk3588/Kconfig| 12 +
> board/rockchip/toybrick_rk3588/MAINTAINERS| 8 +
> board/rockchip/toybrick_rk3588/Makefile | 6 +
> .../toybrick_rk3588/toybrick-rk3588.c | 39 +
> configs/toybrick-rk3588_defconfig | 82 +++
> doc/board/rockchip/rockchip.rst | 1 +
> include/configs/toybrick_rk3588.h | 15 +
> 10 files changed, 872 insertions(+)
> create mode 100644 arch/arm/dts/rk3588-toybrick-x0-u-boot.dtsi
> create mode 100644 arch/arm/dts/rk3588-toybrick-x0.dts
> create mode 100644 board/rockchip/toybrick_rk3588/Kconfig
> create mode 100644 board/rockchip/toybrick_rk3588/MAINTAINERS
> create mode 100644 board/rockchip/toybrick_rk3588/Makefile
> create mode 100644 board/rockchip/toybrick_rk3588/toybrick-rk3588.c
> create mode 100644 configs/toybrick-rk3588_defconfig
> create mode 100644 include/configs/toybrick_rk3588.h
>
> diff --git a/arch/arm/dts/rk3588-toybrick-x0-u-boot.dtsi
> b/arch/arm/dts/rk3588-toybrick-x0-u-boot.dtsi
> new file mode 100644
> index 00..1aeb5410e4
> --- /dev/null
> +++ b/arch/arm/dts/rk3588-toybrick-x0-u-boot.dtsi
> @@ -0,0 +1,12 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2024 Rockchip Electronics Co., Ltd.
> + */
> +
> +#include "rk3588-u-boot.dtsi"
> +
> +/ {
> + chosen {
> + u-boot,spl-boot-order = "same-as-spl",
> + };
> +};
> diff --git a/arch/arm/dts/rk3588-toybrick-x0.dts
> b/arch/arm/dts/rk3588-toybrick-x0.dts
> new file mode 100644
> index 00..f3f7c1d35e
> --- /dev/null
> +++ b/arch/arm/dts/rk3588-toybrick-x0.dts
> @@ -0,0 +1,672 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2024 Rockchip Electronics Co., Ltd.
> + *
> + */
> +
> +/dts-v1/;
> +
> +#include
> +#include
> +#include
> +#include "rk3588.dtsi"
> +
> +/ {
> + model = "Rockchip Toybrick TB-RK3588X Board";
> + compatible = "rockchip,rk3588-toybrick-x0", "rockchip,rk3588";
> +
> + aliases {
> + mmc0 =
> + serial2 =
> + };
> +
> + chosen {
> + stdout-path = "serial2:150n8";
> + };
> +
> + adc-keys {
> + compatible = "adc-keys";
> + io-channels = < 1>;
> + io-channel-names = "buttons";
> + keyup-threshold-microvolt = <180>;
> + poll-interval = <100>;
> +
> + button-vol-up {
> + label = "Volume Up";
> + linux,code = ;
> + press-threshold-microvolt = <17000>;
> + };
> +
> + button-vol-down {
> + label = "Volume Down";
> + linux,code = ;
> + press-threshold-microvolt = <417000>;
> + };
> +
> + button-menu {
> + label = "Menu";
> + linux,code = ;
> + press-threshold-microvolt = <89>;
> +
Re: [PATCH] cmd: sf: Fix sf probe crash
Gentle ping. Not merged yet.
BR,
Weizhao
On Thu, Jan 4, 2024 at 7:46 PM Weizhao Ouyang wrote:
>
> Handle the return value of spi_flash_probe_bus_cs() to avoid sf probe
> crashes.
>
> Signed-off-by: Weizhao Ouyang
> ---
> cmd/sf.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/cmd/sf.c b/cmd/sf.c
> index 730996c02b..e3866899f6 100644
> --- a/cmd/sf.c
> +++ b/cmd/sf.c
> @@ -135,8 +135,9 @@ static int do_spi_flash_probe(int argc, char *const
> argv[])
> }
> flash = NULL;
> if (use_dt) {
> - spi_flash_probe_bus_cs(bus, cs, );
> - flash = dev_get_uclass_priv(new);
> + ret = spi_flash_probe_bus_cs(bus, cs, );
> + if (!ret)
> + flash = dev_get_uclass_priv(new);
> } else {
> flash = spi_flash_probe(bus, cs, speed, mode);
> }
> --
> 2.39.2
>
[PATCH v4 3/3] cmd: rng: Add rng list command
The 'rng list' command probes all RNG devices and list those devices
that are successfully probed. Also update the help info.
Reviewed-by: Heinrich Schuchardt
Signed-off-by: Weizhao Ouyang
---
v4: update doc/usage/cmd/rng.rst
---
cmd/rng.c | 23 ++-
doc/usage/cmd/rng.rst | 14 ++
2 files changed, 28 insertions(+), 9 deletions(-)
diff --git a/cmd/rng.c b/cmd/rng.c
index 52f722c7af..b073a6c849 100644
--- a/cmd/rng.c
+++ b/cmd/rng.c
@@ -19,6 +19,22 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc,
char *const argv[])
struct udevice *dev;
int ret = CMD_RET_SUCCESS;
+ if (argc == 2 && !strcmp(argv[1], "list")) {
+ int idx = 0;
+
+ uclass_foreach_dev_probe(UCLASS_RNG, dev) {
+ idx++;
+ printf("RNG #%d - %s\n", dev->seq_, dev->name);
+ }
+
+ if (!idx) {
+ log_err("No RNG device\n");
+ return CMD_RET_FAILURE;
+ }
+
+ return CMD_RET_SUCCESS;
+ }
+
switch (argc) {
case 1:
devnum = 0;
@@ -56,12 +72,9 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc,
char *const argv[])
return ret;
}
-U_BOOT_LONGHELP(rng,
- "[dev [n]]\n"
- " - print n random bytes(max 64) read from dev\n");
-
U_BOOT_CMD(
rng, 3, 0, do_rng,
"print bytes from the hardware random number generator",
- rng_help_text
+ "list - list all the probed rng devices\n"
+ "rng [dev] [n]- print n random bytes(max 64) read from dev\n"
);
diff --git a/doc/usage/cmd/rng.rst b/doc/usage/cmd/rng.rst
index 274e4d88df..4a61e33d27 100644
--- a/doc/usage/cmd/rng.rst
+++ b/doc/usage/cmd/rng.rst
@@ -11,16 +11,22 @@ Synopsis
::
-rng [devnum [n]]
+rng list
+rng [dev] [n]
-Description
+rng list
+
+
+List all the probed rng devices.
+
+rng [dev] [n]
+-
The *rng* command reads the random number generator(RNG) device and
prints the random bytes read on the console. A maximum of 64 bytes can
be read in one invocation of the command.
-devnum
+dev
The RNG device from which the random bytes are to be
read. Defaults to 0.
--
2.40.1
[PATCH v4 2/3] driver: rng: Fix SMCCC TRNG crash
Fix a SMCCC TRNG null pointer crash due to a failed smccc feature
binding.
Fixes: 53355bb86c25 ("drivers: rng: add smccc trng driver")
Reviewed-by: Heinrich Schuchardt
Signed-off-by: Weizhao Ouyang
---
v3: add Fixes tag
---
drivers/rng/smccc_trng.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/rng/smccc_trng.c b/drivers/rng/smccc_trng.c
index 3a4bb33941..3087cb991a 100644
--- a/drivers/rng/smccc_trng.c
+++ b/drivers/rng/smccc_trng.c
@@ -166,7 +166,7 @@ static int smccc_trng_probe(struct udevice *dev)
struct smccc_trng_priv *priv = dev_get_priv(dev);
struct arm_smccc_res res;
- if (!(smccc_trng_is_supported(smccc->invoke_fn)))
+ if (!smccc || !(smccc_trng_is_supported(smccc->invoke_fn)))
return -ENODEV;
/* At least one of 64bit and 32bit interfaces is available */
--
2.40.1
[PATCH v4 1/3] firmware: psci: Fix bind_smccc_features psci check
According to PSCI specification DEN0022F, PSCI_FEATURES is used to check whether the SMCCC is implemented by discovering SMCCC_VERSION. Signed-off-by: Weizhao Ouyang --- v3: remove fallback smc call v2: check SMCCC_ARCH_FEATURES --- drivers/firmware/psci.c | 5 - include/linux/arm-smccc.h | 6 ++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c index c6b9efab41..03544d76ed 100644 --- a/drivers/firmware/psci.c +++ b/drivers/firmware/psci.c @@ -135,10 +135,13 @@ static int bind_smccc_features(struct udevice *dev, int psci_method) PSCI_VERSION_MAJOR(psci_0_2_get_version()) == 0) return 0; - if (request_psci_features(ARM_SMCCC_ARCH_FEATURES) == + if (request_psci_features(ARM_SMCCC_VERSION) == PSCI_RET_NOT_SUPPORTED) return 0; + if (invoke_psci_fn(ARM_SMCCC_VERSION, 0, 0, 0) < ARM_SMCCC_VERSION_1_1) + return 0; + if (psci_method == PSCI_METHOD_HVC) pdata->invoke_fn = smccc_invoke_hvc; else diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h index f44e9e8f93..da3d29aabe 100644 --- a/include/linux/arm-smccc.h +++ b/include/linux/arm-smccc.h @@ -55,8 +55,14 @@ #define ARM_SMCCC_QUIRK_NONE 0 #define ARM_SMCCC_QUIRK_QCOM_A61 /* Save/restore register a6 */ +#define ARM_SMCCC_VERSION 0x8000 #define ARM_SMCCC_ARCH_FEATURES0x8001 +#define ARM_SMCCC_VERSION_1_0 0x1 +#define ARM_SMCCC_VERSION_1_1 0x10001 +#define ARM_SMCCC_VERSION_1_2 0x10002 +#define ARM_SMCCC_VERSION_1_3 0x10003 + #define ARM_SMCCC_RET_NOT_SUPPORTED((unsigned long)-1) #ifndef __ASSEMBLY__ -- 2.40.1
[PATCH v4 0/3] Random Number Generator fixes
This series aim to fix smccc bind issue and add a list command for RNG devices. Changelog: v3 --> v4 - update doc/usage/cmd/rng.rst v2 --> v3 - remove fallback smc call - add Fixes tag v1 --> v2 - check SMCCC_ARCH_FEATURES - update commit message and rng help info Weizhao Ouyang (3): firmware: psci: Fix bind_smccc_features psci check driver: rng: Fix SMCCC TRNG crash cmd: rng: Add rng list command cmd/rng.c | 23 ++- doc/usage/cmd/rng.rst | 14 ++ drivers/firmware/psci.c | 5 - drivers/rng/smccc_trng.c | 2 +- include/linux/arm-smccc.h | 6 ++ 5 files changed, 39 insertions(+), 11 deletions(-) -- 2.40.1
Re: [PATCH v3 3/3] cmd: rng: Add rng list command
Hi Tom, On Tue, Feb 6, 2024 at 2:14 AM Tom Rini wrote: > > On Wed, Jan 31, 2024 at 02:14:26PM +, Weizhao Ouyang wrote: > > > The 'rng list' command probes all RNG devices and list those devices > > that are successfully probed. Also update the help info. > > > > Reviewed-by: Heinrich Schuchardt > > Signed-off-by: Weizhao Ouyang > > Reviewed-by: Matthias Brugger > > Reviewed-by: Igor Opaniuk > > --- > > cmd/rng.c | 23 ++- > > 1 file changed, 18 insertions(+), 5 deletions(-) > > Please update doc/usage/cmd/rng.rst as well, thanks. Ok, I will update it. BR, Weizhao > > -- > Tom
Re: [PATCH v2 2/3] rockchip: rk35xx: Enable eMMC HS200 mode by default
On Mon, Feb 5, 2024 at 4:53 AM Jonas Karlman wrote: > > Testing has shown that writing to eMMC using a slower mode then HS200 > typically generate an ERROR on first attempt on RK3588. > > # Rescan using MMC legacy mode > => mmc rescan 0 > > # Write a single block to sector 0x4000 fails with ERROR > => mmc write 2000 4000 1 > > # Write a single block to sector 0x4000 now works > => mmc write 2000 4000 1 > > With the MMC_SPEED_MODE_SET Kconfig option enabled. > > Writing to eMMC using HS200 mode work more reliably than slower modes on > RK35xx boards. Enable MMC_HS200_SUPPORT Kconfig option by default to > prefer use of HS200 mode on RK356x and RK3588. > > Signed-off-by: Jonas Karlman Reviewed-by: Weizhao Ouyang BR, Weizhao > --- > Changes in v2: > - Imply MMC_HS200_SUPPORT and SPL_MMC_HS200_SUPPORT in arch Kconfig > instead of adding to each boards defconfig > - R-b tags not collected because of above change > - Combine changes for rk356x and rk3588 in one patch > - Update commit message > > Link to v1: https://patchwork.ozlabs.org/patch/1891693/ > --- > arch/arm/mach-rockchip/Kconfig | 4 > configs/nanopi-r5c-rk3568_defconfig | 2 -- > configs/nanopi-r5s-rk3568_defconfig | 2 -- > configs/radxa-e25-rk3568_defconfig | 2 -- > 4 files changed, 4 insertions(+), 6 deletions(-) > > diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig > index 6ff0aa6911e2..946ef5d7023d 100644 > --- a/arch/arm/mach-rockchip/Kconfig > +++ b/arch/arm/mach-rockchip/Kconfig > @@ -292,6 +292,8 @@ config ROCKCHIP_RK3568 > imply OF_LIBFDT_OVERLAY > imply ROCKCHIP_OTP > imply MISC_INIT_R > + imply MMC_HS200_SUPPORT if MMC_SDHCI_ROCKCHIP > + imply SPL_MMC_HS200_SUPPORT if SPL_MMC && MMC_HS200_SUPPORT > help > The Rockchip RK3568 is a ARM-based SoC with quad-core Cortex-A55, > including NEON and GPU, 512K L3 cache, Mali-G52 based graphics, > @@ -317,6 +319,8 @@ config ROCKCHIP_RK3588 > imply OF_LIBFDT_OVERLAY > imply ROCKCHIP_OTP > imply MISC_INIT_R > + imply MMC_HS200_SUPPORT if MMC_SDHCI_ROCKCHIP > + imply SPL_MMC_HS200_SUPPORT if SPL_MMC && MMC_HS200_SUPPORT > imply CLK_SCMI > imply SCMI_FIRMWARE > help > diff --git a/configs/nanopi-r5c-rk3568_defconfig > b/configs/nanopi-r5c-rk3568_defconfig > index 833cff0e457d..f5a472d03d78 100644 > --- a/configs/nanopi-r5c-rk3568_defconfig > +++ b/configs/nanopi-r5c-rk3568_defconfig > @@ -58,8 +58,6 @@ CONFIG_ROCKCHIP_GPIO=y > CONFIG_SYS_I2C_ROCKCHIP=y > CONFIG_MISC=y > CONFIG_SUPPORT_EMMC_RPMB=y > -CONFIG_MMC_HS200_SUPPORT=y > -CONFIG_SPL_MMC_HS200_SUPPORT=y > CONFIG_MMC_DW=y > CONFIG_MMC_DW_ROCKCHIP=y > CONFIG_MMC_SDHCI=y > diff --git a/configs/nanopi-r5s-rk3568_defconfig > b/configs/nanopi-r5s-rk3568_defconfig > index 2736d382a352..99692d341f44 100644 > --- a/configs/nanopi-r5s-rk3568_defconfig > +++ b/configs/nanopi-r5s-rk3568_defconfig > @@ -58,8 +58,6 @@ CONFIG_ROCKCHIP_GPIO=y > CONFIG_SYS_I2C_ROCKCHIP=y > CONFIG_MISC=y > CONFIG_SUPPORT_EMMC_RPMB=y > -CONFIG_MMC_HS200_SUPPORT=y > -CONFIG_SPL_MMC_HS200_SUPPORT=y > CONFIG_MMC_DW=y > CONFIG_MMC_DW_ROCKCHIP=y > CONFIG_MMC_SDHCI=y > diff --git a/configs/radxa-e25-rk3568_defconfig > b/configs/radxa-e25-rk3568_defconfig > index 5a613abe0d2d..fedb137877ab 100644 > --- a/configs/radxa-e25-rk3568_defconfig > +++ b/configs/radxa-e25-rk3568_defconfig > @@ -60,8 +60,6 @@ CONFIG_ROCKCHIP_GPIO=y > CONFIG_SYS_I2C_ROCKCHIP=y > CONFIG_MISC=y > CONFIG_SUPPORT_EMMC_RPMB=y > -CONFIG_MMC_HS200_SUPPORT=y > -CONFIG_SPL_MMC_HS200_SUPPORT=y > CONFIG_MMC_DW=y > CONFIG_MMC_DW_ROCKCHIP=y > CONFIG_MMC_SDHCI=y > -- > 2.43.0 >
Re: [PATCH v2 1/3] rockchip: rk35xx: Remove use of eMMC DDR52 mode
On Mon, Feb 5, 2024 at 4:53 AM Jonas Karlman wrote:
>
> Testing has shown that writing to eMMC using DDR52 mode does not seem to
> work on RK356x and RK3588 boards.
>
> A simple test of writing a single block to e.g. sector 0x4000 fails:
>
> # Rescan using DDR52 mode
> => mmc rescan 4
>
> # Write a single block to sector 0x4000 fails with ERROR
> => mmc write 2000 4000 1
>
> With the MMC_SPEED_MODE_SET Kconfig option enabled.
>
> Fix this by removing the mmc-ddr-1_8v prop from sdhci nodes in affected
> board u-boot.dtsi files.
>
> Signed-off-by: Jonas Karlman
Reviewed-by: Weizhao Ouyang
BR,
Weizhao
> ---
> Changes in v2:
> - Update commit message
>
> Link to v1: https://patchwork.ozlabs.org/patch/1891695/
> ---
> arch/arm/dts/rk3566-quartz64-a-u-boot.dtsi | 1 -
> arch/arm/dts/rk3566-quartz64-b-u-boot.dtsi | 1 -
> arch/arm/dts/rk3566-radxa-cm3-io-u-boot.dtsi | 1 -
> arch/arm/dts/rk3566-soquartz-u-boot.dtsi | 1 -
> arch/arm/dts/rk3568-lubancat-2-u-boot.dtsi | 1 -
> arch/arm/dts/rk3568-nanopi-r5s-u-boot.dtsi | 1 -
> arch/arm/dts/rk3568-odroid-m1-u-boot.dtsi| 1 -
> arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi| 1 -
> arch/arm/dts/rk3568-rock-3a-u-boot.dtsi | 1 -
> arch/arm/dts/rk3588-rock-5b-u-boot.dtsi | 1 -
> arch/arm/dts/rk3588-turing-rk1-u-boot.dtsi | 1 -
> arch/arm/dts/rk3588s-rock-5a-u-boot.dtsi | 1 -
> 12 files changed, 12 deletions(-)
>
> diff --git a/arch/arm/dts/rk3566-quartz64-a-u-boot.dtsi
> b/arch/arm/dts/rk3566-quartz64-a-u-boot.dtsi
> index 11976fd3a6e0..930d660868bb 100644
> --- a/arch/arm/dts/rk3566-quartz64-a-u-boot.dtsi
> +++ b/arch/arm/dts/rk3566-quartz64-a-u-boot.dtsi
> @@ -8,7 +8,6 @@
>
> {
> cap-mmc-highspeed;
> - mmc-ddr-1_8v;
> pinctrl-names = "default";
> pinctrl-0 = <_bus8 _clk _cmd _datastrobe>;
> };
> diff --git a/arch/arm/dts/rk3566-quartz64-b-u-boot.dtsi
> b/arch/arm/dts/rk3566-quartz64-b-u-boot.dtsi
> index 8de9d1535efb..c235b4357f7d 100644
> --- a/arch/arm/dts/rk3566-quartz64-b-u-boot.dtsi
> +++ b/arch/arm/dts/rk3566-quartz64-b-u-boot.dtsi
> @@ -4,7 +4,6 @@
>
> {
> cap-mmc-highspeed;
> - mmc-ddr-1_8v;
> pinctrl-names = "default";
> pinctrl-0 = <_bus8 _clk _cmd _datastrobe>;
> };
> diff --git a/arch/arm/dts/rk3566-radxa-cm3-io-u-boot.dtsi
> b/arch/arm/dts/rk3566-radxa-cm3-io-u-boot.dtsi
> index 158f652cb3b1..e0e501deccfe 100644
> --- a/arch/arm/dts/rk3566-radxa-cm3-io-u-boot.dtsi
> +++ b/arch/arm/dts/rk3566-radxa-cm3-io-u-boot.dtsi
> @@ -7,5 +7,4 @@
>
> {
> cap-mmc-highspeed;
> - mmc-ddr-1_8v;
> };
> diff --git a/arch/arm/dts/rk3566-soquartz-u-boot.dtsi
> b/arch/arm/dts/rk3566-soquartz-u-boot.dtsi
> index f65f4067f3e9..5e46a2422d60 100644
> --- a/arch/arm/dts/rk3566-soquartz-u-boot.dtsi
> +++ b/arch/arm/dts/rk3566-soquartz-u-boot.dtsi
> @@ -4,7 +4,6 @@
>
> {
> cap-mmc-highspeed;
> - mmc-ddr-1_8v;
> pinctrl-names = "default";
> pinctrl-0 = <_bus8 _clk _cmd _datastrobe>;
> };
> diff --git a/arch/arm/dts/rk3568-lubancat-2-u-boot.dtsi
> b/arch/arm/dts/rk3568-lubancat-2-u-boot.dtsi
> index a44ac35bdacd..1597473017ed 100644
> --- a/arch/arm/dts/rk3568-lubancat-2-u-boot.dtsi
> +++ b/arch/arm/dts/rk3568-lubancat-2-u-boot.dtsi
> @@ -8,7 +8,6 @@
>
> {
> cap-mmc-highspeed;
> - mmc-ddr-1_8v;
> mmc-hs400-1_8v;
> mmc-hs400-enhanced-strobe;
> pinctrl-0 = <_bus8 _clk _cmd _datastrobe>;
> diff --git a/arch/arm/dts/rk3568-nanopi-r5s-u-boot.dtsi
> b/arch/arm/dts/rk3568-nanopi-r5s-u-boot.dtsi
> index 62f572c4cf9f..64c43374c042 100644
> --- a/arch/arm/dts/rk3568-nanopi-r5s-u-boot.dtsi
> +++ b/arch/arm/dts/rk3568-nanopi-r5s-u-boot.dtsi
> @@ -14,7 +14,6 @@
>
> {
> cap-mmc-highspeed;
> - mmc-ddr-1_8v;
> mmc-hs200-1_8v;
> mmc-hs400-1_8v;
> mmc-hs400-enhanced-strobe;
> diff --git a/arch/arm/dts/rk3568-odroid-m1-u-boot.dtsi
> b/arch/arm/dts/rk3568-odroid-m1-u-boot.dtsi
> index ecba91aa30f5..1fc71faa9e07 100644
> --- a/arch/arm/dts/rk3568-odroid-m1-u-boot.dtsi
> +++ b/arch/arm/dts/rk3568-odroid-m1-u-boot.dtsi
> @@ -8,7 +8,6 @@
>
> {
> cap-mmc-highspeed;
> - mmc-ddr-1_8v;
> mmc-hs200-1_8v;
> mmc-hs400-1_8v;
> mmc-hs400-enhanced-strobe;
> diff --git a/arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi
> b/arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi
> index caf524443079..74755a44eaee 100644
> --- a/arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi
> +++ b/arch/arm/d
Re: [PATCH 01/18] rockchip: rk3588: use mainline pmu-grf compatible
On Tue, Jan 23, 2024 at 10:50 PM Quentin Schulz wrote:
>
> From: Heiko Stuebner
>
> The compatible for the pmugrf in the mainline kernel is dfferent from the
> one currently used in u-boot. Adapt the -u-boot.dtsi and syscon driver
> to use the correct compatible.
Reviewed-by: Weizhao Ouyang
BR,
Weizhao
>
> Signed-off-by: Heiko Stuebner
> Signed-off-by: Quentin Schulz
> ---
> arch/arm/dts/rk3588s-u-boot.dtsi | 2 +-
> arch/arm/mach-rockchip/rk3588/syscon_rk3588.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/dts/rk3588s-u-boot.dtsi
> b/arch/arm/dts/rk3588s-u-boot.dtsi
> index c0fd16c4022..9a5ffec926e 100644
> --- a/arch/arm/dts/rk3588s-u-boot.dtsi
> +++ b/arch/arm/dts/rk3588s-u-boot.dtsi
> @@ -66,7 +66,7 @@
>
> pmu1_grf: syscon@fd58a000 {
> bootph-all;
> - compatible = "rockchip,rk3588-pmu1-grf", "syscon";
> + compatible = "rockchip,rk3588-pmugrf", "syscon";
> reg = <0x0 0xfd58a000 0x0 0x2000>;
> };
>
> diff --git a/arch/arm/mach-rockchip/rk3588/syscon_rk3588.c
> b/arch/arm/mach-rockchip/rk3588/syscon_rk3588.c
> index e8772d3a382..7b2cf37d9da 100644
> --- a/arch/arm/mach-rockchip/rk3588/syscon_rk3588.c
> +++ b/arch/arm/mach-rockchip/rk3588/syscon_rk3588.c
> @@ -10,7 +10,7 @@
>
> static const struct udevice_id rk3588_syscon_ids[] = {
> { .compatible = "rockchip,rk3588-sys-grf", .data =
> ROCKCHIP_SYSCON_GRF },
> - { .compatible = "rockchip,rk3588-pmu1-grf", .data =
> ROCKCHIP_SYSCON_PMUGRF },
> + { .compatible = "rockchip,rk3588-pmugrf", .data =
> ROCKCHIP_SYSCON_PMUGRF },
> { .compatible = "rockchip,rk3588-vop-grf", .data =
> ROCKCHIP_SYSCON_VOP_GRF },
> { .compatible = "rockchip,rk3588-vo-grf", .data =
> ROCKCHIP_SYSCON_VO_GRF },
> { .compatible = "rockchip,pcie30-phy-grf", .data =
> ROCKCHIP_SYSCON_PCIE30_PHY_GRF },
>
> --
> 2.43.0
>
Re: [PATCH v3 1/3] firmware: psci: Fix bind_smccc_features psci check
Hi Igor, On Thu, Feb 1, 2024 at 10:36 PM Igor Opaniuk wrote: > > Hello Weizhao, > > On Wed, Jan 31, 2024 at 3:15 PM Weizhao Ouyang wrote: > > > > According to PSCI specification DEN0022F, PSCI_FEATURES is used to check > > whether the SMCCC is implemented by discovering SMCCC_VERSION. > > > Could you please add more details to your commit message or as a comment > explaining what exact steps should be done for a full discovery sequence of > Arm > Architecture Service functions, so people don't need to search for > that information explicitly? > > For instance: > Step 1: Determine if SMCCC_VERSION is implemented > - Use firmware data, device tree PSCI node, or ACPI FADT PSCI flag, to > determine whether a > PSCI implementation is present. > - Use PSCI_VERSION to learn whether PSCI_FEATURES is provided. PSCI_FEATURES > is > mandatory from version 1.0 of PSCI > Step 2. etc. > > I would just pull that info from the latest SMC Calling Convention version > 1.5, > from 9 Appendix B: Discovery of Arm Architecture Service functions. > > Thank you! > > Signed-off-by: Weizhao Ouyang > > --- > > v3: remove fallback smc call > > v2: check SMCCC_ARCH_FEATURES > > --- > > drivers/firmware/psci.c | 5 - > > include/linux/arm-smccc.h | 6 ++ > > 2 files changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c > > index c6b9efab41..03544d76ed 100644 > > --- a/drivers/firmware/psci.c > > +++ b/drivers/firmware/psci.c > > @@ -135,10 +135,13 @@ static int bind_smccc_features(struct udevice *dev, > > int psci_method) > > PSCI_VERSION_MAJOR(psci_0_2_get_version()) == 0) > > return 0; > > > > - if (request_psci_features(ARM_SMCCC_ARCH_FEATURES) == > > + if (request_psci_features(ARM_SMCCC_VERSION) == > > PSCI_RET_NOT_SUPPORTED) > > return 0; > > > > + if (invoke_psci_fn(ARM_SMCCC_VERSION, 0, 0, 0) < > > ARM_SMCCC_VERSION_1_1) > > + return 0; > > + > IMO, to fully comply with SMC Calling Convention version 1.5 > we should also check for SMCCC_ARCH_WORKAROUND_1: > > From 9 Appendix B: Discovery of Arm Architecture Service functions, > Step 2: Determine if Arm Architecture Service function is implemented > - Use SMCCC_VERSION to learn whether the calling convention complies > to version 1.1 or above. > - Use SMCCC_ARCH_FEATURES to query whether the Arm Architecture > Service function is implemented > on this system <--- we lack of this step Thanks for your review. The 9 Appendix B describes an approach to discovery the maximize ability without causing unsafe behavior on existing platforms. Regarding the second step, it is just using SMCCC_ARCH_WORKAROUND_1 as an example to test SMCCC_ARCH_FEATURES. For the U-Boot case, we can revisit this from two perspectives: 1. SMCCC_ARCH_FEATURES is MANDATORY from SMCCC v1.1. 2. SMCCC_VERSION is OPTIONAL for SMCCC v1.0, so we should consider a safe behavior. 3. U-Boot is using CONFIG_ARM_SMCCC_FEATURES to probe and bind SMCCC service driver if this driver is reported as supported. 4. U-Boot SMCCC service driver can embed its discovery process in is_supported() callback. So now we can choose the following approach in U-Boot: - Use firmware data (check "arm,psci-1.0") to determine whether a PSCI implementation is present. - Use PSCI_VERSION (psci_0_2_get_version() major version >= 0) to learn whether PSCI_FEATURES is provided. - Use PSCI_FEATURES with the SMCCC_VERSION Function Identifier as a parameter to determine that SMCCC_VERSION is implemented. - Use SMCCC_VERSION to learn whether the calling convention complies to version 1.1 or above. - Trying to probe and bind SMCCC service driver. BR, Weizhao > > > if (psci_method == PSCI_METHOD_HVC) > > pdata->invoke_fn = smccc_invoke_hvc; > > else > > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h > > index f44e9e8f93..da3d29aabe 100644 > > --- a/include/linux/arm-smccc.h > > +++ b/include/linux/arm-smccc.h > > @@ -55,8 +55,14 @@ > > #define ARM_SMCCC_QUIRK_NONE 0 > > #define ARM_SMCCC_QUIRK_QCOM_A61 /* Save/restore register > > a6 */ > > > > +#define ARM_SMCCC_VERSION 0x8000 > > #define ARM_SMCCC_ARCH_FEATURES0x8001 > > > > +#define ARM_SMCCC_VERSION_1_0 0x1 > > +#define ARM_SMCCC_VERSION_1_1 0x10001 > > +#define ARM_SMCCC_VERSION_1_2 0x10002 > > +#define ARM_SMCCC_VERSION_1_3 0x10003 > > + > > #define ARM_SMCCC_RET_NOT_SUPPORTED((unsigned long)-1) > > > > #ifndef __ASSEMBLY__ > > -- > > 2.39.2 > > > > > -- > Best regards - Freundliche Grüsse - Meilleures salutations > > Igor Opaniuk > Senior Software Engineer, Embedded & Security > E: igor.opan...@foundries.io > W: www.foundries.io
Re: [PATCH v3 1/3] firmware: psci: Fix bind_smccc_features psci check
Hi Abdellatif, On Thu, Feb 1, 2024 at 7:40 PM Abdellatif El Khlifi wrote: > > Hi Weizhao, > > > - if (request_psci_features(ARM_SMCCC_ARCH_FEATURES) == > > + if (request_psci_features(ARM_SMCCC_VERSION) == > > PSCI_RET_NOT_SUPPORTED) > > return 0; > > > > + if (invoke_psci_fn(ARM_SMCCC_VERSION, 0, 0, 0) < > > ARM_SMCCC_VERSION_1_1) > > + return 0; > > It makes sense to me, thanks. > > > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h > > index f44e9e8f93..da3d29aabe 100644 > > --- a/include/linux/arm-smccc.h > > +++ b/include/linux/arm-smccc.h > > @@ -55,8 +55,14 @@ > > #define ARM_SMCCC_QUIRK_NONE 0 > > #define ARM_SMCCC_QUIRK_QCOM_A6 1 /* Save/restore register a6 > > */ > > > > +#define ARM_SMCCC_VERSION0x8000 > > #define ARM_SMCCC_ARCH_FEATURES 0x8001 > > > > +#define ARM_SMCCC_VERSION_1_00x1 > > +#define ARM_SMCCC_VERSION_1_10x10001 > > +#define ARM_SMCCC_VERSION_1_20x10002 > > +#define ARM_SMCCC_VERSION_1_30x10003 > > Apart from ARM_SMCCC_VERSION_1_1, are the other ARM_SMCCC_VERSION_1_x defines > needed ? I'm trying to synchronize with linux kernel, it might be a bit odd to add only one version. BR, Weizhao > > Cheers, > Abdellatif
Re: [PATCH 3/3] rockchip: rk3588: Enable eMMC HS200 mode
On Sat, Jan 27, 2024 at 7:27 AM Jonas Karlman wrote: > > Writing to eMMC using HS200 mode work more reliably then other modes on > RK3588 boards. > > Enable MMC_HS200_SUPPORT Kconfig option to prefer use of HS200 mode. > > Signed-off-by: Jonas Karlman Reviewed-by: Weizhao Ouyang BR, Weizhao > --- > configs/evb-rk3588_defconfig | 2 ++ > configs/nanopc-t6-rk3588_defconfig | 2 ++ > configs/neu6a-io-rk3588_defconfig| 2 ++ > configs/neu6b-io-rk3588_defconfig| 2 ++ > configs/orangepi-5-plus-rk3588_defconfig | 2 ++ > configs/quartzpro64-rk3588_defconfig | 2 ++ > configs/rock5a-rk3588s_defconfig | 2 ++ > configs/rock5b-rk3588_defconfig | 2 ++ > configs/turing-rk1-rk3588_defconfig | 2 ++ > 9 files changed, 18 insertions(+) > > diff --git a/configs/evb-rk3588_defconfig b/configs/evb-rk3588_defconfig > index 0b7b4f2f627a..2dfdc71259f7 100644 > --- a/configs/evb-rk3588_defconfig > +++ b/configs/evb-rk3588_defconfig > @@ -53,6 +53,8 @@ CONFIG_ROCKCHIP_GPIO=y > CONFIG_SYS_I2C_ROCKCHIP=y > CONFIG_MISC=y > CONFIG_SUPPORT_EMMC_RPMB=y > +CONFIG_MMC_HS200_SUPPORT=y > +CONFIG_SPL_MMC_HS200_SUPPORT=y > CONFIG_MMC_DW=y > CONFIG_MMC_DW_ROCKCHIP=y > CONFIG_MMC_SDHCI=y > diff --git a/configs/nanopc-t6-rk3588_defconfig > b/configs/nanopc-t6-rk3588_defconfig > index 760993220929..26dcf3aae21c 100644 > --- a/configs/nanopc-t6-rk3588_defconfig > +++ b/configs/nanopc-t6-rk3588_defconfig > @@ -66,6 +66,8 @@ CONFIG_ROCKCHIP_GPIO=y > CONFIG_SYS_I2C_ROCKCHIP=y > CONFIG_MISC=y > CONFIG_SUPPORT_EMMC_RPMB=y > +CONFIG_MMC_HS200_SUPPORT=y > +CONFIG_SPL_MMC_HS200_SUPPORT=y > CONFIG_MMC_DW=y > CONFIG_MMC_DW_ROCKCHIP=y > CONFIG_MMC_SDHCI=y > diff --git a/configs/neu6a-io-rk3588_defconfig > b/configs/neu6a-io-rk3588_defconfig > index d5301c630b2a..a6549420c01e 100644 > --- a/configs/neu6a-io-rk3588_defconfig > +++ b/configs/neu6a-io-rk3588_defconfig > @@ -48,6 +48,8 @@ CONFIG_ROCKCHIP_GPIO=y > CONFIG_SYS_I2C_ROCKCHIP=y > CONFIG_MISC=y > CONFIG_SUPPORT_EMMC_RPMB=y > +CONFIG_MMC_HS200_SUPPORT=y > +CONFIG_SPL_MMC_HS200_SUPPORT=y > CONFIG_MMC_DW=y > CONFIG_MMC_DW_ROCKCHIP=y > CONFIG_MMC_SDHCI=y > diff --git a/configs/neu6b-io-rk3588_defconfig > b/configs/neu6b-io-rk3588_defconfig > index b13c9b5db1b0..b5739de147d8 100644 > --- a/configs/neu6b-io-rk3588_defconfig > +++ b/configs/neu6b-io-rk3588_defconfig > @@ -48,6 +48,8 @@ CONFIG_ROCKCHIP_GPIO=y > CONFIG_SYS_I2C_ROCKCHIP=y > CONFIG_MISC=y > CONFIG_SUPPORT_EMMC_RPMB=y > +CONFIG_MMC_HS200_SUPPORT=y > +CONFIG_SPL_MMC_HS200_SUPPORT=y > CONFIG_MMC_DW=y > CONFIG_MMC_DW_ROCKCHIP=y > CONFIG_MMC_SDHCI=y > diff --git a/configs/orangepi-5-plus-rk3588_defconfig > b/configs/orangepi-5-plus-rk3588_defconfig > index a58f96d57798..e5325158d2af 100644 > --- a/configs/orangepi-5-plus-rk3588_defconfig > +++ b/configs/orangepi-5-plus-rk3588_defconfig > @@ -69,6 +69,8 @@ CONFIG_ROCKCHIP_GPIO=y > CONFIG_SYS_I2C_ROCKCHIP=y > CONFIG_MISC=y > CONFIG_SUPPORT_EMMC_RPMB=y > +CONFIG_MMC_HS200_SUPPORT=y > +CONFIG_SPL_MMC_HS200_SUPPORT=y > CONFIG_MMC_DW=y > CONFIG_MMC_DW_ROCKCHIP=y > CONFIG_MMC_SDHCI=y > diff --git a/configs/quartzpro64-rk3588_defconfig > b/configs/quartzpro64-rk3588_defconfig > index 85af4c4ff955..fd8304debdbb 100644 > --- a/configs/quartzpro64-rk3588_defconfig > +++ b/configs/quartzpro64-rk3588_defconfig > @@ -53,6 +53,8 @@ CONFIG_ROCKCHIP_GPIO=y > CONFIG_SYS_I2C_ROCKCHIP=y > CONFIG_MISC=y > CONFIG_SUPPORT_EMMC_RPMB=y > +CONFIG_MMC_HS200_SUPPORT=y > +CONFIG_SPL_MMC_HS200_SUPPORT=y > CONFIG_MMC_DW=y > CONFIG_MMC_DW_ROCKCHIP=y > CONFIG_MMC_SDHCI=y > diff --git a/configs/rock5a-rk3588s_defconfig > b/configs/rock5a-rk3588s_defconfig > index efa7bcbdcda6..10d6f6580490 100644 > --- a/configs/rock5a-rk3588s_defconfig > +++ b/configs/rock5a-rk3588s_defconfig > @@ -56,6 +56,8 @@ CONFIG_ROCKCHIP_GPIO=y > CONFIG_SYS_I2C_ROCKCHIP=y > CONFIG_MISC=y > CONFIG_SUPPORT_EMMC_RPMB=y > +CONFIG_MMC_HS200_SUPPORT=y > +CONFIG_SPL_MMC_HS200_SUPPORT=y > CONFIG_MMC_DW=y > CONFIG_MMC_DW_ROCKCHIP=y > CONFIG_MMC_SDHCI=y > diff --git a/configs/rock5b-rk3588_defconfig b/configs/rock5b-rk3588_defconfig > index a0678ff1290c..76f57340df5a 100644 > --- a/configs/rock5b-rk3588_defconfig > +++ b/configs/rock5b-rk3588_defconfig > @@ -71,6 +71,8 @@ CONFIG_ROCKCHIP_GPIO=y > CONFIG_SYS_I2C_ROCKCHIP=y > CONFIG_MISC=y > CONFIG_SUPPORT_EMMC_RPMB=y > +CONFIG_MMC_HS200_SUPPORT=y > +CONFIG_SPL_MMC_HS200_SUPPORT=y > CONFIG_MMC_DW=y > CONFIG_MMC_DW_ROCKCHIP=y > CONFIG_MMC_SDHCI=y > diff --git a/configs/turing-rk1-rk3588_defconfig > b/co
Re: [PATCH 2/3] rockchip: rk356x: Enable eMMC HS200 mode
On Sat, Jan 27, 2024 at 10:32 AM Jonas Karlman wrote: > > Writing to eMMC using HS200 mode work more reliably then other modes on > RK356x boards. > > Enable MMC_HS200_SUPPORT Kconfig option to prefer use of HS200 mode. > > Signed-off-by: Jonas Karlman Reviewed-by: Weizhao Ouyang BR, Weizhao > --- > configs/anbernic-rgxx3-rk3566_defconfig | 2 ++ > configs/bpi-r2-pro-rk3568_defconfig | 2 ++ > configs/evb-rk3568_defconfig | 2 ++ > configs/lubancat-2-rk3568_defconfig | 2 ++ > configs/odroid-m1-rk3568_defconfig| 2 ++ > configs/quartz64-a-rk3566_defconfig | 2 ++ > configs/quartz64-b-rk3566_defconfig | 2 ++ > configs/radxa-cm3-io-rk3566_defconfig | 2 ++ > configs/rock-3a-rk3568_defconfig | 2 ++ > configs/soquartz-blade-rk3566_defconfig | 2 ++ > configs/soquartz-cm4-rk3566_defconfig | 2 ++ > configs/soquartz-model-a-rk3566_defconfig | 2 ++ > 12 files changed, 24 insertions(+) > > diff --git a/configs/anbernic-rgxx3-rk3566_defconfig > b/configs/anbernic-rgxx3-rk3566_defconfig > index ed6643d9d4fa..295c0bd3fc61 100644 > --- a/configs/anbernic-rgxx3-rk3566_defconfig > +++ b/configs/anbernic-rgxx3-rk3566_defconfig > @@ -61,6 +61,8 @@ CONFIG_ROCKCHIP_GPIO=y > CONFIG_SYS_I2C_ROCKCHIP=y > CONFIG_MISC=y > CONFIG_SUPPORT_EMMC_RPMB=y > +CONFIG_MMC_HS200_SUPPORT=y > +CONFIG_SPL_MMC_HS200_SUPPORT=y > CONFIG_MMC_DW=y > CONFIG_MMC_DW_ROCKCHIP=y > CONFIG_MMC_SDHCI=y > diff --git a/configs/bpi-r2-pro-rk3568_defconfig > b/configs/bpi-r2-pro-rk3568_defconfig > index e6e0e6fc6fa6..c9e1cd2c2c85 100644 > --- a/configs/bpi-r2-pro-rk3568_defconfig > +++ b/configs/bpi-r2-pro-rk3568_defconfig > @@ -60,6 +60,8 @@ CONFIG_SYS_I2C_ROCKCHIP=y > CONFIG_MISC=y > CONFIG_SUPPORT_EMMC_RPMB=y > CONFIG_SUPPORT_EMMC_BOOT=y > +CONFIG_MMC_HS200_SUPPORT=y > +CONFIG_SPL_MMC_HS200_SUPPORT=y > CONFIG_MMC_DW=y > CONFIG_MMC_DW_ROCKCHIP=y > CONFIG_MMC_SDHCI=y > diff --git a/configs/evb-rk3568_defconfig b/configs/evb-rk3568_defconfig > index cb9b87ff12cb..19eab9bf00ac 100644 > --- a/configs/evb-rk3568_defconfig > +++ b/configs/evb-rk3568_defconfig > @@ -54,6 +54,8 @@ CONFIG_ROCKCHIP_GPIO=y > CONFIG_SYS_I2C_ROCKCHIP=y > CONFIG_MISC=y > CONFIG_SUPPORT_EMMC_RPMB=y > +CONFIG_MMC_HS200_SUPPORT=y > +CONFIG_SPL_MMC_HS200_SUPPORT=y > CONFIG_MMC_DW=y > CONFIG_MMC_DW_ROCKCHIP=y > CONFIG_MMC_SDHCI=y > diff --git a/configs/lubancat-2-rk3568_defconfig > b/configs/lubancat-2-rk3568_defconfig > index 80ae6ec3a2e9..c06a447fda26 100644 > --- a/configs/lubancat-2-rk3568_defconfig > +++ b/configs/lubancat-2-rk3568_defconfig > @@ -55,6 +55,8 @@ CONFIG_ROCKCHIP_GPIO=y > CONFIG_SYS_I2C_ROCKCHIP=y > CONFIG_MISC=y > CONFIG_SUPPORT_EMMC_RPMB=y > +CONFIG_MMC_HS200_SUPPORT=y > +CONFIG_SPL_MMC_HS200_SUPPORT=y > CONFIG_MMC_DW=y > CONFIG_MMC_DW_ROCKCHIP=y > CONFIG_MMC_SDHCI=y > diff --git a/configs/odroid-m1-rk3568_defconfig > b/configs/odroid-m1-rk3568_defconfig > index 3130e341e776..7fed6e7da597 100644 > --- a/configs/odroid-m1-rk3568_defconfig > +++ b/configs/odroid-m1-rk3568_defconfig > @@ -72,6 +72,8 @@ CONFIG_ROCKCHIP_GPIO=y > CONFIG_SYS_I2C_ROCKCHIP=y > CONFIG_MISC=y > CONFIG_SUPPORT_EMMC_RPMB=y > +CONFIG_MMC_HS200_SUPPORT=y > +CONFIG_SPL_MMC_HS200_SUPPORT=y > CONFIG_MMC_DW=y > CONFIG_MMC_DW_ROCKCHIP=y > CONFIG_MMC_SDHCI=y > diff --git a/configs/quartz64-a-rk3566_defconfig > b/configs/quartz64-a-rk3566_defconfig > index ade08867f60f..fd6b0e528834 100644 > --- a/configs/quartz64-a-rk3566_defconfig > +++ b/configs/quartz64-a-rk3566_defconfig > @@ -71,6 +71,8 @@ CONFIG_ROCKCHIP_GPIO=y > CONFIG_SYS_I2C_ROCKCHIP=y > CONFIG_MISC=y > CONFIG_SUPPORT_EMMC_RPMB=y > +CONFIG_MMC_HS200_SUPPORT=y > +CONFIG_SPL_MMC_HS200_SUPPORT=y > CONFIG_MMC_DW=y > CONFIG_MMC_DW_ROCKCHIP=y > CONFIG_MMC_SDHCI=y > diff --git a/configs/quartz64-b-rk3566_defconfig > b/configs/quartz64-b-rk3566_defconfig > index 8d01db54407d..ec7a677fd3d3 100644 > --- a/configs/quartz64-b-rk3566_defconfig > +++ b/configs/quartz64-b-rk3566_defconfig > @@ -69,6 +69,8 @@ CONFIG_ROCKCHIP_GPIO=y > CONFIG_SYS_I2C_ROCKCHIP=y > CONFIG_MISC=y > CONFIG_SUPPORT_EMMC_RPMB=y > +CONFIG_MMC_HS200_SUPPORT=y > +CONFIG_SPL_MMC_HS200_SUPPORT=y > CONFIG_MMC_DW=y > CONFIG_MMC_DW_ROCKCHIP=y > CONFIG_MMC_SDHCI=y > diff --git a/configs/radxa-cm3-io-rk3566_defconfig > b/configs/radxa-cm3-io-rk3566_defconfig > index 4b606dcb8e94..10626acfdea2 100644 > --- a/configs/radxa-cm3-io-rk3566_defconfig > +++ b/configs/radxa-cm3-io-rk3566_defconfig > @@ -55,6 +55,8 @@ CONFIG_ROCKCHIP_GPIO=y > CONFIG_SYS_I2C_ROCKCHIP=y > CONFIG_MISC=y > CO
[PATCH v3 3/3] cmd: rng: Add rng list command
The 'rng list' command probes all RNG devices and list those devices
that are successfully probed. Also update the help info.
Reviewed-by: Heinrich Schuchardt
Signed-off-by: Weizhao Ouyang
---
cmd/rng.c | 23 ++-
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/cmd/rng.c b/cmd/rng.c
index 52f722c7af..b073a6c849 100644
--- a/cmd/rng.c
+++ b/cmd/rng.c
@@ -19,6 +19,22 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc,
char *const argv[])
struct udevice *dev;
int ret = CMD_RET_SUCCESS;
+ if (argc == 2 && !strcmp(argv[1], "list")) {
+ int idx = 0;
+
+ uclass_foreach_dev_probe(UCLASS_RNG, dev) {
+ idx++;
+ printf("RNG #%d - %s\n", dev->seq_, dev->name);
+ }
+
+ if (!idx) {
+ log_err("No RNG device\n");
+ return CMD_RET_FAILURE;
+ }
+
+ return CMD_RET_SUCCESS;
+ }
+
switch (argc) {
case 1:
devnum = 0;
@@ -56,12 +72,9 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc,
char *const argv[])
return ret;
}
-U_BOOT_LONGHELP(rng,
- "[dev [n]]\n"
- " - print n random bytes(max 64) read from dev\n");
-
U_BOOT_CMD(
rng, 3, 0, do_rng,
"print bytes from the hardware random number generator",
- rng_help_text
+ "list - list all the probed rng devices\n"
+ "rng [dev] [n]- print n random bytes(max 64) read from dev\n"
);
--
2.39.2
[PATCH v3 2/3] driver: rng: Fix SMCCC TRNG crash
Fix a SMCCC TRNG null pointer crash due to a failed smccc feature
binding.
Fixes: 53355bb86c25 ("drivers: rng: add smccc trng driver")
Reviewed-by: Heinrich Schuchardt
Signed-off-by: Weizhao Ouyang
---
v3: add Fixes tag
---
drivers/rng/smccc_trng.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/rng/smccc_trng.c b/drivers/rng/smccc_trng.c
index 3a4bb33941..3087cb991a 100644
--- a/drivers/rng/smccc_trng.c
+++ b/drivers/rng/smccc_trng.c
@@ -166,7 +166,7 @@ static int smccc_trng_probe(struct udevice *dev)
struct smccc_trng_priv *priv = dev_get_priv(dev);
struct arm_smccc_res res;
- if (!(smccc_trng_is_supported(smccc->invoke_fn)))
+ if (!smccc || !(smccc_trng_is_supported(smccc->invoke_fn)))
return -ENODEV;
/* At least one of 64bit and 32bit interfaces is available */
--
2.39.2
[PATCH v3 1/3] firmware: psci: Fix bind_smccc_features psci check
According to PSCI specification DEN0022F, PSCI_FEATURES is used to check whether the SMCCC is implemented by discovering SMCCC_VERSION. Signed-off-by: Weizhao Ouyang --- v3: remove fallback smc call v2: check SMCCC_ARCH_FEATURES --- drivers/firmware/psci.c | 5 - include/linux/arm-smccc.h | 6 ++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c index c6b9efab41..03544d76ed 100644 --- a/drivers/firmware/psci.c +++ b/drivers/firmware/psci.c @@ -135,10 +135,13 @@ static int bind_smccc_features(struct udevice *dev, int psci_method) PSCI_VERSION_MAJOR(psci_0_2_get_version()) == 0) return 0; - if (request_psci_features(ARM_SMCCC_ARCH_FEATURES) == + if (request_psci_features(ARM_SMCCC_VERSION) == PSCI_RET_NOT_SUPPORTED) return 0; + if (invoke_psci_fn(ARM_SMCCC_VERSION, 0, 0, 0) < ARM_SMCCC_VERSION_1_1) + return 0; + if (psci_method == PSCI_METHOD_HVC) pdata->invoke_fn = smccc_invoke_hvc; else diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h index f44e9e8f93..da3d29aabe 100644 --- a/include/linux/arm-smccc.h +++ b/include/linux/arm-smccc.h @@ -55,8 +55,14 @@ #define ARM_SMCCC_QUIRK_NONE 0 #define ARM_SMCCC_QUIRK_QCOM_A61 /* Save/restore register a6 */ +#define ARM_SMCCC_VERSION 0x8000 #define ARM_SMCCC_ARCH_FEATURES0x8001 +#define ARM_SMCCC_VERSION_1_0 0x1 +#define ARM_SMCCC_VERSION_1_1 0x10001 +#define ARM_SMCCC_VERSION_1_2 0x10002 +#define ARM_SMCCC_VERSION_1_3 0x10003 + #define ARM_SMCCC_RET_NOT_SUPPORTED((unsigned long)-1) #ifndef __ASSEMBLY__ -- 2.39.2
[PATCH v3 0/3] Random Number Generator fixes
This series aim to fix smccc bind issue and add a list command for RNG devices. Changelog: v2 --> v3 - remove fallback smc call - add Fixes tag v1 --> v2 - check SMCCC_ARCH_FEATURES - update commit message and rng help info Weizhao Ouyang (3): firmware: psci: Fix bind_smccc_features psci check driver: rng: Fix SMCCC TRNG crash cmd: rng: Add rng list command cmd/rng.c | 23 ++- drivers/firmware/psci.c | 5 - drivers/rng/smccc_trng.c | 2 +- include/linux/arm-smccc.h | 6 ++ 4 files changed, 29 insertions(+), 7 deletions(-) -- 2.39.2
Re: [PATCH v2 1/3] firmware: psci: Fix bind_smccc_features psci check
On Wed, Jan 31, 2024 at 8:27 PM Heinrich Schuchardt wrote: > > On 31.01.24 12:12, Weizhao Ouyang wrote: > > According to PSCI specification DEN0022F, PSCI_FEATURES is used to check > > whether the SMCCC is implemented by discovering SMCCC_VERSION. > > > > Signed-off-by: Weizhao Ouyang > > --- > > v2: check SMCCC_ARCH_FEATURES > > --- > > drivers/firmware/psci.c | 9 - > > include/linux/arm-smccc.h | 6 ++ > > 2 files changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c > > index c6b9efab41..ed701cd1e4 100644 > > --- a/drivers/firmware/psci.c > > +++ b/drivers/firmware/psci.c > > @@ -135,7 +135,7 @@ static int bind_smccc_features(struct udevice *dev, int > > psci_method) > > PSCI_VERSION_MAJOR(psci_0_2_get_version()) == 0) > > return 0; > > > > - if (request_psci_features(ARM_SMCCC_ARCH_FEATURES) == > > + if (request_psci_features(ARM_SMCCC_VERSION) == > > PSCI_RET_NOT_SUPPORTED) > > return 0; > > > > @@ -144,6 +144,13 @@ static int bind_smccc_features(struct udevice *dev, > > int psci_method) > > else > > pdata->invoke_fn = smccc_invoke_smc; > > > > + /* > > + * SMCCC_ARCH_FEATURES is MANDATORY from SMCCC v1.1, but we still > > remain > > + * the invoke_fn() even the SMCCC version is v1.0. > > This sentence is a bit hard to understand. Did you mean, > > "but we keep calling invoke_fn() even if the SMCC version is v1.0" ? > > > + */ > > + if (invoke_psci_fn(ARM_SMCCC_VERSION, 0, 0, 0) < > > ARM_SMCCC_VERSION_1_1) > > + return 0; > > > Why do we leave the function for > invoke_psci_fn(ARM_SMCCC_VERSION, 0, 0, 0) == ARM_SMCCC_VERSION_1_0 > despite the comment above? > Previously I was trying to leave the driver a fallback SMC call for SMCCC v1.0, but since the arm-ffa driver not use the legacy SMC invoke function and also we can directly call invoke_psci_fn(), so maybe we can drop this fallback function. Will update in v3. BR, Weizhao > Best regards > > Heinrich > > > + > > feature_cnt = ll_entry_count(struct arm_smccc_feature, > > arm_smccc_feature); > > feature = ll_entry_start(struct arm_smccc_feature, arm_smccc_feature); > > > > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h > > index f44e9e8f93..da3d29aabe 100644 > > --- a/include/linux/arm-smccc.h > > +++ b/include/linux/arm-smccc.h > > @@ -55,8 +55,14 @@ > > #define ARM_SMCCC_QUIRK_NONE0 > > #define ARM_SMCCC_QUIRK_QCOM_A6 1 /* Save/restore register a6 > > */ > > > > +#define ARM_SMCCC_VERSION0x8000 > > #define ARM_SMCCC_ARCH_FEATURES 0x8001 > > > > +#define ARM_SMCCC_VERSION_1_00x1 > > +#define ARM_SMCCC_VERSION_1_10x10001 > > +#define ARM_SMCCC_VERSION_1_20x10002 > > +#define ARM_SMCCC_VERSION_1_30x10003 > > + > > #define ARM_SMCCC_RET_NOT_SUPPORTED ((unsigned long)-1) > > > > #ifndef __ASSEMBLY__ >
[PATCH v2 3/3] cmd: rng: Add rng list command
The 'rng list' command probes all RNG devices and list those devices
that are successfully probed. Also update the help info.
Signed-off-by: Weizhao Ouyang
---
v2: update commit message and rng help info
---
cmd/rng.c | 23 ++-
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/cmd/rng.c b/cmd/rng.c
index 52f722c7af..b073a6c849 100644
--- a/cmd/rng.c
+++ b/cmd/rng.c
@@ -19,6 +19,22 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc,
char *const argv[])
struct udevice *dev;
int ret = CMD_RET_SUCCESS;
+ if (argc == 2 && !strcmp(argv[1], "list")) {
+ int idx = 0;
+
+ uclass_foreach_dev_probe(UCLASS_RNG, dev) {
+ idx++;
+ printf("RNG #%d - %s\n", dev->seq_, dev->name);
+ }
+
+ if (!idx) {
+ log_err("No RNG device\n");
+ return CMD_RET_FAILURE;
+ }
+
+ return CMD_RET_SUCCESS;
+ }
+
switch (argc) {
case 1:
devnum = 0;
@@ -56,12 +72,9 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc,
char *const argv[])
return ret;
}
-U_BOOT_LONGHELP(rng,
- "[dev [n]]\n"
- " - print n random bytes(max 64) read from dev\n");
-
U_BOOT_CMD(
rng, 3, 0, do_rng,
"print bytes from the hardware random number generator",
- rng_help_text
+ "list - list all the probed rng devices\n"
+ "rng [dev] [n]- print n random bytes(max 64) read from dev\n"
);
--
2.39.2
[PATCH v2 2/3] driver: rng: Fix SMCCC TRNG crash
Fix a SMCCC TRNG null pointer crash due to a failed smccc feature binding. Reviewed-by: Heinrich Schuchardt Signed-off-by: Weizhao Ouyang --- drivers/rng/smccc_trng.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/rng/smccc_trng.c b/drivers/rng/smccc_trng.c index 3a4bb33941..3087cb991a 100644 --- a/drivers/rng/smccc_trng.c +++ b/drivers/rng/smccc_trng.c @@ -166,7 +166,7 @@ static int smccc_trng_probe(struct udevice *dev) struct smccc_trng_priv *priv = dev_get_priv(dev); struct arm_smccc_res res; - if (!(smccc_trng_is_supported(smccc->invoke_fn))) + if (!smccc || !(smccc_trng_is_supported(smccc->invoke_fn))) return -ENODEV; /* At least one of 64bit and 32bit interfaces is available */ -- 2.39.2
[PATCH v2 1/3] firmware: psci: Fix bind_smccc_features psci check
According to PSCI specification DEN0022F, PSCI_FEATURES is used to check whether the SMCCC is implemented by discovering SMCCC_VERSION. Signed-off-by: Weizhao Ouyang --- v2: check SMCCC_ARCH_FEATURES --- drivers/firmware/psci.c | 9 - include/linux/arm-smccc.h | 6 ++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c index c6b9efab41..ed701cd1e4 100644 --- a/drivers/firmware/psci.c +++ b/drivers/firmware/psci.c @@ -135,7 +135,7 @@ static int bind_smccc_features(struct udevice *dev, int psci_method) PSCI_VERSION_MAJOR(psci_0_2_get_version()) == 0) return 0; - if (request_psci_features(ARM_SMCCC_ARCH_FEATURES) == + if (request_psci_features(ARM_SMCCC_VERSION) == PSCI_RET_NOT_SUPPORTED) return 0; @@ -144,6 +144,13 @@ static int bind_smccc_features(struct udevice *dev, int psci_method) else pdata->invoke_fn = smccc_invoke_smc; + /* +* SMCCC_ARCH_FEATURES is MANDATORY from SMCCC v1.1, but we still remain +* the invoke_fn() even the SMCCC version is v1.0. +*/ + if (invoke_psci_fn(ARM_SMCCC_VERSION, 0, 0, 0) < ARM_SMCCC_VERSION_1_1) + return 0; + feature_cnt = ll_entry_count(struct arm_smccc_feature, arm_smccc_feature); feature = ll_entry_start(struct arm_smccc_feature, arm_smccc_feature); diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h index f44e9e8f93..da3d29aabe 100644 --- a/include/linux/arm-smccc.h +++ b/include/linux/arm-smccc.h @@ -55,8 +55,14 @@ #define ARM_SMCCC_QUIRK_NONE 0 #define ARM_SMCCC_QUIRK_QCOM_A61 /* Save/restore register a6 */ +#define ARM_SMCCC_VERSION 0x8000 #define ARM_SMCCC_ARCH_FEATURES0x8001 +#define ARM_SMCCC_VERSION_1_0 0x1 +#define ARM_SMCCC_VERSION_1_1 0x10001 +#define ARM_SMCCC_VERSION_1_2 0x10002 +#define ARM_SMCCC_VERSION_1_3 0x10003 + #define ARM_SMCCC_RET_NOT_SUPPORTED((unsigned long)-1) #ifndef __ASSEMBLY__ -- 2.39.2
[PATCH v2 0/3] Random Number Generator fixes
This series aim to fix smccc bind issue and add a list command for RNG devices. Changelog: v1 --> v2 - check SMCCC_ARCH_FEATURES - update commit message and rng help info Weizhao Ouyang (3): firmware: psci: Fix bind_smccc_features psci check driver: rng: Fix SMCCC TRNG crash cmd: rng: Add rng list command cmd/rng.c | 23 ++- drivers/firmware/psci.c | 9 - drivers/rng/smccc_trng.c | 2 +- include/linux/arm-smccc.h | 6 ++ 4 files changed, 33 insertions(+), 7 deletions(-) -- 2.39.2
Re: [PATCH 1/3] firmware: psci: Fix bind_smccc_features psci check
Hi Abdellatif, On Fri, Jan 26, 2024 at 7:20 PM Abdellatif El Khlifi wrote: > > Hi Weizhao, > > On Thu, Jan 25, 2024 at 02:05:00PM +0000, Weizhao Ouyang wrote: > > According to PSCI specification DEN0022F, PSCI_FEATURES is used to check > > whether the SMCCC is implemented by discovering SMCCC_VERSION. > > > > Signed-off-by: Weizhao Ouyang > > --- > > drivers/firmware/psci.c | 2 +- > > include/linux/arm-smccc.h | 1 + > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c > > index c6b9efab41..894be8128d 100644 > > --- a/drivers/firmware/psci.c > > +++ b/drivers/firmware/psci.c > > @@ -135,7 +135,7 @@ static int bind_smccc_features(struct udevice *dev, int > > psci_method) > > PSCI_VERSION_MAJOR(psci_0_2_get_version()) == 0) > > return 0; > > > > - if (request_psci_features(ARM_SMCCC_ARCH_FEATURES) == > > + if (request_psci_features(ARM_SMCCC_VERSION) == > > PSCI_RET_NOT_SUPPORTED) > > return 0; > > I suggest we add checks on the SMCCC version like this: > > if SMCCC_VERSION >= 1.1 >assume SMCCC_ARCH_FEATURES is supported >discover arch features and bind drivers/devices > else >error Okay, I will add it. BR, Weizhao > > For more details [1][2]. > > > [1]: SMC Calling Convention 1.5 > (https://developer.arm.com/documentation/den0028/latest/) > > SMCCC_ARCH_FEATURES MANDATORY from SMCCC v1.1 > > The implementation of this function can be detected by checking the SMCCC > version. This function is mandatory > if SMCCC_VERSION indicates that version 1.1 or later is implemented. > > [2]: Linux checks SMCCC v1.1: > https://elixir.bootlin.com/linux/latest/source/drivers/firmware/psci/psci.c#L597 > > Cheers, > Abdellatif
Re: [PATCH 3/3] cmd: rng: Add rng list command
On Thu, Jan 25, 2024 at 10:49 PM Heinrich Schuchardt wrote:
>
> On 25.01.24 15:05, Weizhao Ouyang wrote:
> > Add rng list command to list all probed RNG device.
>
> Thank you for your contribution.
>
> Would the following be more accurate?
>
> The 'rng list' command probes all RNG devices and list those devices
> that are successfully probed.
Yeah.
>
> >
> > Signed-off-by: Weizhao Ouyang
> > ---
> > cmd/rng.c | 15 +++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/cmd/rng.c b/cmd/rng.c
> > index 52f722c7af..4818133f94 100644
> > --- a/cmd/rng.c
> > +++ b/cmd/rng.c
> > @@ -18,6 +18,19 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int
> > argc, char *const argv[])
> > int devnum;
> > struct udevice *dev;
> > int ret = CMD_RET_SUCCESS;
> > + int idx;
>
> Please, put the definition into the code block where it is used.
Sorry I imported the draft code, will remove it in the next version.
>
> > +
> > + if (argc == 2 && !strcmp(argv[1], "list")) {
>
> int idx = 0;
>
> > + uclass_foreach_dev_probe(UCLASS_RNG, dev) {
> > + printf("RNG #%d - %s\n", dev->seq_, dev->name);
>
> ++idx;
>
> > + }
> > +
> > + if (!idx) {
> > + printf("*** no RNG devices available ***\n");
>
> If idx is not initialized, depending on the random value of idx on the
> stack you get a printout or not, e.g.
>
> => rng list
> RNG #0 - riscv_zkr
> RNG #1 - virtio-rng#8
> *** no RNG devices available ***
>
> Do we need the message to be so noisy? How about
>
> log_err("No RNG device\n");
>
>
> > + return CMD_RET_FAILURE;
> > + }
>
> We prefer having an empty line before return statements.
>
> Best regards
>
> Heinrich
>
> > + return CMD_RET_SUCCESS;
> > + }
> >
> > switch (argc) {
> > case 1:
> > @@ -57,6 +70,8 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int
> > argc, char *const argv[])
> > }
> >
> > U_BOOT_LONGHELP(rng,
> > + "[list]\n"
> > + " - list all the probe rng device\n"
> > "[dev [n]]\n"
> > " - print n random bytes(max 64) read from dev\n");
> >
>
BR,
Weizhao
[PATCH 3/3] cmd: rng: Add rng list command
Add rng list command to list all probed RNG device.
Signed-off-by: Weizhao Ouyang
---
cmd/rng.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/cmd/rng.c b/cmd/rng.c
index 52f722c7af..4818133f94 100644
--- a/cmd/rng.c
+++ b/cmd/rng.c
@@ -18,6 +18,19 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc,
char *const argv[])
int devnum;
struct udevice *dev;
int ret = CMD_RET_SUCCESS;
+ int idx;
+
+ if (argc == 2 && !strcmp(argv[1], "list")) {
+ uclass_foreach_dev_probe(UCLASS_RNG, dev) {
+ printf("RNG #%d - %s\n", dev->seq_, dev->name);
+ }
+
+ if (!idx) {
+ printf("*** no RNG devices available ***\n");
+ return CMD_RET_FAILURE;
+ }
+ return CMD_RET_SUCCESS;
+ }
switch (argc) {
case 1:
@@ -57,6 +70,8 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc,
char *const argv[])
}
U_BOOT_LONGHELP(rng,
+ "[list]\n"
+ " - list all the probe rng device\n"
"[dev [n]]\n"
" - print n random bytes(max 64) read from dev\n");
--
2.39.2
[PATCH 2/3] driver: rng: Fix SMCCC TRNG crash
Fix a SMCCC TRNG null pointer crash due to a failed smccc feature binding. Signed-off-by: Weizhao Ouyang --- drivers/rng/smccc_trng.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/rng/smccc_trng.c b/drivers/rng/smccc_trng.c index 3a4bb33941..3087cb991a 100644 --- a/drivers/rng/smccc_trng.c +++ b/drivers/rng/smccc_trng.c @@ -166,7 +166,7 @@ static int smccc_trng_probe(struct udevice *dev) struct smccc_trng_priv *priv = dev_get_priv(dev); struct arm_smccc_res res; - if (!(smccc_trng_is_supported(smccc->invoke_fn))) + if (!smccc || !(smccc_trng_is_supported(smccc->invoke_fn))) return -ENODEV; /* At least one of 64bit and 32bit interfaces is available */ -- 2.39.2
[PATCH 1/3] firmware: psci: Fix bind_smccc_features psci check
According to PSCI specification DEN0022F, PSCI_FEATURES is used to check whether the SMCCC is implemented by discovering SMCCC_VERSION. Signed-off-by: Weizhao Ouyang --- drivers/firmware/psci.c | 2 +- include/linux/arm-smccc.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c index c6b9efab41..894be8128d 100644 --- a/drivers/firmware/psci.c +++ b/drivers/firmware/psci.c @@ -135,7 +135,7 @@ static int bind_smccc_features(struct udevice *dev, int psci_method) PSCI_VERSION_MAJOR(psci_0_2_get_version()) == 0) return 0; - if (request_psci_features(ARM_SMCCC_ARCH_FEATURES) == + if (request_psci_features(ARM_SMCCC_VERSION) == PSCI_RET_NOT_SUPPORTED) return 0; diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h index f44e9e8f93..453017eb5f 100644 --- a/include/linux/arm-smccc.h +++ b/include/linux/arm-smccc.h @@ -55,6 +55,7 @@ #define ARM_SMCCC_QUIRK_NONE 0 #define ARM_SMCCC_QUIRK_QCOM_A61 /* Save/restore register a6 */ +#define ARM_SMCCC_VERSION 0x8000 #define ARM_SMCCC_ARCH_FEATURES0x8001 #define ARM_SMCCC_RET_NOT_SUPPORTED((unsigned long)-1) -- 2.39.2
[PATCH 0/3] Random Number Generator fixes
This series aim to fix smccc bind issue and add a list command for RNG devices. Weizhao Ouyang (3): firmware: psci: Fix bind_smccc_features psci check driver: rng: Fix SMCCC TRNG crash cmd: rng: Add rng list command cmd/rng.c | 15 +++ drivers/firmware/psci.c | 2 +- drivers/rng/smccc_trng.c | 2 +- include/linux/arm-smccc.h | 1 + 4 files changed, 18 insertions(+), 2 deletions(-) -- 2.39.2
Re: [PATCH] cmd: sf: Fix sf probe crash
On Thu, Jan 4, 2024 at 8:29 PM Michael Nazzareno Trimarchi
wrote:
>
> Hi
>
> On Thu, Jan 4, 2024 at 1:16 PM Weizhao Ouyang wrote:
> >
> > On Thu, Jan 4, 2024 at 8:00 PM Michal Simek wrote:
> > >
> > >
> > >
> > > On 1/4/24 12:46, Weizhao Ouyang wrote:
> > > > Handle the return value of spi_flash_probe_bus_cs() to avoid sf probe
> > > > crashes.
> > > >
> > > > Signed-off-by: Weizhao Ouyang
> > > > ---
> > > > cmd/sf.c | 5 +++--
> > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/cmd/sf.c b/cmd/sf.c
> > > > index 730996c02b..e3866899f6 100644
> > > > --- a/cmd/sf.c
> > > > +++ b/cmd/sf.c
> > > > @@ -135,8 +135,9 @@ static int do_spi_flash_probe(int argc, char *const
> > > > argv[])
> > > > }
> > > > flash = NULL;
> > > > if (use_dt) {
> > > > - spi_flash_probe_bus_cs(bus, cs, );
> > > > - flash = dev_get_uclass_priv(new);
> > > > + ret = spi_flash_probe_bus_cs(bus, cs, );
> > >
> > > if (ret)
> > > return ret;
> > >
> > > don't you want to rather propagate that error?
> > >
> >
> > Well, since the spi_flash is empty, the following code will
> > print the error message and return.
> >
>
> flash = NULL;
>
> if (!flash) {
> printf("Failed to initialize SPI flash at %u:%u (error %d)\n",
>bus, cs, ret);
> return 1;
> }
>
> This code does not change error handling that is already present here.
Hi Michael,
Sorry I didn't get your point. This patch is designed to avoid
spi_flash_probe_bus_cs() crash by null pointer, and let the
current existing error handling to prompt it. So I'm not
trying to change the current error handling.
BR,
Weizhao
> Michael
>
> > BR,
> > Weizhao
>
>
>
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> mich...@amarulasolutions.com
> __
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> i...@amarulasolutions.com
> www.amarulasolutions.com
Re: [PATCH] cmd: sf: Fix sf probe crash
On Thu, Jan 4, 2024 at 8:21 PM Michal Simek wrote:
>
>
>
> On 1/4/24 13:15, Weizhao Ouyang wrote:
> > On Thu, Jan 4, 2024 at 8:00 PM Michal Simek wrote:
> >>
> >>
> >>
> >> On 1/4/24 12:46, Weizhao Ouyang wrote:
> >>> Handle the return value of spi_flash_probe_bus_cs() to avoid sf probe
> >>> crashes.
> >>>
> >>> Signed-off-by: Weizhao Ouyang
> >>> ---
> >>>cmd/sf.c | 5 +++--
> >>>1 file changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/cmd/sf.c b/cmd/sf.c
> >>> index 730996c02b..e3866899f6 100644
> >>> --- a/cmd/sf.c
> >>> +++ b/cmd/sf.c
> >>> @@ -135,8 +135,9 @@ static int do_spi_flash_probe(int argc, char *const
> >>> argv[])
> >>>}
> >>>flash = NULL;
> >>>if (use_dt) {
> >>> - spi_flash_probe_bus_cs(bus, cs, );
> >>> - flash = dev_get_uclass_priv(new);
> >>> + ret = spi_flash_probe_bus_cs(bus, cs, );
> >>
> >> if (ret)
> >> return ret;
> >>
> >> don't you want to rather propagate that error?
> >>
> >
> > Well, since the spi_flash is empty, the following code will
> > print the error message and return.
>
> And you return 0 which means everything is fine. But is everything fine in
> this
> case? Or do you want to see the error?
>
> This is command it means you can simply use && and if previous command succeed
> you can call something else.
>
Hi Michal,
Please check the code that follows this commit snippet:
if (!flash) {
printf("Failed to initialize SPI flash at %u:%u (error %d)\n",
bus, cs, ret);
return 1;
}
it will print the error and return 1 as the return value.
BR,
Weizhao
Re: [PATCH] cmd: sf: Fix sf probe crash
On Thu, Jan 4, 2024 at 8:00 PM Michal Simek wrote:
>
>
>
> On 1/4/24 12:46, Weizhao Ouyang wrote:
> > Handle the return value of spi_flash_probe_bus_cs() to avoid sf probe
> > crashes.
> >
> > Signed-off-by: Weizhao Ouyang
> > ---
> > cmd/sf.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/cmd/sf.c b/cmd/sf.c
> > index 730996c02b..e3866899f6 100644
> > --- a/cmd/sf.c
> > +++ b/cmd/sf.c
> > @@ -135,8 +135,9 @@ static int do_spi_flash_probe(int argc, char *const
> > argv[])
> > }
> > flash = NULL;
> > if (use_dt) {
> > - spi_flash_probe_bus_cs(bus, cs, );
> > - flash = dev_get_uclass_priv(new);
> > + ret = spi_flash_probe_bus_cs(bus, cs, );
>
> if (ret)
> return ret;
>
> don't you want to rather propagate that error?
>
Well, since the spi_flash is empty, the following code will
print the error message and return.
BR,
Weizhao
[PATCH] cmd: sf: Fix sf probe crash
Handle the return value of spi_flash_probe_bus_cs() to avoid sf probe
crashes.
Signed-off-by: Weizhao Ouyang
---
cmd/sf.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/cmd/sf.c b/cmd/sf.c
index 730996c02b..e3866899f6 100644
--- a/cmd/sf.c
+++ b/cmd/sf.c
@@ -135,8 +135,9 @@ static int do_spi_flash_probe(int argc, char *const argv[])
}
flash = NULL;
if (use_dt) {
- spi_flash_probe_bus_cs(bus, cs, );
- flash = dev_get_uclass_priv(new);
+ ret = spi_flash_probe_bus_cs(bus, cs, );
+ if (!ret)
+ flash = dev_get_uclass_priv(new);
} else {
flash = spi_flash_probe(bus, cs, speed, mode);
}
--
2.39.2
Re: [RESEND PATCH v2] efi_loader: Fix UEFI variable error handling
On Wed, Nov 15, 2023 at 6:15 PM Heinrich Schuchardt wrote:
>
> On 11/13/23 17:10, Weizhao Ouyang wrote:
> > Try to catch error the earlier way.
> >
> > Signed-off-by: Weizhao Ouyang
> > ---
> > lib/efi_loader/efi_var_file.c | 4 +++-
> > lib/efi_loader/efi_variable.c | 2 --
> > 2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
> > index 62e071bd83..fe1c462f17 100644
> > --- a/lib/efi_loader/efi_var_file.c
> > +++ b/lib/efi_loader/efi_var_file.c
> > @@ -192,8 +192,10 @@ efi_status_t efi_var_restore(struct efi_var_file *buf,
> > bool safe)
> > ret = efi_var_mem_ins(var->name, >guid, var->attr,
> > var->length, data, 0, NULL,
> > var->time);
> > - if (ret != EFI_SUCCESS)
> > + if (ret != EFI_SUCCESS) {
> > log_err("Failed to set EFI variable %ls\n",
> > var->name);
> > + return ret;
>
> This change implies that if a failure occurs, initialization of the EFI
> sub-system fails and you will not be able to boot via EFI.
>
> Such a failure will occur if ubootefi.var contains more variables than
> fit into the memory buffer.
>
> Please, describe why you want to disable U-Boot's EFI sub-system in this
> case.
Hi Heinrich,
In my case, if a variable wants to persist as a non-volatile variable to
ubootefi.var, we should keep the same behavior as runtime variable does.
Otherwise the ubootefi.var cannot be trusted.
BR,
Weizhao
>
> > + }
> > }
> > return EFI_SUCCESS;
> > }
> > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> > index be95ed44e6..2b2ca8c090 100644
> > --- a/lib/efi_loader/efi_variable.c
> > +++ b/lib/efi_loader/efi_variable.c
> > @@ -350,8 +350,6 @@ efi_status_t efi_set_variable_int(const u16
> > *variable_name,
> >
> > if (var_type == EFI_AUTH_VAR_PK)
> > ret = efi_init_secure_state();
> > - else
> > - ret = EFI_SUCCESS;
>
> This change is ok.
>
> Best regards
>
> Heinrich
>
> >
> > /*
> >* Write non-volatile EFI variables to file
>
[RESEND PATCH v2] efi_loader: Fix UEFI variable error handling
Try to catch error the earlier way.
Signed-off-by: Weizhao Ouyang
---
lib/efi_loader/efi_var_file.c | 4 +++-
lib/efi_loader/efi_variable.c | 2 --
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
index 62e071bd83..fe1c462f17 100644
--- a/lib/efi_loader/efi_var_file.c
+++ b/lib/efi_loader/efi_var_file.c
@@ -192,8 +192,10 @@ efi_status_t efi_var_restore(struct efi_var_file *buf,
bool safe)
ret = efi_var_mem_ins(var->name, >guid, var->attr,
var->length, data, 0, NULL,
var->time);
- if (ret != EFI_SUCCESS)
+ if (ret != EFI_SUCCESS) {
log_err("Failed to set EFI variable %ls\n", var->name);
+ return ret;
+ }
}
return EFI_SUCCESS;
}
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index be95ed44e6..2b2ca8c090 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -350,8 +350,6 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
if (var_type == EFI_AUTH_VAR_PK)
ret = efi_init_secure_state();
- else
- ret = EFI_SUCCESS;
/*
* Write non-volatile EFI variables to file
--
2.39.2
[PATCH v2] efi_loader: Fix UEFI error handling
Try to catch error the earlier way.
Signed-off-by: Weizhao Ouyang
---
Changes in v2:
- Avoid to stop the boot process.
lib/efi_loader/efi_var_file.c | 4 +++-
lib/efi_loader/efi_variable.c | 2 --
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
index 62e071bd83..fe1c462f17 100644
--- a/lib/efi_loader/efi_var_file.c
+++ b/lib/efi_loader/efi_var_file.c
@@ -192,8 +192,10 @@ efi_status_t efi_var_restore(struct efi_var_file *buf,
bool safe)
ret = efi_var_mem_ins(var->name, >guid, var->attr,
var->length, data, 0, NULL,
var->time);
- if (ret != EFI_SUCCESS)
+ if (ret != EFI_SUCCESS) {
log_err("Failed to set EFI variable %ls\n", var->name);
+ return ret;
+ }
}
return EFI_SUCCESS;
}
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index be95ed44e6..2b2ca8c090 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -350,8 +350,6 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
if (var_type == EFI_AUTH_VAR_PK)
ret = efi_init_secure_state();
- else
- ret = EFI_SUCCESS;
/*
* Write non-volatile EFI variables to file
--
2.39.2
Re: [PATCH 1/1] efi_loader: improve efi_var_from_file() description
On Mon, Nov 13, 2023 at 10:50 PM Heinrich Schuchardt wrote: > > It is unclear to developers why efi_var_from_file() returns EFI_SUCCESS if > file ubootefi.var is missing or corrupted. Improve the description. > > Reported-by: Weizhao Ouyang > Signed-off-by: Heinrich Schuchardt > --- > lib/efi_loader/efi_var_file.c | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c > index 62e071bd83..dbf76f93de 100644 > --- a/lib/efi_loader/efi_var_file.c > +++ b/lib/efi_loader/efi_var_file.c > @@ -204,8 +204,11 @@ efi_status_t efi_var_restore(struct efi_var_file *buf, > bool safe) > * File ubootefi.var is read from the EFI system partitions and the variables > * stored in the file are created. > * > - * In case the file does not exist yet or a variable cannot be set > EFI_SUCCESS > - * is returned. > + * On first boot the file ubootefi.var does not exist yet. This is if why we > + * must return EFI_SUCCESS in this case. > + * > + * If the variable file is corrupted, e.g. incorrect CRC32, we do not want to > + * stop the boot process. We deliberately return EFI_SUCCESS in this case, > too. Reviewed-by: Weizhao Ouyang > * > * Return: status code > */ > -- > 2.40.1 >
Re: [PATCH] efi_loader: Fix UEFI variable error handling
On Fri, Nov 10, 2023 at 9:12 PM Heinrich Schuchardt wrote:
>
>
>
> Am 10. November 2023 11:04:24 MEZ schrieb Ilias Apalodimas
> :
> >Hi Heinrich, Weizhao
> >
> >On Thu, 9 Nov 2023 at 15:57, Heinrich Schuchardt wrote:
> >>
> >> On 11/9/23 04:55, Weizhao Ouyang wrote:
> >> > Correct some UEFI variable error handing code paths.
> >> >
> >> > Signed-off-by: Weizhao Ouyang
> >> > ---
> >> > lib/efi_loader/efi_var_file.c | 1 +
> >> > lib/efi_loader/efi_variable.c | 8
> >> > 2 files changed, 5 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/lib/efi_loader/efi_var_file.c
> >> > b/lib/efi_loader/efi_var_file.c
> >> > index 62e071bd83..dbb9b1f3fc 100644
> >> > --- a/lib/efi_loader/efi_var_file.c
> >> > +++ b/lib/efi_loader/efi_var_file.c
> >> > @@ -236,6 +236,7 @@ efi_status_t efi_var_from_file(void)
> >> > log_err("Invalid EFI variables file\n");
> >> > error:
> >> > free(buf);
> >> > + return ret;
> >>
> >> Hello Weizhao,
> >>
> >> thank you for looking into the error handling.
> >>
> >> U-Boot's UEFI variables can either be stored in file ubootefi.var in the
> >> ESP or in the RPMB (Replay Protected Memory Block) partition of the
> >> eMMC. The suggested changes are about handling of errors when reading or
> >> writing the file ubootefi.var. Security relevant variables (PK, KEK, db,
> >> dbx) are never read from file.
> >>
> >> On first boot a file ubootefi.var will not exist. It has to be created
> >> by U-Boot. If efi_var_from_file() would return an error if the file does
> >> not exist or is corrupted, we would never be able to boot such a system.
> >> This is why deliberately we return EFI_SUCCESS here. What is missing in
> >> the code is a comment explaining the design decision.
> >
> >Yes, that's correct. The function description tries to explain that
> >but is a bit vague.
> >
> >>
> >> > #endif
> >> > return EFI_SUCCESS;
> >> > }
> >> > diff --git a/lib/efi_loader/efi_variable.c
> >> > b/lib/efi_loader/efi_variable.c
> >> > index be95ed44e6..13966297c6 100644
> >> > --- a/lib/efi_loader/efi_variable.c
> >> > +++ b/lib/efi_loader/efi_variable.c
> >> > @@ -350,17 +350,17 @@ efi_status_t efi_set_variable_int(const u16
> >> > *variable_name,
> >> >
> >> > if (var_type == EFI_AUTH_VAR_PK)
> >> > ret = efi_init_secure_state();
> >> > - else
> >> > - ret = EFI_SUCCESS;
> >>
> >> The two lines are unreachable code and should be removed.
> >>
> >> > + if (ret != EFI_SUCCESS)
> >> > + return ret;
> >>
> >> These new lines should only be reached if var_type == EFI_AUTH_VAR_PK.
> >>
> >
> >Yea agree here
> >
> >> I am not sure what Would be the right error handling if
> >> efi_init_secure_state() fails:
> >>
> >> * Do we have to set PK to the old value?
> >
> >What do you mean by old value?
>
> We are in SetVariable() and set, changed, or deleted PK in memory. But this
> has lead to some inconsistency. Should the prior state be restored?
Shall we restore the variable "SecureBoot"?
>
> Regards
>
> Heinrich
>
> >
> >> * Should we still persist PK to ubootefi.var?
> >
> >I would argue that we don't really care about what happens in this
> >case. Writing authenticated variables on a file is only supported if
> >preseeding is disabled and it *never* gets restored. We basically
> >allow that code to test EFI secure boot by writing PK, KEK, DB on the
> >fly, but once we reboot those variables are gone.
> >If preseeding is enabled we don't write that at all. We return
> >EFI_WRITE_PROTECTED. We could do that regardless. But since we have
> >those tests, the memory backend should still be allowed to write
> >those.
> >
> >>
> >> However we decide we should describe our decisions in a code comment.
> >
> >I think the logic here should be
> >1. If variables are preseeded and restoring any authenticated
> >variables fails, the EFI subsystem should refuse to start (which it
> >already does)
> >2. If preseeding is not configured we can continue as best effort and
> >tr
Re: [PATCH] efi_loader: Fix UEFI variable error handling
On Thu, Nov 9, 2023 at 9:57 PM Heinrich Schuchardt wrote:
>
> On 11/9/23 04:55, Weizhao Ouyang wrote:
> > Correct some UEFI variable error handing code paths.
> >
> > Signed-off-by: Weizhao Ouyang
> > ---
> > lib/efi_loader/efi_var_file.c | 1 +
> > lib/efi_loader/efi_variable.c | 8
> > 2 files changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
> > index 62e071bd83..dbb9b1f3fc 100644
> > --- a/lib/efi_loader/efi_var_file.c
> > +++ b/lib/efi_loader/efi_var_file.c
> > @@ -236,6 +236,7 @@ efi_status_t efi_var_from_file(void)
> > log_err("Invalid EFI variables file\n");
> > error:
> > free(buf);
> > + return ret;
>
> Hello Weizhao,
>
> thank you for looking into the error handling.
>
> U-Boot's UEFI variables can either be stored in file ubootefi.var in the
> ESP or in the RPMB (Replay Protected Memory Block) partition of the
> eMMC. The suggested changes are about handling of errors when reading or
> writing the file ubootefi.var. Security relevant variables (PK, KEK, db,
> dbx) are never read from file.
>
Hi Heinrich,
Yes, my intention was to check for errors related to non-volatile
variables.
> On first boot a file ubootefi.var will not exist. It has to be created
> by U-Boot. If efi_var_from_file() would return an error if the file does
> not exist or is corrupted, we would never be able to boot such a system.
> This is why deliberately we return EFI_SUCCESS here. What is missing in
> the code is a comment explaining the design decision.
Sorry, I missed this scene. Maybe a comment is needed or we can split
the scene.
>
> > #endif
> > return EFI_SUCCESS;
> > }
> > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> > index be95ed44e6..13966297c6 100644
> > --- a/lib/efi_loader/efi_variable.c
> > +++ b/lib/efi_loader/efi_variable.c
> > @@ -350,17 +350,17 @@ efi_status_t efi_set_variable_int(const u16
> > *variable_name,
> >
> > if (var_type == EFI_AUTH_VAR_PK)
> > ret = efi_init_secure_state();
> > - else
> > - ret = EFI_SUCCESS;
>
> The two lines are unreachable code and should be removed.
>
> > + if (ret != EFI_SUCCESS)
> > + return ret;
>
> These new lines should only be reached if var_type == EFI_AUTH_VAR_PK.
>
> I am not sure what Would be the right error handling if
> efi_init_secure_state() fails:
>
> * Do we have to set PK to the old value?
> * Should we still persist PK to ubootefi.var?
>
> However we decide we should describe our decisions in a code comment.
IMO, efi_init_secure_state() is not just dealing with PK but also
includes Secure Boot mode transitions. So it may returns some
appropriate error when dealing with variable authentication.
>
> >
> > /*
> >* Write non-volatile EFI variables to file
> >* TODO: check if a value change has occured to avoid superfluous
> > writes
> >*/
> > if (attributes & EFI_VARIABLE_NON_VOLATILE)
> > - efi_var_to_file();
> > + ret = efi_var_to_file();
>
> The discussion here should focus on the treatment of errors in the
> file-system.
>
> The following error cases come to my mind:
>
> * ESP partition is missing
> * ESP FAT file system is corrupted
> * There is no space on the ESP.
> * The medium (e.g. SD card) is in write only mode
>
> The current code gives priority to enable booting in all adverse
> situations. Do you think this is a bad choice?
I think the ESP status (including fs corruption) is the most likely
situation to have an effect. But we should catch it earlier anyway.
BR,
Weizhao
>
> @Ilias: Please, chime in.
>
> Best regards
>
> Heinrich
>
> >
> > - return EFI_SUCCESS;
> > + return ret;
> > }
> >
> > efi_status_t efi_query_variable_info_int(u32 attributes,
>
[PATCH] efi_loader: Fix UEFI variable error handling
Correct some UEFI variable error handing code paths.
Signed-off-by: Weizhao Ouyang
---
lib/efi_loader/efi_var_file.c | 1 +
lib/efi_loader/efi_variable.c | 8
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
index 62e071bd83..dbb9b1f3fc 100644
--- a/lib/efi_loader/efi_var_file.c
+++ b/lib/efi_loader/efi_var_file.c
@@ -236,6 +236,7 @@ efi_status_t efi_var_from_file(void)
log_err("Invalid EFI variables file\n");
error:
free(buf);
+ return ret;
#endif
return EFI_SUCCESS;
}
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index be95ed44e6..13966297c6 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -350,17 +350,17 @@ efi_status_t efi_set_variable_int(const u16
*variable_name,
if (var_type == EFI_AUTH_VAR_PK)
ret = efi_init_secure_state();
- else
- ret = EFI_SUCCESS;
+ if (ret != EFI_SUCCESS)
+ return ret;
/*
* Write non-volatile EFI variables to file
* TODO: check if a value change has occured to avoid superfluous writes
*/
if (attributes & EFI_VARIABLE_NON_VOLATILE)
- efi_var_to_file();
+ ret = efi_var_to_file();
- return EFI_SUCCESS;
+ return ret;
}
efi_status_t efi_query_variable_info_int(u32 attributes,
--
2.39.2
Re: [PATCH] scripts/Makefile.lib: print error when no public key is specified
On Fri, Oct 27, 2023 at 3:45 PM Masahisa Kojima wrote: > > The current build system embeds the EFI Signature List(ESL) > into the dtb to be used in the EFI capsule authentication. > This ESL file is specified through the CONFIG_EFI_CAPSULE_ESL_FILE > Kconfig option. If CONFIG_EFI_CAPSULE_ESL_FILE is not specified, > U-boot build ends up with failure but the cause of failure is not > easily understandable. Current error message is as follows. > > FATAL ERROR: Error reading file into data: Is a directoryCheck > /home/ubuntu/src/ledge/u-boot/arch/arm/dts/.synquacer-sc2a11-developerbox.dtb.pre.tmp > for errors > make[2]: *** [scripts/Makefile.lib:355: > arch/arm/dts/synquacer-sc2a11-developerbox.dtb] Error 1 > make[1]: *** [dts/Makefile:44: arch-dtbs] Error 2 > make: *** [Makefile:1165: dts/dt.dtb] Error 2 > make: *** Waiting for unfinished jobs > > This commit shows the error message that CONFIG_EFI_CAPSULE_ESL_FILE > must be specified when the EFI capsule authentication is enabled, then > terminate the build with error. > > Signed-off-by: Masahisa Kojima > --- > scripts/Makefile.lib | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib > index 8dc6ec82cd..16bbc277a9 100644 > --- a/scripts/Makefile.lib > +++ b/scripts/Makefile.lib > @@ -339,7 +339,12 @@ cmd_capsule_esl_gen = \ > $(shell sed "s:ESL_BIN_FILE:$(capsule_esl_path):" > $(capsule_esl_input_file) > $@) > > $(obj)/.capsule_esl.dtsi: FORCE > +ifeq ($(CONFIG_EFI_CAPSULE_ESL_FILE),"") > + $(error "CONFIG_EFI_CAPSULE_ESL_FILE is empty, EFI capsule > authentication \ > + public key must be specified when CONFIG_EFI_CAPSULE_AUTHENTICATE is > enabled") > +else > $(call cmd_capsule_esl_gen) > +endif > > capsule_esl_input_file=$(srctree)/lib/efi_loader/capsule_esl.dtsi.in > capsule_esl_dtsi = .capsule_esl.dtsi > -- > 2.34.1 > Reviewed-by: Weizhao Ouyang
[PATCH] cyclic: doc: Update documentation for CONFIG_CYCLIC_MAX_CPU_TIME_US
Cyclic now just print a warning once instead of disabling the cyclic
function when the cyclic function upon exceeding CPU time usage.
Fixes: ddc8d36a7455 ("cyclic: Don't disable cylic function upon exceeding CPU
time")
Signed-off-by: Weizhao Ouyang
---
doc/develop/cyclic.rst | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/doc/develop/cyclic.rst b/doc/develop/cyclic.rst
index 43bedacb9f..be70a54f67 100644
--- a/doc/develop/cyclic.rst
+++ b/doc/develop/cyclic.rst
@@ -11,8 +11,8 @@ on a high frequent polling (e.g. UART rx char ready check)
might be
delayed too much. To detect cyclic functions with a too long execution
time, the Kconfig option `CONFIG_CYCLIC_MAX_CPU_TIME_US` is introduced,
which configures the max allowed time for such a cyclic function. If it's
-execution time exceeds this time, this cyclic function will get removed
-from the cyclic list.
+execution time exceeds this time, cyclic will print a warning once to show
+this potential problem to the user.
Registering a cyclic function
-
--
2.39.2
