Re: [PATCH 1/1] efi/efi_test: read RuntimeServicesSupported

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 12/3/20 2:20 AM, ivanhu wrote:
>
>
> On 12/2/20 7:38 PM, Heinrich Schuchardt wrote:
>> On 11/30/20 11:38 AM, ivanhu wrote:
>>>
>>>
>>> On 11/30/20 5:17 PM, Heinrich Schuchardt wrote:
>>>> On 11/30/20 9:16 AM, ivanhu wrote:
>>>>> Hi Heinrich,
>>>>>
>>>>> Thanks for the patch.
>>>>> It looks good to me, but I noticed that the runtime_supported_mask was
>>>>> introduced after 5.7-rc1.
>>>>> Maybe we should add the kernel version checking for the old kernels.
>>>>
>>>> This is a kernel patch. Why should we check the kernel version in the
>>>> kernel code?
>>>>
>>>> As patches may be back-ported we should not make any assumptions in fwts
>>>> based on the kernel version. If the ioctl() call fails with errno =
>>>> ENOTTY, we know that the kernel does not implement the ioctl call and we
>>>> have to assume that all runtime services are available.
>>>
>>> Sounds good to me,
>>> Acked-by: Ivan Hu <ivan.hu@canonical.com>
>>>
>>> And I will replace the reading RuntimeServicesSupported efi variable by
>>> using efi_test in fwts RuntimeServicesSupported tests.
>>>
>>> FWTS will still test those Unsupported Runtime services to check if it
>>> returns EFI_UNSUPPORTED correctly.
>>> Is that could solve your problem?
>>> If I remember correctly, the problem from you is not to test those
>>> marked Unsupported Runtime services. But from the Spec. 8.1 Runtime
>>> Services Rules and Restrictions,
>>
>> The problem I reported was that it is impossible to test UEFI runtime
>> services on U-Boot because FWTS tries to read the non-existent
>> RuntimeServicesSupported UEFI variable and mistakenly assumes that if
>> the variable does not exist none of the runtime services is implemented.
>
> Could you provide the result log for me to check?

https://github.com/U-Boot-EFI/u-boot-fwts-results/blob/master/fwts_20_11_fails.txt

is the results log from FWTS 20.11.

https://github.com/U-Boot-EFI/u-boot-fwts-results/blob/master/results-2020-10-31.txt

is the results log from a FWTS built from this branch:
https://github.com/xypron/fwts/commits/bugfixes

Best regards

Heinrich

>
> Ivan
>>
>> The correct thing to do in FWTS is:
>>
>> * read RuntimeServicesSupported via the ioctl
>> * if the ioctl fails assume that all runtime services
>>    are implemented
>> * if the ioctl fails with errno != ENOTTY write an error message
>> * for each runtime service marked as not supported
>>    check that it returns EFI_UNSUPPORTED
>> * for each service marked as supported
>>    check that it works correctly
>>
>> Best regards
>>
>> Heinrich
>>
>>> "
>>> Note that this is merely a hint to the OS, which it is free to ignore,
>>> and so the platform is still required to provide callable
>>> implementations of unsupported runtime services that simply return
>>> EFI_UNSUPPORTED.
>>> "
>>>
>>> Cheers,
>>> Ivan
>>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>>
>>>>> Cheers,
>>>>> Ivan
>>>>>
>>>>> On 11/28/20 3:20 AM, Heinrich Schuchardt wrote:
>>>>>> Since the UEFI 2.8A specification the UEFI enabled firmware provides a
>>>>>> configuration table EFI_RT_PROPERTIES_TABLE which indicates which
>>>>>> runtime
>>>>>> services are enabled. The EFI stub reads this table and saves the
>>>>>> value of
>>>>>> the field RuntimeServicesSupported internally.
>>>>>>
>>>>>> The Firmware Test Suite requires the value to determine if UEFI
>>>>>> runtime
>>>>>> services are correctly implemented.
>>>>>>
>>>>>> With this patch an IOCTL call is provided to read the value of the
>>>>>> field
>>>>>> RuntimeServicesSupported, e.g.
>>>>>>
>>>>>>        #define EFI_RUNTIME_GET_SUPPORTED_MASK \
>>>>>>                _IOR('p', 0x0C, unsigned int)
>>>>>>        unsigned int mask;
>>>>>>        fd = open("/dev/efi_test", O_RDWR);
>>>>>>        ret = ioctl(fd, EFI_RUNTIME_GET_SUPPORTED_MASK, &mask);
>>>>>>
>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>> ---
>>>>>>     drivers/firmware/efi/test/efi_test.c | 16 ++++++++++++++++
>>>>>>     drivers/firmware/efi/test/efi_test.h |  3 +++
>>>>>>     2 files changed, 19 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/firmware/efi/test/efi_test.c
>>>>>> b/drivers/firmware/efi/test/efi_test.c
>>>>>> index ddf9eae396fe..47d67bb0a516 100644
>>>>>> --- a/drivers/firmware/efi/test/efi_test.c
>>>>>> +++ b/drivers/firmware/efi/test/efi_test.c
>>>>>> @@ -663,6 +663,19 @@ static long
>>>>>> efi_runtime_query_capsulecaps(unsigned long arg)
>>>>>>         return rv;
>>>>>>     }
>>>>>>
>>>>>> +static long efi_runtime_get_supported_mask(unsigned long arg)
>>>>>> +{
>>>>>> +    unsigned int __user *supported_mask;
>>>>>> +    int rv = 0;
>>>>>> +
>>>>>> +    supported_mask = (unsigned int *)arg;
>>>>>> +
>>>>>> +    if (put_user(efi.runtime_supported_mask, supported_mask))
>>>>>> +        rv = -EFAULT;
>>>>>> +
>>>>>> +    return rv;
>>>>>> +}
>>>>>> +
>>>>>>     static long efi_test_ioctl(struct file *file, unsigned int cmd,
>>>>>>                                 unsigned long arg)
>>>>>>     {
>>>>>> @@ -699,6 +712,9 @@ static long efi_test_ioctl(struct file *file,
>>>>>> unsigned int cmd,
>>>>>>
>>>>>>         case EFI_RUNTIME_RESET_SYSTEM:
>>>>>>             return efi_runtime_reset_system(arg);
>>>>>> +
>>>>>> +    case EFI_RUNTIME_GET_SUPPORTED_MASK:
>>>>>> +        return efi_runtime_get_supported_mask(arg);
>>>>>>         }
>>>>>>
>>>>>>         return -ENOTTY;
>>>>>> diff --git a/drivers/firmware/efi/test/efi_test.h
>>>>>> b/drivers/firmware/efi/test/efi_test.h
>>>>>> index f2446aa1c2e3..117349e57993 100644
>>>>>> --- a/drivers/firmware/efi/test/efi_test.h
>>>>>> +++ b/drivers/firmware/efi/test/efi_test.h
>>>>>> @@ -118,4 +118,7 @@ struct efi_resetsystem {
>>>>>>     #define EFI_RUNTIME_RESET_SYSTEM \
>>>>>>         _IOW('p', 0x0B, struct efi_resetsystem)
>>>>>>
>>>>>> +#define EFI_RUNTIME_GET_SUPPORTED_MASK \
>>>>>> +    _IOR('p', 0x0C, unsigned int)
>>>>>> +
>>>>>>     #endif /* _DRIVERS_FIRMWARE_EFI_TEST_H_ */
>>>>>> --
>>>>>> 2.29.2
>>>>>>
>>>>
>>