[PATCH v10 3/4] Boot var automatic management for removable medias

[Heinrich Schuchardt]

On 18.07.23 11:03, Masahisa Kojima wrote:
> Hi Raymond,
>
> On Tue, 18 Jul 2023 at 05:23, Raymond Mao <raymond.mao at linaro.org> wrote:
>>
>> Hi Heinrich,
>>
>> I run 'make tests' with the patches locally but no errors observed from 'test_signed.py', below are few lines from the output console:
>> ```
>> test/py/tests/test_efi_secboot/test_signed.py ssssssss                                                                                                                                              [      90%]
>> ```
>
> I think the test cases are just skipped, not executed.
>
> I guess that some required packages are missing on the host machine,
> such as sbsigntool, efitools and libguestfs-tools.
> build-sandbox/test-log.html will help to analyze why the tests are skipped.
>
> The following document describes prerequisites to run python tests.
> https://u-boot.readthedocs.io/en/latest/develop/py_testing.html
>
> 'make tests' runs many test cases and takes time.
> './test/py/test.py --bd sandbox --build -k efi_secboot' will run only
> the efi_secboot test
> and it will be useful for debugging purposes.
>
> Thanks,
> Masahisa Kojima

To get more output describing why tests are skipped or fail use

    export PYTEST_ADDOPTS="-ra"

https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/16948
shows the errors with the suggested patch.

Best regards

Heinrich

>
>
>>
>> Regards,
>> Raymond
>>
>> On Sat, 15 Jul 2023 at 05:19, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>>
>>> On 7/9/23 10:56, Heinrich Schuchardt wrote:
>>>> On 6/19/23 23:23, Raymond Mao wrote:
>>>>> Changes for complying to EFI spec §3.5.1.1
>>>>> 'Removable Media Boot Behavior'.
>>>>> Boot variables can be automatically generated during a removable
>>>>> media is probed. At the same time, unused boot variables will be
>>>>> detected and removed.
>>>>>
>>>>> Please note that currently the function 'efi_disk_remove' has no
>>>>> ability to distinguish below two scenarios
>>>>> a) Unplugging of a removable media under U-Boot
>>>>> b) U-Boot exiting and booting an OS
>>>>> Thus currently the boot variables management is not added into
>>>>> 'efi_disk_remove' to avoid boot options being added/erased
>>>>> repeatedly under scenario b) during power cycles
>>>>> See TODO comments under function 'efi_disk_remove' for more details
>>>>>
>>>>> Signed-off-by: Raymond Mao <raymond.mao at linaro.org>
>>>>> ---
>>>>> Changes in v3
>>>>> - Split the patch into moving and renaming functions and
>>>>>     individual patches for each changed functionality
>>>>> Changes in v5
>>>>> - Move function call of efi_bootmgr_update_media_device_boot_option()
>>>>>     from efi_init_variables() to efi_init_obj_list()
>>>>> Changes in v6
>>>>> - Revert unrelated changes
>>>>> Changes in v7
>>>>> - adapt the return code of function
>>>>>     efi_bootmgr_update_media_device_boot_option()
>>>>> Changes in v8
>>>>> - add a note in the commit message for future reference
>>>>> Changes in v9
>>>>> - amend the note text in the commit message
>>>>> - add a TODO comment in 'efi_disk_remove'
>>>>> Changes in v10
>>>>> - fix typo and build failures with 'CONFIG_CMD_BOOTEFI_BOOTMGR=n'
>>>>>
>>>>>    lib/efi_loader/efi_disk.c  | 18 ++++++++++++++++++
>>>>>    lib/efi_loader/efi_setup.c |  7 +++++++
>>>>>    2 files changed, 25 insertions(+)
>>>>>
>>>>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
>>>>> index d2256713a8..911a4adfb1 100644
>>>>> --- a/lib/efi_loader/efi_disk.c
>>>>> +++ b/lib/efi_loader/efi_disk.c
>>>>> @@ -687,6 +687,13 @@ int efi_disk_probe(void *ctx, struct event *event)
>>>>>                return -1;
>>>>>        }
>>>>> +    /* only do the boot option management when UEFI sub-system is
>>>>> initialized */
>>>>> +    if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR) &&
>>>>> efi_obj_list_initialized == EFI_SUCCESS) {
>>>>> +        ret = efi_bootmgr_update_media_device_boot_option();
>>>>> +        if (ret != EFI_SUCCESS)
>>>>> +            return -1;
>>>>> +    }
>>>>> +
>>>>>        return 0;
>>>>>    }
>>>>> @@ -773,6 +780,17 @@ int efi_disk_remove(void *ctx, struct event *event)
>>>>>            return efi_disk_delete_part(dev);
>>>>>        else
>>>>>            return 0;
>>>>> +
>>>>> +    /*
>>>>> +     * TODO A flag to distinguish below 2 different scenarios of this
>>>>> +     * function call is needed:
>>>>> +     * a) Unplugging of a removable media under U-Boot
>>>>> +     * b) U-Boot exiting and booting an OS
>>>>> +     * In case of scenario a),
>>>>> efi_bootmgr_update_media_device_boot_option()
>>>>> +     * needs to be invoked here to update the boot options and remove
>>>>> the
>>>>> +     * unnecessary ones.
>>>>> +     */
>>>>
>>>> As in future we want to integrate more EFI drivers with the driver model
>>>> such a status should be in a global variable. It will have to be set in
>>>> ExitBootServices() before invoking dm_remove_devices_flags().
>>>>
>>>> Reviewed-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>>>
>>>>> +
>>>>>    }
>>>>>    /**
>>>>> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
>>>>> index 58d4e13402..69c8b27730 100644
>>>>> --- a/lib/efi_loader/efi_setup.c
>>>>> +++ b/lib/efi_loader/efi_setup.c
>>>>> @@ -245,6 +245,13 @@ efi_status_t efi_init_obj_list(void)
>>>>>        if (ret != EFI_SUCCESS)
>>>>>            goto out;
>>>>> +    if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {
>>>>> +        /* update boot option after variable service initialized */
>>>>> +        ret = efi_bootmgr_update_media_device_boot_option();
>>>>> +        if (ret != EFI_SUCCESS)
>>>>> +            goto out;
>>>>> +    }
>>>>> +
>>>>>        /* Define supported languages */
>>>>>        ret = efi_init_platform_lang();
>>>>>        if (ret != EFI_SUCCESS)
>>>>
>>>
>>> This patch leads to a test failure in
>>>
>>> test/py/tests/test_efi_secboot/test_signed.py::TestEfiSignedImage::test_efi_signed_image_auth2
>>>
>>> No EFI system partition
>>> Failed to persist EFI variables
>>>
>>> Please, run 'make tests' before resubmitting.
>>>
>>> Best regards
>>>
>>> Heinrich