| Message ID | 1350376671-29604-1-git-send-email-ivan.hu@canonical.com |
|---|---|
| State | Rejected |
| Headers | show |
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@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
On Tue, Oct 16, 2012 at 7:09 PM, Colin Ian King <colin.king@canonical.com> wrote: > 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@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? > This is what is missing now. We have a separate debian package to install the driver source and dkms builds and installs the binary in the postinst step. But at the moment there is no code in fwts to modprobe/insmod the driver binary. > 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. > A system()-like to call the modprobe command along with the checking the existence of /dev/efi_runtime could be the most simple way. >> + >> + 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 > > > -- > fwts-devel mailing list > fwts-devel@lists.ubuntu.com > Modify settings or unsubscribe at: > https://lists.ubuntu.com/mailman/listinfo/fwts-devel
Thanks Colin, Keng-yu for your comments. I will modify and resend the patch. Ivan On 10/17/2012 05:41 PM, Keng-Yu Lin wrote: > On Tue, Oct 16, 2012 at 7:09 PM, Colin Ian King > <colin.king@canonical.com> wrote: >> 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: >>> + 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? >> >> The value of the timezone is followed by the UEFI spec 2.3.1 errata C, page 233,234. you could download from: http://www.uefi.org/specs/ >>> + >>> +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? >> > > This is what is missing now. > We have a separate debian package to install the driver source and > dkms builds and installs the binary in the postinst step. > But at the moment there is no code in fwts to modprobe/insmod the driver binary. > >> 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. >> > > A system()-like to call the modprobe command along with the checking > the existence of /dev/efi_runtime could be the most simple way. > It seems that we can not force load the efi_runtime driver if the driver doesn't exist, but I will add the checking nonresistance code first and then do the open driver. >>> + >>> + return FWTS_OK; >>> +} >> >>
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 + * + * 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)) + +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; + + 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; + + 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; + } + + return FWTS_OK; +} + +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; + } + + fwts_passed(fw, "Test passed."); + + 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))) { + 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; + } + + 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; + } + + 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, + .minor_tests = uefirttime_tests +}; + +FWTS_REGISTER(uefirttime, &uefirttime_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV);
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@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