| Message ID | 20201227205225.1091-3-xypron.glpk@gmx.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | uefi: fix fwts_uefi_rt_support_status_get() | expand |
On 12/28/20 4:52 AM, Heinrich Schuchardt wrote: > The UEFI 2.8 Errata A specification has clarified that > RuntimeServicesSupported is not a UEFI variable but a value in the > EFI_RT_PROPERTIES_TABLE configuration table. Since Linux 5.11 the value can > be retrieved via an IOCTL call. > > Replace the code trying to read the non-existent RuntimeServicesSupported > variable by the IOCTL call. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > src/lib/include/fwts_uefi.h | 4 +- > src/lib/src/fwts_uefi.c | 47 ++++++------------------ > src/uefi/uefirtmisc/uefirtmisc.c | 12 +----- > src/uefi/uefirttime/uefirttime.c | 13 +------ > src/uefi/uefirtvariable/uefirtvariable.c | 13 +------ > 5 files changed, 18 insertions(+), 71 deletions(-) > > diff --git a/src/lib/include/fwts_uefi.h b/src/lib/include/fwts_uefi.h > index 36cd5824..5e69ec62 100644 > --- a/src/lib/include/fwts_uefi.h > +++ b/src/lib/include/fwts_uefi.h > @@ -137,6 +137,8 @@ enum { > #define EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES 0x1000 > #define EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO 0x2000 > > +#define EFI_RT_SUPPORTED_ALL 0x3fff > + > #define EFI_CERT_SHA256_GUID \ > { 0xc1c41626, 0x504c, 0x4092, { 0xac, 0xa9, 0x41, 0xf9, 0x36, 0x93, 0x43, 0x28 }} > > @@ -680,7 +682,7 @@ char *fwts_uefi_attribute_info(uint32_t attr); > > bool fwts_uefi_efivars_iface_exist(void); > > -void fwts_uefi_rt_support_status_get(int fd, bool *getvar_supported, uint32_t *var_rtsupported); > +void fwts_uefi_rt_support_status_get(int fd, uint32_t *var_rtsupported); > > PRAGMA_POP > > diff --git a/src/lib/src/fwts_uefi.c b/src/lib/src/fwts_uefi.c > index 0fb6b799..83799a98 100644 > --- a/src/lib/src/fwts_uefi.c > +++ b/src/lib/src/fwts_uefi.c > @@ -541,45 +541,22 @@ bool fwts_uefi_efivars_iface_exist(void) > > /* > * fwts_uefi_rt_support_status_get() > - * get the status of runtime service support and the value of > - * the RuntimeServicesSupported variable > + * get the status of runtime service support. > + * > + * Since UEFI 2.8 Errata A the EFI_RT_PROPERTIES_TABLE configuration table > + * can be used to indicate which UEFI runtime services are not implemented > + * via the bitmask RuntimeServicesSupported. If the table is not present, > + * all runtime services are to be considered available. Since Linux 5.11 > + * this bitmask can be read via an IOCTL call. Before Linux 5.11 the value > + * cannot be determined. > */ > -void fwts_uefi_rt_support_status_get(int fd, bool *getvar_supported, uint32_t *var_rtsupported) > +void fwts_uefi_rt_support_status_get(int fd, uint32_t *var_rtsupported) > { > long ioret; > - struct efi_getvariable getvariable; > - uint64_t status = ~0ULL; > - uint8_t data[512]; > - uint64_t getdatasize = sizeof(data); > - *var_rtsupported = 0xFFFF; > - > - uint32_t attributes = FWTS_UEFI_VAR_NON_VOLATILE | > - FWTS_UEFI_VAR_BOOTSERVICE_ACCESS | > - FWTS_UEFI_VAR_RUNTIME_ACCESS; > - static uint16_t varname[] = {'R', 'u', 'n', 't', 'i', 'm', 'e', 'S', 'e', > - 'r', 'v', 'i', 'c', 'e', 's', 'S', 'u', 'p', > - 'p', 'o', 'r', 't', 'e', 'd', '\0'}; > - EFI_GUID global_var_guid = EFI_GLOBAL_VARIABLE; > - getvariable.VariableName = varname; > - getvariable.VendorGuid = &global_var_guid; > - getvariable.Attributes = &attributes; > - getvariable.DataSize = &getdatasize; > - getvariable.Data = data; > - getvariable.status = &status; > - > - ioret = ioctl(fd, EFI_RUNTIME_GET_VARIABLE, &getvariable); > - if (ioret == -1) { > - if (status == EFI_NOT_FOUND) { > - *getvar_supported = true; > - } else { > - *getvar_supported = false; > - } > - return; > - } > > - *getvar_supported = true; > - *var_rtsupported = data[0] | data[1] << 8; > + ioret = ioctl(fd, EFI_RUNTIME_GET_SUPPORTED_MASK, var_rtsupported); > + if (ioret == -1) > + *var_rtsupported = EFI_RT_SUPPORTED_ALL; Hi Heinrich, Thanks for the patches. We should check the RuntimeServicesSupported supported or not and skip the tests with some info messages, not just assume all supported. Or we will get the false results. Such as, for those not support RuntimeServicesSupported machines, they will be tested and get pass results. Cheers, Ivan > > return; > - > } > diff --git a/src/uefi/uefirtmisc/uefirtmisc.c b/src/uefi/uefirtmisc/uefirtmisc.c > index a2dc6967..4b04f78a 100644 > --- a/src/uefi/uefirtmisc/uefirtmisc.c > +++ b/src/uefi/uefirtmisc/uefirtmisc.c > @@ -245,22 +245,12 @@ static int uefirtmisc_test4(fwts_framework *fw) > { > long ioret; > uint64_t status; > - bool getvar_supported; > uint32_t var_runtimeservicessupported; > > struct efi_getnexthighmonotoniccount getnexthighmonotoniccount; > uint32_t highcount; > > - fwts_uefi_rt_support_status_get(fd, &getvar_supported, > - &var_runtimeservicessupported); > - if (!getvar_supported || (var_runtimeservicessupported == 0xFFFF)) { > - fwts_skipped(fw, "Cannot get the RuntimeServicesSupported " > - "variable, maybe the runtime service " > - "GetVariable is not supported or " > - "RuntimeServicesSupported not provided by " > - "firmware."); > - return FWTS_SKIP; > - } > + fwts_uefi_rt_support_status_get(fd, &var_runtimeservicessupported); > > getnexthighmonotoniccount.HighCount = &highcount; > getnexthighmonotoniccount.status = &status; > diff --git a/src/uefi/uefirttime/uefirttime.c b/src/uefi/uefirttime/uefirttime.c > index e0aa7071..67365759 100644 > --- a/src/uefi/uefirttime/uefirttime.c > +++ b/src/uefi/uefirttime/uefirttime.c > @@ -1142,7 +1142,6 @@ static int uefirttime_test37(fwts_framework *fw) > static int uefirttime_test38(fwts_framework *fw) > { > long ioret; > - bool getvar_supported; > uint32_t var_runtimeservicessupported; > > struct efi_settime settime; > @@ -1155,17 +1154,7 @@ static int uefirttime_test38(fwts_framework *fw) > EFI_TIME efi_time; > EFI_TIME_CAPABILITIES efi_time_cap; > > - fwts_uefi_rt_support_status_get(fd, &getvar_supported, > - &var_runtimeservicessupported); > - > - if (!getvar_supported || (var_runtimeservicessupported == 0xFFFF)) { > - fwts_skipped(fw, "Cannot get the RuntimeServicesSupported " > - "variable, maybe the runtime service " > - "GetVariable is not supported or " > - "RuntimeServicesSupported not provided by " > - "firmware."); > - return FWTS_SKIP; > - } > + fwts_uefi_rt_support_status_get(fd, &var_runtimeservicessupported); > > gettime.Capabilities = &efi_time_cap; > gettime.Time = &efi_time; > diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c > index 713803dc..385c5405 100644 > --- a/src/uefi/uefirtvariable/uefirtvariable.c > +++ b/src/uefi/uefirtvariable/uefirtvariable.c > @@ -2020,7 +2020,6 @@ static int uefirtvariable_test9(fwts_framework *fw) > { > long ioret; > > - bool getvar_supported; > uint32_t var_runtimeservicessupported; > > struct efi_getvariable getvariable; > @@ -2041,17 +2040,7 @@ static int uefirtvariable_test9(fwts_framework *fw) > uint64_t getdatasize = sizeof(testdata); > uint32_t attr; > > - fwts_uefi_rt_support_status_get(fd, &getvar_supported, > - &var_runtimeservicessupported); > - > - if (!getvar_supported || (var_runtimeservicessupported == 0xFFFF)) { > - fwts_skipped(fw, "Cannot get the RuntimeServicesSupported " > - "variable, maybe the runtime service " > - "GetVariable is not supported or " > - "RuntimeServicesSupported not provided by " > - "firmware."); > - return FWTS_SKIP; > - } > + fwts_uefi_rt_support_status_get(fd, &var_runtimeservicessupported); > > setvariable.VariableName = variablenametest; > setvariable.VendorGuid = >estguid1; > -- > 2.29.2 >
On 12/28/20 8:09 AM, ivanhu wrote: > > > On 12/28/20 4:52 AM, Heinrich Schuchardt wrote: >> The UEFI 2.8 Errata A specification has clarified that >> RuntimeServicesSupported is not a UEFI variable but a value in the >> EFI_RT_PROPERTIES_TABLE configuration table. Since Linux 5.11 the value can >> be retrieved via an IOCTL call. >> >> Replace the code trying to read the non-existent RuntimeServicesSupported >> variable by the IOCTL call. >> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> --- >> src/lib/include/fwts_uefi.h | 4 +- >> src/lib/src/fwts_uefi.c | 47 ++++++------------------ >> src/uefi/uefirtmisc/uefirtmisc.c | 12 +----- >> src/uefi/uefirttime/uefirttime.c | 13 +------ >> src/uefi/uefirtvariable/uefirtvariable.c | 13 +------ >> 5 files changed, 18 insertions(+), 71 deletions(-) >> >> diff --git a/src/lib/include/fwts_uefi.h b/src/lib/include/fwts_uefi.h >> index 36cd5824..5e69ec62 100644 >> --- a/src/lib/include/fwts_uefi.h >> +++ b/src/lib/include/fwts_uefi.h >> @@ -137,6 +137,8 @@ enum { >> #define EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES 0x1000 >> #define EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO 0x2000 >> >> +#define EFI_RT_SUPPORTED_ALL 0x3fff >> + >> #define EFI_CERT_SHA256_GUID \ >> { 0xc1c41626, 0x504c, 0x4092, { 0xac, 0xa9, 0x41, 0xf9, 0x36, 0x93, 0x43, 0x28 }} >> >> @@ -680,7 +682,7 @@ char *fwts_uefi_attribute_info(uint32_t attr); >> >> bool fwts_uefi_efivars_iface_exist(void); >> >> -void fwts_uefi_rt_support_status_get(int fd, bool *getvar_supported, uint32_t *var_rtsupported); >> +void fwts_uefi_rt_support_status_get(int fd, uint32_t *var_rtsupported); >> >> PRAGMA_POP >> >> diff --git a/src/lib/src/fwts_uefi.c b/src/lib/src/fwts_uefi.c >> index 0fb6b799..83799a98 100644 >> --- a/src/lib/src/fwts_uefi.c >> +++ b/src/lib/src/fwts_uefi.c >> @@ -541,45 +541,22 @@ bool fwts_uefi_efivars_iface_exist(void) >> >> /* >> * fwts_uefi_rt_support_status_get() >> - * get the status of runtime service support and the value of >> - * the RuntimeServicesSupported variable >> + * get the status of runtime service support. >> + * >> + * Since UEFI 2.8 Errata A the EFI_RT_PROPERTIES_TABLE configuration table >> + * can be used to indicate which UEFI runtime services are not implemented >> + * via the bitmask RuntimeServicesSupported. If the table is not present, >> + * all runtime services are to be considered available. Since Linux 5.11 >> + * this bitmask can be read via an IOCTL call. Before Linux 5.11 the value >> + * cannot be determined. >> */ >> -void fwts_uefi_rt_support_status_get(int fd, bool *getvar_supported, uint32_t *var_rtsupported) >> +void fwts_uefi_rt_support_status_get(int fd, uint32_t *var_rtsupported) >> { >> long ioret; >> - struct efi_getvariable getvariable; >> - uint64_t status = ~0ULL; >> - uint8_t data[512]; >> - uint64_t getdatasize = sizeof(data); >> - *var_rtsupported = 0xFFFF; >> - >> - uint32_t attributes = FWTS_UEFI_VAR_NON_VOLATILE | >> - FWTS_UEFI_VAR_BOOTSERVICE_ACCESS | >> - FWTS_UEFI_VAR_RUNTIME_ACCESS; >> - static uint16_t varname[] = {'R', 'u', 'n', 't', 'i', 'm', 'e', 'S', 'e', >> - 'r', 'v', 'i', 'c', 'e', 's', 'S', 'u', 'p', >> - 'p', 'o', 'r', 't', 'e', 'd', '\0'}; >> - EFI_GUID global_var_guid = EFI_GLOBAL_VARIABLE; >> - getvariable.VariableName = varname; >> - getvariable.VendorGuid = &global_var_guid; >> - getvariable.Attributes = &attributes; >> - getvariable.DataSize = &getdatasize; >> - getvariable.Data = data; >> - getvariable.status = &status; >> - >> - ioret = ioctl(fd, EFI_RUNTIME_GET_VARIABLE, &getvariable); >> - if (ioret == -1) { >> - if (status == EFI_NOT_FOUND) { >> - *getvar_supported = true; >> - } else { >> - *getvar_supported = false; >> - } >> - return; >> - } >> >> - *getvar_supported = true; >> - *var_rtsupported = data[0] | data[1] << 8; >> + ioret = ioctl(fd, EFI_RUNTIME_GET_SUPPORTED_MASK, var_rtsupported); >> + if (ioret == -1) >> + *var_rtsupported = EFI_RT_SUPPORTED_ALL; > > Hi Heinrich, > > Thanks for the patches. > We should check the RuntimeServicesSupported supported or not and skip > the tests with some info messages, not just assume all supported. Or we > will get the false results. Such as, for those not support > RuntimeServicesSupported machines, they will be tested and get pass results. Thanks for reviewing. fwts_uefi_rt_support_status_get() only determines the bits describing if a runtime service should be implemented. In uefirtmisc.c, uefirttime.c, uefirtvariable.c a message is written if a test is skipped because the the bit is not set or the if the bit is set but the runtime service returns EFI_UNSUPPORTED. You can see such a SKIPPED message at https://github.com/U-Boot-EFI/u-boot-fwts-results/blob/master/results-2020-12-27.txt#L158 So it is unclear to me what you are missing. Best regards Heinrich > > Cheers, > Ivan > >> >> return; >> - >> } >> diff --git a/src/uefi/uefirtmisc/uefirtmisc.c b/src/uefi/uefirtmisc/uefirtmisc.c >> index a2dc6967..4b04f78a 100644 >> --- a/src/uefi/uefirtmisc/uefirtmisc.c >> +++ b/src/uefi/uefirtmisc/uefirtmisc.c >> @@ -245,22 +245,12 @@ static int uefirtmisc_test4(fwts_framework *fw) >> { >> long ioret; >> uint64_t status; >> - bool getvar_supported; >> uint32_t var_runtimeservicessupported; >> >> struct efi_getnexthighmonotoniccount getnexthighmonotoniccount; >> uint32_t highcount; >> >> - fwts_uefi_rt_support_status_get(fd, &getvar_supported, >> - &var_runtimeservicessupported); >> - if (!getvar_supported || (var_runtimeservicessupported == 0xFFFF)) { >> - fwts_skipped(fw, "Cannot get the RuntimeServicesSupported " >> - "variable, maybe the runtime service " >> - "GetVariable is not supported or " >> - "RuntimeServicesSupported not provided by " >> - "firmware."); >> - return FWTS_SKIP; >> - } >> + fwts_uefi_rt_support_status_get(fd, &var_runtimeservicessupported); >> >> getnexthighmonotoniccount.HighCount = &highcount; >> getnexthighmonotoniccount.status = &status; >> diff --git a/src/uefi/uefirttime/uefirttime.c b/src/uefi/uefirttime/uefirttime.c >> index e0aa7071..67365759 100644 >> --- a/src/uefi/uefirttime/uefirttime.c >> +++ b/src/uefi/uefirttime/uefirttime.c >> @@ -1142,7 +1142,6 @@ static int uefirttime_test37(fwts_framework *fw) >> static int uefirttime_test38(fwts_framework *fw) >> { >> long ioret; >> - bool getvar_supported; >> uint32_t var_runtimeservicessupported; >> >> struct efi_settime settime; >> @@ -1155,17 +1154,7 @@ static int uefirttime_test38(fwts_framework *fw) >> EFI_TIME efi_time; >> EFI_TIME_CAPABILITIES efi_time_cap; >> >> - fwts_uefi_rt_support_status_get(fd, &getvar_supported, >> - &var_runtimeservicessupported); >> - >> - if (!getvar_supported || (var_runtimeservicessupported == 0xFFFF)) { >> - fwts_skipped(fw, "Cannot get the RuntimeServicesSupported " >> - "variable, maybe the runtime service " >> - "GetVariable is not supported or " >> - "RuntimeServicesSupported not provided by " >> - "firmware."); >> - return FWTS_SKIP; >> - } >> + fwts_uefi_rt_support_status_get(fd, &var_runtimeservicessupported); >> >> gettime.Capabilities = &efi_time_cap; >> gettime.Time = &efi_time; >> diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c >> index 713803dc..385c5405 100644 >> --- a/src/uefi/uefirtvariable/uefirtvariable.c >> +++ b/src/uefi/uefirtvariable/uefirtvariable.c >> @@ -2020,7 +2020,6 @@ static int uefirtvariable_test9(fwts_framework *fw) >> { >> long ioret; >> >> - bool getvar_supported; >> uint32_t var_runtimeservicessupported; >> >> struct efi_getvariable getvariable; >> @@ -2041,17 +2040,7 @@ static int uefirtvariable_test9(fwts_framework *fw) >> uint64_t getdatasize = sizeof(testdata); >> uint32_t attr; >> >> - fwts_uefi_rt_support_status_get(fd, &getvar_supported, >> - &var_runtimeservicessupported); >> - >> - if (!getvar_supported || (var_runtimeservicessupported == 0xFFFF)) { >> - fwts_skipped(fw, "Cannot get the RuntimeServicesSupported " >> - "variable, maybe the runtime service " >> - "GetVariable is not supported or " >> - "RuntimeServicesSupported not provided by " >> - "firmware."); >> - return FWTS_SKIP; >> - } >> + fwts_uefi_rt_support_status_get(fd, &var_runtimeservicessupported); >> >> setvariable.VariableName = variablenametest; >> setvariable.VendorGuid = >estguid1; >> -- >> 2.29.2 >>
On 12/28/20 3:37 PM, Heinrich Schuchardt wrote: > On 12/28/20 8:09 AM, ivanhu wrote: >> >> >> On 12/28/20 4:52 AM, Heinrich Schuchardt wrote: >>> The UEFI 2.8 Errata A specification has clarified that >>> RuntimeServicesSupported is not a UEFI variable but a value in the >>> EFI_RT_PROPERTIES_TABLE configuration table. Since Linux 5.11 the >>> value can >>> be retrieved via an IOCTL call. >>> >>> Replace the code trying to read the non-existent >>> RuntimeServicesSupported >>> variable by the IOCTL call. >>> >>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >>> --- >>> src/lib/include/fwts_uefi.h | 4 +- >>> src/lib/src/fwts_uefi.c | 47 ++++++------------------ >>> src/uefi/uefirtmisc/uefirtmisc.c | 12 +----- >>> src/uefi/uefirttime/uefirttime.c | 13 +------ >>> src/uefi/uefirtvariable/uefirtvariable.c | 13 +------ >>> 5 files changed, 18 insertions(+), 71 deletions(-) >>> >>> diff --git a/src/lib/include/fwts_uefi.h b/src/lib/include/fwts_uefi.h >>> index 36cd5824..5e69ec62 100644 >>> --- a/src/lib/include/fwts_uefi.h >>> +++ b/src/lib/include/fwts_uefi.h >>> @@ -137,6 +137,8 @@ enum { >>> #define EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES 0x1000 >>> #define EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO 0x2000 >>> >>> +#define EFI_RT_SUPPORTED_ALL 0x3fff >>> + >>> #define EFI_CERT_SHA256_GUID \ >>> { 0xc1c41626, 0x504c, 0x4092, { 0xac, 0xa9, 0x41, 0xf9, 0x36, 0x93, >>> 0x43, 0x28 }} >>> >>> @@ -680,7 +682,7 @@ char *fwts_uefi_attribute_info(uint32_t attr); >>> >>> bool fwts_uefi_efivars_iface_exist(void); >>> >>> -void fwts_uefi_rt_support_status_get(int fd, bool *getvar_supported, >>> uint32_t *var_rtsupported); >>> +void fwts_uefi_rt_support_status_get(int fd, uint32_t >>> *var_rtsupported); >>> >>> PRAGMA_POP >>> >>> diff --git a/src/lib/src/fwts_uefi.c b/src/lib/src/fwts_uefi.c >>> index 0fb6b799..83799a98 100644 >>> --- a/src/lib/src/fwts_uefi.c >>> +++ b/src/lib/src/fwts_uefi.c >>> @@ -541,45 +541,22 @@ bool fwts_uefi_efivars_iface_exist(void) >>> >>> /* >>> * fwts_uefi_rt_support_status_get() >>> - * get the status of runtime service support and the value of >>> - * the RuntimeServicesSupported variable >>> + * get the status of runtime service support. >>> + * >>> + * Since UEFI 2.8 Errata A the EFI_RT_PROPERTIES_TABLE >>> configuration table >>> + * can be used to indicate which UEFI runtime services are not >>> implemented >>> + * via the bitmask RuntimeServicesSupported. If the table is not >>> present, >>> + * all runtime services are to be considered available. Since >>> Linux 5.11 >>> + * this bitmask can be read via an IOCTL call. Before Linux 5.11 >>> the value >>> + * cannot be determined. >>> */ >>> -void fwts_uefi_rt_support_status_get(int fd, bool *getvar_supported, >>> uint32_t *var_rtsupported) >>> +void fwts_uefi_rt_support_status_get(int fd, uint32_t *var_rtsupported) >>> { >>> long ioret; >>> - struct efi_getvariable getvariable; >>> - uint64_t status = ~0ULL; >>> - uint8_t data[512]; >>> - uint64_t getdatasize = sizeof(data); >>> - *var_rtsupported = 0xFFFF; >>> - >>> - uint32_t attributes = FWTS_UEFI_VAR_NON_VOLATILE | >>> - FWTS_UEFI_VAR_BOOTSERVICE_ACCESS | >>> - FWTS_UEFI_VAR_RUNTIME_ACCESS; >>> - static uint16_t varname[] = {'R', 'u', 'n', 't', 'i', 'm', 'e', >>> 'S', 'e', >>> - 'r', 'v', 'i', 'c', 'e', 's', 'S', 'u', 'p', >>> - 'p', 'o', 'r', 't', 'e', 'd', '\0'}; >>> - EFI_GUID global_var_guid = EFI_GLOBAL_VARIABLE; >>> - getvariable.VariableName = varname; >>> - getvariable.VendorGuid = &global_var_guid; >>> - getvariable.Attributes = &attributes; >>> - getvariable.DataSize = &getdatasize; >>> - getvariable.Data = data; >>> - getvariable.status = &status; >>> - >>> - ioret = ioctl(fd, EFI_RUNTIME_GET_VARIABLE, &getvariable); >>> - if (ioret == -1) { >>> - if (status == EFI_NOT_FOUND) { >>> - *getvar_supported = true; >>> - } else { >>> - *getvar_supported = false; >>> - } >>> - return; >>> - } >>> >>> - *getvar_supported = true; >>> - *var_rtsupported = data[0] | data[1] << 8; >>> + ioret = ioctl(fd, EFI_RUNTIME_GET_SUPPORTED_MASK, var_rtsupported); >>> + if (ioret == -1) >>> + *var_rtsupported = EFI_RT_SUPPORTED_ALL; >> >> Hi Heinrich, >> >> Thanks for the patches. >> We should check the RuntimeServicesSupported supported or not and skip >> the tests with some info messages, not just assume all supported. Or we >> will get the false results. Such as, for those not support >> RuntimeServicesSupported machines, they will be tested and get pass >> results. > > Thanks for reviewing. > > fwts_uefi_rt_support_status_get() only determines the bits describing if > a runtime service should be implemented. In uefirtmisc.c, uefirttime.c, > uefirtvariable.c a message is written if a test is skipped because the > the bit is not set or the if the bit is set but the runtime service > returns EFI_UNSUPPORTED. You can see such a SKIPPED message at > > https://github.com/U-Boot-EFI/u-boot-fwts-results/blob/master/results-2020-12-27.txt#L158 > > > So it is unclear to me what you are missing. The tests uefirttime_test38, uefirtmisc_test4 and uefirtvariable_test9 test the consistency between RuntimeServicesSupported and actually implemented by firmware. These tests should be skipped if RuntimeServicesSupported not supported, or we will get false results. Such as, on my machine, RuntimeServicesSupported is not supported but I get pass for those tests. Test 9 of 9: Test UEFI RT variable services supported status. PASSED: Test 9, UEFI SetVariable runtime service supported status test passed. PASSED: Test 9, UEFI GetVariable runtime service supported status test passed. PASSED: Test 9, UEFI GetNextVarName runtime service supported status test passed. PASSED: Test 9, UEFI QueryVarInfo runtime service supported status test passed. Cheers, Ivan > > Best regards > > Heinrich > >> >> Cheers, >> Ivan >> >>> >>> return; >>> - >>> } >>> diff --git a/src/uefi/uefirtmisc/uefirtmisc.c >>> b/src/uefi/uefirtmisc/uefirtmisc.c >>> index a2dc6967..4b04f78a 100644 >>> --- a/src/uefi/uefirtmisc/uefirtmisc.c >>> +++ b/src/uefi/uefirtmisc/uefirtmisc.c >>> @@ -245,22 +245,12 @@ static int uefirtmisc_test4(fwts_framework *fw) >>> { >>> long ioret; >>> uint64_t status; >>> - bool getvar_supported; >>> uint32_t var_runtimeservicessupported; >>> >>> struct efi_getnexthighmonotoniccount getnexthighmonotoniccount; >>> uint32_t highcount; >>> >>> - fwts_uefi_rt_support_status_get(fd, &getvar_supported, >>> - &var_runtimeservicessupported); >>> - if (!getvar_supported || (var_runtimeservicessupported == >>> 0xFFFF)) { >>> - fwts_skipped(fw, "Cannot get the RuntimeServicesSupported " >>> - "variable, maybe the runtime service " >>> - "GetVariable is not supported or " >>> - "RuntimeServicesSupported not provided by " >>> - "firmware."); >>> - return FWTS_SKIP; >>> - } >>> + fwts_uefi_rt_support_status_get(fd, &var_runtimeservicessupported); >>> >>> getnexthighmonotoniccount.HighCount = &highcount; >>> getnexthighmonotoniccount.status = &status; >>> diff --git a/src/uefi/uefirttime/uefirttime.c >>> b/src/uefi/uefirttime/uefirttime.c >>> index e0aa7071..67365759 100644 >>> --- a/src/uefi/uefirttime/uefirttime.c >>> +++ b/src/uefi/uefirttime/uefirttime.c >>> @@ -1142,7 +1142,6 @@ static int uefirttime_test37(fwts_framework *fw) >>> static int uefirttime_test38(fwts_framework *fw) >>> { >>> long ioret; >>> - bool getvar_supported; >>> uint32_t var_runtimeservicessupported; >>> >>> struct efi_settime settime; >>> @@ -1155,17 +1154,7 @@ static int uefirttime_test38(fwts_framework *fw) >>> EFI_TIME efi_time; >>> EFI_TIME_CAPABILITIES efi_time_cap; >>> >>> - fwts_uefi_rt_support_status_get(fd, &getvar_supported, >>> - &var_runtimeservicessupported); >>> - >>> - if (!getvar_supported || (var_runtimeservicessupported == >>> 0xFFFF)) { >>> - fwts_skipped(fw, "Cannot get the RuntimeServicesSupported " >>> - "variable, maybe the runtime service " >>> - "GetVariable is not supported or " >>> - "RuntimeServicesSupported not provided by " >>> - "firmware."); >>> - return FWTS_SKIP; >>> - } >>> + fwts_uefi_rt_support_status_get(fd, &var_runtimeservicessupported); >>> >>> gettime.Capabilities = &efi_time_cap; >>> gettime.Time = &efi_time; >>> diff --git a/src/uefi/uefirtvariable/uefirtvariable.c >>> b/src/uefi/uefirtvariable/uefirtvariable.c >>> index 713803dc..385c5405 100644 >>> --- a/src/uefi/uefirtvariable/uefirtvariable.c >>> +++ b/src/uefi/uefirtvariable/uefirtvariable.c >>> @@ -2020,7 +2020,6 @@ static int uefirtvariable_test9(fwts_framework >>> *fw) >>> { >>> long ioret; >>> >>> - bool getvar_supported; >>> uint32_t var_runtimeservicessupported; >>> >>> struct efi_getvariable getvariable; >>> @@ -2041,17 +2040,7 @@ static int uefirtvariable_test9(fwts_framework >>> *fw) >>> uint64_t getdatasize = sizeof(testdata); >>> uint32_t attr; >>> >>> - fwts_uefi_rt_support_status_get(fd, &getvar_supported, >>> - &var_runtimeservicessupported); >>> - >>> - if (!getvar_supported || (var_runtimeservicessupported == >>> 0xFFFF)) { >>> - fwts_skipped(fw, "Cannot get the RuntimeServicesSupported " >>> - "variable, maybe the runtime service " >>> - "GetVariable is not supported or " >>> - "RuntimeServicesSupported not provided by " >>> - "firmware."); >>> - return FWTS_SKIP; >>> - } >>> + fwts_uefi_rt_support_status_get(fd, &var_runtimeservicessupported); >>> >>> setvariable.VariableName = variablenametest; >>> setvariable.VendorGuid = >estguid1; >>> -- >>> 2.29.2 >>> >
diff --git a/src/lib/include/fwts_uefi.h b/src/lib/include/fwts_uefi.h index 36cd5824..5e69ec62 100644 --- a/src/lib/include/fwts_uefi.h +++ b/src/lib/include/fwts_uefi.h @@ -137,6 +137,8 @@ enum { #define EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES 0x1000 #define EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO 0x2000 +#define EFI_RT_SUPPORTED_ALL 0x3fff + #define EFI_CERT_SHA256_GUID \ { 0xc1c41626, 0x504c, 0x4092, { 0xac, 0xa9, 0x41, 0xf9, 0x36, 0x93, 0x43, 0x28 }} @@ -680,7 +682,7 @@ char *fwts_uefi_attribute_info(uint32_t attr); bool fwts_uefi_efivars_iface_exist(void); -void fwts_uefi_rt_support_status_get(int fd, bool *getvar_supported, uint32_t *var_rtsupported); +void fwts_uefi_rt_support_status_get(int fd, uint32_t *var_rtsupported); PRAGMA_POP diff --git a/src/lib/src/fwts_uefi.c b/src/lib/src/fwts_uefi.c index 0fb6b799..83799a98 100644 --- a/src/lib/src/fwts_uefi.c +++ b/src/lib/src/fwts_uefi.c @@ -541,45 +541,22 @@ bool fwts_uefi_efivars_iface_exist(void) /* * fwts_uefi_rt_support_status_get() - * get the status of runtime service support and the value of - * the RuntimeServicesSupported variable + * get the status of runtime service support. + * + * Since UEFI 2.8 Errata A the EFI_RT_PROPERTIES_TABLE configuration table + * can be used to indicate which UEFI runtime services are not implemented + * via the bitmask RuntimeServicesSupported. If the table is not present, + * all runtime services are to be considered available. Since Linux 5.11 + * this bitmask can be read via an IOCTL call. Before Linux 5.11 the value + * cannot be determined. */ -void fwts_uefi_rt_support_status_get(int fd, bool *getvar_supported, uint32_t *var_rtsupported) +void fwts_uefi_rt_support_status_get(int fd, uint32_t *var_rtsupported) { long ioret; - struct efi_getvariable getvariable; - uint64_t status = ~0ULL; - uint8_t data[512]; - uint64_t getdatasize = sizeof(data); - *var_rtsupported = 0xFFFF; - - uint32_t attributes = FWTS_UEFI_VAR_NON_VOLATILE | - FWTS_UEFI_VAR_BOOTSERVICE_ACCESS | - FWTS_UEFI_VAR_RUNTIME_ACCESS; - static uint16_t varname[] = {'R', 'u', 'n', 't', 'i', 'm', 'e', 'S', 'e', - 'r', 'v', 'i', 'c', 'e', 's', 'S', 'u', 'p', - 'p', 'o', 'r', 't', 'e', 'd', '\0'}; - EFI_GUID global_var_guid = EFI_GLOBAL_VARIABLE; - getvariable.VariableName = varname; - getvariable.VendorGuid = &global_var_guid; - getvariable.Attributes = &attributes; - getvariable.DataSize = &getdatasize; - getvariable.Data = data; - getvariable.status = &status; - - ioret = ioctl(fd, EFI_RUNTIME_GET_VARIABLE, &getvariable); - if (ioret == -1) { - if (status == EFI_NOT_FOUND) { - *getvar_supported = true; - } else { - *getvar_supported = false; - } - return; - } - *getvar_supported = true; - *var_rtsupported = data[0] | data[1] << 8; + ioret = ioctl(fd, EFI_RUNTIME_GET_SUPPORTED_MASK, var_rtsupported); + if (ioret == -1) + *var_rtsupported = EFI_RT_SUPPORTED_ALL; return; - } diff --git a/src/uefi/uefirtmisc/uefirtmisc.c b/src/uefi/uefirtmisc/uefirtmisc.c index a2dc6967..4b04f78a 100644 --- a/src/uefi/uefirtmisc/uefirtmisc.c +++ b/src/uefi/uefirtmisc/uefirtmisc.c @@ -245,22 +245,12 @@ static int uefirtmisc_test4(fwts_framework *fw) { long ioret; uint64_t status; - bool getvar_supported; uint32_t var_runtimeservicessupported; struct efi_getnexthighmonotoniccount getnexthighmonotoniccount; uint32_t highcount; - fwts_uefi_rt_support_status_get(fd, &getvar_supported, - &var_runtimeservicessupported); - if (!getvar_supported || (var_runtimeservicessupported == 0xFFFF)) { - fwts_skipped(fw, "Cannot get the RuntimeServicesSupported " - "variable, maybe the runtime service " - "GetVariable is not supported or " - "RuntimeServicesSupported not provided by " - "firmware."); - return FWTS_SKIP; - } + fwts_uefi_rt_support_status_get(fd, &var_runtimeservicessupported); getnexthighmonotoniccount.HighCount = &highcount; getnexthighmonotoniccount.status = &status; diff --git a/src/uefi/uefirttime/uefirttime.c b/src/uefi/uefirttime/uefirttime.c index e0aa7071..67365759 100644 --- a/src/uefi/uefirttime/uefirttime.c +++ b/src/uefi/uefirttime/uefirttime.c @@ -1142,7 +1142,6 @@ static int uefirttime_test37(fwts_framework *fw) static int uefirttime_test38(fwts_framework *fw) { long ioret; - bool getvar_supported; uint32_t var_runtimeservicessupported; struct efi_settime settime; @@ -1155,17 +1154,7 @@ static int uefirttime_test38(fwts_framework *fw) EFI_TIME efi_time; EFI_TIME_CAPABILITIES efi_time_cap; - fwts_uefi_rt_support_status_get(fd, &getvar_supported, - &var_runtimeservicessupported); - - if (!getvar_supported || (var_runtimeservicessupported == 0xFFFF)) { - fwts_skipped(fw, "Cannot get the RuntimeServicesSupported " - "variable, maybe the runtime service " - "GetVariable is not supported or " - "RuntimeServicesSupported not provided by " - "firmware."); - return FWTS_SKIP; - } + fwts_uefi_rt_support_status_get(fd, &var_runtimeservicessupported); gettime.Capabilities = &efi_time_cap; gettime.Time = &efi_time; diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c index 713803dc..385c5405 100644 --- a/src/uefi/uefirtvariable/uefirtvariable.c +++ b/src/uefi/uefirtvariable/uefirtvariable.c @@ -2020,7 +2020,6 @@ static int uefirtvariable_test9(fwts_framework *fw) { long ioret; - bool getvar_supported; uint32_t var_runtimeservicessupported; struct efi_getvariable getvariable; @@ -2041,17 +2040,7 @@ static int uefirtvariable_test9(fwts_framework *fw) uint64_t getdatasize = sizeof(testdata); uint32_t attr; - fwts_uefi_rt_support_status_get(fd, &getvar_supported, - &var_runtimeservicessupported); - - if (!getvar_supported || (var_runtimeservicessupported == 0xFFFF)) { - fwts_skipped(fw, "Cannot get the RuntimeServicesSupported " - "variable, maybe the runtime service " - "GetVariable is not supported or " - "RuntimeServicesSupported not provided by " - "firmware."); - return FWTS_SKIP; - } + fwts_uefi_rt_support_status_get(fd, &var_runtimeservicessupported); setvariable.VariableName = variablenametest; setvariable.VendorGuid = >estguid1;
The UEFI 2.8 Errata A specification has clarified that RuntimeServicesSupported is not a UEFI variable but a value in the EFI_RT_PROPERTIES_TABLE configuration table. Since Linux 5.11 the value can be retrieved via an IOCTL call. Replace the code trying to read the non-existent RuntimeServicesSupported variable by the IOCTL call. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- src/lib/include/fwts_uefi.h | 4 +- src/lib/src/fwts_uefi.c | 47 ++++++------------------ src/uefi/uefirtmisc/uefirtmisc.c | 12 +----- src/uefi/uefirttime/uefirttime.c | 13 +------ src/uefi/uefirtvariable/uefirtvariable.c | 13 +------ 5 files changed, 18 insertions(+), 71 deletions(-) -- 2.29.2