efi_runtime: add efi_runtime kernel driver module into fwts

efi_runtime kernel driver provides the runtime UEFI interfaces for fwts
to test the UEFI runtime service implementiation.
Current capabilities:
* provide the RT service interfaces:
  * GetVariable
  * SetVariable
  * GetTime
  * SetTime
  * GetWakeupTime
  * SetWakeupTime
  * GetNextVariableName

Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
---
 efi_runtime/Makefile      |    6 +
 efi_runtime/efi_runtime.c |  311 +++++++++++++++++++++++++++++++++++++++++++++
 efi_runtime/efi_runtime.h |  128 +++++++++++++++++++
 3 files changed, 445 insertions(+)
 create mode 100644 efi_runtime/Makefile
 create mode 100644 efi_runtime/efi_runtime.c
 create mode 100644 efi_runtime/efi_runtime.h

On Wed, Sep 19, 2012 at 4:58 PM, Ivan Hu <ivan.hu@canonical.com> wrote:
> efi_runtime kernel driver provides the runtime UEFI interfaces for fwts
> to test the UEFI runtime service implementiation.
> Current capabilities:
> * provide the RT service interfaces:
>   * GetVariable
>   * SetVariable
>   * GetTime
>   * SetTime
>   * GetWakeupTime
>   * SetWakeupTime
>   * GetNextVariableName
>
> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
> ---
>  efi_runtime/Makefile      |    6 +
>  efi_runtime/efi_runtime.c |  311 +++++++++++++++++++++++++++++++++++++++++++++
>  efi_runtime/efi_runtime.h |  128 +++++++++++++++++++
>  3 files changed, 445 insertions(+)
>  create mode 100644 efi_runtime/Makefile
>  create mode 100644 efi_runtime/efi_runtime.c
>  create mode 100644 efi_runtime/efi_runtime.h
>
> diff --git a/efi_runtime/Makefile b/efi_runtime/Makefile
> new file mode 100644
> index 0000000..8ed7dea
> --- /dev/null
> +++ b/efi_runtime/Makefile
> @@ -0,0 +1,6 @@
> +obj-m += efi_runtime.o
> +all:
> +       make -C /lib/modules/`uname -r`/build M=`pwd` modules
> +
> +clean:
> +       make -C /lib/modules/`uname -r`/build M=`pwd` clean
> diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c
> new file mode 100644
> index 0000000..71eae2b
> --- /dev/null
> +++ b/efi_runtime/efi_runtime.c
> @@ -0,0 +1,311 @@
> +/*
> + * EFI Runtime driver
> + *
> + * Copyright(C) 2012 Canonical Ltd.
> + *
> + *  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., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#include <linux/types.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/proc_fs.h>
> +#include <linux/efi.h>
> +
> +#include <linux/uaccess.h>
> +
> +#include "efi_runtime.h"
> +
> +#define EFI_FWTSEFI_VERSION    "0.1"
> +
> +MODULE_AUTHOR("Ivan Hu");
> +MODULE_DESCRIPTION("EFI Runtime Driver");
> +MODULE_LICENSE("GPL");
> +
> +static void convert_from_efi_time(efi_time_t *eft, EFI_TIME *time)
> +{
> +       memset(time, 0, sizeof(EFI_TIME));
> +       time->Year = eft->year;
> +       time->Month = eft->month;
> +       time->Day  = eft->day;
> +       time->Hour = eft->hour;
> +       time->Minute = eft->minute;
> +       time->Second  = eft->second;
> +       time->Pad1 = eft->pad1;
> +       time->Nanosecond = eft->nanosecond;
> +       time->TimeZone = eft->timezone;
> +       time->Daylight = eft->daylight;
> +       time->Pad2 = eft->pad2;
> +}
> +
> +static void convert_to_efi_time(efi_time_t *eft, EFI_TIME *time)
> +{
> +       memset(eft, 0, sizeof(eft));
> +       eft->year = time->Year;
> +       eft->month = time->Month;
> +       eft->day = time->Day;
> +       eft->hour = time->Hour;
> +       eft->minute = time->Minute;
> +       eft->second = time->Second;
> +       eft->pad1 = time->Pad1;
> +       eft->nanosecond = time->Nanosecond;
> +       eft->timezone = time->TimeZone;
> +       eft->daylight = time->Daylight;
> +       eft->pad2 = time->Pad2;
> +}
> +
> +static void convert_from_guid(efi_guid_t *vendor, EFI_GUID *vendor_guid)
> +{
> +       int i;
> +       for (i = 0; i < 16; i++) {
> +               if (i < 4)
> +                       vendor->b[i] = (vendor_guid->Data1 >> (8*i)) & 0xff;
> +               else if (i < 6)
> +                       vendor->b[i] = (vendor_guid->Data2 >> (8*(i-4))) & 0xff;
> +               else if (i < 8)
> +                       vendor->b[i] = (vendor_guid->Data3 >> (8*(i-6))) & 0xff;
> +               else
> +                       vendor->b[i] = (vendor_guid->Data4[i-8]);
> +       }
> +}
> +
> +static void convert_to_guid(efi_guid_t *vendor, EFI_GUID *vendor_guid)
> +{
> +       int i;
> +       vendor_guid->Data1 = vendor->b[0] + (vendor->b[1] << 8) +
> +                               (vendor->b[2] << 16) + (vendor->b[3] << 24);
> +       vendor_guid->Data2 = vendor->b[4] + (vendor->b[5] << 8);
> +       vendor_guid->Data3 = vendor->b[6] + (vendor->b[7] << 8);
> +       for (i = 0; i < 8; i++)
> +               vendor_guid->Data4[i] = vendor->b[i+8];
> +}
> +
> +static long efi_runtime_ioctl(struct file *file, unsigned int cmd,
> +                                                       unsigned long arg)
> +{
> +       efi_status_t status;
> +       struct efi_getvariable __user *pgetvariable;
> +       struct efi_setvariable __user *psetvariable;
> +
> +       efi_guid_t vendor;
> +       EFI_GUID vendor_guid;
> +       unsigned long datasize;
> +       UINT32 attr;
> +
> +       efi_time_t eft;
> +       efi_time_cap_t cap;
> +       struct efi_gettime __user *pgettime;
> +       struct efi_settime __user *psettime;
> +
> +       unsigned char enabled, pending;
> +       EFI_TIME efi_time;
> +       struct efi_getwakeuptime __user *pgetwakeuptime;
> +       struct efi_setwakeuptime __user *psetwakeuptime;
> +
> +       struct efi_getnextvariablename __user *pgetnextvariablename;
> +       unsigned long name_size;
> +
> +       switch (cmd) {
> +       case EFI_RUNTIME_GET_VARIABLE:
> +               pgetvariable = (struct efi_getvariable __user *)arg;
> +
> +               if (get_user(datasize, pgetvariable->DataSize) ||
> +                       copy_from_user(&vendor_guid, pgetvariable->VendorGuid,
> +                                                       sizeof(EFI_GUID)))
> +                       return -EFAULT;
> +
> +               convert_from_guid(&vendor, &vendor_guid);
> +               status = efi.get_variable(pgetvariable->VariableName, &vendor,
> +                                       &attr, &datasize, pgetvariable->Data);
> +
> +               if (status == EFI_SUCCESS) {
> +                       if (put_user(attr, pgetvariable->Attributes) ||
> +                               put_user(datasize, pgetvariable->DataSize))
> +                               return -EFAULT;
> +                       return 0;
> +               } else {
> +                       printk(KERN_ERR "efi_runtime: can't get variable\n");
> +                       return -EINVAL;
> +               }
> +
> +       case EFI_RUNTIME_SET_VARIABLE:
> +               psetvariable = (struct efi_setvariable __user *)arg;
> +               if (get_user(datasize, &psetvariable->DataSize) ||
> +                       get_user(attr, &psetvariable->Attributes) ||
> +                       copy_from_user(&vendor_guid, psetvariable->VendorGuid,
> +                                                       sizeof(EFI_GUID)))
> +                       return -EFAULT;
> +
> +               convert_from_guid(&vendor, &vendor_guid);
> +               status = efi.set_variable(psetvariable->VariableName, &vendor,
> +                                       attr, datasize, psetvariable->Data);
> +               return status == EFI_SUCCESS ? 0 : -EINVAL;
> +
> +       case EFI_RUNTIME_GET_TIME:
> +               status = efi.get_time(&eft, &cap);
> +               if (status != EFI_SUCCESS) {
> +                       printk(KERN_ERR "efitime: can't read time\n");
> +                       return -EINVAL;
> +               }
> +
> +               pgettime = (struct efi_gettime __user *)arg;
> +               if (put_user(cap.resolution,
> +                                       &pgettime->Capabilities->Resolution) ||
> +                                       put_user(cap.accuracy,
> +                                       &pgettime->Capabilities->Accuracy) ||
> +                                       put_user(cap.sets_to_zero,
> +                                       &pgettime->Capabilities->SetsToZero))
> +                       return -EFAULT;
> +               return copy_to_user(pgettime->Time, &eft,
> +                               sizeof(EFI_TIME)) ? -EFAULT : 0;
> +
> +       case EFI_RUNTIME_SET_TIME:
> +
> +               psettime = (struct efi_settime __user *)arg;
> +               if (copy_from_user(&efi_time, psettime->Time,
> +                                               sizeof(EFI_TIME)))
> +                       return -EFAULT;
> +               convert_to_efi_time(&eft, &efi_time);
> +               status = efi.set_time(&eft);
> +               return status == EFI_SUCCESS ? 0 : -EINVAL;
> +
> +       case EFI_RUNTIME_GET_WAKETIME:
> +
> +               status = efi.get_wakeup_time((efi_bool_t *)&enabled,
> +                                               (efi_bool_t *)&pending, &eft);
> +
> +               if (status != EFI_SUCCESS)
> +                       return -EINVAL;
> +
> +               pgetwakeuptime = (struct efi_getwakeuptime __user *)arg;
> +
> +               if (put_user(enabled, pgetwakeuptime->Enabled) ||
> +                               put_user(pending, pgetwakeuptime->Pending))
> +                       return -EFAULT;
> +
> +               convert_from_efi_time(&eft, &efi_time);
> +
> +               return copy_to_user(pgetwakeuptime->Time, &efi_time,
> +                               sizeof(EFI_TIME)) ? -EFAULT : 0;
> +
> +       case EFI_RUNTIME_SET_WAKETIME:
> +
> +               psetwakeuptime = (struct efi_setwakeuptime __user *)arg;
> +
> +               if (get_user(enabled, &psetwakeuptime->Enabled) ||
> +                                       copy_from_user(&efi_time,
> +                                       psetwakeuptime->Time,
> +                                       sizeof(EFI_TIME)))
> +                       return -EFAULT;
> +
> +               convert_to_efi_time(&eft, &efi_time);
> +
> +               status = efi.set_wakeup_time(enabled, &eft);
> +
> +               return status == EFI_SUCCESS ? 0 : -EINVAL;
> +
> +       case EFI_RUNTIME_GET_NEXTVARIABLENAME:
> +
> +               pgetnextvariablename = (struct efi_getnextvariablename
> +                                                               __user *)arg;
> +
> +               if (get_user(name_size, pgetnextvariablename->VariableNameSize)
> +                               || copy_from_user(&vendor_guid,
> +                                       pgetnextvariablename->VendorGuid,
> +                                       sizeof(EFI_GUID)))
> +                       return -EFAULT;
> +               if (name_size > 1024)
> +                       return -EFAULT;
> +
> +               convert_from_guid(&vendor, &vendor_guid);
> +
> +               status = efi.get_next_variable(&name_size,
> +                                       pgetnextvariablename->VariableName,
> +                                                               &vendor);
> +
> +               if (status != EFI_SUCCESS)
> +                       return -EINVAL;
> +               convert_to_guid(&vendor, &vendor_guid);
> +
> +               if (put_user(name_size, pgetnextvariablename->VariableNameSize))
> +                       return -EFAULT;
> +
> +               if (copy_to_user(pgetnextvariablename->VendorGuid,
> +                                               &vendor_guid, sizeof(EFI_GUID)))
> +                       return -EFAULT;
> +               return 0;
> +       }
> +
> +       return -ENOTTY;
> +}
> +
> +static int efi_runtime_open(struct inode *inode, struct file *file)
> +{
> +       /*
> +        * nothing special to do here
> +        * We do accept multiple open files at the same time as we
> +        * synchronize on the per call operation.
> +        */
> +       return 0;
> +}
> +
> +static int efi_runtime_close(struct inode *inode, struct file *file)
> +{
> +       return 0;
> +}
> +
> +/*
> + *     The various file operations we support.
> + */
> +static const struct file_operations efi_runtime_fops = {
> +       .owner          = THIS_MODULE,
> +       .unlocked_ioctl = efi_runtime_ioctl,
> +       .open           = efi_runtime_open,
> +       .release        = efi_runtime_close,
> +       .llseek         = no_llseek,
> +};
> +
> +static struct miscdevice efi_runtime_dev = {
> +       MISC_DYNAMIC_MINOR,
> +       "efi_runtime",
> +       &efi_runtime_fops
> +};
> +
> +static int __init efi_runtime_init(void)
> +{
> +       int ret;
> +
> +       printk(KERN_INFO "EFI_RUNTIME Driver v%s\n", EFI_FWTSEFI_VERSION);
> +
> +       ret = misc_register(&efi_runtime_dev);
> +       if (ret) {
> +               printk(KERN_ERR "efi_runtime: can't misc_register on minor=%d\n",
> +                               MISC_DYNAMIC_MINOR);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static void __exit efi_runtime_exit(void)
> +{
> +       printk(KERN_INFO "EFI_RUNTIME Driver Exit.\n");
> +       misc_deregister(&efi_runtime_dev);
> +}
> +
> +module_init(efi_runtime_init);
> +module_exit(efi_runtime_exit);
> +
> diff --git a/efi_runtime/efi_runtime.h b/efi_runtime/efi_runtime.h
> new file mode 100644
> index 0000000..bac4dc5
> --- /dev/null
> +++ b/efi_runtime/efi_runtime.h
> @@ -0,0 +1,128 @@
> +/*
> + * EFI Runtime driver
> + *
> + * Copyright(C) 2012 Canonical Ltd.
> + *
> + *  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., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#ifndef _EFI_RUNTIME_H_
> +#define _EFI_RUNTIME_H_
> +
> +#include <linux/types.h>
> +
> +typedef unsigned long int      UINTN;
> +typedef unsigned long long     UINT64;
> +typedef long long              INT64;
> +typedef unsigned int           UINT32;
> +typedef int                    INT32;
> +typedef unsigned short         UINT16;
> +typedef unsigned short         CHAR16;
> +typedef short                  INT16;
> +typedef unsigned char          BOOLEAN;
> +typedef unsigned char          UINT8;
> +typedef char                   CHAR8;
> +typedef char                   INT8;
> +typedef void                   VOID;
> +

It will be good to get these typedefs a prefix.

> +typedef struct {
> +       UINT32  Data1;
> +       UINT16  Data2;
> +       UINT16  Data3;
> +       UINT8   Data4[8];
> +} __attribute__ ((packed)) EFI_GUID;
> +
> +typedef struct {
> +       UINT16  Year;           /* 1900 – 9999 */
> +       UINT8   Month;          /* 1 – 12 */
> +       UINT8   Day;            /* 1 – 31 */
> +       UINT8   Hour;           /* 0 – 23 */
> +       UINT8   Minute;         /* 0 – 59 */
> +       UINT8   Second;         /* 0 – 59 */
> +       UINT8   Pad1;
> +       UINT32  Nanosecond;     /* 0 – 999,999,999 */
> +       INT16   TimeZone;       /* -1440 to 1440 or 2047 */
> +       UINT8   Daylight;
> +       UINT8   Pad2;
> +} __attribute__ ((packed)) EFI_TIME;
> +
> +typedef struct {
> +       UINT32  Resolution;
> +       UINT32  Accuracy;
> +       BOOLEAN SetsToZero;
> +} __attribute__ ((packed)) EFI_TIME_CAPABILITIES;
> +
> +struct efi_getvariable {
> +       CHAR16          *VariableName;
> +       EFI_GUID        *VendorGuid;
> +       UINT32          *Attributes;
> +       UINTN           *DataSize;
> +       VOID            *Data;
> +} __attribute__ ((packed));
> +
> +struct efi_setvariable {
> +       CHAR16          *VariableName;
> +       EFI_GUID        *VendorGuid;
> +       UINT32          Attributes;
> +       UINTN           DataSize;
> +       VOID            *Data;
> +} __attribute__ ((packed));
> +
> +struct efi_getnextvariablename {
> +       UINTN           *VariableNameSize;
> +       CHAR16          *VariableName;
> +       EFI_GUID        *VendorGuid;
> +} __attribute__ ((packed));
> +
> +struct efi_gettime {
> +       EFI_TIME                *Time;
> +       EFI_TIME_CAPABILITIES   *Capabilities;
> +} __attribute__ ((packed));
> +
> +struct efi_settime {
> +       EFI_TIME                *Time;
> +} __attribute__ ((packed));
> +
> +struct efi_getwakeuptime {
> +       BOOLEAN         *Enabled;
> +       BOOLEAN         *Pending;
> +       EFI_TIME        *Time;
> +} __attribute__ ((packed));
> +
> +struct efi_setwakeuptime {
> +       BOOLEAN         Enabled;
> +       EFI_TIME        *Time;
> +} __attribute__ ((packed));
> +
> +/* ioctl calls that are permitted to the /dev/efi_runtime interface. */
> +#define EFI_RUNTIME_GET_VARIABLE \
> +       _IOWR('p', 0x01, struct efi_getvariable)
> +#define EFI_RUNTIME_SET_VARIABLE \
> +       _IOW('p', 0x02, struct efi_setvariable)
> +
> +#define EFI_RUNTIME_GET_TIME \
> +       _IOR('p', 0x03, struct efi_gettime)
> +#define EFI_RUNTIME_SET_TIME \
> +       _IOW('p', 0x04, struct efi_settime)
> +
> +#define EFI_RUNTIME_GET_WAKETIME \
> +       _IOR('p', 0x05, struct efi_getwakeuptime)
> +#define EFI_RUNTIME_SET_WAKETIME \
> +       _IOW('p', 0x06, struct efi_setwakeuptime)
> +
> +#define EFI_RUNTIME_GET_NEXTVARIABLENAME \
> +       _IOWR('p', 0x07, struct efi_getnextvariablename)
> +
> +#endif /* _EFI_RUNTIME_H_ */
> --
> 1.7.9.5
>
>
> --
> fwts-devel mailing list
> fwts-devel@lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/fwts-devel

On 19/09/12 09:58, Ivan Hu wrote:
> efi_runtime kernel driver provides the runtime UEFI interfaces for fwts
> to test the UEFI runtime service implementiation.
> Current capabilities:
> * provide the RT service interfaces:
>    * GetVariable
>    * SetVariable
>    * GetTime
>    * SetTime
>    * GetWakeupTime
>    * SetWakeupTime
>    * GetNextVariableName
>
> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
> ---
>   efi_runtime/Makefile      |    6 +
>   efi_runtime/efi_runtime.c |  311 +++++++++++++++++++++++++++++++++++++++++++++
>   efi_runtime/efi_runtime.h |  128 +++++++++++++++++++
>   3 files changed, 445 insertions(+)
>   create mode 100644 efi_runtime/Makefile
>   create mode 100644 efi_runtime/efi_runtime.c
>   create mode 100644 efi_runtime/efi_runtime.h
>
> diff --git a/efi_runtime/Makefile b/efi_runtime/Makefile
> new file mode 100644
> index 0000000..8ed7dea
> --- /dev/null
> +++ b/efi_runtime/Makefile
> @@ -0,0 +1,6 @@
> +obj-m += efi_runtime.o
> +all:
> +	make -C /lib/modules/`uname -r`/build M=`pwd` modules
> +
> +clean:
> +	make -C /lib/modules/`uname -r`/build M=`pwd` clean
> diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c
> new file mode 100644
> index 0000000..71eae2b
> --- /dev/null
> +++ b/efi_runtime/efi_runtime.c
> @@ -0,0 +1,311 @@
> +/*
> + * EFI Runtime driver
> + *
> + * Copyright(C) 2012 Canonical Ltd.
> + *
> + *  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., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#include <linux/types.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/proc_fs.h>
> +#include <linux/efi.h>
> +
> +#include <linux/uaccess.h>
> +
> +#include "efi_runtime.h"
> +
> +#define EFI_FWTSEFI_VERSION	"0.1"

How about calling this EFI_FWTS_EFI_VERSION instead?

> +
> +MODULE_AUTHOR("Ivan Hu");
> +MODULE_DESCRIPTION("EFI Runtime Driver");
> +MODULE_LICENSE("GPL");
> +
> +static void convert_from_efi_time(efi_time_t *eft, EFI_TIME *time)
> +{
> +	memset(time, 0, sizeof(EFI_TIME));
> +	time->Year = eft->year;
> +	time->Month = eft->month;
> +	time->Day  = eft->day;
> +	time->Hour = eft->hour;
> +	time->Minute = eft->minute;
> +	time->Second  = eft->second;
> +	time->Pad1 = eft->pad1;
> +	time->Nanosecond = eft->nanosecond;
> +	time->TimeZone = eft->timezone;
> +	time->Daylight = eft->daylight;
> +	time->Pad2 = eft->pad2;
> +}
> +
> +static void convert_to_efi_time(efi_time_t *eft, EFI_TIME *time)
> +{
> +	memset(eft, 0, sizeof(eft));
> +	eft->year = time->Year;
> +	eft->month = time->Month;
> +	eft->day = time->Day;
> +	eft->hour = time->Hour;
> +	eft->minute = time->Minute;
> +	eft->second = time->Second;
> +	eft->pad1 = time->Pad1;
> +	eft->nanosecond = time->Nanosecond;
> +	eft->timezone = time->TimeZone;
> +	eft->daylight = time->Daylight;
> +	eft->pad2 = time->Pad2;
> +}
> +
> +static void convert_from_guid(efi_guid_t *vendor, EFI_GUID *vendor_guid)
> +{
> +	int i;
> +	for (i = 0; i < 16; i++) {
> +		if (i < 4)
> +			vendor->b[i] = (vendor_guid->Data1 >> (8*i)) & 0xff;
> +		else if (i < 6)
> +			vendor->b[i] = (vendor_guid->Data2 >> (8*(i-4))) & 0xff;
> +		else if (i < 8)
> +			vendor->b[i] = (vendor_guid->Data3 >> (8*(i-6))) & 0xff;
> +		else
> +			vendor->b[i] = (vendor_guid->Data4[i-8]);
> +	}
> +}

It's too early in the day for my head to process this. Perhaps it would 
be simpler to just do 16 assignments for each element in the GUID. In 
fact, I'm a little confused why we are shuffling these around. Is 
userspace passing in a GUID in a different order from the EFI expected 
order?  In which case, can't this shuffling be done in the user space 
test app instead?

> +
> +static void convert_to_guid(efi_guid_t *vendor, EFI_GUID *vendor_guid)
> +{
> +	int i;
> +	vendor_guid->Data1 = vendor->b[0] + (vendor->b[1] << 8) +
> +				(vendor->b[2] << 16) + (vendor->b[3] << 24);
> +	vendor_guid->Data2 = vendor->b[4] + (vendor->b[5] << 8);
> +	vendor_guid->Data3 = vendor->b[6] + (vendor->b[7] << 8);
> +	for (i = 0; i < 8; i++)
> +		vendor_guid->Data4[i] = vendor->b[i+8];
> +}

Likewise.
> +
> +static long efi_runtime_ioctl(struct file *file, unsigned int cmd,
> +							unsigned long arg)
> +{
> +	efi_status_t status;
> +	struct efi_getvariable __user *pgetvariable;
> +	struct efi_setvariable __user *psetvariable;
> +
> +	efi_guid_t vendor;
> +	EFI_GUID vendor_guid;
> +	unsigned long datasize;
> +	UINT32 attr;
> +
> +	efi_time_t eft;
> +	efi_time_cap_t cap;
> +	struct efi_gettime __user *pgettime;
> +	struct efi_settime __user *psettime;
> +
> +	unsigned char enabled, pending;
> +	EFI_TIME efi_time;
> +	struct efi_getwakeuptime __user *pgetwakeuptime;
> +	struct efi_setwakeuptime __user *psetwakeuptime;
> +
> +	struct efi_getnextvariablename __user *pgetnextvariablename;
> +	unsigned long name_size;
> +
> +	switch (cmd) {
> +	case EFI_RUNTIME_GET_VARIABLE:
> +		pgetvariable = (struct efi_getvariable __user *)arg;
> +
> +		if (get_user(datasize, pgetvariable->DataSize) ||
> +			copy_from_user(&vendor_guid, pgetvariable->VendorGuid,
> +							sizeof(EFI_GUID)))
> +			return -EFAULT;
> +
> +		convert_from_guid(&vendor, &vendor_guid);
> +		status = efi.get_variable(pgetvariable->VariableName, &vendor,
> +					&attr, &datasize, pgetvariable->Data);
> +
> +		if (status == EFI_SUCCESS) {
> +			if (put_user(attr, pgetvariable->Attributes) ||
> +				put_user(datasize, pgetvariable->DataSize))
> +				return -EFAULT;
> +			return 0;
> +		} else {
> +			printk(KERN_ERR "efi_runtime: can't get variable\n");

Is it worth reporting the EFI failure status value too, just to help for 
debugging purposes?

> +			return -EINVAL;
> +		}
> +
> +	case EFI_RUNTIME_SET_VARIABLE:
> +		psetvariable = (struct efi_setvariable __user *)arg;
> +		if (get_user(datasize, &psetvariable->DataSize) ||
> +			get_user(attr, &psetvariable->Attributes) ||
> +			copy_from_user(&vendor_guid, psetvariable->VendorGuid,
> +							sizeof(EFI_GUID)))
> +			return -EFAULT;
> +
> +		convert_from_guid(&vendor, &vendor_guid);
> +		status = efi.set_variable(psetvariable->VariableName, &vendor,
> +					attr, datasize, psetvariable->Data);
> +		return status == EFI_SUCCESS ? 0 : -EINVAL;

Perhaps we should report the failure and status value here too.

> +
> +	case EFI_RUNTIME_GET_TIME:
> +		status = efi.get_time(&eft, &cap);
> +		if (status != EFI_SUCCESS) {
> +			printk(KERN_ERR "efitime: can't read time\n");
> +			return -EINVAL;

and here too.
> +		}
> +
> +		pgettime = (struct efi_gettime __user *)arg;
> +		if (put_user(cap.resolution,
> +					&pgettime->Capabilities->Resolution) ||
> +					put_user(cap.accuracy,
> +					&pgettime->Capabilities->Accuracy) ||
> +					put_user(cap.sets_to_zero,
> +					&pgettime->Capabilities->SetsToZero))
> +			return -EFAULT;
> +		return copy_to_user(pgettime->Time, &eft,
> +				sizeof(EFI_TIME)) ? -EFAULT : 0;
> +
> +	case EFI_RUNTIME_SET_TIME:
> +
> +		psettime = (struct efi_settime __user *)arg;
> +		if (copy_from_user(&efi_time, psettime->Time,
> +						sizeof(EFI_TIME)))
> +			return -EFAULT;
> +		convert_to_efi_time(&eft, &efi_time);
> +		status = efi.set_time(&eft);
> +		return status == EFI_SUCCESS ? 0 : -EINVAL;

and here too
> +
> +	case EFI_RUNTIME_GET_WAKETIME:
> +
> +		status = efi.get_wakeup_time((efi_bool_t *)&enabled,
> +						(efi_bool_t *)&pending, &eft);
> +
> +		if (status != EFI_SUCCESS)
> +			return -EINVAL;

and here too
> +
> +		pgetwakeuptime = (struct efi_getwakeuptime __user *)arg;
> +
> +		if (put_user(enabled, pgetwakeuptime->Enabled) ||
> +				put_user(pending, pgetwakeuptime->Pending))
> +			return -EFAULT;
> +
> +		convert_from_efi_time(&eft, &efi_time);
> +
> +		return copy_to_user(pgetwakeuptime->Time, &efi_time,
> +				sizeof(EFI_TIME)) ? -EFAULT : 0;
> +
> +	case EFI_RUNTIME_SET_WAKETIME:
> +
> +		psetwakeuptime = (struct efi_setwakeuptime __user *)arg;
> +
> +		if (get_user(enabled, &psetwakeuptime->Enabled) ||
> +					copy_from_user(&efi_time,
> +					psetwakeuptime->Time,
> +					sizeof(EFI_TIME)))
> +			return -EFAULT;
> +
> +		convert_to_efi_time(&eft, &efi_time);
> +
> +		status = efi.set_wakeup_time(enabled, &eft);
and here too
> +
> +		return status == EFI_SUCCESS ? 0 : -EINVAL;
> +
> +	case EFI_RUNTIME_GET_NEXTVARIABLENAME:
> +
> +		pgetnextvariablename = (struct efi_getnextvariablename
> +								__user *)arg;
> +
> +		if (get_user(name_size, pgetnextvariablename->VariableNameSize)
> +				|| copy_from_user(&vendor_guid,
> +					pgetnextvariablename->VendorGuid,
> +					sizeof(EFI_GUID)))
> +			return -EFAULT;
> +		if (name_size > 1024)
> +			return -EFAULT;
> +
> +		convert_from_guid(&vendor, &vendor_guid);
> +
> +		status = efi.get_next_variable(&name_size,
> +					pgetnextvariablename->VariableName,
> +								&vendor);

and here too
> +
> +		if (status != EFI_SUCCESS)
> +			return -EINVAL;
> +		convert_to_guid(&vendor, &vendor_guid);
> +
> +		if (put_user(name_size, pgetnextvariablename->VariableNameSize))
> +			return -EFAULT;
> +
> +		if (copy_to_user(pgetnextvariablename->VendorGuid,
> +						&vendor_guid, sizeof(EFI_GUID)))
> +			return -EFAULT;
> +		return 0;
> +	}
> +
> +	return -ENOTTY;
> +}
> +
> +static int efi_runtime_open(struct inode *inode, struct file *file)
> +{
> +	/*
> +	 * nothing special to do here
> +	 * We do accept multiple open files at the same time as we
> +	 * synchronize on the per call operation.
> +	 */
> +	return 0;
> +}
> +
> +static int efi_runtime_close(struct inode *inode, struct file *file)
> +{
> +	return 0;
> +}
> +
> +/*
> + *	The various file operations we support.
> + */
> +static const struct file_operations efi_runtime_fops = {
> +	.owner		= THIS_MODULE,
> +	.unlocked_ioctl	= efi_runtime_ioctl,
> +	.open		= efi_runtime_open,
> +	.release	= efi_runtime_close,
> +	.llseek		= no_llseek,
> +};
> +
> +static struct miscdevice efi_runtime_dev = {
> +	MISC_DYNAMIC_MINOR,
> +	"efi_runtime",
> +	&efi_runtime_fops
> +};
> +
> +static int __init efi_runtime_init(void)
> +{
> +	int ret;
> +
> +	printk(KERN_INFO "EFI_RUNTIME Driver v%s\n", EFI_FWTSEFI_VERSION);
> +
> +	ret = misc_register(&efi_runtime_dev);
> +	if (ret) {
> +		printk(KERN_ERR "efi_runtime: can't misc_register on minor=%d\n",
> +				MISC_DYNAMIC_MINOR);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void __exit efi_runtime_exit(void)
> +{
> +	printk(KERN_INFO "EFI_RUNTIME Driver Exit.\n");
> +	misc_deregister(&efi_runtime_dev);
> +}
> +
> +module_init(efi_runtime_init);
> +module_exit(efi_runtime_exit);
> +
> diff --git a/efi_runtime/efi_runtime.h b/efi_runtime/efi_runtime.h
> new file mode 100644
> index 0000000..bac4dc5
> --- /dev/null
> +++ b/efi_runtime/efi_runtime.h
> @@ -0,0 +1,128 @@
> +/*
> + * EFI Runtime driver
> + *
> + * Copyright(C) 2012 Canonical Ltd.
> + *
> + *  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., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#ifndef _EFI_RUNTIME_H_
> +#define _EFI_RUNTIME_H_
> +
> +#include <linux/types.h>
> +
> +typedef unsigned long int	UINTN;
> +typedef unsigned long long	UINT64;
> +typedef long long		INT64;
> +typedef unsigned int		UINT32;
> +typedef int			INT32;
> +typedef unsigned short		UINT16;
> +typedef unsigned short		CHAR16;
> +typedef short			INT16;
> +typedef unsigned char		BOOLEAN;
> +typedef unsigned char		UINT8;
> +typedef char			CHAR8;
> +typedef char			INT8;
> +typedef void			VOID;

Is there any reason why we are typedef'ing all of these?
Why can we use standard Linux types, such as uint8_t, etc?

> +
> +typedef struct {
> +	UINT32  Data1;
> +	UINT16  Data2;
> +	UINT16  Data3;
> +	UINT8   Data4[8];
> +} __attribute__ ((packed)) EFI_GUID;
> +
> +typedef struct {
> +	UINT16	Year;		/* 1900 – 9999 */
> +	UINT8	Month;		/* 1 – 12 */
> +	UINT8	Day;		/* 1 – 31 */
> +	UINT8	Hour;		/* 0 – 23 */
> +	UINT8	Minute;		/* 0 – 59 */
> +	UINT8	Second;		/* 0 – 59 */
> +	UINT8	Pad1;
> +	UINT32	Nanosecond;	/* 0 – 999,999,999 */
> +	INT16	TimeZone;	/* -1440 to 1440 or 2047 */
> +	UINT8	Daylight;
> +	UINT8	Pad2;
> +} __attribute__ ((packed)) EFI_TIME;
> +
> +typedef struct {
> +	UINT32	Resolution;
> +	UINT32	Accuracy;
> +	BOOLEAN	SetsToZero;
> +} __attribute__ ((packed)) EFI_TIME_CAPABILITIES;
> +
> +struct efi_getvariable {
> +	CHAR16		*VariableName;
> +	EFI_GUID	*VendorGuid;
> +	UINT32		*Attributes;
> +	UINTN		*DataSize;
> +	VOID		*Data;
> +} __attribute__ ((packed));
> +
> +struct efi_setvariable {
> +	CHAR16		*VariableName;
> +	EFI_GUID	*VendorGuid;
> +	UINT32		Attributes;
> +	UINTN		DataSize;
> +	VOID		*Data;
> +} __attribute__ ((packed));
> +
> +struct efi_getnextvariablename {
> +	UINTN		*VariableNameSize;
> +	CHAR16		*VariableName;
> +	EFI_GUID	*VendorGuid;
> +} __attribute__ ((packed));
> +
> +struct efi_gettime {
> +	EFI_TIME		*Time;
> +	EFI_TIME_CAPABILITIES	*Capabilities;
> +} __attribute__ ((packed));
> +
> +struct efi_settime {
> +	EFI_TIME		*Time;
> +} __attribute__ ((packed));
> +
> +struct efi_getwakeuptime {
> +	BOOLEAN		*Enabled;
> +	BOOLEAN		*Pending;
> +	EFI_TIME	*Time;
> +} __attribute__ ((packed));
> +
> +struct efi_setwakeuptime {
> +	BOOLEAN		Enabled;
> +	EFI_TIME	*Time;
> +} __attribute__ ((packed));
> +
> +/* ioctl calls that are permitted to the /dev/efi_runtime interface. */
> +#define EFI_RUNTIME_GET_VARIABLE \
> +	_IOWR('p', 0x01, struct efi_getvariable)
> +#define EFI_RUNTIME_SET_VARIABLE \
> +	_IOW('p', 0x02, struct efi_setvariable)
> +
> +#define EFI_RUNTIME_GET_TIME \
> +	_IOR('p', 0x03, struct efi_gettime)
> +#define EFI_RUNTIME_SET_TIME \
> +	_IOW('p', 0x04, struct efi_settime)
> +
> +#define EFI_RUNTIME_GET_WAKETIME \
> +	_IOR('p', 0x05, struct efi_getwakeuptime)
> +#define EFI_RUNTIME_SET_WAKETIME \
> +	_IOW('p', 0x06, struct efi_setwakeuptime)
> +
> +#define EFI_RUNTIME_GET_NEXTVARIABLENAME \
> +	_IOWR('p', 0x07, struct efi_getnextvariablename)
> +
> +#endif /* _EFI_RUNTIME_H_ */
>

On 09/19/2012 06:46 PM, Colin Ian King wrote:
> On 19/09/12 09:58, Ivan Hu wrote:
>> efi_runtime kernel driver provides the runtime UEFI interfaces for fwts
>> to test the UEFI runtime service implementiation.
>> Current capabilities:
>> * provide the RT service interfaces:
>>    * GetVariable
>>    * SetVariable
>>    * GetTime
>>    * SetTime
>>    * GetWakeupTime
>>    * SetWakeupTime
>>    * GetNextVariableName
>>
>> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
>> ---
>>   efi_runtime/Makefile      |    6 +
>>   efi_runtime/efi_runtime.c |  311
>> +++++++++++++++++++++++++++++++++++++++++++++
>>   efi_runtime/efi_runtime.h |  128 +++++++++++++++++++
>>   3 files changed, 445 insertions(+)
>>   create mode 100644 efi_runtime/Makefile
>>   create mode 100644 efi_runtime/efi_runtime.c
>>   create mode 100644 efi_runtime/efi_runtime.h
>>
>> diff --git a/efi_runtime/Makefile b/efi_runtime/Makefile
>> new file mode 100644
>> index 0000000..8ed7dea
>> --- /dev/null
>> +++ b/efi_runtime/Makefile
>> @@ -0,0 +1,6 @@
>> +obj-m += efi_runtime.o
>> +all:
>> +    make -C /lib/modules/`uname -r`/build M=`pwd` modules
>> +
>> +clean:
>> +    make -C /lib/modules/`uname -r`/build M=`pwd` clean
>> diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c
>> new file mode 100644
>> index 0000000..71eae2b
>> --- /dev/null
>> +++ b/efi_runtime/efi_runtime.c
>> @@ -0,0 +1,311 @@
>> +/*
>> + * EFI Runtime driver
>> + *
>> + * Copyright(C) 2012 Canonical Ltd.
>> + *
>> + *  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., 59 Temple Place, Suite 330, Boston, MA
>> 02111-1307  USA
>> + */
>> +
>> +#include <linux/types.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/proc_fs.h>
>> +#include <linux/efi.h>
>> +
>> +#include <linux/uaccess.h>
>> +
>> +#include "efi_runtime.h"
>> +
>> +#define EFI_FWTSEFI_VERSION    "0.1"
>
> How about calling this EFI_FWTS_EFI_VERSION instead?
>

agree. Thanks!

>> +
>> +MODULE_AUTHOR("Ivan Hu");
>> +MODULE_DESCRIPTION("EFI Runtime Driver");
>> +MODULE_LICENSE("GPL");
>> +
>> +static void convert_from_efi_time(efi_time_t *eft, EFI_TIME *time)
>> +{
>> +    memset(time, 0, sizeof(EFI_TIME));
>> +    time->Year = eft->year;
>> +    time->Month = eft->month;
>> +    time->Day  = eft->day;
>> +    time->Hour = eft->hour;
>> +    time->Minute = eft->minute;
>> +    time->Second  = eft->second;
>> +    time->Pad1 = eft->pad1;
>> +    time->Nanosecond = eft->nanosecond;
>> +    time->TimeZone = eft->timezone;
>> +    time->Daylight = eft->daylight;
>> +    time->Pad2 = eft->pad2;
>> +}
>> +
>> +static void convert_to_efi_time(efi_time_t *eft, EFI_TIME *time)
>> +{
>> +    memset(eft, 0, sizeof(eft));
>> +    eft->year = time->Year;
>> +    eft->month = time->Month;
>> +    eft->day = time->Day;
>> +    eft->hour = time->Hour;
>> +    eft->minute = time->Minute;
>> +    eft->second = time->Second;
>> +    eft->pad1 = time->Pad1;
>> +    eft->nanosecond = time->Nanosecond;
>> +    eft->timezone = time->TimeZone;
>> +    eft->daylight = time->Daylight;
>> +    eft->pad2 = time->Pad2;
>> +}
>> +
>> +static void convert_from_guid(efi_guid_t *vendor, EFI_GUID *vendor_guid)
>> +{
>> +    int i;
>> +    for (i = 0; i < 16; i++) {
>> +        if (i < 4)
>> +            vendor->b[i] = (vendor_guid->Data1 >> (8*i)) & 0xff;
>> +        else if (i < 6)
>> +            vendor->b[i] = (vendor_guid->Data2 >> (8*(i-4))) & 0xff;
>> +        else if (i < 8)
>> +            vendor->b[i] = (vendor_guid->Data3 >> (8*(i-6))) & 0xff;
>> +        else
>> +            vendor->b[i] = (vendor_guid->Data4[i-8]);
>> +    }
>> +}
>
> It's too early in the day for my head to process this. Perhaps it would
> be simpler to just do 16 assignments for each element in the GUID. In
> fact, I'm a little confused why we are shuffling these around. Is
> userspace passing in a GUID in a different order from the EFI expected
> order?  In which case, can't this shuffling be done in the user space
> test app instead?

This convert function is used for the inconsistent type define between 
efi_guid_t (define in the efi.h) and EFI_GUID (define exactly the same 
as UEFI spec).
sure. this could be done in the user space to use the efi_guid_t without 
EFI_GUID define and the convert function instead. But I define the 
EFI_GUID exactly follow the UEFI spec two main purposes:
1. this runtime kernel module provide UEFI RT service interfaces which 
could be used more straightforward followed by the UEFI spec.
2. It only needs slightly efforts that if we have a UEFI shell 
application/driver and would like it to run on linux with this driver in 
the future.

>
>> +
>> +static void convert_to_guid(efi_guid_t *vendor, EFI_GUID *vendor_guid)
>> +{
>> +    int i;
>> +    vendor_guid->Data1 = vendor->b[0] + (vendor->b[1] << 8) +
>> +                (vendor->b[2] << 16) + (vendor->b[3] << 24);
>> +    vendor_guid->Data2 = vendor->b[4] + (vendor->b[5] << 8);
>> +    vendor_guid->Data3 = vendor->b[6] + (vendor->b[7] << 8);
>> +    for (i = 0; i < 8; i++)
>> +        vendor_guid->Data4[i] = vendor->b[i+8];
>> +}
>
> Likewise.
>> +
>> +static long efi_runtime_ioctl(struct file *file, unsigned int cmd,
>> +                            unsigned long arg)
>> +{
>> +    efi_status_t status;
>> +    struct efi_getvariable __user *pgetvariable;
>> +    struct efi_setvariable __user *psetvariable;
>> +
>> +    efi_guid_t vendor;
>> +    EFI_GUID vendor_guid;
>> +    unsigned long datasize;
>> +    UINT32 attr;
>> +
>> +    efi_time_t eft;
>> +    efi_time_cap_t cap;
>> +    struct efi_gettime __user *pgettime;
>> +    struct efi_settime __user *psettime;
>> +
>> +    unsigned char enabled, pending;
>> +    EFI_TIME efi_time;
>> +    struct efi_getwakeuptime __user *pgetwakeuptime;
>> +    struct efi_setwakeuptime __user *psetwakeuptime;
>> +
>> +    struct efi_getnextvariablename __user *pgetnextvariablename;
>> +    unsigned long name_size;
>> +
>> +    switch (cmd) {
>> +    case EFI_RUNTIME_GET_VARIABLE:
>> +        pgetvariable = (struct efi_getvariable __user *)arg;
>> +
>> +        if (get_user(datasize, pgetvariable->DataSize) ||
>> +            copy_from_user(&vendor_guid, pgetvariable->VendorGuid,
>> +                            sizeof(EFI_GUID)))
>> +            return -EFAULT;
>> +
>> +        convert_from_guid(&vendor, &vendor_guid);
>> +        status = efi.get_variable(pgetvariable->VariableName, &vendor,
>> +                    &attr, &datasize, pgetvariable->Data);
>> +
>> +        if (status == EFI_SUCCESS) {
>> +            if (put_user(attr, pgetvariable->Attributes) ||
>> +                put_user(datasize, pgetvariable->DataSize))
>> +                return -EFAULT;
>> +            return 0;
>> +        } else {
>> +            printk(KERN_ERR "efi_runtime: can't get variable\n");
>
> Is it worth reporting the EFI failure status value too, just to help for
> debugging purposes?

agree. Thanks!
>
>> +            return -EINVAL;
>> +        }
>> +
>> +    case EFI_RUNTIME_SET_VARIABLE:
>> +        psetvariable = (struct efi_setvariable __user *)arg;
>> +        if (get_user(datasize, &psetvariable->DataSize) ||
>> +            get_user(attr, &psetvariable->Attributes) ||
>> +            copy_from_user(&vendor_guid, psetvariable->VendorGuid,
>> +                            sizeof(EFI_GUID)))
>> +            return -EFAULT;
>> +
>> +        convert_from_guid(&vendor, &vendor_guid);
>> +        status = efi.set_variable(psetvariable->VariableName, &vendor,
>> +                    attr, datasize, psetvariable->Data);
>> +        return status == EFI_SUCCESS ? 0 : -EINVAL;
>
> Perhaps we should report the failure and status value here too.
>
agree.

>> +
>> +    case EFI_RUNTIME_GET_TIME:
>> +        status = efi.get_time(&eft, &cap);
>> +        if (status != EFI_SUCCESS) {
>> +            printk(KERN_ERR "efitime: can't read time\n");
>> +            return -EINVAL;
>
> and here too.
agree.
>> +        }
>> +
>> +        pgettime = (struct efi_gettime __user *)arg;
>> +        if (put_user(cap.resolution,
>> +                    &pgettime->Capabilities->Resolution) ||
>> +                    put_user(cap.accuracy,
>> +                    &pgettime->Capabilities->Accuracy) ||
>> +                    put_user(cap.sets_to_zero,
>> +                    &pgettime->Capabilities->SetsToZero))
>> +            return -EFAULT;
>> +        return copy_to_user(pgettime->Time, &eft,
>> +                sizeof(EFI_TIME)) ? -EFAULT : 0;
>> +
>> +    case EFI_RUNTIME_SET_TIME:
>> +
>> +        psettime = (struct efi_settime __user *)arg;
>> +        if (copy_from_user(&efi_time, psettime->Time,
>> +                        sizeof(EFI_TIME)))
>> +            return -EFAULT;
>> +        convert_to_efi_time(&eft, &efi_time);
>> +        status = efi.set_time(&eft);
>> +        return status == EFI_SUCCESS ? 0 : -EINVAL;
>
> and here too
agree.
>> +
>> +    case EFI_RUNTIME_GET_WAKETIME:
>> +
>> +        status = efi.get_wakeup_time((efi_bool_t *)&enabled,
>> +                        (efi_bool_t *)&pending, &eft);
>> +
>> +        if (status != EFI_SUCCESS)
>> +            return -EINVAL;
>
> and here too
agree.
>> +
>> +        pgetwakeuptime = (struct efi_getwakeuptime __user *)arg;
>> +
>> +        if (put_user(enabled, pgetwakeuptime->Enabled) ||
>> +                put_user(pending, pgetwakeuptime->Pending))
>> +            return -EFAULT;
>> +
>> +        convert_from_efi_time(&eft, &efi_time);
>> +
>> +        return copy_to_user(pgetwakeuptime->Time, &efi_time,
>> +                sizeof(EFI_TIME)) ? -EFAULT : 0;
>> +
>> +    case EFI_RUNTIME_SET_WAKETIME:
>> +
>> +        psetwakeuptime = (struct efi_setwakeuptime __user *)arg;
>> +
>> +        if (get_user(enabled, &psetwakeuptime->Enabled) ||
>> +                    copy_from_user(&efi_time,
>> +                    psetwakeuptime->Time,
>> +                    sizeof(EFI_TIME)))
>> +            return -EFAULT;
>> +
>> +        convert_to_efi_time(&eft, &efi_time);
>> +
>> +        status = efi.set_wakeup_time(enabled, &eft);
> and here too
agree.
>> +
>> +        return status == EFI_SUCCESS ? 0 : -EINVAL;
>> +
>> +    case EFI_RUNTIME_GET_NEXTVARIABLENAME:
>> +
>> +        pgetnextvariablename = (struct efi_getnextvariablename
>> +                                __user *)arg;
>> +
>> +        if (get_user(name_size, pgetnextvariablename->VariableNameSize)
>> +                || copy_from_user(&vendor_guid,
>> +                    pgetnextvariablename->VendorGuid,
>> +                    sizeof(EFI_GUID)))
>> +            return -EFAULT;
>> +        if (name_size > 1024)
>> +            return -EFAULT;
>> +
>> +        convert_from_guid(&vendor, &vendor_guid);
>> +
>> +        status = efi.get_next_variable(&name_size,
>> +                    pgetnextvariablename->VariableName,
>> +                                &vendor);
>
> and here too
agree.
>> +
>> +        if (status != EFI_SUCCESS)
>> +            return -EINVAL;
>> +        convert_to_guid(&vendor, &vendor_guid);
>> +
>> +        if (put_user(name_size, pgetnextvariablename->VariableNameSize))
>> +            return -EFAULT;
>> +
>> +        if (copy_to_user(pgetnextvariablename->VendorGuid,
>> +                        &vendor_guid, sizeof(EFI_GUID)))
>> +            return -EFAULT;
>> +        return 0;
>> +    }
>> +
>> +    return -ENOTTY;
>> +}
>> +
>> +static int efi_runtime_open(struct inode *inode, struct file *file)
>> +{
>> +    /*
>> +     * nothing special to do here
>> +     * We do accept multiple open files at the same time as we
>> +     * synchronize on the per call operation.
>> +     */
>> +    return 0;
>> +}
>> +
>> +static int efi_runtime_close(struct inode *inode, struct file *file)
>> +{
>> +    return 0;
>> +}
>> +
>> +/*
>> + *    The various file operations we support.
>> + */
>> +static const struct file_operations efi_runtime_fops = {
>> +    .owner        = THIS_MODULE,
>> +    .unlocked_ioctl    = efi_runtime_ioctl,
>> +    .open        = efi_runtime_open,
>> +    .release    = efi_runtime_close,
>> +    .llseek        = no_llseek,
>> +};
>> +
>> +static struct miscdevice efi_runtime_dev = {
>> +    MISC_DYNAMIC_MINOR,
>> +    "efi_runtime",
>> +    &efi_runtime_fops
>> +};
>> +
>> +static int __init efi_runtime_init(void)
>> +{
>> +    int ret;
>> +
>> +    printk(KERN_INFO "EFI_RUNTIME Driver v%s\n", EFI_FWTSEFI_VERSION);
>> +
>> +    ret = misc_register(&efi_runtime_dev);
>> +    if (ret) {
>> +        printk(KERN_ERR "efi_runtime: can't misc_register on
>> minor=%d\n",
>> +                MISC_DYNAMIC_MINOR);
>> +        return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void __exit efi_runtime_exit(void)
>> +{
>> +    printk(KERN_INFO "EFI_RUNTIME Driver Exit.\n");
>> +    misc_deregister(&efi_runtime_dev);
>> +}
>> +
>> +module_init(efi_runtime_init);
>> +module_exit(efi_runtime_exit);
>> +
>> diff --git a/efi_runtime/efi_runtime.h b/efi_runtime/efi_runtime.h
>> new file mode 100644
>> index 0000000..bac4dc5
>> --- /dev/null
>> +++ b/efi_runtime/efi_runtime.h
>> @@ -0,0 +1,128 @@
>> +/*
>> + * EFI Runtime driver
>> + *
>> + * Copyright(C) 2012 Canonical Ltd.
>> + *
>> + *  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., 59 Temple Place, Suite 330, Boston, MA
>> 02111-1307  USA
>> + */
>> +
>> +#ifndef _EFI_RUNTIME_H_
>> +#define _EFI_RUNTIME_H_
>> +
>> +#include <linux/types.h>
>> +
>> +typedef unsigned long int    UINTN;
>> +typedef unsigned long long    UINT64;
>> +typedef long long        INT64;
>> +typedef unsigned int        UINT32;
>> +typedef int            INT32;
>> +typedef unsigned short        UINT16;
>> +typedef unsigned short        CHAR16;
>> +typedef short            INT16;
>> +typedef unsigned char        BOOLEAN;
>> +typedef unsigned char        UINT8;
>> +typedef char            CHAR8;
>> +typedef char            INT8;
>> +typedef void            VOID;
>
> Is there any reason why we are typedef'ing all of these?
> Why can we use standard Linux types, such as uint8_t, etc?

like what I mentioned above, I'd like to define the below structure 
exactly the same as UEFI spec definition for user space test application 
to include and could be coding more straightforward followed the UEFI spec.

>
>> +
>> +typedef struct {
>> +    UINT32  Data1;
>> +    UINT16  Data2;
>> +    UINT16  Data3;
>> +    UINT8   Data4[8];
>> +} __attribute__ ((packed)) EFI_GUID;
>> +
>> +typedef struct {
>> +    UINT16    Year;        /* 1900 – 9999 */
>> +    UINT8    Month;        /* 1 – 12 */
>> +    UINT8    Day;        /* 1 – 31 */
>> +    UINT8    Hour;        /* 0 – 23 */
>> +    UINT8    Minute;        /* 0 – 59 */
>> +    UINT8    Second;        /* 0 – 59 */
>> +    UINT8    Pad1;
>> +    UINT32    Nanosecond;    /* 0 – 999,999,999 */
>> +    INT16    TimeZone;    /* -1440 to 1440 or 2047 */
>> +    UINT8    Daylight;
>> +    UINT8    Pad2;
>> +} __attribute__ ((packed)) EFI_TIME;
>> +
>> +typedef struct {
>> +    UINT32    Resolution;
>> +    UINT32    Accuracy;
>> +    BOOLEAN    SetsToZero;
>> +} __attribute__ ((packed)) EFI_TIME_CAPABILITIES;
>> +
>> +struct efi_getvariable {
>> +    CHAR16        *VariableName;
>> +    EFI_GUID    *VendorGuid;
>> +    UINT32        *Attributes;
>> +    UINTN        *DataSize;
>> +    VOID        *Data;
>> +} __attribute__ ((packed));
>> +
>> +struct efi_setvariable {
>> +    CHAR16        *VariableName;
>> +    EFI_GUID    *VendorGuid;
>> +    UINT32        Attributes;
>> +    UINTN        DataSize;
>> +    VOID        *Data;
>> +} __attribute__ ((packed));
>> +
>> +struct efi_getnextvariablename {
>> +    UINTN        *VariableNameSize;
>> +    CHAR16        *VariableName;
>> +    EFI_GUID    *VendorGuid;
>> +} __attribute__ ((packed));
>> +
>> +struct efi_gettime {
>> +    EFI_TIME        *Time;
>> +    EFI_TIME_CAPABILITIES    *Capabilities;
>> +} __attribute__ ((packed));
>> +
>> +struct efi_settime {
>> +    EFI_TIME        *Time;
>> +} __attribute__ ((packed));
>> +
>> +struct efi_getwakeuptime {
>> +    BOOLEAN        *Enabled;
>> +    BOOLEAN        *Pending;
>> +    EFI_TIME    *Time;
>> +} __attribute__ ((packed));
>> +
>> +struct efi_setwakeuptime {
>> +    BOOLEAN        Enabled;
>> +    EFI_TIME    *Time;
>> +} __attribute__ ((packed));
>> +
>> +/* ioctl calls that are permitted to the /dev/efi_runtime interface. */
>> +#define EFI_RUNTIME_GET_VARIABLE \
>> +    _IOWR('p', 0x01, struct efi_getvariable)
>> +#define EFI_RUNTIME_SET_VARIABLE \
>> +    _IOW('p', 0x02, struct efi_setvariable)
>> +
>> +#define EFI_RUNTIME_GET_TIME \
>> +    _IOR('p', 0x03, struct efi_gettime)
>> +#define EFI_RUNTIME_SET_TIME \
>> +    _IOW('p', 0x04, struct efi_settime)
>> +
>> +#define EFI_RUNTIME_GET_WAKETIME \
>> +    _IOR('p', 0x05, struct efi_getwakeuptime)
>> +#define EFI_RUNTIME_SET_WAKETIME \
>> +    _IOW('p', 0x06, struct efi_setwakeuptime)
>> +
>> +#define EFI_RUNTIME_GET_NEXTVARIABLENAME \
>> +    _IOWR('p', 0x07, struct efi_getnextvariablename)
>> +
>> +#endif /* _EFI_RUNTIME_H_ */
>>
>
>

let me know if you have any comments, let us decide these typedef'ings 
are necessary or not.

cheers,
Ivan