uefirtvariable: Add new test for UEFI runtime GetVariable

This tests the UEFI runtime service GetVariable interface by
checking data, datasize and different attributes of variable.

Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
---
 src/Makefile.am                          |    3 +-
 src/uefi/uefirtvariable/uefirtvariable.c |  178 ++++++++++++++++++++++++++++++
 2 files changed, 180 insertions(+), 1 deletion(-)
 create mode 100644 src/uefi/uefirtvariable/uefirtvariable.c

Thanks Ivan, I like this...

On 23/11/12 09:25, Ivan Hu wrote:
> This tests the UEFI runtime service GetVariable interface by
> checking data, datasize and different attributes of variable.
>
> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
> ---
>   src/Makefile.am                          |    3 +-
>   src/uefi/uefirtvariable/uefirtvariable.c |  178 ++++++++++++++++++++++++++++++
>   2 files changed, 180 insertions(+), 1 deletion(-)
>   create mode 100644 src/uefi/uefirtvariable/uefirtvariable.c
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index e82c7a9..fc09423 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -67,7 +67,8 @@ fwts_SOURCES = main.c \
>   	kernel/oops/oops.c \
>   	uefi/csm/csm.c \
>   	uefi/uefidump/uefidump.c \
> -	uefi/uefirttime/uefirttime.c
> +	uefi/uefirttime/uefirttime.c \
> +	uefi/uefirtvariable/uefirtvariable.c
>
>   fwts_LDFLAGS = -ljson -lm
>
> diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c
> new file mode 100644
> index 0000000..6eea0ec
> --- /dev/null
> +++ b/src/uefi/uefirtvariable/uefirtvariable.c
> @@ -0,0 +1,178 @@
> +/*
> + * 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 <inttypes.h>
> +#include <stdio.h>
> +#include <stddef.h>
> +#include <sys/ioctl.h>
> +#include <fcntl.h>
> +
> +#include "fwts.h"
> +#include "fwts_uefi.h"
> +#include "efi_runtime.h"
> +#include "fwts_efi_module.h"
> +
> +#define TEST_GUID1 \
> +{ \
> +	0xF6FAB04F, 0xACAF, 0x4af3, {0xB9, 0xFA, 0xDC, \
> +						0xF9, 0x7F, 0xB4, 0x42, 0x6F} \
> +}
> +
> +#define EFI_VARIABLE_NON_VOLATILE       0x0000000000000001
> +#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x0000000000000002
> +#define EFI_VARIABLE_RUNTIME_ACCESS     0x0000000000000004

I believe we have something similarly defined for these constants
in lib/include/fwts_uefi.h

> +
> +#define EFI_SUCCESS	0
> +
> +static int fd;
> +EFI_GUID gtestguid1 = TEST_GUID1;
> +
> +unsigned short variablenametest[] = {'T', 'e', 's', 't', 'v', 'a', 'r', '\0'};
> +
> +static int uefirtvariable_init(fwts_framework *fw)
> +{
> +	if (fwts_firmware_detect() != FWTS_FIRMWARE_UEFI) {
> +		fwts_log_info(fw, "Cannot detect any UEFI firmware. Aborted.");
> +		return FWTS_ABORTED;
> +	}
> +
> +	if (fwts_lib_efi_runtime_load_module(fw) != FWTS_OK) {
> +		fwts_log_info(fw, "Cannot load efi_runtime module. 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 uefirtvariable_deinit(fwts_framework *fw)
> +{
> +	FWTS_UNUSED(fw);
> +
> +	close(fd);

Should we also unload the EFI runtime module too?

> +
> +	return FWTS_OK;
> +}
> +
> +static int uefirtvariable_test1(fwts_framework *fw)
> +{
> +	long ioret;
> +	struct efi_getvariable getvariable;
> +	struct efi_setvariable setvariable;
> +
> +	uint64_t status;
> +	uint8_t data[1024], testdata[1024];
> +	uint64_t dataindex, subindex, datasize = 10;
> +	uint32_t attributestest;
> +	uint32_t attributesarray[] = { EFI_VARIABLE_BOOTSERVICE_ACCESS,
> +				       EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS,
> +				       EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
> +				       EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS };
> +
> +	for (subindex = 0; subindex < 4; subindex++) {
> +		for (dataindex = 0; dataindex < 10; dataindex++)
> +			data[dataindex] = (uint8_t)dataindex;

maybe dataindex < 10 should be dataindex < datasize since that's what is 
being set in setvariable.DataSize later on.

> +
> +		setvariable.VariableName = variablenametest;
> +		setvariable.VendorGuid = &gtestguid1;
> +		setvariable.Attributes = attributesarray[subindex];
> +		setvariable.DataSize = datasize;
> +		setvariable.Data = data;
> +		setvariable.status = &status;
> +
> +		ioret = ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
> +
> +		if (ioret == -1) {
> +			fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeSetVariable",
> +				"Failed to set variable with UEFI runtime service.");
> +			return FWTS_ERROR;
> +		}
> +
> +		getvariable.VariableName = variablenametest;
> +		getvariable.VendorGuid = &gtestguid1;
> +		getvariable.Attributes = &attributestest;
> +		getvariable.DataSize = &datasize;
> +		getvariable.Data = testdata;
> +		getvariable.status = &status;
> +
> +		ioret = ioctl(fd, EFI_RUNTIME_GET_VARIABLE, &getvariable);
> +		if (ioret == -1) {
> +			fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeGetVariable",
> +				"Failed to get variable with UEFI runtime service.");
> +			return FWTS_ERROR;
> +		}
> +
> +		if (*getvariable.status != EFI_SUCCESS) {
> +			fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeGetVariableStatus",
> +				"Failed to get variable, return status isn't EFI_SUCCESS.");
> +			return FWTS_ERROR;
> +		} else if (*getvariable.Attributes != attributesarray[subindex]) {
> +			fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeGetVariableAttributes",
> +				"Failed to get variable with right attributes.");

It may helpful to explain which attributes were failing too.

> +			return FWTS_ERROR;
> +		} else if (datasize != 10) {

again, hard coded size, should that be datasize instead?

> +			fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeGetVariableDataSize",
> +				"Failed to get variable with correct datasize.");
> +			return FWTS_ERROR;
> +		} else {
> +			for (dataindex = 0; dataindex < 10; dataindex++) {
> +				if (data[dataindex] != (uint8_t)dataindex) {
> +					fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeGetVariableData",
> +						"Failed to get variable with correct data.");
> +					return FWTS_ERROR;
> +				}
> +			}
> +		}
> +
> +		/* delete the variable */
> +		setvariable.DataSize = 0;
> +
> +		ioret = ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
> +
> +		if (ioret == -1) {
> +			fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeSetVariable",
> +				"Failed to set variable with UEFI runtime service.");
> +			return FWTS_ERROR;
> +		}
> +	}
> +
> +	fwts_passed(fw, "UEFI runtime service GetVariable interface test passed.");
> +
> +	return FWTS_OK;
> +}
> +
> +
> +static fwts_framework_minor_test uefirtvariable_tests[] = {
> +	{ uefirtvariable_test1, "Test UEFI RT service get variable interface." },
> +	{ NULL, NULL }
> +};
> +
> +static fwts_framework_ops uefirtvariable_ops = {
> +	.description = "UEFI Runtime service variable interface tests.",
> +	.init        = uefirtvariable_init,
> +	.deinit      = uefirtvariable_deinit,
> +	.minor_tests = uefirtvariable_tests
> +};
> +
> +FWTS_REGISTER(uefirtvariable, &uefirtvariable_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV);
>

OK, so this exercises a variable with a variety of attributes, which is 
good.  Should we also be pushing the test a bit harder by exercising 
this multiple times with a variety of variable sizes (we can easily push 
it to 1K can't we?).  And the variable name is quite undemanding too. 
Perhaps we could expand the test to exercise the name length too.

I suggest re-organising the code so you have a generic test helper 
function which takes parameters such as number of iterations you want to 
make, size of variable you want to set, size of variable name you want 
to set.  Basically, like test1 but more generic, with parameters:

n - number of iterations
varsize - size of variable
varnamesize - size of variable name

Then write a bunch of small tests that exercise:

1.  Simple set, get, delete, just once, with the varname and variable 
size as in test1

2.  Set,get,delete multiple times, how about something like 256 or 1024 
times just to exericse it well.

3.  Set,get,delete with varying variable size, from 1 byte to 1K

4.  Set,get,delete with varying variable name size

And how about multiple variables at a time too with varying sizes and 
varying variable name lengths?  Maybe exercise 1 to 16 variables just 
for fun.  Let's be evil and really exercise these because if we find we 
break the machines in the lab it is much less expensive than fixing 
10,000 units on the field.

Colin.

Hi Colin,

Thanks for your suggestions.


On 11/23/2012 09:02 PM, Colin Ian King wrote:
> Thanks Ivan, I like this...
>
> On 23/11/12 09:25, Ivan Hu wrote:
>> This tests the UEFI runtime service GetVariable interface by
>> checking data, datasize and different attributes of variable.
>>
>> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
>> ---
>>   src/Makefile.am                          |    3 +-
>>   src/uefi/uefirtvariable/uefirtvariable.c |  178
>> ++++++++++++++++++++++++++++++
>>   2 files changed, 180 insertions(+), 1 deletion(-)
>>   create mode 100644 src/uefi/uefirtvariable/uefirtvariable.c
>>
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index e82c7a9..fc09423 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -67,7 +67,8 @@ fwts_SOURCES = main.c \
>>       kernel/oops/oops.c \
>>       uefi/csm/csm.c \
>>       uefi/uefidump/uefidump.c \
>> -    uefi/uefirttime/uefirttime.c
>> +    uefi/uefirttime/uefirttime.c \
>> +    uefi/uefirtvariable/uefirtvariable.c
>>
>>   fwts_LDFLAGS = -ljson -lm
>>
>> diff --git a/src/uefi/uefirtvariable/uefirtvariable.c
>> b/src/uefi/uefirtvariable/uefirtvariable.c
>> new file mode 100644
>> index 0000000..6eea0ec
>> --- /dev/null
>> +++ b/src/uefi/uefirtvariable/uefirtvariable.c
>> @@ -0,0 +1,178 @@
>> +/*
>> + * 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 <inttypes.h>
>> +#include <stdio.h>
>> +#include <stddef.h>
>> +#include <sys/ioctl.h>
>> +#include <fcntl.h>
>> +
>> +#include "fwts.h"
>> +#include "fwts_uefi.h"
>> +#include "efi_runtime.h"
>> +#include "fwts_efi_module.h"
>> +
>> +#define TEST_GUID1 \
>> +{ \
>> +    0xF6FAB04F, 0xACAF, 0x4af3, {0xB9, 0xFA, 0xDC, \
>> +                        0xF9, 0x7F, 0xB4, 0x42, 0x6F} \
>> +}
>> +
>> +#define EFI_VARIABLE_NON_VOLATILE       0x0000000000000001
>> +#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x0000000000000002
>> +#define EFI_VARIABLE_RUNTIME_ACCESS     0x0000000000000004
>
> I believe we have something similarly defined for these constants
> in lib/include/fwts_uefi.h
I'll modify this.

> >> +
>> +#define EFI_SUCCESS    0
>> +
>> +static int fd;
>> +EFI_GUID gtestguid1 = TEST_GUID1;
>> +
>> +unsigned short variablenametest[] = {'T', 'e', 's', 't', 'v', 'a',
>> 'r', '\0'};
>> +
>> +static int uefirtvariable_init(fwts_framework *fw)
>> +{
>> +    if (fwts_firmware_detect() != FWTS_FIRMWARE_UEFI) {
>> +        fwts_log_info(fw, "Cannot detect any UEFI firmware. Aborted.");
>> +        return FWTS_ABORTED;
>> +    }
>> +
>> +    if (fwts_lib_efi_runtime_load_module(fw) != FWTS_OK) {
>> +        fwts_log_info(fw, "Cannot load efi_runtime module. 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 uefirtvariable_deinit(fwts_framework *fw)
>> +{
>> +    FWTS_UNUSED(fw);
>> +
>> +    close(fd);
>
> Should we also unload the EFI runtime module too?
I am not sure if we should unload this. EFI_runtime module is there when 
fwts is installed by DKMS. If it's unloaded, other UEFI tests may not be 
run.
>
>> +
>> +    return FWTS_OK;
>> +}
>> +
>> +static int uefirtvariable_test1(fwts_framework *fw)
>> +{
>> +    long ioret;
>> +    struct efi_getvariable getvariable;
>> +    struct efi_setvariable setvariable;
>> +
>> +    uint64_t status;
>> +    uint8_t data[1024], testdata[1024];
>> +    uint64_t dataindex, subindex, datasize = 10;
>> +    uint32_t attributestest;
>> +    uint32_t attributesarray[] = { EFI_VARIABLE_BOOTSERVICE_ACCESS,
>> +                       EFI_VARIABLE_NON_VOLATILE |
>> EFI_VARIABLE_BOOTSERVICE_ACCESS,
>> +                       EFI_VARIABLE_BOOTSERVICE_ACCESS |
>> EFI_VARIABLE_RUNTIME_ACCESS,
>> +                       EFI_VARIABLE_NON_VOLATILE |
>> EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS };
>> +
>> +    for (subindex = 0; subindex < 4; subindex++) {
>> +        for (dataindex = 0; dataindex < 10; dataindex++)
>> +            data[dataindex] = (uint8_t)dataindex;
>
> maybe dataindex < 10 should be dataindex < datasize since that's what is
> being set in setvariable.DataSize later on.
will be modified.
>
>> +
>> +        setvariable.VariableName = variablenametest;
>> +        setvariable.VendorGuid = &gtestguid1;
>> +        setvariable.Attributes = attributesarray[subindex];
>> +        setvariable.DataSize = datasize;
>> +        setvariable.Data = data;
>> +        setvariable.status = &status;
>> +
>> +        ioret = ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
>> +
>> +        if (ioret == -1) {
>> +            fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeSetVariable",
>> +                "Failed to set variable with UEFI runtime service.");
>> +            return FWTS_ERROR;
>> +        }
>> +
>> +        getvariable.VariableName = variablenametest;
>> +        getvariable.VendorGuid = &gtestguid1;
>> +        getvariable.Attributes = &attributestest;
>> +        getvariable.DataSize = &datasize;
>> +        getvariable.Data = testdata;
>> +        getvariable.status = &status;
>> +
>> +        ioret = ioctl(fd, EFI_RUNTIME_GET_VARIABLE, &getvariable);
>> +        if (ioret == -1) {
>> +            fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeGetVariable",
>> +                "Failed to get variable with UEFI runtime service.");
>> +            return FWTS_ERROR;
>> +        }
>> +
>> +        if (*getvariable.status != EFI_SUCCESS) {
>> +            fwts_failed(fw, LOG_LEVEL_HIGH,
>> "UEFIRuntimeGetVariableStatus",
>> +                "Failed to get variable, return status isn't
>> EFI_SUCCESS.");
>> +            return FWTS_ERROR;
>> +        } else if (*getvariable.Attributes !=
>> attributesarray[subindex]) {
>> +            fwts_failed(fw, LOG_LEVEL_HIGH,
>> "UEFIRuntimeGetVariableAttributes",
>> +                "Failed to get variable with right attributes.");
>
> It may helpful to explain which attributes were failing too.
will be added.
>
>> +            return FWTS_ERROR;
>> +        } else if (datasize != 10) {
>
> again, hard coded size, should that be datasize instead?
>
>> +            fwts_failed(fw, LOG_LEVEL_HIGH,
>> "UEFIRuntimeGetVariableDataSize",
>> +                "Failed to get variable with correct datasize.");
>> +            return FWTS_ERROR;
>> +        } else {
>> +            for (dataindex = 0; dataindex < 10; dataindex++) {
>> +                if (data[dataindex] != (uint8_t)dataindex) {
>> +                    fwts_failed(fw, LOG_LEVEL_HIGH,
>> "UEFIRuntimeGetVariableData",
>> +                        "Failed to get variable with correct data.");
>> +                    return FWTS_ERROR;
>> +                }
>> +            }
>> +        }
>> +
>> +        /* delete the variable */
>> +        setvariable.DataSize = 0;
>> +
>> +        ioret = ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
>> +
>> +        if (ioret == -1) {
>> +            fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeSetVariable",
>> +                "Failed to set variable with UEFI runtime service.");
>> +            return FWTS_ERROR;
>> +        }
>> +    }
>> +
>> +    fwts_passed(fw, "UEFI runtime service GetVariable interface test
>> passed.");
>> +
>> +    return FWTS_OK;
>> +}
>> +
>> +
>> +static fwts_framework_minor_test uefirtvariable_tests[] = {
>> +    { uefirtvariable_test1, "Test UEFI RT service get variable
>> interface." },
>> +    { NULL, NULL }
>> +};
>> +
>> +static fwts_framework_ops uefirtvariable_ops = {
>> +    .description = "UEFI Runtime service variable interface tests.",
>> +    .init        = uefirtvariable_init,
>> +    .deinit      = uefirtvariable_deinit,
>> +    .minor_tests = uefirtvariable_tests
>> +};
>> +
>> +FWTS_REGISTER(uefirtvariable, &uefirtvariable_ops, FWTS_TEST_ANYTIME,
>> FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV);
>>
>
> OK, so this exercises a variable with a variety of attributes, which is
> good.  Should we also be pushing the test a bit harder by exercising
> this multiple times with a variety of variable sizes (we can easily push
> it to 1K can't we?).  And the variable name is quite undemanding too.
> Perhaps we could expand the test to exercise the name length too.

actually, my plan of UEFI variable tests is blow:
1. test1 - getvariable interface function tests
2. test2 - getnextvariablename interface function tests - 
GetNextVariableName when the next variable exists
3. test3 - setvariable interface function test
    i. SetVariable when the variable does not exist.
    ii.  SetVariable when the variable exists.
    iii. SetVariable when the similar variable exists.
    ix. SetVariable when the DataSize is 0.
    x. SetVariable when the Attributes is 0.
and maybe these two, they need reset the system. right now, I have no 
idea how:
    xi. Non-volatile variable exists after system reset
    xii. Volatile variable does not exist after system reset.
4. test 4 - stress test
    i. multiple stress test -getvariable, getnextvariablename, 
setvariable stress test.
    maybe:
    ii. overflow stree test - need reset the system to check nv variable.

any thoughts or suggestions?

>
> I suggest re-organising the code so you have a generic test helper
> function which takes parameters such as number of iterations you want to
> make, size of variable you want to set, size of variable name you want
> to set.  Basically, like test1 but more generic, with parameters:
>
> n - number of iterations
> varsize - size of variable
> varnamesize - size of variable name
>
> Then write a bunch of small tests that exercise:
>
> 1.  Simple set, get, delete, just once, with the varname and variable
> size as in test1
>
> 2.  Set,get,delete multiple times, how about something like 256 or 1024
> times just to exericse it well.
>
> 3.  Set,get,delete with varying variable size, from 1 byte to 1K
>
> 4.  Set,get,delete with varying variable name size
>
> And how about multiple variables at a time too with varying sizes and
> varying variable name lengths?  Maybe exercise 1 to 16 variables just
> for fun.  Let's be evil and really exercise these because if we find we
> break the machines in the lab it is much less expensive than fixing
> 10,000 units on the field.
great idea, I'll put above tests into stress tests. Thanks!

>
> Colin.
>


Cheers,
Ivan

On 27/11/12 10:09, IvanHu wrote:
> Hi Colin,
>
> Thanks for your suggestions.
>
>
> On 11/23/2012 09:02 PM, Colin Ian King wrote:
>> Thanks Ivan, I like this...
>>
>> On 23/11/12 09:25, Ivan Hu wrote:
>>> This tests the UEFI runtime service GetVariable interface by
>>> checking data, datasize and different attributes of variable.
>>>
>>> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
>>> ---
>>>   src/Makefile.am                          |    3 +-
>>>   src/uefi/uefirtvariable/uefirtvariable.c |  178
>>> ++++++++++++++++++++++++++++++
>>>   2 files changed, 180 insertions(+), 1 deletion(-)
>>>   create mode 100644 src/uefi/uefirtvariable/uefirtvariable.c
>>>
>>> diff --git a/src/Makefile.am b/src/Makefile.am
>>> index e82c7a9..fc09423 100644
>>> --- a/src/Makefile.am
>>> +++ b/src/Makefile.am
>>> @@ -67,7 +67,8 @@ fwts_SOURCES = main.c \
>>>       kernel/oops/oops.c \
>>>       uefi/csm/csm.c \
>>>       uefi/uefidump/uefidump.c \
>>> -    uefi/uefirttime/uefirttime.c
>>> +    uefi/uefirttime/uefirttime.c \
>>> +    uefi/uefirtvariable/uefirtvariable.c
>>>
>>>   fwts_LDFLAGS = -ljson -lm
>>>
>>> diff --git a/src/uefi/uefirtvariable/uefirtvariable.c
>>> b/src/uefi/uefirtvariable/uefirtvariable.c
>>> new file mode 100644
>>> index 0000000..6eea0ec
>>> --- /dev/null
>>> +++ b/src/uefi/uefirtvariable/uefirtvariable.c
>>> @@ -0,0 +1,178 @@
>>> +/*
>>> + * 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 <inttypes.h>
>>> +#include <stdio.h>
>>> +#include <stddef.h>
>>> +#include <sys/ioctl.h>
>>> +#include <fcntl.h>
>>> +
>>> +#include "fwts.h"
>>> +#include "fwts_uefi.h"
>>> +#include "efi_runtime.h"
>>> +#include "fwts_efi_module.h"
>>> +
>>> +#define TEST_GUID1 \
>>> +{ \
>>> +    0xF6FAB04F, 0xACAF, 0x4af3, {0xB9, 0xFA, 0xDC, \
>>> +                        0xF9, 0x7F, 0xB4, 0x42, 0x6F} \
>>> +}
>>> +
>>> +#define EFI_VARIABLE_NON_VOLATILE       0x0000000000000001
>>> +#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x0000000000000002
>>> +#define EFI_VARIABLE_RUNTIME_ACCESS     0x0000000000000004
>>
>> I believe we have something similarly defined for these constants
>> in lib/include/fwts_uefi.h
> I'll modify this.
>
>> >> +
>>> +#define EFI_SUCCESS    0
>>> +
>>> +static int fd;
>>> +EFI_GUID gtestguid1 = TEST_GUID1;
>>> +
>>> +unsigned short variablenametest[] = {'T', 'e', 's', 't', 'v', 'a',
>>> 'r', '\0'};
>>> +
>>> +static int uefirtvariable_init(fwts_framework *fw)
>>> +{
>>> +    if (fwts_firmware_detect() != FWTS_FIRMWARE_UEFI) {
>>> +        fwts_log_info(fw, "Cannot detect any UEFI firmware. Aborted.");
>>> +        return FWTS_ABORTED;
>>> +    }
>>> +
>>> +    if (fwts_lib_efi_runtime_load_module(fw) != FWTS_OK) {
>>> +        fwts_log_info(fw, "Cannot load efi_runtime module. 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 uefirtvariable_deinit(fwts_framework *fw)
>>> +{
>>> +    FWTS_UNUSED(fw);
>>> +
>>> +    close(fd);
>>
>> Should we also unload the EFI runtime module too?
> I am not sure if we should unload this. EFI_runtime module is there when
> fwts is installed by DKMS. If it's unloaded, other UEFI tests may not be
> run.

Maybe I am mistaken, but I thought fwts_lib_efi_runtime_load_module() 
loaded the module, so if it is not loaded already, or unloaded it would 
be always loaded at the test init() stage.


>>
>>> +
>>> +    return FWTS_OK;
>>> +}
>>> +
>>> +static int uefirtvariable_test1(fwts_framework *fw)
>>> +{
>>> +    long ioret;
>>> +    struct efi_getvariable getvariable;
>>> +    struct efi_setvariable setvariable;
>>> +
>>> +    uint64_t status;
>>> +    uint8_t data[1024], testdata[1024];
>>> +    uint64_t dataindex, subindex, datasize = 10;
>>> +    uint32_t attributestest;
>>> +    uint32_t attributesarray[] = { EFI_VARIABLE_BOOTSERVICE_ACCESS,
>>> +                       EFI_VARIABLE_NON_VOLATILE |
>>> EFI_VARIABLE_BOOTSERVICE_ACCESS,
>>> +                       EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>> EFI_VARIABLE_RUNTIME_ACCESS,
>>> +                       EFI_VARIABLE_NON_VOLATILE |
>>> EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS };
>>> +
>>> +    for (subindex = 0; subindex < 4; subindex++) {
>>> +        for (dataindex = 0; dataindex < 10; dataindex++)
>>> +            data[dataindex] = (uint8_t)dataindex;
>>
>> maybe dataindex < 10 should be dataindex < datasize since that's what is
>> being set in setvariable.DataSize later on.
> will be modified.
>>
>>> +
>>> +        setvariable.VariableName = variablenametest;
>>> +        setvariable.VendorGuid = &gtestguid1;
>>> +        setvariable.Attributes = attributesarray[subindex];
>>> +        setvariable.DataSize = datasize;
>>> +        setvariable.Data = data;
>>> +        setvariable.status = &status;
>>> +
>>> +        ioret = ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
>>> +
>>> +        if (ioret == -1) {
>>> +            fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeSetVariable",
>>> +                "Failed to set variable with UEFI runtime service.");
>>> +            return FWTS_ERROR;
>>> +        }
>>> +
>>> +        getvariable.VariableName = variablenametest;
>>> +        getvariable.VendorGuid = &gtestguid1;
>>> +        getvariable.Attributes = &attributestest;
>>> +        getvariable.DataSize = &datasize;
>>> +        getvariable.Data = testdata;
>>> +        getvariable.status = &status;
>>> +
>>> +        ioret = ioctl(fd, EFI_RUNTIME_GET_VARIABLE, &getvariable);
>>> +        if (ioret == -1) {
>>> +            fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeGetVariable",
>>> +                "Failed to get variable with UEFI runtime service.");
>>> +            return FWTS_ERROR;
>>> +        }
>>> +
>>> +        if (*getvariable.status != EFI_SUCCESS) {
>>> +            fwts_failed(fw, LOG_LEVEL_HIGH,
>>> "UEFIRuntimeGetVariableStatus",
>>> +                "Failed to get variable, return status isn't
>>> EFI_SUCCESS.");
>>> +            return FWTS_ERROR;
>>> +        } else if (*getvariable.Attributes !=
>>> attributesarray[subindex]) {
>>> +            fwts_failed(fw, LOG_LEVEL_HIGH,
>>> "UEFIRuntimeGetVariableAttributes",
>>> +                "Failed to get variable with right attributes.");
>>
>> It may helpful to explain which attributes were failing too.
> will be added.
>>
>>> +            return FWTS_ERROR;
>>> +        } else if (datasize != 10) {
>>
>> again, hard coded size, should that be datasize instead?
>>
>>> +            fwts_failed(fw, LOG_LEVEL_HIGH,
>>> "UEFIRuntimeGetVariableDataSize",
>>> +                "Failed to get variable with correct datasize.");
>>> +            return FWTS_ERROR;
>>> +        } else {
>>> +            for (dataindex = 0; dataindex < 10; dataindex++) {
>>> +                if (data[dataindex] != (uint8_t)dataindex) {
>>> +                    fwts_failed(fw, LOG_LEVEL_HIGH,
>>> "UEFIRuntimeGetVariableData",
>>> +                        "Failed to get variable with correct data.");
>>> +                    return FWTS_ERROR;
>>> +                }
>>> +            }
>>> +        }
>>> +
>>> +        /* delete the variable */
>>> +        setvariable.DataSize = 0;
>>> +
>>> +        ioret = ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
>>> +
>>> +        if (ioret == -1) {
>>> +            fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeSetVariable",
>>> +                "Failed to set variable with UEFI runtime service.");
>>> +            return FWTS_ERROR;
>>> +        }
>>> +    }
>>> +
>>> +    fwts_passed(fw, "UEFI runtime service GetVariable interface test
>>> passed.");
>>> +
>>> +    return FWTS_OK;
>>> +}
>>> +
>>> +
>>> +static fwts_framework_minor_test uefirtvariable_tests[] = {
>>> +    { uefirtvariable_test1, "Test UEFI RT service get variable
>>> interface." },
>>> +    { NULL, NULL }
>>> +};
>>> +
>>> +static fwts_framework_ops uefirtvariable_ops = {
>>> +    .description = "UEFI Runtime service variable interface tests.",
>>> +    .init        = uefirtvariable_init,
>>> +    .deinit      = uefirtvariable_deinit,
>>> +    .minor_tests = uefirtvariable_tests
>>> +};
>>> +
>>> +FWTS_REGISTER(uefirtvariable, &uefirtvariable_ops, FWTS_TEST_ANYTIME,
>>> FWTS_FLAG_UNSAFE | FWTS_FLAG_ROOT_PRIV);
>>>
>>
>> OK, so this exercises a variable with a variety of attributes, which is
>> good.  Should we also be pushing the test a bit harder by exercising
>> this multiple times with a variety of variable sizes (we can easily push
>> it to 1K can't we?).  And the variable name is quite undemanding too.
>> Perhaps we could expand the test to exercise the name length too.
>
> actually, my plan of UEFI variable tests is blow:
> 1. test1 - getvariable interface function tests
> 2. test2 - getnextvariablename interface function tests -
> GetNextVariableName when the next variable exists
> 3. test3 - setvariable interface function test
>     i. SetVariable when the variable does not exist.
>     ii.  SetVariable when the variable exists.
>     iii. SetVariable when the similar variable exists.
>     ix. SetVariable when the DataSize is 0.
>     x. SetVariable when the Attributes is 0.
> and maybe these two, they need reset the system. right now, I have no
> idea how:
>     xi. Non-volatile variable exists after system reset
>     xii. Volatile variable does not exist after system reset.

The last two xi and xii are problematic since keeping state and 
rebooting are something that fwts does not do.  I'm not sure what the 
solution is because I want to keep fwts not tied to any distro way of 
doing reboots and running again once booted.  I've usually just avoided 
this in the past because it is not easily solvable.

> 4. test 4 - stress test
>     i. multiple stress test -getvariable, getnextvariablename,
> setvariable stress test.
>     maybe:
>     ii. overflow stree test - need reset the system to check nv variable.
>
> any thoughts or suggestions?

Sounds good.  Does "stress" involve setting all sorts of variable 
contents sizes and variable name sizes and generally thrashing the 
variable space? If so, then +1 from me.

>
>>
>> I suggest re-organising the code so you have a generic test helper
>> function which takes parameters such as number of iterations you want to
>> make, size of variable you want to set, size of variable name you want
>> to set.  Basically, like test1 but more generic, with parameters:
>>
>> n - number of iterations
>> varsize - size of variable
>> varnamesize - size of variable name
>>
>> Then write a bunch of small tests that exercise:
>>
>> 1.  Simple set, get, delete, just once, with the varname and variable
>> size as in test1
>>
>> 2.  Set,get,delete multiple times, how about something like 256 or 1024
>> times just to exericse it well.
>>
>> 3.  Set,get,delete with varying variable size, from 1 byte to 1K
>>
>> 4.  Set,get,delete with varying variable name size
>>
>> And how about multiple variables at a time too with varying sizes and
>> varying variable name lengths?  Maybe exercise 1 to 16 variables just
>> for fun.  Let's be evil and really exercise these because if we find we
>> break the machines in the lab it is much less expensive than fixing
>> 10,000 units on the field.
> great idea, I'll put above tests into stress tests. Thanks!

Many thanks, sorry if this creates more work, but in the long run we get 
a set of tests that really exercises systems well.

Colin
>
>>
>> Colin.
>>
>
>
> Cheers,
> Ivan