[PATCH] uefirttime: add fwts tests for the UEFI get/set time runtime services
Colin Ian King
colin.king at canonical.com
Tue Oct 16 11:09:18 UTC 2012
Thanks Ivan,
The bulk of this is essentially OK, but I have a few notes so to improve
this a little more. Thanks for the work on this initial version.
On 16/10/12 09:37, Ivan Hu wrote:
> Add the set and get time tests of the UEFI runtime service interfaces
> which test via efi_runtime driver.
>
> Signed-off-by: Ivan Hu <ivan.hu at canonical.com>
> ---
> src/Makefile.am | 5 +-
> src/lib/include/fwts_uefi.h | 7 +
> src/uefi/uefirttime/uefirttime.c | 267 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 277 insertions(+), 2 deletions(-)
> create mode 100644 src/uefi/uefirttime/uefirttime.c
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 6054eb3..b7adc20 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -6,7 +6,7 @@
> # but libfwts.so depends on libraries produced by acpica/source/compiler.
> SUBDIRS = acpica/source/compiler lib acpica
>
> -AM_CPPFLAGS = -I$(top_srcdir)/src/lib/include -I$(top_srcdir)/src/acpica/source/include -Wall -Werror
> +AM_CPPFLAGS = -I$(top_srcdir)/src/lib/include -I$(top_srcdir)/src/acpica/source/include -I$(top_srcdir)/efi_runtime -Wall -Werror
>
> bin_PROGRAMS = fwts
> fwts_SOURCES = main.c \
> @@ -66,7 +66,8 @@ fwts_SOURCES = main.c \
> kernel/version/version.c \
> kernel/oops/oops.c \
> uefi/csm/csm.c \
> - uefi/uefidump/uefidump.c
> + uefi/uefidump/uefidump.c \
> + uefi/uefirttime/uefirttime.c
>
> fwts_LDFLAGS = -ljson -lm
>
> diff --git a/src/lib/include/fwts_uefi.h b/src/lib/include/fwts_uefi.h
> index f45027d..73cd773 100644
> --- a/src/lib/include/fwts_uefi.h
> +++ b/src/lib/include/fwts_uefi.h
> @@ -41,6 +41,13 @@ enum {
> FWTS_UEFI_VAR_RUNTIME_ACCESS = 0x00000004
> };
>
> +enum {
> + FWTS_UEFI_TIME_ADJUST_DAYLIGHT = 0x01,
> + FWTS_UEFI_TIME_IN_DAYLIGHT = 0x02
> +};
> +
> +#define FWTS_UEFI_UNSPECIFIED_TIMEZONE 0x07FF
> +
> #if 0
> typedef struct {
> char *description;
> diff --git a/src/uefi/uefirttime/uefirttime.c b/src/uefi/uefirttime/uefirttime.c
> new file mode 100644
> index 0000000..bba15d2
> --- /dev/null
> +++ b/src/uefi/uefirttime/uefirttime.c
> @@ -0,0 +1,267 @@
> +/*
> + * Copyright (C) 2010-2012 Canonical
Since this test was written in 2012, perhaps it should read:
* Copyright (C) 2012 Canonical
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> + *
> + */
> +
> +#include <stdio.h>
> +#include <stddef.h>
> +#include <sys/ioctl.h>
> +#include <fcntl.h>
> +
> +#include "fwts.h"
> +#include "fwts_uefi.h"
> +#include "efi_runtime.h"
> +
> +#define is_leap(year) \
> + ((year) % 4 == 0 && ((year) % 100 != 0 || (year) % 400 == 0))
if possible, can we have macros in capitals, e.g.
#define IS_LEAP() \
> +
> +static int fd;
> +static uint32_t dayofmonth[12] = {31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
> +
> +static bool dayvalid(EFI_TIME *Time)
> +{
> + if (Time->Day < 1)
> + return false;
> +
> + if (Time->Day > dayofmonth[Time->Month - 1])
> + return false;
> +
> + /* check month 2 */
> + if (Time->Month == 2 && (!is_leap(Time->Year) && Time->Day > 28))
> + return false;
> +
> + return true;
> +}
> +
> +static bool checktimefields(EFI_TIME *Time)
> +{
> + if (Time->Year < 1998 || Time->Year > 2099)
> + return false;
I would prefer it if we could log here why it is failing to help the
user see why we have an invalid date. For example:
if (Time->Year < 1998 || Time->Year > 2099) {
fwts_failed(fw, LOG_LEVEL_HIGH,
"UEFIRuntimeGetTimeBadYear",
"GetTime returned an invalid year %" PRIu16
", should be between 1998 and 2099.");
return false;
}
..same for all the failures below.
> +
> + if (Time->Month < 1 || Time->Month > 12)
> + return false;
> +
> + if (!dayvalid(Time))
> + return false;
> +
> + if (Time->Hour > 23)
> + return false;
> +
> + if (Time->Minute > 59)
> + return false;
> +
> + if (Time->Second > 59)
> + return false;
> +
> + if (Time->Nanosecond > 999999999)
> + return false;
> +
> + if (!(Time->TimeZone == FWTS_UEFI_UNSPECIFIED_TIMEZONE ||
> + (Time->TimeZone >= -1440 && Time->TimeZone <= 1440)))
> + return false;
What is the significance of -1440 to 1440? is there a specification (url
+ page) I can refer to to check this out?
> +
> + if (Time->Daylight & (~(FWTS_UEFI_TIME_ADJUST_DAYLIGHT |
> + FWTS_UEFI_TIME_IN_DAYLIGHT)))
too many tabs?
how about:
if (Time->Daylight & (~(FWTS_UEFI_TIME_ADJUST_DAYLIGHT |
FWTS_UEFI_TIME_IN_DAYLIGHT)))
> + return false;
> +
> + return true;
> +}
> +
> +static int uefirttime_init(fwts_framework *fw)
> +{
> + if (fwts_firmware_detect() != FWTS_FIRMWARE_UEFI) {
> + fwts_log_info(fw, "Cannot detect any UEFI firmware. Aborted.");
> + return FWTS_ABORTED;
> + }
> +
> + fd = open("/dev/efi_runtime", O_RDONLY);
> + if (fd == -1) {
> + fwts_log_info(fw, "Cannot open efi_runtime driver. Aborted.");
> + return FWTS_ABORTED;
> + }
I'm a little confused about the mechanism that installs the efi_runtime
driver. I realise dkms is used to build the driver, but how does it get
installed at run time? Is that something that fwts should try to load?
Perhaps we should first check if /dev/efi_runtime exists, if not, then
try to force load the driver. If it still does not exist, we should then
bail out and report that the driver is probably not loaded. Then we
should do the open, and if that fails we have some form of driver error.
As it stands, just reporting that /dev/efi_runtime couldn't be opened
does not really help a user who sees a test fail because of some
mechanism that they don't need to know about.
> +
> + return FWTS_OK;
> +}
There is no deinit() function to tidy up, for example, we are missing a
close(fd) when we complete these tests, so we leak a file descriptor.
> +
> +static int uefirttime_test1(fwts_framework *fw)
> +{
> + long ioret;
> + struct efi_gettime gettime;
> + EFI_TIME efi_time;
> +
> + EFI_TIME_CAPABILITIES efi_time_cap;
> + uint64_t status;
> +
> + gettime.Capabilities = &efi_time_cap;
> + gettime.Time = &efi_time;
> + gettime.status = &status;
> +
> + ioret = ioctl(fd, EFI_RUNTIME_GET_TIME, &gettime);
> +
> + if (ioret == -1) {
> + fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeGetTime",
> + "Failed to get time with UEFI runtime service.");
> + return FWTS_ERROR;
> + }
> +
> + if (!checktimefields(gettime.Time)) {
> + fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeGetTime",
> + "Time field invalid with gettime runtime service.");
> + return FWTS_ERROR;
This is OK, but perhaps we could put more diagnostics into
checktimefields() to report *why* it is failing. If we add more
diagnostics into checktimefields() then we just need:
if (!checktimefields(gettime.Time))
return FWTS_ERROR;
> + }
> +
> + fwts_passed(fw, "Test passed.");
perhaps something more descriptive than just "Test passed.", how about
something like:
fwts_passed(fw, "UEFI runtime service GetTime test passed.");
..but I will leave that to you to word it the way you feel is most
appropriate.
> +
> + return FWTS_OK;
> +}
> +
> +static int uefirttime_test2(fwts_framework *fw)
> +{
> +
> + long ioret;
> + struct efi_settime settime;
> + uint64_t status;
> + struct efi_gettime gettime;
> +
> + EFI_TIME oldtime;
> + EFI_TIME newtime;
> + EFI_TIME time;
> + EFI_TIME_CAPABILITIES efi_time_cap;
> +
> + gettime.Capabilities = &efi_time_cap;
> + gettime.Time = &oldtime;
> + gettime.status = &status;
> + ioret = ioctl(fd, EFI_RUNTIME_GET_TIME, &gettime);
> +
> + if (ioret == -1) {
> + fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeGetTime",
> + "Failed to get time with UEFI runtime service.");
> + return FWTS_ERROR;
> + }
> +
> + /* refer to UEFI SCT 2.3 test items */
> + /* change year */
> + time = oldtime;
> + if (time.Year != 2012)
> + time.Year = 2012;
> + else
> + time.Year = 2016;
> +
> + /* change month */
> + if (time.Month != 1)
> + time.Month = 1;
> + else
> + time.Month = 12;
> +
> + /* Change daylight */
> + if (time.Daylight & FWTS_UEFI_TIME_ADJUST_DAYLIGHT)
> + time.Daylight &= ~FWTS_UEFI_TIME_ADJUST_DAYLIGHT;
> + else
> + time.Daylight |= FWTS_UEFI_TIME_ADJUST_DAYLIGHT;
> +
> + /* Change time zone */
> + if (time.TimeZone != 0)
> + time.TimeZone = 0;
> + else
> + time.TimeZone = 1;
> +
> + settime.Time = &time;
> + settime.status = &status;
> +
> + ioret = ioctl(fd, EFI_RUNTIME_SET_TIME, &settime);
> + if (ioret == -1) {
> + fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeSetTime",
> + "Failed to set time with UEFI runtime service.");
> + return FWTS_ERROR;
> + }
> +
> + sleep(1);
> +
> + gettime.Time = &newtime;
> +
> + ioret = ioctl(fd, EFI_RUNTIME_GET_TIME, &gettime);
> +
> + if (ioret == -1) {
> + fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeGetTime",
> + "Failed to get time with UEFI runtime service.");
> + return FWTS_ERROR;
> + }
> +
> + if (!((oldtime.Year == 2012) && (newtime.Year == 2016)) &&
> + !((oldtime.Year != 2012) && (newtime.Year == 2012))) {
I'd prefer the indentation to be more like:
if (!((oldtime.Year == 2012) && (newtime.Year == 2016)) &&
!((oldtime.Year != 2012) && (newtime.Year == 2012))) {
..same with all the if statements below.
> + fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeSetTime",
> + "Failed to set year with UEFI runtime service.");
> + return FWTS_ERROR;
> + }
> +
> + if (!((oldtime.Month == 1) && (newtime.Month == 12)) &&
> + !((oldtime.Month != 1) && (newtime.Month == 1))) {
> + fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeSetTime",
> + "Failed to set month with UEFI runtime service.");
> + return FWTS_ERROR;
> + }
The UEFIRuntimeSetTime tags can be more specific; as the code stands at
the moment we have one tag that maps to several different failures, so
perhaps we should make it more specific, e.g.
fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeSetTimeMonth",
etc..
> +
> + if (!((oldtime.Daylight == 1) && (newtime.Month == 12)) &&
> + !((oldtime.Month != 1) && (newtime.Month == 1))) {
> + fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeSetTime",
> + "Failed to set month with UEFI runtime service.");
> + return FWTS_ERROR;
> + }
> +
> + if (!((oldtime.Daylight & FWTS_UEFI_TIME_ADJUST_DAYLIGHT) &&
> + (!(newtime.Daylight & FWTS_UEFI_TIME_ADJUST_DAYLIGHT))) &&
> + !((!(oldtime.Daylight & FWTS_UEFI_TIME_ADJUST_DAYLIGHT)) &&
> + (newtime.Daylight & FWTS_UEFI_TIME_ADJUST_DAYLIGHT))) {
> + fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeSetTime",
> + "Failed to set daylight with UEFI runtime service.");
> + return FWTS_ERROR;
> + }
and likewise, use a tag like "UEFIRuntimeSetTimeDaylight"
etc..
> +
> + if (!((oldtime.TimeZone == 0) && (newtime.TimeZone == 1)) &&
> + !((oldtime.TimeZone != 0) && (newtime.TimeZone == 0))) {
> + fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeSetTime",
> + "Failed to set timezone with UEFI runtime service.");
> + return FWTS_ERROR;
> + }
> +
> + /* restore the previous time. */
> + settime.Time = &oldtime;
> + ioret = ioctl(fd, EFI_RUNTIME_SET_TIME, &settime);
> + if (ioret == -1) {
> + fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeSetTime",
> + "Failed to set time with UEFI runtime service.");
> + return FWTS_ERROR;
> + }
> +
> + fwts_passed(fw, "Test passed.");
> +
> + return FWTS_OK;
> +}
> +
> +static fwts_framework_minor_test uefirttime_tests[] = {
> + { uefirttime_test1, "Test UEFI RT service get time interface." },
> + { uefirttime_test2, "Test UEFI RT service set time interface." },
> + { NULL, NULL }
> +};
> +
> +static fwts_framework_ops uefirttime_ops = {
> + .description = "UEFI Runtime service time interface tests.",
> + .init = uefirttime_init,
..as mentioned earlier, we need a deinit handler here too to close the fd.
> + .minor_tests = uefirttime_tests
> +};
> +
> +FWTS_REGISTER(uefirttime, &uefirttime_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV);
>
Thanks, Colin
More information about the fwts-devel
mailing list