linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/1] efi/efi_test: read RuntimeServicesSupported
@ 2020-11-27 19:20 Heinrich Schuchardt
  2020-11-27 19:28 ` ACK: " Colin Ian King
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Heinrich Schuchardt @ 2020-11-27 19:20 UTC (permalink / raw)
  To: Ivan Hu, Ard Biesheuvel
  Cc: linux-efi, linux-kernel, Colin King, fwts-devel, Heinrich Schuchardt

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* ACK: [PATCH 1/1] efi/efi_test: read RuntimeServicesSupported
  2020-11-27 19:20 [PATCH 1/1] efi/efi_test: read RuntimeServicesSupported Heinrich Schuchardt
@ 2020-11-27 19:28 ` Colin Ian King
  2020-11-27 19:29   ` Ard Biesheuvel
  2020-11-30  8:16 ` ivanhu
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Colin Ian King @ 2020-11-27 19:28 UTC (permalink / raw)
  To: Heinrich Schuchardt, Ivan Hu, Ard Biesheuvel
  Cc: linux-efi, linux-kernel, fwts-devel

On 27/11/2020 19:20, 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
> 

Looks good to me. Thanks Heinrich.

The EFI driver needs to be also updated in the linux kernel - has that
fix been submitted or do you require the fwts team to do that?

Colin

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: ACK: [PATCH 1/1] efi/efi_test: read RuntimeServicesSupported
  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
  0 siblings, 2 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2020-11-27 19:29 UTC (permalink / raw)
  To: Colin Ian King
  Cc: Heinrich Schuchardt, Ivan Hu, linux-efi,
	Linux Kernel Mailing List, fwts-devel

On Fri, 27 Nov 2020 at 20:28, Colin Ian King <colin.king@canonical.com> wrote:
>
> On 27/11/2020 19:20, 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
> >
>
> Looks good to me. Thanks Heinrich.
>
> The EFI driver needs to be also updated in the linux kernel - has that
> fix been submitted or do you require the fwts team to do that?
>

This /is/ the EFI driver.

I'll take this as an acked-by but I'd like Ivan to chime in as well.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: ACK: [PATCH 1/1] efi/efi_test: read RuntimeServicesSupported
  2020-11-27 19:29   ` Ard Biesheuvel
@ 2020-11-27 19:38     ` Colin Ian King
  2020-11-27 19:39     ` Heinrich Schuchardt
  1 sibling, 0 replies; 14+ messages in thread
From: Colin Ian King @ 2020-11-27 19:38 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Heinrich Schuchardt, Ivan Hu, linux-efi,
	Linux Kernel Mailing List, fwts-devel

On 27/11/2020 19:29, Ard Biesheuvel wrote:
> On Fri, 27 Nov 2020 at 20:28, Colin Ian King <colin.king@canonical.com> wrote:
>>
>> On 27/11/2020 19:20, 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
>>>
>>
>> Looks good to me. Thanks Heinrich.
>>
>> The EFI driver needs to be also updated in the linux kernel - has that
>> fix been submitted or do you require the fwts team to do that?

Oops. It's been a lot week :-(

>>
> 
> This /is/ the EFI driver.
> 
> I'll take this as an acked-by but I'd like Ivan to chime in as well.
> 
+1



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: ACK: [PATCH 1/1] efi/efi_test: read RuntimeServicesSupported
  2020-11-27 19:29   ` Ard Biesheuvel
  2020-11-27 19:38     ` Colin Ian King
@ 2020-11-27 19:39     ` Heinrich Schuchardt
  1 sibling, 0 replies; 14+ messages in thread
From: Heinrich Schuchardt @ 2020-11-27 19:39 UTC (permalink / raw)
  To: Ard Biesheuvel, Colin Ian King
  Cc: Ivan Hu, linux-efi, Linux Kernel Mailing List, fwts-devel

On 11/27/20 8:29 PM, Ard Biesheuvel wrote:
> On Fri, 27 Nov 2020 at 20:28, Colin Ian King <colin.king@canonical.com> wrote:
>>
>> On 27/11/2020 19:20, 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.
>>>
<snip />
>> Looks good to me. Thanks Heinrich.
>>
>> The EFI driver needs to be also updated in the linux kernel - has that
>> fix been submitted or do you require the fwts team to do that?
>>
>
> This /is/ the EFI driver.
>
> I'll take this as an acked-by but I'd like Ivan to chime in as well.
>

Hello Ard, hello Colin,

I have tested the patch with Linux 5.8.9.

Somehow Linux managed to break the kernel for my board between Linux
5.8.9 and 5.8.18. It does not boot form iSCSI with a newer kernel.

You probably want to run a user program against your latest kernel. This
is what I used:

#include <stdio.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/ioctl.h>
#include <sys/stat.h>
#include <sys/types.h>

#define EFI_RUNTIME_GET_SUPPORTED_MASK \
         _IOR('p', 0x0C, unsigned int)

int main()
{
         unsigned int i, flag;
         int fd, ret;
         unsigned int mask;

         fd = open("/dev/efi_test", O_RDWR);
         if (fd == -1) {
                 perror("open");
                 return 1;
         }

         ret = ioctl(fd, EFI_RUNTIME_GET_SUPPORTED_MASK, &mask);
         if (ret == -1) {
                 perror("ioctl");
                 return 1;
         }
         printf("mask 0x%08x\n", mask);

         flag = 1;
         for (i = 0x80000000; i; i >>= 1) {
                 if (i & mask) {
                         printf("%4s 0x%08x\n", flag ? "=" : "|", i);
                         flag = 0;
                 }
         }

         close(fd);

         return 0;
}

Best regards

Heinrich

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/1] efi/efi_test: read RuntimeServicesSupported
  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-30  8:16 ` ivanhu
  2020-11-30  9:17   ` Heinrich Schuchardt
  2020-12-10 11:49 ` [tip: efi/core] " tip-bot2 for Heinrich Schuchardt
  2020-12-26 10:16 ` [PATCH 1/1] " Heinrich Schuchardt
  3 siblings, 1 reply; 14+ messages in thread
From: ivanhu @ 2020-11-30  8:16 UTC (permalink / raw)
  To: Heinrich Schuchardt, Ard Biesheuvel
  Cc: linux-efi, linux-kernel, Colin King, fwts-devel

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.

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
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/1] efi/efi_test: read RuntimeServicesSupported
  2020-11-30  8:16 ` ivanhu
@ 2020-11-30  9:17   ` Heinrich Schuchardt
  2020-11-30 10:38     ` ivanhu
  0 siblings, 1 reply; 14+ messages in thread
From: Heinrich Schuchardt @ 2020-11-30  9:17 UTC (permalink / raw)
  To: ivanhu, Ard Biesheuvel; +Cc: linux-efi, linux-kernel, Colin King, fwts-devel

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.

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/1] efi/efi_test: read RuntimeServicesSupported
  2020-11-30  9:17   ` Heinrich Schuchardt
@ 2020-11-30 10:38     ` ivanhu
  2020-12-02 11:38       ` Heinrich Schuchardt
  0 siblings, 1 reply; 14+ messages in thread
From: ivanhu @ 2020-11-30 10:38 UTC (permalink / raw)
  To: Heinrich Schuchardt, Ard Biesheuvel
  Cc: linux-efi, linux-kernel, Colin King, fwts-devel



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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/1] efi/efi_test: read RuntimeServicesSupported
  2020-11-30 10:38     ` ivanhu
@ 2020-12-02 11:38       ` Heinrich Schuchardt
  2020-12-03  1:20         ` ivanhu
  0 siblings, 1 reply; 14+ messages in thread
From: Heinrich Schuchardt @ 2020-12-02 11:38 UTC (permalink / raw)
  To: ivanhu, Ard Biesheuvel; +Cc: linux-efi, linux-kernel, Colin King, fwts-devel

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.

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/1] efi/efi_test: read RuntimeServicesSupported
  2020-12-02 11:38       ` Heinrich Schuchardt
@ 2020-12-03  1:20         ` ivanhu
  2020-12-03  6:48           ` Heinrich Schuchardt
  0 siblings, 1 reply; 14+ messages in thread
From: ivanhu @ 2020-12-03  1:20 UTC (permalink / raw)
  To: Heinrich Schuchardt, Ard Biesheuvel
  Cc: linux-efi, linux-kernel, Colin King, fwts-devel



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?

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/1] efi/efi_test: read RuntimeServicesSupported
  2020-12-03  1:20         ` ivanhu
@ 2020-12-03  6:48           ` Heinrich Schuchardt
  0 siblings, 0 replies; 14+ messages in thread
From: Heinrich Schuchardt @ 2020-12-03  6:48 UTC (permalink / raw)
  To: ivanhu, Ard Biesheuvel; +Cc: linux-efi, linux-kernel, Colin King, fwts-devel

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [tip: efi/core] efi/efi_test: read RuntimeServicesSupported
  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-30  8:16 ` ivanhu
@ 2020-12-10 11:49 ` tip-bot2 for Heinrich Schuchardt
  2020-12-26 10:16 ` [PATCH 1/1] " Heinrich Schuchardt
  3 siblings, 0 replies; 14+ messages in thread
From: tip-bot2 for Heinrich Schuchardt @ 2020-12-10 11:49 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Heinrich Schuchardt, Colin Ian King, Ivan Hu, Ard Biesheuvel,
	x86, linux-kernel

The following commit has been merged into the efi/core branch of tip:

Commit-ID:     ff20661bb54cd57a18207b33cc57eb8d5c758a86
Gitweb:        https://git.kernel.org/tip/ff20661bb54cd57a18207b33cc57eb8d5c758a86
Author:        Heinrich Schuchardt <xypron.glpk@gmx.de>
AuthorDate:    Fri, 27 Nov 2020 20:20:51 +01:00
Committer:     Ard Biesheuvel <ardb@kernel.org>
CommitterDate: Wed, 09 Dec 2020 08:37:27 +01:00

efi/efi_test: read RuntimeServicesSupported

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>
Link: https://lore.kernel.org/r/20201127192051.1430-1-xypron.glpk@gmx.de
Acked-by: Colin Ian King <colin.king@canonical.com>
Acked-by: Ivan Hu <ivan.hu@canonical.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 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 ddf9eae..47d67bb 100644
--- a/drivers/firmware/efi/test/efi_test.c
+++ b/drivers/firmware/efi/test/efi_test.c
@@ -663,6 +663,19 @@ out:
 	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 f2446aa..117349e 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_ */

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/1] efi/efi_test: read RuntimeServicesSupported
  2020-11-27 19:20 [PATCH 1/1] efi/efi_test: read RuntimeServicesSupported Heinrich Schuchardt
                   ` (2 preceding siblings ...)
  2020-12-10 11:49 ` [tip: efi/core] " tip-bot2 for Heinrich Schuchardt
@ 2020-12-26 10:16 ` Heinrich Schuchardt
  2020-12-29 13:01   ` Ard Biesheuvel
  3 siblings, 1 reply; 14+ messages in thread
From: Heinrich Schuchardt @ 2020-12-26 10:16 UTC (permalink / raw)
  To: Ivan Hu, Ard Biesheuvel; +Cc: linux-efi, linux-kernel, Colin King, fwts-devel

On 11/27/20 8:20 PM, 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>

Hello Ard,

the patch has now been admitted to Linus' branch.

Could we, please, have this patch applied to the 5.10 long term release,
too.

Best regards

Heinrich

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/1] efi/efi_test: read RuntimeServicesSupported
  2020-12-26 10:16 ` [PATCH 1/1] " Heinrich Schuchardt
@ 2020-12-29 13:01   ` Ard Biesheuvel
  0 siblings, 0 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2020-12-29 13:01 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: Ivan Hu, linux-efi, Linux Kernel Mailing List, Colin King, fwts-devel

On Sat, 26 Dec 2020 at 11:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 11/27/20 8:20 PM, 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>
>
> Hello Ard,
>
> the patch has now been admitted to Linus' branch.
>
> Could we, please, have this patch applied to the 5.10 long term release,
> too.
>

If you think this patch needs to go to -stable, please send an email
to stable@vger.kernel.org containing the commit title and SHA1, and a
short motivation why this patch needs to be backported.

If the stable maintainers are willing to take it, I won't object to it.

Thanks,
Ard.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-12-29 13:02 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

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