linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Heinrich Schuchardt <xypron.glpk@gmx.de>
To: ivanhu <ivan.hu@canonical.com>, Ard Biesheuvel <ardb@kernel.org>
Cc: linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org,
	Colin King <colin.king@canonical.com>,
	fwts-devel@lists.ubuntu.com
Subject: Re: [PATCH 1/1] efi/efi_test: read RuntimeServicesSupported
Date: Thu, 3 Dec 2020 07:48:57 +0100	[thread overview]
Message-ID: <5c5ed9f7-616d-877c-6bc2-a2deef89b532@gmx.de> (raw)
In-Reply-To: <1315e36e-44f1-d753-935e-b6fbe71756e3@canonical.com>

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
>>>>>>
>>>>
>>


  reply	other threads:[~2020-12-03  6:50 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-27 19:20 [PATCH 1/1] efi/efi_test: read RuntimeServicesSupported Heinrich Schuchardt
2020-11-27 19:28 ` ACK: " Colin Ian King
2020-11-27 19:29   ` Ard Biesheuvel
2020-11-27 19:38     ` Colin Ian King
2020-11-27 19:39     ` Heinrich Schuchardt
2020-11-30  8:16 ` ivanhu
2020-11-30  9:17   ` Heinrich Schuchardt
2020-11-30 10:38     ` ivanhu
2020-12-02 11:38       ` Heinrich Schuchardt
2020-12-03  1:20         ` ivanhu
2020-12-03  6:48           ` Heinrich Schuchardt [this message]
2020-12-10 11:49 ` [tip: efi/core] " tip-bot2 for Heinrich Schuchardt
2020-12-26 10:16 ` [PATCH 1/1] " Heinrich Schuchardt
2020-12-29 13:01   ` Ard Biesheuvel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5c5ed9f7-616d-877c-6bc2-a2deef89b532@gmx.de \
    --to=xypron.glpk@gmx.de \
    --cc=ardb@kernel.org \
    --cc=colin.king@canonical.com \
    --cc=fwts-devel@lists.ubuntu.com \
    --cc=ivan.hu@canonical.com \
    --cc=linux-efi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).