[PATCH v4 0/5] xen: Add EFI support

[PATCH v4 0/5] xen: Add EFI support

[Daniel Kiper]

Hey,

This patch series adds EFI support for Xen dom0 guests.
It is based on Jan Beulich and Tang Liang work. I was
trying to take into account all previous comments,
however, if I missed something sorry for that.

I am still not sure what to do with /sys/firmware/efi/config_table,
/sys/firmware/efi/{fw_vendor,runtime,systab} files. On bare metal
they contain physical addresses of relevant structures. However,
in Xen case they does not make sens. So maybe they should contain
invalid values (e.g. 0) or should not appear at all on Xen (I prefer
last one). What do you think about that?

Daniel

 arch/x86/kernel/setup.c          |    6 +-
 arch/x86/platform/efi/efi.c      |   60 ++++++++++----
 arch/x86/xen/enlighten.c         |   26 ++++++
 drivers/xen/Kconfig              |    3 +
 drivers/xen/Makefile             |    1 +
 drivers/xen/efi.c                |  374 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/efi.h              |   13 +--
 include/xen/interface/platform.h |  123 +++++++++++++++++++++++++++
 8 files changed, 582 insertions(+), 24 deletions(-)

Daniel Kiper (5):
      efi: Introduce EFI_DIRECT flag
      xen: Define EFI related stuff
      xen: Put EFI machinery in place
      arch/x86: Replace plain strings with constants
      arch/x86: Remove redundant set_bit() call

[PATCH v4 1/5] efi: Introduce EFI_DIRECT flag

[Daniel Kiper]

Introduce EFI_DIRECT flag. If it is set this means that Linux
Kernel has direct access to EFI infrastructure. If not then
kernel runs on EFI platform but it has not direct control
on EFI stuff. This functionality is used in Xen dom0.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 arch/x86/kernel/setup.c     |    2 ++
 arch/x86/platform/efi/efi.c |   58 ++++++++++++++++++++++++++++++++-----------
 include/linux/efi.h         |   13 +++++-----
 3 files changed, 53 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 09c76d2..f41f648 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -926,9 +926,11 @@ void __init setup_arch(char **cmdline_p)
 	if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature,
 		     "EL32", 4)) {
 		set_bit(EFI_BOOT, &efi.flags);
+		set_bit(EFI_DIRECT, &efi.flags);
 	} else if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature,
 		     "EL64", 4)) {
 		set_bit(EFI_BOOT, &efi.flags);
+		set_bit(EFI_DIRECT, &efi.flags);
 		set_bit(EFI_64BIT, &efi.flags);
 	}
 
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 3781dd3..7fcef06 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -352,6 +352,9 @@ int __init efi_memblock_x86_reserve_range(void)
 	struct efi_info *e = &boot_params.efi_info;
 	unsigned long pmap;
 
+	if (!efi_enabled(EFI_DIRECT))
+		return 0;
+
 #ifdef CONFIG_X86_32
 	/* Can't handle data above 4GB at this time */
 	if (e->efi_memmap_hi) {
@@ -457,6 +460,21 @@ void __init efi_free_boot_services(void)
 	efi_unmap_memmap();
 }
 
+static void __init __iomem *efi_early_ioremap(resource_size_t phys_addr,
+							unsigned long size)
+{
+	if (efi_enabled(EFI_DIRECT))
+		return early_ioremap(phys_addr, size);
+
+	return (__force void __iomem *)phys_addr;
+}
+
+static void __init efi_early_iounmap(void __iomem *addr, unsigned long size)
+{
+	if (efi_enabled(EFI_DIRECT))
+		early_iounmap(addr, size);
+}
+
 static int __init efi_systab_init(void *phys)
 {
 	if (efi_enabled(EFI_64BIT)) {
@@ -469,8 +487,8 @@ static int __init efi_systab_init(void *phys)
 			if (!data)
 				return -ENOMEM;
 		}
-		systab64 = early_ioremap((unsigned long)phys,
-					 sizeof(*systab64));
+		systab64 = efi_early_ioremap((unsigned long)phys,
+							sizeof(*systab64));
 		if (systab64 == NULL) {
 			pr_err("Couldn't map the system table!\n");
 			if (data)
@@ -506,7 +524,7 @@ static int __init efi_systab_init(void *phys)
 					   systab64->tables;
 		tmp |= data ? data->tables : systab64->tables;
 
-		early_iounmap(systab64, sizeof(*systab64));
+		efi_early_iounmap(systab64, sizeof(*systab64));
 		if (data)
 			early_iounmap(data, sizeof(*data));
 #ifdef CONFIG_X86_32
@@ -518,8 +536,8 @@ static int __init efi_systab_init(void *phys)
 	} else {
 		efi_system_table_32_t *systab32;
 
-		systab32 = early_ioremap((unsigned long)phys,
-					 sizeof(*systab32));
+		systab32 = efi_early_ioremap((unsigned long)phys,
+							sizeof(*systab32));
 		if (systab32 == NULL) {
 			pr_err("Couldn't map the system table!\n");
 			return -ENOMEM;
@@ -539,7 +557,7 @@ static int __init efi_systab_init(void *phys)
 		efi_systab.nr_tables = systab32->nr_tables;
 		efi_systab.tables = systab32->tables;
 
-		early_iounmap(systab32, sizeof(*systab32));
+		efi_early_iounmap(systab32, sizeof(*systab32));
 	}
 
 	efi.systab = &efi_systab;
@@ -619,13 +637,16 @@ static int __init efi_runtime_init(void)
 	 * address of several of the EFI runtime functions, needed to
 	 * set the firmware into virtual mode.
 	 */
-	if (efi_enabled(EFI_64BIT))
-		rv = efi_runtime_init64();
-	else
-		rv = efi_runtime_init32();
 
-	if (rv)
-		return rv;
+	if (efi_enabled(EFI_DIRECT)) {
+		if (efi_enabled(EFI_64BIT))
+			rv = efi_runtime_init64();
+		else
+			rv = efi_runtime_init32();
+
+		if (rv)
+			return rv;
+	}
 
 	set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
 
@@ -634,6 +655,9 @@ static int __init efi_runtime_init(void)
 
 static int __init efi_memmap_init(void)
 {
+	if (!efi_enabled(EFI_DIRECT))
+		return 0;
+
 	/* Map the EFI memory map */
 	memmap.map = early_ioremap((unsigned long)memmap.phys_map,
 				   memmap.nr_map * memmap.desc_size);
@@ -739,14 +763,14 @@ void __init efi_init(void)
 	/*
 	 * Show what we know for posterity
 	 */
-	c16 = tmp = early_ioremap(efi.systab->fw_vendor, 2);
+	c16 = tmp = efi_early_ioremap(efi.systab->fw_vendor, 2);
 	if (c16) {
 		for (i = 0; i < sizeof(vendor) - 1 && *c16; ++i)
 			vendor[i] = *c16++;
 		vendor[i] = '\0';
 	} else
 		pr_err("Could not map the firmware vendor!\n");
-	early_iounmap(tmp, 2);
+	efi_early_iounmap(tmp, 2);
 
 	pr_info("EFI v%u.%.02u by %s\n",
 		efi.systab->hdr.revision >> 16,
@@ -1187,6 +1211,9 @@ static void __init __efi_enter_virtual_mode(void)
 
 void __init efi_enter_virtual_mode(void)
 {
+	if (!efi_enabled(EFI_DIRECT))
+		return;
+
 	if (efi_setup)
 		kexec_enter_virtual_mode();
 	else
@@ -1219,6 +1246,9 @@ u64 efi_mem_attributes(unsigned long phys_addr)
 	efi_memory_desc_t *md;
 	void *p;
 
+	if (!efi_enabled(EFI_MEMMAP))
+		return 0;
+
 	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
 		md = p;
 		if ((md->phys_addr <= phys_addr) &&
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 6c100ff..4113c4e 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -893,12 +893,13 @@ extern int __init efi_setup_pcdp_console(char *);
  * possible, remove EFI-related code altogether.
  */
 #define EFI_BOOT		0	/* Were we booted from EFI? */
-#define EFI_SYSTEM_TABLES	1	/* Can we use EFI system tables? */
-#define EFI_CONFIG_TABLES	2	/* Can we use EFI config tables? */
-#define EFI_RUNTIME_SERVICES	3	/* Can we use runtime services? */
-#define EFI_MEMMAP		4	/* Can we use EFI memory map? */
-#define EFI_64BIT		5	/* Is the firmware 64-bit? */
-#define EFI_ARCH_1		6	/* First arch-specific bit */
+#define EFI_DIRECT		1	/* Can we access EFI directly? */
+#define EFI_SYSTEM_TABLES	2	/* Can we use EFI system tables? */
+#define EFI_CONFIG_TABLES	3	/* Can we use EFI config tables? */
+#define EFI_RUNTIME_SERVICES	4	/* Can we use runtime services? */
+#define EFI_MEMMAP		5	/* Can we use EFI memory map? */
+#define EFI_64BIT		6	/* Is the firmware 64-bit? */
+#define EFI_ARCH_1		7	/* First arch-specific bit */
 
 #ifdef CONFIG_EFI
 /*
-- 
1.7.10.4

Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag

[Jan Beulich]

>>> On 16.05.14 at 22:41, <daniel.kiper@oracle.com> wrote:
> @@ -457,6 +460,21 @@ void __init efi_free_boot_services(void)
>  	efi_unmap_memmap();
>  }
>  
> +static void __init __iomem *efi_early_ioremap(resource_size_t phys_addr,
> +							unsigned long size)
> +{
> +	if (efi_enabled(EFI_DIRECT))
> +		return early_ioremap(phys_addr, size);
> +
> +	return (__force void __iomem *)phys_addr;

Now that surely needs some explanation: I can't see how this can
ever be correct, Xen or not being completely irrelevant.

Jan

Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag

[Daniel Kiper]

On Mon, May 19, 2014 at 02:30:45PM +0100, Jan Beulich wrote:
> >>> On 16.05.14 at 22:41, <daniel.kiper-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote:
> > @@ -457,6 +460,21 @@ void __init efi_free_boot_services(void)
> >  	efi_unmap_memmap();
> >  }
> >
> > +static void __init __iomem *efi_early_ioremap(resource_size_t phys_addr,
> > +							unsigned long size)
> > +{
> > +	if (efi_enabled(EFI_DIRECT))
> > +		return early_ioremap(phys_addr, size);
> > +
> > +	return (__force void __iomem *)phys_addr;
>
> Now that surely needs some explanation: I can't see how this can
> ever be correct, Xen or not being completely irrelevant.

I hope that efi_enabled(EFI_DIRECT) is obvious. However, in case of
!efi_enabled(EFI_DIRECT) some structures are created artificially
and they live in virtual address space. So that is why they should
not be mapped. If you wish I could add relevant comment here.

Daniel

Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag

[Jan Beulich]

>>> On 19.05.14 at 22:46, <daniel.kiper@oracle.com> wrote:
> On Mon, May 19, 2014 at 02:30:45PM +0100, Jan Beulich wrote:
>> >>> On 16.05.14 at 22:41, <daniel.kiper@oracle.com> wrote:
>> > @@ -457,6 +460,21 @@ void __init efi_free_boot_services(void)
>> >  	efi_unmap_memmap();
>> >  }
>> >
>> > +static void __init __iomem *efi_early_ioremap(resource_size_t phys_addr,
>> > +							unsigned long size)
>> > +{
>> > +	if (efi_enabled(EFI_DIRECT))
>> > +		return early_ioremap(phys_addr, size);
>> > +
>> > +	return (__force void __iomem *)phys_addr;
>>
>> Now that surely needs some explanation: I can't see how this can
>> ever be correct, Xen or not being completely irrelevant.
> 
> I hope that efi_enabled(EFI_DIRECT) is obvious. However, in case of
> !efi_enabled(EFI_DIRECT) some structures are created artificially
> and they live in virtual address space. So that is why they should
> not be mapped. If you wish I could add relevant comment here.

That would be the very minimum I suppose. But I wonder whether
you wouldn't be better off storing their physical addresses in the
first place (and then decide whether you can stay with early_ioremap()
or want/need to use early_memremap() if !EFI_DIRECT).

Jan

Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag

[David Vrabel]

On 16/05/14 21:41, Daniel Kiper wrote:
> Introduce EFI_DIRECT flag. If it is set this means that Linux
> Kernel has direct access to EFI infrastructure. If not then
> kernel runs on EFI platform but it has not direct control
> on EFI stuff. This functionality is used in Xen dom0.

This is backwards.  It should flag should indicate the weird platform
not the standard, default one.

You also need to explain why this is needed rather than presenting a
more complete EFI interface to PV guests.  And you should explain why
each use is necessary.

> +static void __init __iomem *efi_early_ioremap(resource_size_t phys_addr,
> +							unsigned long size)
> +{
> +	if (efi_enabled(EFI_DIRECT))
> +		return early_ioremap(phys_addr, size);
> +
> +	return (__force void __iomem *)phys_addr;
> +}

Er...  Perhaps you mean BUG_ON(!efi_enabled(EFI_DIRECT))? Or return NULL?

> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -893,12 +893,13 @@ extern int __init efi_setup_pcdp_console(char *);
>   * possible, remove EFI-related code altogether.
>   */
>  #define EFI_BOOT		0	/* Were we booted from EFI? */
> -#define EFI_SYSTEM_TABLES	1	/* Can we use EFI system tables? */
> -#define EFI_CONFIG_TABLES	2	/* Can we use EFI config tables? */
> -#define EFI_RUNTIME_SERVICES	3	/* Can we use runtime services? */
> -#define EFI_MEMMAP		4	/* Can we use EFI memory map? */
> -#define EFI_64BIT		5	/* Is the firmware 64-bit? */
> -#define EFI_ARCH_1		6	/* First arch-specific bit */
> +#define EFI_DIRECT		1	/* Can we access EFI directly? */
> +#define EFI_SYSTEM_TABLES	2	/* Can we use EFI system tables? */
> +#define EFI_CONFIG_TABLES	3	/* Can we use EFI config tables? */
> +#define EFI_RUNTIME_SERVICES	4	/* Can we use runtime services? */
> +#define EFI_MEMMAP		5	/* Can we use EFI memory map? */
> +#define EFI_64BIT		6	/* Is the firmware 64-bit? */
> +#define EFI_ARCH_1		7	/* First arch-specific bit */

Unnecessary shuffling of these values.  Why didn't you stick it after
EFI_64BIT?

David

Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag

[Daniel Kiper]

On Mon, May 19, 2014 at 04:58:32PM +0100, David Vrabel wrote:
> On 16/05/14 21:41, Daniel Kiper wrote:
> > Introduce EFI_DIRECT flag. If it is set this means that Linux
> > Kernel has direct access to EFI infrastructure. If not then
> > kernel runs on EFI platform but it has not direct control
> > on EFI stuff. This functionality is used in Xen dom0.
>
> This is backwards.  It should flag should indicate the weird platform
> not the standard, default one.

Do you wish EFI_NO_DIRECT? I understand your idea but I would like to avoid this
name because it is quite difficult to read e.g. !efi_enabled(EFI_NO_DIRECT).
You must think a bit to know what is going on. However, maybe you have a better idea?

> You also need to explain why this is needed rather than presenting a
> more complete EFI interface to PV guests.  And you should explain why
> each use is necessary.

OK.

> > +static void __init __iomem *efi_early_ioremap(resource_size_t phys_addr,
> > +							unsigned long size)
> > +{
> > +	if (efi_enabled(EFI_DIRECT))
> > +		return early_ioremap(phys_addr, size);
> > +
> > +	return (__force void __iomem *)phys_addr;
> > +}
>
> Er...  Perhaps you mean BUG_ON(!efi_enabled(EFI_DIRECT))? Or return NULL?

It is correct. As I said earlier: in case of !efi_enabled(EFI_DIRECT) some
structures are created artificially and they live in virtual address space.
So that is why they should not be mapped.

> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -893,12 +893,13 @@ extern int __init efi_setup_pcdp_console(char *);
> >   * possible, remove EFI-related code altogether.
> >   */
> >  #define EFI_BOOT		0	/* Were we booted from EFI? */
> > -#define EFI_SYSTEM_TABLES	1	/* Can we use EFI system tables? */
> > -#define EFI_CONFIG_TABLES	2	/* Can we use EFI config tables? */
> > -#define EFI_RUNTIME_SERVICES	3	/* Can we use runtime services? */
> > -#define EFI_MEMMAP		4	/* Can we use EFI memory map? */
> > -#define EFI_64BIT		5	/* Is the firmware 64-bit? */
> > -#define EFI_ARCH_1		6	/* First arch-specific bit */
> > +#define EFI_DIRECT		1	/* Can we access EFI directly? */
> > +#define EFI_SYSTEM_TABLES	2	/* Can we use EFI system tables? */
> > +#define EFI_CONFIG_TABLES	3	/* Can we use EFI config tables? */
> > +#define EFI_RUNTIME_SERVICES	4	/* Can we use runtime services? */
> > +#define EFI_MEMMAP		5	/* Can we use EFI memory map? */
> > +#define EFI_64BIT		6	/* Is the firmware 64-bit? */
> > +#define EFI_ARCH_1		7	/* First arch-specific bit */
>
> Unnecessary shuffling of these values.  Why didn't you stick it after
> EFI_64BIT?

I was going to have EFI_DIRECT close to EFI_BOOT which is quite generic
and platform independent name like EFI_BOOT. However, I do not insist
on having it in that place.

Daniel

Re: [PATCH v4 1/5] efi: Introduce EFI_DIRECT flag

[Matt Fleming]

On Mon, 19 May, at 11:02:55PM, Daniel Kiper wrote:
> 
> It is correct. As I said earlier: in case of !efi_enabled(EFI_DIRECT) some
> structures are created artificially and they live in virtual address space.
> So that is why they should not be mapped.
 
So, exploring Jan's idea, is it not possible to store the physical
address and have early_ioremap() just work? Even if they're mapping in
virtual address space they must have a corresponding physical address.

We really need to be keeping these kinds of special code paths to a
minimum. Unless absolutely necessary there should be just one way to do
things.

> I was going to have EFI_DIRECT close to EFI_BOOT which is quite generic
> and platform independent name like EFI_BOOT. However, I do not insist
> on having it in that place.

Right, please don't shuffle these bits.

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH v4 2/5] xen: Define EFI related stuff

[Daniel Kiper]

Define EFI related stuff for Xen.

This patch is based on Jan Beulich and Tang Liang work.

v4 - suggestions/fixes:
   - change some types from generic to Xen specific ones
     (suggested by Stefano Stabellini),
   - do some formating changes
     (suggested by Jan Beulich).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Tang Liang <liang.tang@oracle.com>
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 include/xen/interface/platform.h |  123 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)

diff --git a/include/xen/interface/platform.h b/include/xen/interface/platform.h
index f1331e3..55deeb7 100644
--- a/include/xen/interface/platform.h
+++ b/include/xen/interface/platform.h
@@ -108,11 +108,113 @@ struct xenpf_platform_quirk {
 };
 DEFINE_GUEST_HANDLE_STRUCT(xenpf_platform_quirk_t);
 
+#define XENPF_efi_runtime_call    49
+#define XEN_EFI_get_time                      1
+#define XEN_EFI_set_time                      2
+#define XEN_EFI_get_wakeup_time               3
+#define XEN_EFI_set_wakeup_time               4
+#define XEN_EFI_get_next_high_monotonic_count 5
+#define XEN_EFI_get_variable                  6
+#define XEN_EFI_set_variable                  7
+#define XEN_EFI_get_next_variable_name        8
+#define XEN_EFI_query_variable_info           9
+#define XEN_EFI_query_capsule_capabilities   10
+#define XEN_EFI_update_capsule               11
+
+struct xenpf_efi_runtime_call {
+	uint32_t function;
+	/*
+	 * This field is generally used for per sub-function flags (defined
+	 * below), except for the XEN_EFI_get_next_high_monotonic_count case,
+	 * where it holds the single returned value.
+	 */
+	uint32_t misc;
+	xen_ulong_t status;
+	union {
+#define XEN_EFI_GET_TIME_SET_CLEARS_NS 0x00000001
+	struct {
+		struct xenpf_efi_time {
+			uint16_t year;
+			uint8_t month;
+			uint8_t day;
+			uint8_t hour;
+			uint8_t min;
+			uint8_t sec;
+			uint32_t ns;
+			int16_t tz;
+			uint8_t daylight;
+		} time;
+		uint32_t resolution;
+		uint32_t accuracy;
+	} get_time;
+
+	struct xenpf_efi_time set_time;
+
+#define XEN_EFI_GET_WAKEUP_TIME_ENABLED 0x00000001
+#define XEN_EFI_GET_WAKEUP_TIME_PENDING 0x00000002
+	struct xenpf_efi_time get_wakeup_time;
+
+#define XEN_EFI_SET_WAKEUP_TIME_ENABLE      0x00000001
+#define XEN_EFI_SET_WAKEUP_TIME_ENABLE_ONLY 0x00000002
+	struct xenpf_efi_time set_wakeup_time;
+
+#define XEN_EFI_VARIABLE_NON_VOLATILE       0x00000001
+#define XEN_EFI_VARIABLE_BOOTSERVICE_ACCESS 0x00000002
+#define XEN_EFI_VARIABLE_RUNTIME_ACCESS     0x00000004
+	struct {
+		GUEST_HANDLE(void) name;  /* UCS-2/UTF-16 string */
+		xen_ulong_t size;
+		GUEST_HANDLE(void) data;
+		struct xenpf_efi_guid {
+			uint32_t data1;
+			uint16_t data2;
+			uint16_t data3;
+			uint8_t data4[8];
+			} vendor_guid;
+		} get_variable, set_variable;
+
+	struct {
+		xen_ulong_t size;
+		GUEST_HANDLE(void) name;  /* UCS-2/UTF-16 string */
+		struct xenpf_efi_guid vendor_guid;
+		} get_next_variable_name;
+
+	struct {
+		uint32_t attr;
+		uint64_t max_store_size;
+		uint64_t remain_store_size;
+		uint64_t max_size;
+		} query_variable_info;
+
+	struct {
+		GUEST_HANDLE(void) capsule_header_array;
+		xen_ulong_t capsule_count;
+		uint64_t max_capsule_size;
+		uint32_t reset_type;
+		} query_capsule_capabilities;
+
+	struct {
+		GUEST_HANDLE(void) capsule_header_array;
+		xen_ulong_t capsule_count;
+		uint64_t sg_list; /* machine address */
+		} update_capsule;
+	} u;
+};
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_efi_runtime_call);
+
+#define  XEN_FW_EFI_VERSION        0
+#define  XEN_FW_EFI_CONFIG_TABLE   1
+#define  XEN_FW_EFI_VENDOR         2
+#define  XEN_FW_EFI_MEM_INFO       3
+#define  XEN_FW_EFI_RT_VERSION     4
+
 #define XENPF_firmware_info       50
 #define XEN_FW_DISK_INFO          1 /* from int 13 AH=08/41/48 */
 #define XEN_FW_DISK_MBR_SIGNATURE 2 /* from MBR offset 0x1b8 */
 #define XEN_FW_VBEDDC_INFO        3 /* from int 10 AX=4f15 */
+#define XEN_FW_EFI_INFO           4 /* from EFI */
 #define XEN_FW_KBD_SHIFT_FLAGS    5 /* Int16, Fn02: Get keyboard shift flags. */
+
 struct xenpf_firmware_info {
 	/* IN variables. */
 	uint32_t type;
@@ -144,6 +246,26 @@ struct xenpf_firmware_info {
 			GUEST_HANDLE(uchar) edid;
 		} vbeddc_info; /* XEN_FW_VBEDDC_INFO */
 
+		union xenpf_efi_info {
+			uint32_t version;
+			struct {
+				uint64_t addr;   /* EFI_CONFIGURATION_TABLE */
+				uint32_t nent;
+			} cfg;
+			struct {
+				uint32_t revision;
+				uint32_t bufsz;  /* input, in bytes */
+				GUEST_HANDLE(void) name;
+				/* UCS-2/UTF-16 string */
+				} vendor;
+			struct {
+				uint64_t addr;
+				uint64_t size;
+				uint64_t attr;
+				uint32_t type;
+				} mem;
+		} efi_info; /* XEN_FW_EFI_INFO */
+
 		uint8_t kbd_shift_flags; /* XEN_FW_KBD_SHIFT_FLAGS */
 	} u;
 };
@@ -362,6 +484,7 @@ struct xen_platform_op {
 		struct xenpf_read_memtype      read_memtype;
 		struct xenpf_microcode_update  microcode;
 		struct xenpf_platform_quirk    platform_quirk;
+		struct xenpf_efi_runtime_call  efi_runtime_call;
 		struct xenpf_firmware_info     firmware_info;
 		struct xenpf_enter_acpi_sleep  enter_acpi_sleep;
 		struct xenpf_change_freq       change_freq;
-- 
1.7.10.4

Re: [PATCH v4 2/5] xen: Define EFI related stuff

[David Vrabel]

On 16/05/14 21:41, Daniel Kiper wrote:
> Define EFI related stuff for Xen.

Define "stuff"?  Please write a more descriptive commit message.

David

[PATCH v4 3/5] xen: Put EFI machinery in place

[Daniel Kiper]

Put EFI machinery for Xen in place.

This patch is based on Jan Beulich and Tang Liang work.

v4 - suggestions/fixes:
   - "just populate an efi_system_table_t object"
     (suggested by Matt Fleming).

Signed-off-by: Jan Beulich <jbeulich-IBi9RG/b67k@public.gmane.org>
Signed-off-by: Tang Liang <liang.tang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Daniel Kiper <daniel.kiper-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/xen/enlighten.c |   26 ++++
 drivers/xen/Kconfig      |    3 +
 drivers/xen/Makefile     |    1 +
 drivers/xen/efi.c        |  374 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 404 insertions(+)
 create mode 100644 drivers/xen/efi.c

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index c34bfc4..d5cc21f 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -32,6 +32,7 @@
 #include <linux/gfp.h>
 #include <linux/memblock.h>
 #include <linux/edd.h>
+#include <linux/efi.h>
 
 #include <xen/xen.h>
 #include <xen/events.h>
@@ -150,6 +151,15 @@ struct shared_info *HYPERVISOR_shared_info = &xen_dummy_shared_info;
  */
 static int have_vcpu_info_placement = 1;
 
+#ifdef CONFIG_XEN_EFI
+extern efi_system_table_t __init *xen_efi_probe(void);
+#else
+static efi_system_table_t __init *xen_efi_probe(void)
+{
+	return NULL;
+}
+#endif
+
 struct tls_descs {
 	struct desc_struct desc[3];
 };
@@ -1519,6 +1529,7 @@ asmlinkage __visible void __init xen_start_kernel(void)
 {
 	struct physdev_set_iopl set_iopl;
 	int rc;
+	efi_system_table_t *efi_systab_xen;
 
 	if (!xen_start_info)
 		return;
@@ -1714,6 +1725,21 @@ asmlinkage __visible void __init xen_start_kernel(void)
 
 	xen_setup_runstate_info(0);
 
+	efi_systab_xen = xen_efi_probe();
+
+	if (efi_systab_xen) {
+		strncpy((char *)&boot_params.efi_info.efi_loader_signature, "Xen",
+				sizeof(boot_params.efi_info.efi_loader_signature));
+		boot_params.efi_info.efi_systab = (__u32)((__u64)efi_systab_xen);
+		boot_params.efi_info.efi_systab_hi = (__u32)((__u64)efi_systab_xen >> 32);
+
+		x86_platform.get_wallclock = efi_get_time;
+		x86_platform.set_wallclock = efi_set_rtc_mmss;
+
+		set_bit(EFI_BOOT, &efi.flags);
+		set_bit(EFI_64BIT, &efi.flags);
+	}
+
 	/* Start the world */
 #ifdef CONFIG_X86_32
 	i386_start_kernel();
diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index 38fb36e..cead283 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -240,4 +240,7 @@ config XEN_MCE_LOG
 config XEN_HAVE_PVMMU
        bool
 
+config XEN_EFI
+	def_bool X86_64 && EFI
+
 endmenu
diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
index 45e00af..c35de02 100644
--- a/drivers/xen/Makefile
+++ b/drivers/xen/Makefile
@@ -33,6 +33,7 @@ obj-$(CONFIG_XEN_STUB)			+= xen-stub.o
 obj-$(CONFIG_XEN_ACPI_HOTPLUG_MEMORY)	+= xen-acpi-memhotplug.o
 obj-$(CONFIG_XEN_ACPI_HOTPLUG_CPU)	+= xen-acpi-cpuhotplug.o
 obj-$(CONFIG_XEN_ACPI_PROCESSOR)	+= xen-acpi-processor.o
+obj-$(CONFIG_XEN_EFI)			+= efi.o
 xen-evtchn-y				:= evtchn.o
 xen-gntdev-y				:= gntdev.o
 xen-gntalloc-y				:= gntalloc.o
diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
new file mode 100644
index 0000000..d81458a
--- /dev/null
+++ b/drivers/xen/efi.c
@@ -0,0 +1,374 @@
+/*
+ * EFI support for Xen.
+ *
+ * Copyright (C) 1999 VA Linux Systems
+ * Copyright (C) 1999 Walt Drummond <drummond-jCdQPDEk3idBDgjK7y7TUQ@public.gmane.org>
+ * Copyright (C) 1999-2002 Hewlett-Packard Co.
+ *	David Mosberger-Tang <davidm-sDzT885Ts8HQT0dZR+AlfA@public.gmane.org>
+ *	Stephane Eranian <eranian-sDzT885Ts8HQT0dZR+AlfA@public.gmane.org>
+ * Copyright (C) 2005-2008 Intel Co.
+ *	Fenghua Yu <fenghua.yu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
+ *	Bibo Mao <bibo.mao-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
+ *	Chandramouli Narayanan <mouli-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
+ *	Huang Ying <ying.huang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
+ * Copyright (C) 2011 Novell Co.
+ *	Jan Beulich <JBeulich-IBi9RG/b67k@public.gmane.org>
+ * Copyright (C) 2011-2012 Oracle Co.
+ *	Liang Tang <liang.tang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
+ * Copyright (c) 2014 Daniel Kiper, Oracle Corporation
+ */
+
+#include <linux/bug.h>
+#include <linux/efi.h>
+#include <linux/init.h>
+#include <linux/string.h>
+
+#include <xen/interface/xen.h>
+#include <xen/interface/platform.h>
+
+#include <asm/xen/hypercall.h>
+
+#define call (op.u.efi_runtime_call)
+#define DECLARE_CALL(what) \
+	struct xen_platform_op op; \
+	op.cmd = XENPF_efi_runtime_call; \
+	call.function = XEN_EFI_##what; \
+	call.misc = 0
+
+static efi_status_t xen_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
+{
+	int err;
+	DECLARE_CALL(get_time);
+
+	err = HYPERVISOR_dom0_op(&op);
+	if (err)
+		return EFI_UNSUPPORTED;
+
+	if (tm) {
+		BUILD_BUG_ON(sizeof(*tm) != sizeof(call.u.get_time.time));
+		memcpy(tm, &call.u.get_time.time, sizeof(*tm));
+	}
+
+	if (tc) {
+		tc->resolution = call.u.get_time.resolution;
+		tc->accuracy = call.u.get_time.accuracy;
+		tc->sets_to_zero = !!(call.misc &
+				      XEN_EFI_GET_TIME_SET_CLEARS_NS);
+	}
+
+	return call.status;
+}
+
+static efi_status_t xen_efi_set_time(efi_time_t *tm)
+{
+	DECLARE_CALL(set_time);
+
+	BUILD_BUG_ON(sizeof(*tm) != sizeof(call.u.set_time));
+	memcpy(&call.u.set_time, tm, sizeof(*tm));
+
+	return HYPERVISOR_dom0_op(&op) ? EFI_UNSUPPORTED : call.status;
+}
+
+static efi_status_t xen_efi_get_wakeup_time(efi_bool_t *enabled,
+					    efi_bool_t *pending,
+					    efi_time_t *tm)
+{
+	int err;
+	DECLARE_CALL(get_wakeup_time);
+
+	err = HYPERVISOR_dom0_op(&op);
+	if (err)
+		return EFI_UNSUPPORTED;
+
+	if (tm) {
+		BUILD_BUG_ON(sizeof(*tm) != sizeof(call.u.get_wakeup_time));
+		memcpy(tm, &call.u.get_wakeup_time, sizeof(*tm));
+	}
+
+	if (enabled)
+		*enabled = !!(call.misc & XEN_EFI_GET_WAKEUP_TIME_ENABLED);
+
+	if (pending)
+		*pending = !!(call.misc & XEN_EFI_GET_WAKEUP_TIME_PENDING);
+
+	return call.status;
+}
+
+static efi_status_t xen_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
+{
+	DECLARE_CALL(set_wakeup_time);
+
+	BUILD_BUG_ON(sizeof(*tm) != sizeof(call.u.set_wakeup_time));
+	if (enabled)
+		call.misc = XEN_EFI_SET_WAKEUP_TIME_ENABLE;
+	if (tm)
+		memcpy(&call.u.set_wakeup_time, tm, sizeof(*tm));
+	else
+		call.misc |= XEN_EFI_SET_WAKEUP_TIME_ENABLE_ONLY;
+
+	return HYPERVISOR_dom0_op(&op) ? EFI_UNSUPPORTED : call.status;
+}
+
+static efi_status_t xen_efi_get_variable(efi_char16_t *name,
+					 efi_guid_t *vendor,
+					 u32 *attr,
+					 unsigned long *data_size,
+					 void *data)
+{
+	int err;
+	DECLARE_CALL(get_variable);
+
+	set_xen_guest_handle(call.u.get_variable.name, name);
+	BUILD_BUG_ON(sizeof(*vendor) !=
+		     sizeof(call.u.get_variable.vendor_guid));
+	memcpy(&call.u.get_variable.vendor_guid, vendor, sizeof(*vendor));
+	call.u.get_variable.size = *data_size;
+	set_xen_guest_handle(call.u.get_variable.data, data);
+	err = HYPERVISOR_dom0_op(&op);
+	if (err)
+		return EFI_UNSUPPORTED;
+
+	*data_size = call.u.get_variable.size;
+	*attr = call.misc; /* misc in struction is U32 variable*/
+
+	return call.status;
+}
+
+static efi_status_t xen_efi_get_next_variable(unsigned long *name_size,
+					      efi_char16_t *name,
+					      efi_guid_t *vendor)
+{
+	int err;
+	DECLARE_CALL(get_next_variable_name);
+	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+		return EFI_UNSUPPORTED;
+	call.u.get_next_variable_name.size = *name_size;
+	set_xen_guest_handle(call.u.get_next_variable_name.name, name);
+	BUILD_BUG_ON(sizeof(*vendor) !=
+		     sizeof(call.u.get_next_variable_name.vendor_guid));
+	memcpy(&call.u.get_next_variable_name.vendor_guid, vendor,
+	       sizeof(*vendor));
+	err = HYPERVISOR_dom0_op(&op);
+	if (err)
+		return EFI_UNSUPPORTED;
+
+	*name_size = call.u.get_next_variable_name.size;
+	memcpy(vendor, &call.u.get_next_variable_name.vendor_guid,
+	       sizeof(*vendor));
+
+	return call.status;
+}
+
+static efi_status_t xen_efi_set_variable(efi_char16_t *name,
+					 efi_guid_t *vendor,
+					 u32 attr,
+					 unsigned long data_size,
+					 void *data)
+{
+	DECLARE_CALL(set_variable);
+
+	set_xen_guest_handle(call.u.set_variable.name, name);
+	call.misc = attr;
+	BUILD_BUG_ON(sizeof(*vendor) !=
+		     sizeof(call.u.set_variable.vendor_guid));
+	memcpy(&call.u.set_variable.vendor_guid, vendor, sizeof(*vendor));
+	call.u.set_variable.size = data_size;
+	set_xen_guest_handle(call.u.set_variable.data, data);
+
+	return HYPERVISOR_dom0_op(&op) ? EFI_UNSUPPORTED : call.status;
+}
+
+static efi_status_t xen_efi_query_variable_info(u32 attr,
+						u64 *storage_space,
+						u64 *remaining_space,
+						u64 *max_variable_size)
+{
+	int err;
+	DECLARE_CALL(query_variable_info);
+
+	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+		return EFI_UNSUPPORTED;
+
+	err = HYPERVISOR_dom0_op(&op);
+	if (err)
+		return EFI_UNSUPPORTED;
+
+	*storage_space = call.u.query_variable_info.max_store_size;
+	*remaining_space = call.u.query_variable_info.remain_store_size;
+	*max_variable_size = call.u.query_variable_info.max_size;
+
+	return call.status;
+}
+
+static efi_status_t xen_efi_get_next_high_mono_count(u32 *count)
+{
+	int err;
+	DECLARE_CALL(get_next_high_monotonic_count);
+
+	err = HYPERVISOR_dom0_op(&op);
+	if (err)
+		return EFI_UNSUPPORTED;
+
+	*count = call.misc;
+
+	return call.status;
+}
+
+static efi_status_t xen_efi_update_capsule(efi_capsule_header_t **capsules,
+					   unsigned long count,
+					   unsigned long sg_list)
+{
+	DECLARE_CALL(update_capsule);
+
+	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+		return EFI_UNSUPPORTED;
+
+	set_xen_guest_handle(call.u.update_capsule.capsule_header_array,
+			     capsules);
+	call.u.update_capsule.capsule_count = count;
+	call.u.update_capsule.sg_list = sg_list;
+
+	return HYPERVISOR_dom0_op(&op) ? EFI_UNSUPPORTED : call.status;
+}
+
+static efi_status_t xen_efi_query_capsule_caps(efi_capsule_header_t **capsules,
+					       unsigned long count,
+					       u64 *max_size,
+					       int *reset_type)
+{
+	int err;
+	DECLARE_CALL(query_capsule_capabilities);
+
+	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+		return EFI_UNSUPPORTED;
+
+	set_xen_guest_handle(call.u.query_capsule_capabilities.
+		capsule_header_array, capsules);
+	call.u.query_capsule_capabilities.capsule_count = count;
+
+	err = HYPERVISOR_dom0_op(&op);
+	if (err)
+		return EFI_UNSUPPORTED;
+
+	*max_size = call.u.query_capsule_capabilities.max_capsule_size;
+	*reset_type = call.u.query_capsule_capabilities.reset_type;
+
+	return call.status;
+}
+
+#undef DECLARE_CALL
+#undef call
+
+static efi_char16_t vendor[100] __initdata;
+static const efi_char16_t unknown[] __initconst =
+			{'U', 'N', 'K', 'N', 'O', 'W', 'N', '\0'};
+
+static efi_system_table_t efi_systab_xen __initdata = {
+	.hdr = {
+		.signature	= EFI_SYSTEM_TABLE_SIGNATURE,
+		.revision	= 0, /* Initialized later. */
+		.headersize	= 0, /* Ignored by Linux Kernel. */
+		.crc32		= 0, /* Ignored by Linux Kernel. */
+		.reserved	= 0
+	},
+	.fw_vendor	= (unsigned long)unknown, /* Initialized later. */
+	.fw_revision	= 0,			  /* Initialized later. */
+	.con_in_handle	= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
+	.con_in		= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
+	.con_out_handle	= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
+	.con_out	= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
+	.stderr_handle	= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
+	.stderr		= EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
+	.runtime	= NULL,			  /* Not used under Xen. */
+	.boottime	= NULL,			  /* Not used under Xen. */
+	.nr_tables	= 0,			  /* Initialized later. */
+	.tables		= EFI_INVALID_TABLE_ADDR  /* Initialized later. */
+};
+
+static const struct efi efi_xen __initconst = {
+	.systab                   = NULL, /* Initialized later. */
+	.runtime_version	  = 0,    /* Initialized later. */
+	.mps                      = EFI_INVALID_TABLE_ADDR,
+	.acpi                     = EFI_INVALID_TABLE_ADDR,
+	.acpi20                   = EFI_INVALID_TABLE_ADDR,
+	.smbios                   = EFI_INVALID_TABLE_ADDR,
+	.sal_systab               = EFI_INVALID_TABLE_ADDR,
+	.boot_info                = EFI_INVALID_TABLE_ADDR,
+	.hcdp                     = EFI_INVALID_TABLE_ADDR,
+	.uga                      = EFI_INVALID_TABLE_ADDR,
+	.uv_systab                = EFI_INVALID_TABLE_ADDR,
+	.fw_vendor                = EFI_INVALID_TABLE_ADDR,
+	.runtime                  = EFI_INVALID_TABLE_ADDR,
+	.config_table             = EFI_INVALID_TABLE_ADDR,
+	.get_time                 = xen_efi_get_time,
+	.set_time                 = xen_efi_set_time,
+	.get_wakeup_time          = xen_efi_get_wakeup_time,
+	.set_wakeup_time          = xen_efi_set_wakeup_time,
+	.get_variable             = xen_efi_get_variable,
+	.get_next_variable        = xen_efi_get_next_variable,
+	.set_variable             = xen_efi_set_variable,
+	.query_variable_info      = xen_efi_query_variable_info,
+	.update_capsule           = xen_efi_update_capsule,
+	.query_capsule_caps       = xen_efi_query_capsule_caps,
+	.get_next_high_mono_count = xen_efi_get_next_high_mono_count,
+	.reset_system             = NULL, /* Functionality provided by Xen. */
+	.set_virtual_address_map  = NULL, /* Not used under Xen. */
+	.memmap                   = NULL, /* Not used under Xen. */
+	.flags			  = 0     /* Initialized later. */
+};
+
+efi_system_table_t __init *xen_efi_probe(void)
+{
+	struct xen_platform_op op = {
+		.cmd = XENPF_firmware_info,
+		.u.firmware_info = {
+			.type = XEN_FW_EFI_INFO,
+			.index = XEN_FW_EFI_CONFIG_TABLE
+		}
+	};
+	union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
+
+	if (!xen_initial_domain() || HYPERVISOR_dom0_op(&op))
+		return NULL;
+
+	/* Here we know that Xen runs on EFI platform. */
+
+	efi = efi_xen;
+
+	op.cmd = XENPF_firmware_info;
+	op.u.firmware_info.type = XEN_FW_EFI_INFO;
+	op.u.firmware_info.index = XEN_FW_EFI_VENDOR;
+	info->vendor.bufsz = sizeof(vendor);
+	set_xen_guest_handle(info->vendor.name, vendor);
+
+	if (!HYPERVISOR_dom0_op(&op)) {
+		efi_systab_xen.fw_vendor = (unsigned long)vendor;
+		efi_systab_xen.fw_revision = info->vendor.revision;
+	}
+
+	op.cmd = XENPF_firmware_info;
+	op.u.firmware_info.type = XEN_FW_EFI_INFO;
+	op.u.firmware_info.index = XEN_FW_EFI_VERSION;
+
+	if (!HYPERVISOR_dom0_op(&op))
+		efi_systab_xen.hdr.revision = info->version;
+
+	op.cmd = XENPF_firmware_info;
+	op.u.firmware_info.type = XEN_FW_EFI_INFO;
+	op.u.firmware_info.index = XEN_FW_EFI_RT_VERSION;
+
+	if (!HYPERVISOR_dom0_op(&op))
+		efi.runtime_version = info->version;
+
+	op.cmd = XENPF_firmware_info;
+	op.u.firmware_info.type = XEN_FW_EFI_INFO;
+	op.u.firmware_info.index = XEN_FW_EFI_CONFIG_TABLE;
+
+	if (HYPERVISOR_dom0_op(&op))
+		BUG();
+
+	efi_systab_xen.tables = info->cfg.addr;
+	efi_systab_xen.nr_tables = info->cfg.nent;
+
+	return &efi_systab_xen;
+}
-- 
1.7.10.4

Re: [Xen-devel] [PATCH v4 3/5] xen: Put EFI machinery in place

[Andrew Cooper]

On 16/05/14 21:41, Daniel Kiper wrote:
> @@ -0,0 +1,374 @@
> +/*
> + * EFI support for Xen.
> + *
> + * Copyright (C) 1999 VA Linux Systems
> + * Copyright (C) 1999 Walt Drummond <drummond@valinux.com>
> + * Copyright (C) 1999-2002 Hewlett-Packard Co.
> + *	David Mosberger-Tang <davidm@hpl.hp.com>
> + *	Stephane Eranian <eranian@hpl.hp.com>
> + * Copyright (C) 2005-2008 Intel Co.
> + *	Fenghua Yu <fenghua.yu@intel.com>
> + *	Bibo Mao <bibo.mao@intel.com>
> + *	Chandramouli Narayanan <mouli@linux.intel.com>
> + *	Huang Ying <ying.huang@intel.com>
> + * Copyright (C) 2011 Novell Co.
> + *	Jan Beulich <JBeulich@suse.com>
> + * Copyright (C) 2011-2012 Oracle Co.
> + *	Liang Tang <liang.tang@oracle.com>
> + * Copyright (c) 2014 Daniel Kiper, Oracle Corporation
> + */
> +
> +#include <linux/bug.h>
> +#include <linux/efi.h>
> +#include <linux/init.h>
> +#include <linux/string.h>
> +
> +#include <xen/interface/xen.h>
> +#include <xen/interface/platform.h>
> +
> +#include <asm/xen/hypercall.h>
> +
> +#define call (op.u.efi_runtime_call)

I think not.

> +#define DECLARE_CALL(what) \
> +	struct xen_platform_op op; \
> +	op.cmd = XENPF_efi_runtime_call; \
> +	call.function = XEN_EFI_##what; \
> +	call.misc = 0

Macros like this which explicitly create local variable with implicit
names are evil when reading code.

If you want to do initialisation like this, then at least do something like:

#define INIT_EFI_CALL(what) \
{ .cmd = XENPF_efi_runtime_call, \
   .u.efi_runtime_call.function = XEN_EFI_##what, \
   .u.efi_runtime_call.misc = 0 }

And use it as:

struct xen_platform_op op = INIT_EFI_CALL(foo);

~Andrew

Re: [Xen-devel] [PATCH v4 3/5] xen: Put EFI machinery in place

[Jan Beulich]

>>> On 19.05.14 at 15:39, <andrew.cooper3-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org> wrote:
> On 16/05/14 21:41, Daniel Kiper wrote:
>> @@ -0,0 +1,374 @@
>> +/*
>> + * EFI support for Xen.
>> + *
>> + * Copyright (C) 1999 VA Linux Systems
>> + * Copyright (C) 1999 Walt Drummond <drummond-jCdQPDEk3idBDgjK7y7TUQ@public.gmane.org>
>> + * Copyright (C) 1999-2002 Hewlett-Packard Co.
>> + *	David Mosberger-Tang <davidm-sDzT885Ts8HQT0dZR+AlfA@public.gmane.org>
>> + *	Stephane Eranian <eranian-sDzT885Ts8HQT0dZR+AlfA@public.gmane.org>
>> + * Copyright (C) 2005-2008 Intel Co.
>> + *	Fenghua Yu <fenghua.yu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> + *	Bibo Mao <bibo.mao-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> + *	Chandramouli Narayanan <mouli-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>> + *	Huang Ying <ying.huang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> + * Copyright (C) 2011 Novell Co.
>> + *	Jan Beulich <JBeulich-IBi9RG/b67k@public.gmane.org>
>> + * Copyright (C) 2011-2012 Oracle Co.
>> + *	Liang Tang <liang.tang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>> + * Copyright (c) 2014 Daniel Kiper, Oracle Corporation
>> + */
>> +
>> +#include <linux/bug.h>
>> +#include <linux/efi.h>
>> +#include <linux/init.h>
>> +#include <linux/string.h>
>> +
>> +#include <xen/interface/xen.h>
>> +#include <xen/interface/platform.h>
>> +
>> +#include <asm/xen/hypercall.h>
>> +
>> +#define call (op.u.efi_runtime_call)
> 
> I think not.

??? (Read: What's wrong with putting this shortcut in place?)

>> +#define DECLARE_CALL(what) \
>> +	struct xen_platform_op op; \
>> +	op.cmd = XENPF_efi_runtime_call; \
>> +	call.function = XEN_EFI_##what; \
>> +	call.misc = 0
> 
> Macros like this which explicitly create local variable with implicit
> names are evil when reading code.
> 
> If you want to do initialisation like this, then at least do something like:
> 
> #define INIT_EFI_CALL(what) \
> { .cmd = XENPF_efi_runtime_call, \
>    .u.efi_runtime_call.function = XEN_EFI_##what, \
>    .u.efi_runtime_call.misc = 0 }
> 
> And use it as:
> 
> struct xen_platform_op op = INIT_EFI_CALL(foo);

That's minimal redundancy as a goal versus (to you, but not to me)
legibility.

Jan

Re: [PATCH v4 3/5] xen: Put EFI machinery in place

[Jan Beulich]

>>> On 16.05.14 at 22:41, <daniel.kiper@oracle.com> wrote:
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -240,4 +240,7 @@ config XEN_MCE_LOG
>  config XEN_HAVE_PVMMU
>         bool
>  
> +config XEN_EFI
> +	def_bool X86_64 && EFI

Constructs like this are bogus - they needlessly add a line to .config
when the expression evaluates to false.

config XEN_EFI
	def_bool y
	depends on X86_64 && EFI

is what avoids this.

> +static efi_status_t xen_efi_get_variable(efi_char16_t *name,
> +					 efi_guid_t *vendor,
> +					 u32 *attr,
> +					 unsigned long *data_size,
> +					 void *data)
> +{
> +	int err;
> +	DECLARE_CALL(get_variable);
> +
> +	set_xen_guest_handle(call.u.get_variable.name, name);
> +	BUILD_BUG_ON(sizeof(*vendor) !=
> +		     sizeof(call.u.get_variable.vendor_guid));
> +	memcpy(&call.u.get_variable.vendor_guid, vendor, sizeof(*vendor));
> +	call.u.get_variable.size = *data_size;
> +	set_xen_guest_handle(call.u.get_variable.data, data);
> +	err = HYPERVISOR_dom0_op(&op);
> +	if (err)
> +		return EFI_UNSUPPORTED;
> +
> +	*data_size = call.u.get_variable.size;
> +	*attr = call.misc; /* misc in struction is U32 variable*/

Iirc attr is an optional output, i.e. can be NULL, which hence needs
to be checked for here. I remember that this was broken in the
original EFI patch in our trees, so perhaps it would be good for you
to re-check against recent sources of ours for eventual other bug
fixes.

> +static efi_status_t xen_efi_query_variable_info(u32 attr,
> +						u64 *storage_space,
> +						u64 *remaining_space,
> +						u64 *max_variable_size)
> +{
> +	int err;
> +	DECLARE_CALL(query_variable_info);
> +
> +	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
> +		return EFI_UNSUPPORTED;
> +

Here's a similar case - there is

	call.u.query_variable_info.attr = attr;

missing.

> +static efi_char16_t vendor[100] __initdata;
> +static const efi_char16_t unknown[] __initconst =
> +			{'U', 'N', 'K', 'N', 'O', 'W', 'N', '\0'};

If you enforced -fshort-wchar for the file's compilation, this could be
written in a more legible manner (and probably you wouldn't need a
named variable at all).

Jan

Re: [PATCH v4 3/5] xen: Put EFI machinery in place

[David Vrabel]

On 16/05/14 21:41, Daniel Kiper wrote:
> Put EFI machinery for Xen in place.

Put what machinery to do what?

> @@ -1714,6 +1725,21 @@ asmlinkage __visible void __init xen_start_kernel(void)
>  
>  	xen_setup_runstate_info(0);
>  
> +	efi_systab_xen = xen_efi_probe();
> +
> +	if (efi_systab_xen) {
> +		strncpy((char *)&boot_params.efi_info.efi_loader_signature, "Xen",
> +				sizeof(boot_params.efi_info.efi_loader_signature));
> +		boot_params.efi_info.efi_systab = (__u32)((__u64)efi_systab_xen);
> +		boot_params.efi_info.efi_systab_hi = (__u32)((__u64)efi_systab_xen >> 32);
> +
> +		x86_platform.get_wallclock = efi_get_time;

x86_platform.get_wallclock should always be xen_get_wallclock().

> +		x86_platform.set_wallclock = efi_set_rtc_mmss;
> +
> +		set_bit(EFI_BOOT, &efi.flags);
> +		set_bit(EFI_64BIT, &efi.flags);
> +	}
> +
>  	/* Start the world */
>  #ifdef CONFIG_X86_32
>  	i386_start_kernel();
> --- /dev/null
> +++ b/drivers/xen/efi.c
> @@ -0,0 +1,374 @@
> + * Copyright (c) 2014 Daniel Kiper, Oracle Corporation

Is this really copyright by you personally and not Oracle?


> +#define call (op.u.efi_runtime_call)
> +#define DECLARE_CALL(what) \
> +	struct xen_platform_op op; \
> +	op.cmd = XENPF_efi_runtime_call; \
> +	call.function = XEN_EFI_##what; \
> +	call.misc = 0

Macros that declare local variables are awful.

Use what Andrew suggested and something like

struct xen_blah *call = &op.u.efi_runtime_call;


> +static const struct efi efi_xen __initconst = {
> +	.systab                   = NULL, /* Initialized later. */
> +	.runtime_version	  = 0,    /* Initialized later. */
> +	.mps                      = EFI_INVALID_TABLE_ADDR,
> +	.acpi                     = EFI_INVALID_TABLE_ADDR,
> +	.acpi20                   = EFI_INVALID_TABLE_ADDR,
> +	.smbios                   = EFI_INVALID_TABLE_ADDR,
> +	.sal_systab               = EFI_INVALID_TABLE_ADDR,
> +	.boot_info                = EFI_INVALID_TABLE_ADDR,
> +	.hcdp                     = EFI_INVALID_TABLE_ADDR,
> +	.uga                      = EFI_INVALID_TABLE_ADDR,
> +	.uv_systab                = EFI_INVALID_TABLE_ADDR,
> +	.fw_vendor                = EFI_INVALID_TABLE_ADDR,
> +	.runtime                  = EFI_INVALID_TABLE_ADDR,
> +	.config_table             = EFI_INVALID_TABLE_ADDR,
> +	.get_time                 = xen_efi_get_time,
> +	.set_time                 = xen_efi_set_time,
> +	.get_wakeup_time          = xen_efi_get_wakeup_time,
> +	.set_wakeup_time          = xen_efi_set_wakeup_time,
> +	.get_variable             = xen_efi_get_variable,
> +	.get_next_variable        = xen_efi_get_next_variable,
> +	.set_variable             = xen_efi_set_variable,
> +	.query_variable_info      = xen_efi_query_variable_info,
> +	.update_capsule           = xen_efi_update_capsule,
> +	.query_capsule_caps       = xen_efi_query_capsule_caps,
> +	.get_next_high_mono_count = xen_efi_get_next_high_mono_count,
> +	.reset_system             = NULL, /* Functionality provided by Xen. */

Xen provides functionality to reset (just maybe not via an EFI call).
Should an implementation be provided that does this?

> +	.set_virtual_address_map  = NULL, /* Not used under Xen. */
> +	.memmap                   = NULL, /* Not used under Xen. */
> +	.flags			  = 0     /* Initialized later. */
> +};
> +
> +efi_system_table_t __init *xen_efi_probe(void)
> +{
> +	struct xen_platform_op op = {
> +		.cmd = XENPF_firmware_info,
> +		.u.firmware_info = {
> +			.type = XEN_FW_EFI_INFO,
> +			.index = XEN_FW_EFI_CONFIG_TABLE
> +		}
> +	};
> +	union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> +
> +	if (!xen_initial_domain() || HYPERVISOR_dom0_op(&op))
> +		return NULL;

	if (!xen_initial_domain())
		return NULL;

	if (HYPERVISOR_dom0_op(&op) < 0)
		return NULL;

> +
> +	/* Here we know that Xen runs on EFI platform. */
> +
> +	efi = efi_xen;
> +
> +	op.cmd = XENPF_firmware_info;
> +	op.u.firmware_info.type = XEN_FW_EFI_INFO;
> +	op.u.firmware_info.index = XEN_FW_EFI_VENDOR;
> +	info->vendor.bufsz = sizeof(vendor);
> +	set_xen_guest_handle(info->vendor.name, vendor);
> +
> +	if (!HYPERVISOR_dom0_op(&op)) {

if (HYPERVISOR_dom0_op(&op) == 0)

David

Re: [PATCH v4 3/5] xen: Put EFI machinery in place

[Daniel Kiper]

On Tue, May 20, 2014 at 10:47:00AM +0100, David Vrabel wrote:
> On 16/05/14 21:41, Daniel Kiper wrote:
> > Put EFI machinery for Xen in place.
>
> Put what machinery to do what?
>
> > @@ -1714,6 +1725,21 @@ asmlinkage __visible void __init xen_start_kernel(void)
> >
> >  	xen_setup_runstate_info(0);
> >
> > +	efi_systab_xen = xen_efi_probe();
> > +
> > +	if (efi_systab_xen) {
> > +		strncpy((char *)&boot_params.efi_info.efi_loader_signature, "Xen",
> > +				sizeof(boot_params.efi_info.efi_loader_signature));
> > +		boot_params.efi_info.efi_systab = (__u32)((__u64)efi_systab_xen);
> > +		boot_params.efi_info.efi_systab_hi = (__u32)((__u64)efi_systab_xen >> 32);
> > +
> > +		x86_platform.get_wallclock = efi_get_time;
>
> x86_platform.get_wallclock should always be xen_get_wallclock().

Hmmm... Make sens... Jan, why did you replace x86_platform.get_wallclock
with efi_get_time() in your implementation?

> > +		x86_platform.set_wallclock = efi_set_rtc_mmss;

Now I am not sure even about that. As I can see Linux Kernel
running on bare metal EFI platform directly sets RTC omitting
efi_set_rtc_mmss(). Am I missing something? IIRC, there was
discussion about that once. But where and when?

Anyway, now I think that this initialization should be done
in arch/x86/xen/time.c if needed.

> > +
> > +		set_bit(EFI_BOOT, &efi.flags);
> > +		set_bit(EFI_64BIT, &efi.flags);
> > +	}
> > +
> >  	/* Start the world */
> >  #ifdef CONFIG_X86_32
> >  	i386_start_kernel();
> > --- /dev/null
> > +++ b/drivers/xen/efi.c
> > @@ -0,0 +1,374 @@
> > + * Copyright (c) 2014 Daniel Kiper, Oracle Corporation
>
> Is this really copyright by you personally and not Oracle?

As I know it is correct but I will double check it.

>
>
> > +#define call (op.u.efi_runtime_call)
> > +#define DECLARE_CALL(what) \
> > +	struct xen_platform_op op; \
> > +	op.cmd = XENPF_efi_runtime_call; \
> > +	call.function = XEN_EFI_##what; \
> > +	call.misc = 0
>
> Macros that declare local variables are awful.
>
> Use what Andrew suggested and something like
>
> struct xen_blah *call = &op.u.efi_runtime_call;
>
>
> > +static const struct efi efi_xen __initconst = {
> > +	.systab                   = NULL, /* Initialized later. */
> > +	.runtime_version	  = 0,    /* Initialized later. */
> > +	.mps                      = EFI_INVALID_TABLE_ADDR,
> > +	.acpi                     = EFI_INVALID_TABLE_ADDR,
> > +	.acpi20                   = EFI_INVALID_TABLE_ADDR,
> > +	.smbios                   = EFI_INVALID_TABLE_ADDR,
> > +	.sal_systab               = EFI_INVALID_TABLE_ADDR,
> > +	.boot_info                = EFI_INVALID_TABLE_ADDR,
> > +	.hcdp                     = EFI_INVALID_TABLE_ADDR,
> > +	.uga                      = EFI_INVALID_TABLE_ADDR,
> > +	.uv_systab                = EFI_INVALID_TABLE_ADDR,
> > +	.fw_vendor                = EFI_INVALID_TABLE_ADDR,
> > +	.runtime                  = EFI_INVALID_TABLE_ADDR,
> > +	.config_table             = EFI_INVALID_TABLE_ADDR,
> > +	.get_time                 = xen_efi_get_time,
> > +	.set_time                 = xen_efi_set_time,
> > +	.get_wakeup_time          = xen_efi_get_wakeup_time,
> > +	.set_wakeup_time          = xen_efi_set_wakeup_time,
> > +	.get_variable             = xen_efi_get_variable,
> > +	.get_next_variable        = xen_efi_get_next_variable,
> > +	.set_variable             = xen_efi_set_variable,
> > +	.query_variable_info      = xen_efi_query_variable_info,
> > +	.update_capsule           = xen_efi_update_capsule,
> > +	.query_capsule_caps       = xen_efi_query_capsule_caps,
> > +	.get_next_high_mono_count = xen_efi_get_next_high_mono_count,
> > +	.reset_system             = NULL, /* Functionality provided by Xen. */
>
> Xen provides functionality to reset (just maybe not via an EFI call).
> Should an implementation be provided that does this?

I do not think so. efi.reset_system() would not be called on Xen because
all reset related stuff is replaced by Xen reset specific functions.
Please check arch/x86/xen/enlighten.c. Additionally, I think that
situation is a bit similar to time issue. If we are running on Xen
then we should ask Xen to do reset because Xen controls platform
and it knows how to do that.

> > +	.set_virtual_address_map  = NULL, /* Not used under Xen. */
> > +	.memmap                   = NULL, /* Not used under Xen. */
> > +	.flags			  = 0     /* Initialized later. */
> > +};
> > +
> > +efi_system_table_t __init *xen_efi_probe(void)
> > +{
> > +	struct xen_platform_op op = {
> > +		.cmd = XENPF_firmware_info,
> > +		.u.firmware_info = {
> > +			.type = XEN_FW_EFI_INFO,
> > +			.index = XEN_FW_EFI_CONFIG_TABLE
> > +		}
> > +	};
> > +	union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> > +
> > +	if (!xen_initial_domain() || HYPERVISOR_dom0_op(&op))
> > +		return NULL;
>
> 	if (!xen_initial_domain())
> 		return NULL;
>
> 	if (HYPERVISOR_dom0_op(&op) < 0)
> 		return NULL;

What is wrong with my if?

> > +
> > +	/* Here we know that Xen runs on EFI platform. */
> > +
> > +	efi = efi_xen;
> > +
> > +	op.cmd = XENPF_firmware_info;
> > +	op.u.firmware_info.type = XEN_FW_EFI_INFO;
> > +	op.u.firmware_info.index = XEN_FW_EFI_VENDOR;
> > +	info->vendor.bufsz = sizeof(vendor);
> > +	set_xen_guest_handle(info->vendor.name, vendor);
> > +
> > +	if (!HYPERVISOR_dom0_op(&op)) {
>
> if (HYPERVISOR_dom0_op(&op) == 0)

Ditto?

Daniel

Re: [PATCH v4 3/5] xen: Put EFI machinery in place

[Jan Beulich]

>>> On 20.05.14 at 13:29, <daniel.kiper-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue, May 20, 2014 at 10:47:00AM +0100, David Vrabel wrote:
>> On 16/05/14 21:41, Daniel Kiper wrote:
>> > @@ -1714,6 +1725,21 @@ asmlinkage __visible void __init xen_start_kernel(void)
>> >
>> >  	xen_setup_runstate_info(0);
>> >
>> > +	efi_systab_xen = xen_efi_probe();
>> > +
>> > +	if (efi_systab_xen) {
>> > +		strncpy((char *)&boot_params.efi_info.efi_loader_signature, "Xen",
>> > +				sizeof(boot_params.efi_info.efi_loader_signature));
>> > +		boot_params.efi_info.efi_systab = (__u32)((__u64)efi_systab_xen);
>> > +		boot_params.efi_info.efi_systab_hi = (__u32)((__u64)efi_systab_xen >> 32);
>> > +
>> > +		x86_platform.get_wallclock = efi_get_time;
>>
>> x86_platform.get_wallclock should always be xen_get_wallclock().
> 
> Hmmm... Make sens... Jan, why did you replace x86_platform.get_wallclock
> with efi_get_time() in your implementation?

On the basis that (for Dom0 only) this is the equivalent of (and
actually also falls back to) mach_get_cmos_time(). If on Dom0
.get_wallclock doesn't get set to mach_get_cmos_time() on
pv-ops, then that line above should also be dropped.

Jan

Re: [PATCH v4 3/5] xen: Put EFI machinery in place

[David Vrabel]

On 20/05/14 12:29, Daniel Kiper wrote:
> 
>>> +	if (!xen_initial_domain() || HYPERVISOR_dom0_op(&op))
>>> +		return NULL;
>>
>> 	if (!xen_initial_domain())
>> 		return NULL;
>>
>> 	if (HYPERVISOR_dom0_op(&op) < 0)
>> 		return NULL;
> 
> What is wrong with my if?

Style.  The function returns -ve on error not a boolean success/fail.

>>> +
>>> +	/* Here we know that Xen runs on EFI platform. */
>>> +
>>> +	efi = efi_xen;
>>> +
>>> +	op.cmd = XENPF_firmware_info;
>>> +	op.u.firmware_info.type = XEN_FW_EFI_INFO;
>>> +	op.u.firmware_info.index = XEN_FW_EFI_VENDOR;
>>> +	info->vendor.bufsz = sizeof(vendor);
>>> +	set_xen_guest_handle(info->vendor.name, vendor);
>>> +
>>> +	if (!HYPERVISOR_dom0_op(&op)) {
>>
>> if (HYPERVISOR_dom0_op(&op) == 0)
> 
> Ditto?

Again, style.

David

Re: [PATCH v4 0/5] xen: Add EFI support

[David Vrabel]

On 16/05/14 21:41, Daniel Kiper wrote:
> Hey,
> 
> This patch series adds EFI support for Xen dom0 guests.
> It is based on Jan Beulich and Tang Liang work. I was
> trying to take into account all previous comments,
> however, if I missed something sorry for that.

There needs to be a better description of this series.  In particular, a
description of why Xen PV guests are different and the overall design used.

> I am still not sure what to do with /sys/firmware/efi/config_table,
> /sys/firmware/efi/{fw_vendor,runtime,systab} files. On bare metal
> they contain physical addresses of relevant structures. However,
> in Xen case they does not make sens. So maybe they should contain
> invalid values (e.g. 0) or should not appear at all on Xen (I prefer
> last one). What do you think about that?

Generally, dom0 has provided access to BIOS provided data structures.  I
think you should do the same here.  They may be useful for diagnostics
if nothing else.

David

[PATCH v4 4/5] arch/x86: Replace plain strings with constants

[Daniel Kiper]

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 arch/x86/kernel/setup.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index f41f648..7a67f5d 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -924,11 +924,11 @@ void __init setup_arch(char **cmdline_p)
 #endif
 #ifdef CONFIG_EFI
 	if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature,
-		     "EL32", 4)) {
+				EFI32_LOADER_SIGNATURE, 4)) {
 		set_bit(EFI_BOOT, &efi.flags);
 		set_bit(EFI_DIRECT, &efi.flags);
 	} else if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature,
-		     "EL64", 4)) {
+				EFI64_LOADER_SIGNATURE, 4)) {
 		set_bit(EFI_BOOT, &efi.flags);
 		set_bit(EFI_DIRECT, &efi.flags);
 		set_bit(EFI_64BIT, &efi.flags);
-- 
1.7.10.4

Re: [PATCH v4 4/5] arch/x86: Replace plain strings with constants

[Matt Fleming]

On Fri, 16 May, at 10:41:43PM, Daniel Kiper wrote:
> Signed-off-by: Daniel Kiper <daniel.kiper-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/x86/kernel/setup.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index f41f648..7a67f5d 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -924,11 +924,11 @@ void __init setup_arch(char **cmdline_p)
>  #endif
>  #ifdef CONFIG_EFI
>  	if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature,
> -		     "EL32", 4)) {
> +				EFI32_LOADER_SIGNATURE, 4)) {
>  		set_bit(EFI_BOOT, &efi.flags);
>  		set_bit(EFI_DIRECT, &efi.flags);
>  	} else if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature,
> -		     "EL64", 4)) {
> +				EFI64_LOADER_SIGNATURE, 4)) {
>  		set_bit(EFI_BOOT, &efi.flags);
>  		set_bit(EFI_DIRECT, &efi.flags);
>  		set_bit(EFI_64BIT, &efi.flags);

Conceptually this change is a nice cleanup, but you've also changed the
indentation. Please don't do that, stick with the existing format.

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH v4 5/5] arch/x86: Remove redundant set_bit() call

[Daniel Kiper]

Remove redundant set_bit(EFI_SYSTEM_TABLES, &efi.flags) call.
It is executed earlier in efi_systab_init().

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 arch/x86/platform/efi/efi.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 7fcef06..e7008f0 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -754,8 +754,6 @@ void __init efi_init(void)
 	if (efi_systab_init(efi_phys.systab))
 		return;
 
-	set_bit(EFI_SYSTEM_TABLES, &efi.flags);
-
 	efi.config_table = (unsigned long)efi.systab->tables;
 	efi.fw_vendor	 = (unsigned long)efi.systab->fw_vendor;
 	efi.runtime	 = (unsigned long)efi.systab->runtime;
-- 
1.7.10.4

Re: [PATCH v4 5/5] arch/x86: Remove redundant set_bit() call

[Matt Fleming]

On Fri, 16 May, at 10:41:44PM, Daniel Kiper wrote:
> Remove redundant set_bit(EFI_SYSTEM_TABLES, &efi.flags) call.
> It is executed earlier in efi_systab_init().
> 
> Signed-off-by: Daniel Kiper <daniel.kiper-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/x86/platform/efi/efi.c |    2 --
>  1 file changed, 2 deletions(-)

Good catch.

-- 
Matt Fleming, Intel Open Source Technology Center