Fwd: Re: [PATCH] uefirttime: add fwts tests for the UEFI get/set time runtime services
Colin Ian King
colin.king at canonical.com
Wed Oct 17 09:59:47 UTC 2012
On 17/10/12 10:41, Keng-Yu Lin wrote:
> On Tue, Oct 16, 2012 at 7:09 PM, Colin Ian King
> <colin.king at 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 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?
>>
>
> 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.
We should probably have this as one of the fwts library helper functions
in in src/lib/src/... Something like fwts_efi_module.c:
static bool module_already_loaded = true;
int fwts_lib_efi_runtime_load_module(fwts_framework *fw)
{
if module not in /proc/modules {
module_already_loaded = false;
do a modprobe
if (modprobe failed) {
fwts_log_error(fw, "Could not load efi_runtime module.");
return FWTS_ERROR;
}
}
if /dev/efi_runtime not exists {
fwts_log_error(fw, "Loaded efi_runtime module but /dev/efi_runtime is
not present.");
return FWTS_ERROR;
}
return FWTS_OK;
}
int fwts_lib_efi_runtime_unload_module(fwts_framework *fw)
{
if (!module_already_loaded) {
remove module;
if (remove module failed) {
fwts_log_error(fw, "Could not unload efi_runtime module.");
return FWTS_ERROR;
}
}
return FWTS_OK;
}
..which I leave as an exercise for somebody else to implement ;-)
>
>>> +
>>> + 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 at lists.ubuntu.com
>> Modify settings or unsubscribe at:
>> https://lists.ubuntu.com/mailman/listinfo/fwts-devel
More information about the fwts-devel
mailing list