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

[PATCH v3 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.

Additionally, I attempted to leave all credits as is and
put SOB in relevant places. If I made any mistake drop me
a line and I will fix it in next version.

Feel free to play with it. I am looking forward for your comments.

Daniel

 arch/ia64/kernel/efi.c           |   30 ++++--
 arch/x86/kernel/setup.c          |    2 +-
 arch/x86/platform/efi/efi.c      |   20 ++--
 arch/x86/xen/enlighten.c         |   10 ++
 drivers/firmware/efi/efi.c       |   26 ++++++
 drivers/xen/Kconfig              |    3 +
 drivers/xen/Makefile             |    1 +
 drivers/xen/efi.c                |  425 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/efi.h              |   10 ++
 include/xen/interface/platform.h |  123 +++++++++++++++++++++++-
 10 files changed, 633 insertions(+), 17 deletions(-)

Daniel Kiper (5):
      efi: Add efi_init_ops variable
      efi: Export arch_tables variable
      x86: Call efi_memblock_x86_reserve_range() on native EFI platform only
      xen: Define EFI related stuff
      xen: Put EFI machinery in place

[PATCH v3 1/5] efi: Add efi_init_ops variable

[Daniel Kiper]

Add efi_init_ops variable which allows us to replace
EFI init functions on Xen with its specific stuff.

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

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Signed-off-by: Tang Liang <liang.tang@oracle.com>
---
 arch/ia64/kernel/efi.c      |   30 +++++++++++++++++++++---------
 arch/x86/platform/efi/efi.c |   18 +++++++++++++-----
 drivers/firmware/efi/efi.c  |   26 ++++++++++++++++++++++++++
 include/linux/efi.h         |    8 ++++++++
 4 files changed, 68 insertions(+), 14 deletions(-)

diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
index da5b462..f646374 100644
--- a/arch/ia64/kernel/efi.c
+++ b/arch/ia64/kernel/efi.c
@@ -468,8 +468,8 @@ efi_map_pal_code (void)
 	ia64_set_psr(psr);		/* restore psr */
 }
 
-void __init
-efi_init (void)
+static void __init
+efi_init_generic (void)
 {
 	void *efi_map_start, *efi_map_end;
 	efi_char16_t *c16;
@@ -593,8 +593,13 @@ efi_init (void)
 	efi_enter_virtual_mode();
 }
 
-void
-efi_enter_virtual_mode (void)
+static void
+efi_reserve_boot_services_generic (void)
+{
+}
+
+static void
+efi_enter_virtual_mode_generic (void)
 {
 	void *efi_map_start, *efi_map_end, *p;
 	efi_memory_desc_t *md;
@@ -751,8 +756,8 @@ efi_memmap_intersects (unsigned long phys_addr, unsigned long size)
 	return 0;
 }
 
-u32
-efi_mem_type (unsigned long phys_addr)
+static u32
+efi_mem_type_generic (unsigned long phys_addr)
 {
 	efi_memory_desc_t *md = efi_memory_descriptor(phys_addr);
 
@@ -761,8 +766,8 @@ efi_mem_type (unsigned long phys_addr)
 	return 0;
 }
 
-u64
-efi_mem_attributes (unsigned long phys_addr)
+static u64
+efi_mem_attributes_generic (unsigned long phys_addr)
 {
 	efi_memory_desc_t *md = efi_memory_descriptor(phys_addr);
 
@@ -770,7 +775,14 @@ efi_mem_attributes (unsigned long phys_addr)
 		return md->attribute;
 	return 0;
 }
-EXPORT_SYMBOL(efi_mem_attributes);
+
+struct efi_init_ops efi_init_ops = {
+	.efi_init = efi_init_generic,
+	.efi_reserve_boot_services = efi_reserve_boot_services_generic,
+	.efi_enter_virtual_mode = efi_enter_virtual_mode_generic,
+	.efi_mem_type = efi_mem_type_generic,
+	.efi_mem_attributes = efi_mem_attributes_generic
+};
 
 u64
 efi_mem_attribute (unsigned long phys_addr, unsigned long size)
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index b97acec..e4af217 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -422,7 +422,7 @@ static void __init print_efi_memmap(void)
 #endif  /*  EFI_DEBUG  */
 }
 
-void __init efi_reserve_boot_services(void)
+static void __init efi_reserve_boot_services_generic(void)
 {
 	void *p;
 
@@ -700,7 +700,7 @@ out:
 	return ret;
 }
 
-void __init efi_init(void)
+static void __init efi_init_generic(void)
 {
 	efi_char16_t *c16;
 	char vendor[100] = "unknown";
@@ -998,7 +998,7 @@ out:
  * be passed in via setup_data. In that case runtime ranges will be mapped
  * to the same virtual addresses as the first kernel.
  */
-void __init efi_enter_virtual_mode(void)
+static void __init efi_enter_virtual_mode_generic(void)
 {
 	efi_status_t status;
 	void *new_memmap = NULL;
@@ -1085,7 +1085,7 @@ void __init efi_enter_virtual_mode(void)
 /*
  * Convenience functions to obtain memory types and attributes
  */
-u32 efi_mem_type(unsigned long phys_addr)
+static u32 efi_mem_type_generic(unsigned long phys_addr)
 {
 	efi_memory_desc_t *md;
 	void *p;
@@ -1103,7 +1103,7 @@ u32 efi_mem_type(unsigned long phys_addr)
 	return 0;
 }
 
-u64 efi_mem_attributes(unsigned long phys_addr)
+static u64 efi_mem_attributes_generic(unsigned long phys_addr)
 {
 	efi_memory_desc_t *md;
 	void *p;
@@ -1118,6 +1118,14 @@ u64 efi_mem_attributes(unsigned long phys_addr)
 	return 0;
 }
 
+struct efi_init_ops efi_init_ops = {
+	.efi_init = efi_init_generic,
+	.efi_reserve_boot_services = efi_reserve_boot_services_generic,
+	.efi_enter_virtual_mode = efi_enter_virtual_mode_generic,
+	.efi_mem_type = efi_mem_type_generic,
+	.efi_mem_attributes = efi_mem_attributes_generic
+};
+
 /*
  * Some firmware has serious problems when using more than 50% of the EFI
  * variable store, i.e. it triggers bugs that can brick machines. Ensure that
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 4753bac..5ce75cf 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -137,6 +137,32 @@ static void generic_ops_unregister(void)
 	efivars_unregister(&generic_efivars);
 }
 
+void __init efi_init(void)
+{
+	efi_init_ops.efi_init();
+}
+
+void __init efi_reserve_boot_services(void)
+{
+	efi_init_ops.efi_reserve_boot_services();
+}
+
+void __init efi_enter_virtual_mode(void)
+{
+	efi_init_ops.efi_enter_virtual_mode();
+}
+
+u32 efi_mem_type(unsigned long phys_addr)
+{
+	return efi_init_ops.efi_mem_type(phys_addr);
+}
+
+u64 efi_mem_attributes(unsigned long phys_addr)
+{
+	return efi_init_ops.efi_mem_attributes(phys_addr);
+}
+EXPORT_SYMBOL(efi_mem_attributes);
+
 /*
  * We register the efi subsystem with the firmware subsystem and the
  * efivars subsystem with the efi subsystem, if the system was booted with
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 0a819e7..67ba1a0 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -541,6 +541,14 @@ typedef struct _efi_file_io_interface {
 
 #define EFI_INVALID_TABLE_ADDR		(~0UL)
 
+extern struct efi_init_ops {
+	void (*efi_init)(void);
+	void (*efi_reserve_boot_services)(void);
+	void (*efi_enter_virtual_mode)(void);
+	u32 (*efi_mem_type)(unsigned long phys_addr);
+	u64 (*efi_mem_attributes)(unsigned long phys_addr);
+} efi_init_ops;
+
 /*
  * All runtime access to EFI goes through this structure:
  */
-- 
1.7.10.4

Re: [PATCH v3 1/5] efi: Add efi_init_ops variable

[Matt Fleming]

On Tue, 25 Mar, at 09:57:52PM, Daniel Kiper wrote:
> Add efi_init_ops variable which allows us to replace
> EFI init functions on Xen with its specific stuff.
> 
> This patch is based on Jan Beulich and Tang Liang work.
> 
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> Signed-off-by: Tang Liang <liang.tang@oracle.com>
> ---
>  arch/ia64/kernel/efi.c      |   30 +++++++++++++++++++++---------
>  arch/x86/platform/efi/efi.c |   18 +++++++++++++-----
>  drivers/firmware/efi/efi.c  |   26 ++++++++++++++++++++++++++
>  include/linux/efi.h         |    8 ++++++++
>  4 files changed, 68 insertions(+), 14 deletions(-)

[...]

> @@ -1118,6 +1118,14 @@ u64 efi_mem_attributes(unsigned long phys_addr)
>  	return 0;
>  }
>  
> +struct efi_init_ops efi_init_ops = {
> +	.efi_init = efi_init_generic,
> +	.efi_reserve_boot_services = efi_reserve_boot_services_generic,
> +	.efi_enter_virtual_mode = efi_enter_virtual_mode_generic,
> +	.efi_mem_type = efi_mem_type_generic,
> +	.efi_mem_attributes = efi_mem_attributes_generic
> +};

Please don't create another struct of EFI function pointers.

After this we'll have 3 global efi structures defintions and 4 global
efi objects on x86,

  - struct efi_early [in the boot stub]
  - struct efi ['efi_phys' before/'efi' after SetVirtualAddressMap()]
  - struct efi_init_ops [for the benefit of xen]

I have a big problem with exposing .efi_reserve_boot_services as an API.

Also, the fact that in later patches you're copying chunks of the x86
virt_*() funtions suggests to me that this has been implemented at the
wrong layer, because it actively discourages sharing code between x86
and Xen.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v3 1/5] efi: Add efi_init_ops variable

[Daniel Kiper]

On Wed, Mar 26, 2014 at 12:56:23PM +0000, Matt Fleming wrote:
> On Tue, 25 Mar, at 09:57:52PM, Daniel Kiper wrote:
> > Add efi_init_ops variable which allows us to replace
> > EFI init functions on Xen with its specific stuff.
> >
> > This patch is based on Jan Beulich and Tang Liang work.
> >
> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> > Signed-off-by: Tang Liang <liang.tang@oracle.com>
> > ---
> >  arch/ia64/kernel/efi.c      |   30 +++++++++++++++++++++---------
> >  arch/x86/platform/efi/efi.c |   18 +++++++++++++-----
> >  drivers/firmware/efi/efi.c  |   26 ++++++++++++++++++++++++++
> >  include/linux/efi.h         |    8 ++++++++
> >  4 files changed, 68 insertions(+), 14 deletions(-)
>
> [...]
>
> > @@ -1118,6 +1118,14 @@ u64 efi_mem_attributes(unsigned long phys_addr)
> >  	return 0;
> >  }
> >
> > +struct efi_init_ops efi_init_ops = {
> > +	.efi_init = efi_init_generic,
> > +	.efi_reserve_boot_services = efi_reserve_boot_services_generic,
> > +	.efi_enter_virtual_mode = efi_enter_virtual_mode_generic,
> > +	.efi_mem_type = efi_mem_type_generic,
> > +	.efi_mem_attributes = efi_mem_attributes_generic
> > +};
>
> Please don't create another struct of EFI function pointers.
>
> After this we'll have 3 global efi structures defintions and 4 global
> efi objects on x86,
>
>   - struct efi_early [in the boot stub]
>   - struct efi ['efi_phys' before/'efi' after SetVirtualAddressMap()]
>   - struct efi_init_ops [for the benefit of xen]

What do you suggest? Should we fill all EFI related structures
in xen_efi_probe() (called from xen_start_kernel()) and later
they should be parsed by generic/x86 EFI initialization code?
I suppose that many variables will have NULL (or something relevant)
in Xen dom0 and it will require some changes in EFI initialization code.

> I have a big problem with exposing .efi_reserve_boot_services as an API.

Why? efi_reserve_boot_services() is public right now.

Daniel

Re: [PATCH v3 1/5] efi: Add efi_init_ops variable

[Matt Fleming]

On Wed, 26 Mar, at 03:02:17PM, Daniel Kiper wrote:
> On Wed, Mar 26, 2014 at 12:56:23PM +0000, Matt Fleming wrote:
> >
> > Please don't create another struct of EFI function pointers.
> >
> > After this we'll have 3 global efi structures defintions and 4 global
> > efi objects on x86,
> >
> >   - struct efi_early [in the boot stub]
> >   - struct efi ['efi_phys' before/'efi' after SetVirtualAddressMap()]
> >   - struct efi_init_ops [for the benefit of xen]
> 
> What do you suggest? Should we fill all EFI related structures
> in xen_efi_probe() (called from xen_start_kernel()) and later
> they should be parsed by generic/x86 EFI initialization code?
> I suppose that many variables will have NULL (or something relevant)
> in Xen dom0 and it will require some changes in EFI initialization code.
 
Yeah, that is what I was thinking. And if we have to make some changes
to the generic code to accomodate NULL fields, etc, I don't think it's a
big deal.

> > I have a big problem with exposing .efi_reserve_boot_services as an API.
> 
> Why? efi_reserve_boot_services() is public right now.

Sure it's public, but it isn't implemented on ia64 at all, not even as a
stub function, and it's only called inside x86 code (the fact that the
prototype is in include/linux/efi.h is a bit of a bug).

My concern is that if in we bake into our APIs it's going to be
difficult in the future to get rid of it or disable it, or at least
allow people to write code for EFI without caring about it.

Afterall, it's a workaround for buggy firmware.

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH v3 2/5] efi: Export arch_tables variable

[Daniel Kiper]

Export arch_tables variable. Xen init function calls efi_config_init()
which takes it as an argument.

Additionally, put __initdata in place suggested by include/linux/init.h.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 arch/x86/platform/efi/efi.c |    2 +-
 include/linux/efi.h         |    2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index e4af217..26651c6 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -70,7 +70,7 @@ static efi_system_table_t efi_systab __initdata;
 
 unsigned long x86_efi_facility;
 
-static __initdata efi_config_table_type_t arch_tables[] = {
+efi_config_table_type_t arch_tables[] __initdata = {
 #ifdef CONFIG_X86_UV
 	{UV_SYSTEM_TABLE_GUID, "UVsystab", &efi.uv_systab},
 #endif
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 67ba1a0..f00ef14 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -583,6 +583,8 @@ extern struct efi {
 	struct efi_memory_map *memmap;
 } efi;
 
+extern efi_config_table_type_t arch_tables[] __initdata;
+
 static inline int
 efi_guidcmp (efi_guid_t left, efi_guid_t right)
 {
-- 
1.7.10.4

Re: [PATCH v3 2/5] efi: Export arch_tables variable

[Jan Beulich]

>>> On 25.03.14 at 21:57, <daniel.kiper@oracle.com> wrote:
> Export arch_tables variable. Xen init function calls efi_config_init()
> which takes it as an argument.
> 
> Additionally, put __initdata in place suggested by include/linux/init.h.

Which isn't necessarily the most appropriate place.

> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -70,7 +70,7 @@ static efi_system_table_t efi_systab __initdata;
>  
>  unsigned long x86_efi_facility;
>  
> -static __initdata efi_config_table_type_t arch_tables[] = {
> +efi_config_table_type_t arch_tables[] __initdata = {

efi_config_table_type_t __initdata arch_tables[] = {

would be what I'd recommend.

> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -583,6 +583,8 @@ extern struct efi {
>  	struct efi_memory_map *memmap;
>  } efi;
>  
> +extern efi_config_table_type_t arch_tables[] __initdata;

And section placement annotations are bogus on declarations.

Jan

Re: [PATCH v3 2/5] efi: Export arch_tables variable

[Daniel Kiper]

On Wed, Mar 26, 2014 at 01:21:19PM +0000, Jan Beulich wrote:
> >>> On 25.03.14 at 21:57, <daniel.kiper@oracle.com> wrote:
> > Export arch_tables variable. Xen init function calls efi_config_init()
> > which takes it as an argument.
> >
> > Additionally, put __initdata in place suggested by include/linux/init.h.
>
> Which isn't necessarily the most appropriate place.

Why? If comments in include/linux/init.h are not valid they should be changed.

> > --- a/arch/x86/platform/efi/efi.c
> > +++ b/arch/x86/platform/efi/efi.c
> > @@ -70,7 +70,7 @@ static efi_system_table_t efi_systab __initdata;
> >
> >  unsigned long x86_efi_facility;
> >
> > -static __initdata efi_config_table_type_t arch_tables[] = {
> > +efi_config_table_type_t arch_tables[] __initdata = {
>
> efi_config_table_type_t __initdata arch_tables[] = {
>
> would be what I'd recommend.
>
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -583,6 +583,8 @@ extern struct efi {
> >  	struct efi_memory_map *memmap;
> >  } efi;
> >
> > +extern efi_config_table_type_t arch_tables[] __initdata;
>
> And section placement annotations are bogus on declarations.

Hmmm... I am not sure which approach is better. I saw that
in many places declarations have annotations. Could you
point me some docs which states (and explains) that this
is wrong idea.

Daniel

Re: [PATCH v3 2/5] efi: Export arch_tables variable

[Jan Beulich]

>>> On 26.03.14 at 15:08, <daniel.kiper@oracle.com> wrote:
> On Wed, Mar 26, 2014 at 01:21:19PM +0000, Jan Beulich wrote:
>> >>> On 25.03.14 at 21:57, <daniel.kiper@oracle.com> wrote:
>> > Export arch_tables variable. Xen init function calls efi_config_init()
>> > which takes it as an argument.
>> >
>> > Additionally, put __initdata in place suggested by include/linux/init.h.
>>
>> Which isn't necessarily the most appropriate place.
> 
> Why? If comments in include/linux/init.h are not valid they should be 
> changed.

Because they can't go there uniformly: While on function declarations
you can put them there, on function definitions they need to come
before the function name. And placing attributes between type and
name does - iirc - work consistently for everything.

>> > --- a/include/linux/efi.h
>> > +++ b/include/linux/efi.h
>> > @@ -583,6 +583,8 @@ extern struct efi {
>> >  	struct efi_memory_map *memmap;
>> >  } efi;
>> >
>> > +extern efi_config_table_type_t arch_tables[] __initdata;
>>
>> And section placement annotations are bogus on declarations.
> 
> Hmmm... I am not sure which approach is better. I saw that
> in many places declarations have annotations. Could you
> point me some docs which states (and explains) that this
> is wrong idea.

Just use common sense: Attributes that are of concern to the
caller should go on the declaration. Attributes that only affect
code generation for the function/object in question should go on
the definition.

Jan

Re: [PATCH v3 2/5] efi: Export arch_tables variable

[Matt Fleming]

On Wed, 26 Mar, at 03:08:00PM, Daniel Kiper wrote:
> 
> Hmmm... I am not sure which approach is better. I saw that
> in many places declarations have annotations. Could you
> point me some docs which states (and explains) that this
> is wrong idea.

Note I've had the following patch from Joe queued up for weeks. It's in
the tip tree and will be sent to Linus during the merge window.

----

commit 9b7d204982cf
Author: Joe Perches <joe@perches.com>
Date:   Fri Jan 3 16:08:48 2014 -0800

    x86/efi: Style neatening
    
    Coalesce formats and remove spaces before tabs.
    Move __initdata after the variable declaration.
    
    Signed-off-by: Joe Perches <joe@perches.com>
    Signed-off-by: Matt Fleming <matt.fleming@intel.com>

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index dc1dd6d09dec..876dc5b9e4a3 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -67,7 +67,7 @@ struct efi_memory_map memmap;
 static struct efi efi_phys __initdata;
 static efi_system_table_t efi_systab __initdata;
 
-static __initdata efi_config_table_type_t arch_tables[] = {
+static efi_config_table_type_t arch_tables[] __initdata = {
 #ifdef CONFIG_X86_UV
 	{UV_SYSTEM_TABLE_GUID, "UVsystab", &efi.uv_systab},
 #endif
@@ -76,7 +76,7 @@ static __initdata efi_config_table_type_t arch_tables[] = {
 
 u64 efi_setup;		/* efi setup_data physical address */
 
-static bool __initdata disable_runtime = false;
+static bool disable_runtime __initdata = false;
 static int __init setup_noefi(char *arg)
 {
 	disable_runtime = true;
@@ -263,9 +263,9 @@ static efi_status_t __init phys_efi_get_time(efi_time_t *tm,
 int efi_set_rtc_mmss(const struct timespec *now)
 {
 	unsigned long nowtime = now->tv_sec;
-	efi_status_t 	status;
-	efi_time_t 	eft;
-	efi_time_cap_t 	cap;
+	efi_status_t	status;
+	efi_time_t	eft;
+	efi_time_cap_t	cap;
 	struct rtc_time	tm;
 
 	status = efi.get_time(&eft, &cap);
@@ -283,9 +283,8 @@ int efi_set_rtc_mmss(const struct timespec *now)
 		eft.second = tm.tm_sec;
 		eft.nanosecond = 0;
 	} else {
-		printk(KERN_ERR
-		       "%s: Invalid EFI RTC value: write of %lx to EFI RTC failed\n",
-		       __FUNCTION__, nowtime);
+		pr_err("%s: Invalid EFI RTC value: write of %lx to EFI RTC failed\n",
+		       __func__, nowtime);
 		return -1;
 	}
 
@@ -401,8 +400,7 @@ static void __init print_efi_memmap(void)
 	     p < memmap.map_end;
 	     p += memmap.desc_size, i++) {
 		md = p;
-		pr_info("mem%02u: type=%u, attr=0x%llx, "
-			"range=[0x%016llx-0x%016llx) (%lluMB)\n",
+		pr_info("mem%02u: type=%u, attr=0x%llx, range=[0x%016llx-0x%016llx) (%lluMB)\n",
 			i, md->type, md->attribute, md->phys_addr,
 			md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT),
 			(md->num_pages >> (20 - EFI_PAGE_SHIFT)));
@@ -434,9 +432,8 @@ void __init efi_reserve_boot_services(void)
 			memblock_is_region_reserved(start, size)) {
 			/* Could not reserve, skip it */
 			md->num_pages = 0;
-			memblock_dbg("Could not reserve boot range "
-					"[0x%010llx-0x%010llx]\n",
-						start, start+size-1);
+			memblock_dbg("Could not reserve boot range [0x%010llx-0x%010llx]\n",
+				     start, start+size-1);
 		} else
 			memblock_reserve(start, size);
 	}
@@ -572,8 +569,7 @@ static int __init efi_systab_init(void *phys)
 		return -EINVAL;
 	}
 	if ((efi.systab->hdr.revision >> 16) == 0)
-		pr_err("Warning: System table version "
-		       "%d.%02d, expected 1.00 or greater!\n",
+		pr_err("Warning: System table version %d.%02d, expected 1.00 or greater!\n",
 		       efi.systab->hdr.revision >> 16,
 		       efi.systab->hdr.revision & 0xffff);
 
-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH v3 3/5] x86: Call efi_memblock_x86_reserve_range() on native EFI platform only

[Daniel Kiper]

Call efi_memblock_x86_reserve_range() on native EFI platform only.
This is not needed and even it should not be called on platforms
which wraps EFI infrastructure like Xen.

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

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index ce72964..992b67a 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -933,7 +933,7 @@ void __init setup_arch(char **cmdline_p)
 		set_bit(EFI_64BIT, &x86_efi_facility);
 	}
 
-	if (efi_enabled(EFI_BOOT))
+	if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature, "EL", 2))
 		efi_memblock_x86_reserve_range();
 #endif
 
-- 
1.7.10.4

Re: [PATCH v3 3/5] x86: Call efi_memblock_x86_reserve_range() on native EFI platform only

[Matt Fleming]

On Tue, 25 Mar, at 09:57:54PM, Daniel Kiper wrote:
> Call efi_memblock_x86_reserve_range() on native EFI platform only.
> This is not needed and even it should not be called on platforms
> which wraps EFI infrastructure like Xen.
> 
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
>  arch/x86/kernel/setup.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index ce72964..992b67a 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -933,7 +933,7 @@ void __init setup_arch(char **cmdline_p)
>  		set_bit(EFI_64BIT, &x86_efi_facility);
>  	}
>  
> -	if (efi_enabled(EFI_BOOT))
> +	if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature, "EL", 2))
>  		efi_memblock_x86_reserve_range();
>  #endif

This could do with a little bit more explanation. Why is it not
necessary to mark the EFI memory map that was passed to the kernel as
reserved in memblock?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v3 3/5] x86: Call efi_memblock_x86_reserve_range() on native EFI platform only

[Jan Beulich]

>>> On 26.03.14 at 14:00, <matt@console-pimps.org> wrote:
> On Tue, 25 Mar, at 09:57:54PM, Daniel Kiper wrote:
>> Call efi_memblock_x86_reserve_range() on native EFI platform only.
>> This is not needed and even it should not be called on platforms
>> which wraps EFI infrastructure like Xen.
>> 
>> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
>> ---
>>  arch/x86/kernel/setup.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>> index ce72964..992b67a 100644
>> --- a/arch/x86/kernel/setup.c
>> +++ b/arch/x86/kernel/setup.c
>> @@ -933,7 +933,7 @@ void __init setup_arch(char **cmdline_p)
>>  		set_bit(EFI_64BIT, &x86_efi_facility);
>>  	}
>>  
>> -	if (efi_enabled(EFI_BOOT))
>> +	if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature, "EL", 2))
>>  		efi_memblock_x86_reserve_range();
>>  #endif
> 
> This could do with a little bit more explanation. Why is it not
> necessary to mark the EFI memory map that was passed to the kernel as
> reserved in memblock?

Because that's in memory Dom0 doesn't even see: The EFI memory
map is visible to the hypervisor only.

Jan

Re: [PATCH v3 3/5] x86: Call efi_memblock_x86_reserve_range() on native EFI platform only

[Matt Fleming]

On Wed, 26 Mar, at 01:22:49PM, Jan Beulich wrote:
> >>> On 26.03.14 at 14:00, <matt@console-pimps.org> wrote:
> > 
> > This could do with a little bit more explanation. Why is it not
> > necessary to mark the EFI memory map that was passed to the kernel as
> > reserved in memblock?
> 
> Because that's in memory Dom0 doesn't even see: The EFI memory
> map is visible to the hypervisor only.

So where does boot_params.efi_info.efi_memmap point?

If nowhere (i.e. it's NULL) that's no problem because memblock_reserve()
handles zero size regions just fine.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v3 3/5] x86: Call efi_memblock_x86_reserve_range() on native EFI platform only

[Jan Beulich]

>>> On 26.03.14 at 14:31, <matt@console-pimps.org> wrote:
> On Wed, 26 Mar, at 01:22:49PM, Jan Beulich wrote:
>> >>> On 26.03.14 at 14:00, <matt@console-pimps.org> wrote:
>> > 
>> > This could do with a little bit more explanation. Why is it not
>> > necessary to mark the EFI memory map that was passed to the kernel as
>> > reserved in memblock?
>> 
>> Because that's in memory Dom0 doesn't even see: The EFI memory
>> map is visible to the hypervisor only.
> 
> So where does boot_params.efi_info.efi_memmap point?
> 
> If nowhere (i.e. it's NULL) that's no problem because memblock_reserve()
> handles zero size regions just fine.

That's a question to Daniel - in our implementation (with a separate
Xen kernel that can't run on bare hardware) boot_params as a whole
simply doesn't exist.

Jan

Re: [PATCH v3 3/5] x86: Call efi_memblock_x86_reserve_range() on native EFI platform only

[Daniel Kiper]

On Wed, Mar 26, 2014 at 01:39:42PM +0000, Jan Beulich wrote:
> >>> On 26.03.14 at 14:31, <matt@console-pimps.org> wrote:
> > On Wed, 26 Mar, at 01:22:49PM, Jan Beulich wrote:
> >> >>> On 26.03.14 at 14:00, <matt@console-pimps.org> wrote:
> >> >
> >> > This could do with a little bit more explanation. Why is it not
> >> > necessary to mark the EFI memory map that was passed to the kernel as
> >> > reserved in memblock?
> >>
> >> Because that's in memory Dom0 doesn't even see: The EFI memory
> >> map is visible to the hypervisor only.
> >
> > So where does boot_params.efi_info.efi_memmap point?
> >
> > If nowhere (i.e. it's NULL) that's no problem because memblock_reserve()
> > handles zero size regions just fine.
>
> That's a question to Daniel - in our implementation (with a separate
> Xen kernel that can't run on bare hardware) boot_params as a whole
> simply doesn't exist.

On my machine this function crashes on Xen so that is why I have changed
condition. However, if you say that this issue could be solved in
another way I will investigate it further.

Daniel

Re: [PATCH v3 3/5] x86: Call efi_memblock_x86_reserve_range() on native EFI platform only

[Matt Fleming]

On Wed, 26 Mar, at 02:48:45PM, Daniel Kiper wrote:
> 
> On my machine this function crashes on Xen so that is why I have changed
> condition. However, if you say that this issue could be solved in
> another way I will investigate it further.

Daniel, could you paste the crash? Do you get a stack trace?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v3 3/5] x86: Call efi_memblock_x86_reserve_range() on native EFI platform only

[Daniel Kiper]

On Wed, Mar 26, 2014 at 01:57:23PM +0000, Matt Fleming wrote:
> On Wed, 26 Mar, at 02:48:45PM, Daniel Kiper wrote:
> >
> > On my machine this function crashes on Xen so that is why I have changed
> > condition. However, if you say that this issue could be solved in
> > another way I will investigate it further.
>
> Daniel, could you paste the crash? Do you get a stack trace?

Here it is:

[...]

mapping kernel into physical memory
about to get started...
(XEN) traps.c:458:d0v0 Unhandled divide error fault/trap [#0] on VCPU 0 [ec=0000]
(XEN) domain_crash_sync called from entry.S: fault at ffff82d080229d30 int80_direct_trap+0x200/0x210
(XEN) Domain 0 (vcpu#0) crashed on cpu#0:
(XEN) ----[ Xen-4.5-unstable  x86_64  debug=y  Tainted:    C ]----
(XEN) CPU:    0
(XEN) RIP:    e033:[<ffffffff816987c5>]
(XEN) RFLAGS: 0000000000000246   EM: 1   CONTEXT: pv guest
(XEN) rax: 0000000000000000   rbx: 0000000001000000   rcx: 0000000000000000
(XEN) rdx: 0000000000000000   rsi: ffffffff814d7e7e   rdi: 0000000000000000
(XEN) rbp: ffffffff81601e88   rsp: ffffffff81601e48   r8:  0000000000000000
(XEN) r9:  0000000000007ff0   r10: 00000000deadbeef   r11: ffffffff81601df8
(XEN) r12: ffffffff816fe010   r13: ffffffff81601f00   r14: 0000000000000000
(XEN) r15: 0000000000000000   cr0: 0000000080050033   cr4: 00000000000026f0
(XEN) cr3: 000000007b60d000   cr2: 0000000000000000
(XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: e02b   cs: e033
(XEN) Guest stack trace from rsp=ffffffff81601e48:
(XEN)    0000000000000000 ffffffff81601df8 ffffffff816987c5 000000010000e030
(XEN)    0000000000010046 ffffffff81601e88 000000000000e02b ffffffff81601f00
(XEN)    ffffffff81601ed8 ffffffff8168a1a4 ffffffff81601ee8 ffffffff81601ea8
(XEN)    ffff880001e16490 ffffffffffffffff ffffffff816fe010 0000000000000000
(XEN)    0000000000000000 0000000000000000 ffffffff81601f28 ffffffff81685a3e
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    ffff880001e16000 0000000000000000 0000000000000000 0000000000000000
(XEN)    ffffffff81601f38 ffffffff816854c8 ffffffff81601ff8 ffffffff81687442
(XEN)    0000000100000000 0000080000000623 0789cbf580802001 0300000100000032
(XEN)    0000000000000005 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0f00000060c0c748
(XEN)    ccccccccccccc305 cccccccccccccccc cccccccccccccccc cccccccccccccccc
(XEN)    cccccccccccccccc cccccccccccccccc cccccccccccccccc cccccccccccccccc
(XEN)    cccccccccccccccc cccccccccccccccc cccccccccccccccc cccccccccccccccc
(XEN)    cccccccccccccccc cccccccccccccccc cccccccccccccccc cccccccccccccccc
(XEN)    cccccccccccccccc cccccccccccccccc cccccccccccccccc cccccccccccccccc
(XEN)    cccccccccccccccc cccccccccccccccc cccccccccccccccc cccccccccccccccc
(XEN) Domain 0 crashed: rebooting machine in 5 seconds.

Some addresses are solved to:
0xffffffff816987c5: efi_memblock_x86_reserve_range at arch/x86/platform/efi/efi.c:393
0xffffffff8168a1a4: setup_arch at arch/x86/kernel/setup.c:940
0xffffffff81685a3e: setup_command_line at init/main.c:353
0xffffffff816854c8: x86_64_start_reservations at arch/x86/kernel/head64.c:194
0xffffffff81687442: xen_start_kernel at arch/x86/xen/enlighten.c:1733

I am using Linus tree with latest commit b098d6726bbfb94c06d6e1097466187afddae61f
(Linux 3.14-rc8) with my patches applied excluding patch 3.

Daniel

Re: [Xen-devel] [PATCH v3 3/5] x86: Call efi_memblock_x86_reserve_range() on native EFI platform only

[Andrew Cooper]

On 26/03/2014 22:01, Daniel Kiper wrote:
> On Wed, Mar 26, 2014 at 01:57:23PM +0000, Matt Fleming wrote:
>> On Wed, 26 Mar, at 02:48:45PM, Daniel Kiper wrote:
>>> On my machine this function crashes on Xen so that is why I have changed
>>> condition. However, if you say that this issue could be solved in
>>> another way I will investigate it further.
>> Daniel, could you paste the crash? Do you get a stack trace?
> Here it is:
>
> [...]
>
> mapping kernel into physical memory
> about to get started...
> (XEN) traps.c:458:d0v0 Unhandled divide error fault/trap [#0] on VCPU 0 [ec=0000]
> (XEN) domain_crash_sync called from entry.S: fault at ffff82d080229d30 int80_direct_trap+0x200/0x210
> (XEN) Domain 0 (vcpu#0) crashed on cpu#0:
> (XEN) ----[ Xen-4.5-unstable  x86_64  debug=y  Tainted:    C ]----
> (XEN) CPU:    0
> (XEN) RIP:    e033:[<ffffffff816987c5>]
> (XEN) RFLAGS: 0000000000000246   EM: 1   CONTEXT: pv guest
> (XEN) rax: 0000000000000000   rbx: 0000000001000000   rcx: 0000000000000000
> (XEN) rdx: 0000000000000000   rsi: ffffffff814d7e7e   rdi: 0000000000000000
> (XEN) rbp: ffffffff81601e88   rsp: ffffffff81601e48   r8:  0000000000000000
> (XEN) r9:  0000000000007ff0   r10: 00000000deadbeef   r11: ffffffff81601df8
> (XEN) r12: ffffffff816fe010   r13: ffffffff81601f00   r14: 0000000000000000
> (XEN) r15: 0000000000000000   cr0: 0000000080050033   cr4: 00000000000026f0
> (XEN) cr3: 000000007b60d000   cr2: 0000000000000000
> (XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: e02b   cs: e033
> (XEN) Guest stack trace from rsp=ffffffff81601e48:
> (XEN)    0000000000000000 ffffffff81601df8 ffffffff816987c5 000000010000e030
> (XEN)    0000000000010046 ffffffff81601e88 000000000000e02b ffffffff81601f00
> (XEN)    ffffffff81601ed8 ffffffff8168a1a4 ffffffff81601ee8 ffffffff81601ea8
> (XEN)    ffff880001e16490 ffffffffffffffff ffffffff816fe010 0000000000000000
> (XEN)    0000000000000000 0000000000000000 ffffffff81601f28 ffffffff81685a3e
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    ffff880001e16000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    ffffffff81601f38 ffffffff816854c8 ffffffff81601ff8 ffffffff81687442
> (XEN)    0000000100000000 0000080000000623 0789cbf580802001 0300000100000032
> (XEN)    0000000000000005 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0f00000060c0c748
> (XEN)    ccccccccccccc305 cccccccccccccccc cccccccccccccccc cccccccccccccccc
> (XEN)    cccccccccccccccc cccccccccccccccc cccccccccccccccc cccccccccccccccc
> (XEN)    cccccccccccccccc cccccccccccccccc cccccccccccccccc cccccccccccccccc
> (XEN)    cccccccccccccccc cccccccccccccccc cccccccccccccccc cccccccccccccccc
> (XEN)    cccccccccccccccc cccccccccccccccc cccccccccccccccc cccccccccccccccc
> (XEN)    cccccccccccccccc cccccccccccccccc cccccccccccccccc cccccccccccccccc
> (XEN) Domain 0 crashed: rebooting machine in 5 seconds.
>
> Some addresses are solved to:
> 0xffffffff816987c5: efi_memblock_x86_reserve_range at arch/x86/platform/efi/efi.c:393
> 0xffffffff8168a1a4: setup_arch at arch/x86/kernel/setup.c:940
> 0xffffffff81685a3e: setup_command_line at init/main.c:353
> 0xffffffff816854c8: x86_64_start_reservations at arch/x86/kernel/head64.c:194
> 0xffffffff81687442: xen_start_kernel at arch/x86/xen/enlighten.c:1733
>
> I am using Linus tree with latest commit b098d6726bbfb94c06d6e1097466187afddae61f
> (Linux 3.14-rc8) with my patches applied excluding patch 3.
>
> Daniel

Then all you need to do is look up ffffffff816987c5 in your linux
symbols, and whichever variable is being divided on that line of source
has ether has the value 0 or -1. 

~Andrew

[PATCH v3 4/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.

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

diff --git a/include/xen/interface/platform.h b/include/xen/interface/platform.h
index f1331e3..bb3dfad 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;
+	unsigned long 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 */
+		unsigned long 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 {
+		unsigned long 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;
+		unsigned long capsule_count;
+		uint64_t max_capsule_size;
+		unsigned int reset_type;
+		} query_capsule_capabilities;
+
+	struct {
+		GUEST_HANDLE(void) capsule_header_array;
+		unsigned long 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;
@@ -143,7 +245,25 @@ struct xenpf_firmware_info {
 			/* must refer to 128-byte buffer */
 			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 +482,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 v3 4/5] xen: Define EFI related stuff

[Jan Beulich]

>>> On 25.03.14 at 21:57, <daniel.kiper@oracle.com> wrote:
> Define EFI related stuff for Xen.
> 
> This patch is based on Jan Beulich and Tang Liang work.

And with this, ...

> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> Signed-off-by: Tang Liang <liang.tang@oracle.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

... these should be in the reverse order to reflect flow.

> +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;
> +	unsigned long status;
> +	union {

You surely would want to use consistent indentation here (i.e. a leading
hard tab)?

> @@ -143,7 +245,25 @@ struct xenpf_firmware_info {
>  			/* must refer to 128-byte buffer */
>  			GUEST_HANDLE(uchar) edid;
>  		} vbeddc_info; /* XEN_FW_VBEDDC_INFO */
> -

Please retain blank lines like this ...

> +		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 */

... and add one here.

Jan

Re: [PATCH v3 4/5] xen: Define EFI related stuff

[Stefano Stabellini]

On Tue, 25 Mar 2014, Daniel Kiper wrote:
> Define EFI related stuff for Xen.
> 
> This patch is based on Jan Beulich and Tang Liang work.
> 
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> Signed-off-by: Tang Liang <liang.tang@oracle.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
>  include/xen/interface/platform.h |  123 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 122 insertions(+), 1 deletion(-)
> 
> diff --git a/include/xen/interface/platform.h b/include/xen/interface/platform.h
> index f1331e3..bb3dfad 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;
> +	unsigned long status;

I realize that this is just the same as xen/include/public/platform.h,
but this field should be xen_ulong_t.


> +	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 */
> +		unsigned long size;

Same here


> +		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 {
> +		unsigned long size;

Same here


> +		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;
> +		unsigned long capsule_count;

Same here


> +		uint64_t max_capsule_size;
> +		unsigned int reset_type;

I would prefer this to be explicitly sized


> +		} query_capsule_capabilities;
> +
> +	struct {
> +		GUEST_HANDLE(void) capsule_header_array;
> +		unsigned long capsule_count;

Same here


> +		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;
> @@ -143,7 +245,25 @@ struct xenpf_firmware_info {
>  			/* must refer to 128-byte buffer */
>  			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 +482,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 v3 4/5] xen: Define EFI related stuff

[Jan Beulich]

>>> On 26.03.14 at 15:58, <stefano.stabellini@eu.citrix.com> wrote:
>> +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;
>> +	unsigned long status;
> 
> I realize that this is just the same as xen/include/public/platform.h,
> but this field should be xen_ulong_t.

Care to supply a patch to the canonical header?

Jan

Re: [PATCH v3 4/5] xen: Define EFI related stuff

[Stefano Stabellini]

On Wed, 26 Mar 2014, Jan Beulich wrote:
> >>> On 26.03.14 at 15:58, <stefano.stabellini@eu.citrix.com> wrote:
> >> +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;
> >> +	unsigned long status;
> > 
> > I realize that this is just the same as xen/include/public/platform.h,
> > but this field should be xen_ulong_t.
> 
> Care to supply a patch to the canonical header?

I will.

[PATCH v3 5/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.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Signed-off-by: Tang Liang <liang.tang@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
 arch/x86/xen/enlighten.c |   10 ++
 drivers/xen/Kconfig      |    3 +
 drivers/xen/Makefile     |    1 +
 drivers/xen/efi.c        |  425 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 439 insertions(+)
 create mode 100644 drivers/xen/efi.c

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 201d09a..38bdb96 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -150,6 +150,14 @@ struct shared_info *HYPERVISOR_shared_info = &xen_dummy_shared_info;
  */
 static int have_vcpu_info_placement = 1;
 
+#ifdef CONFIG_XEN_EFI
+extern void __init xen_efi_probe(void);
+#else
+static void __init xen_efi_probe(void)
+{
+}
+#endif
+
 struct tls_descs {
 	struct desc_struct desc[3];
 };
@@ -1714,6 +1722,8 @@ asmlinkage void __init xen_start_kernel(void)
 
 	xen_setup_runstate_info(0);
 
+	xen_efi_probe();
+
 	/* 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..f3768e7
--- /dev/null
+++ b/drivers/xen/efi.c
@@ -0,0 +1,425 @@
+/*
+ * 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 Beulic <JBeulich@suse.com>
+ * Copyright (C) 2011-2012 Oracle Co.
+ *	Liang Tang <liang.tang@oracle.com>
+ * Copyright (c) 2014 Daniel Kiper, Oracle Corporation
+ */
+
+#include <linux/efi.h>
+#include <linux/export.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/time.h>
+#include <linux/spinlock.h>
+
+#include <asm/cacheflush.h>
+#include <asm/efi.h>
+#include <asm/tlbflush.h>
+#include <asm/setup.h>
+
+#include <xen/interface/platform.h>
+#include <asm/xen/hypercall.h>
+
+#define PFX		"EFI: "
+
+#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_system_table_t efi_systab __initdata = {};
+
+static const struct efi efi_xen __initconst = {
+	.systab                   = &efi_systab,
+	.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. */
+};
+
+static void __init efi_init_xen(void)
+{
+	efi_char16_t vendor_c16[100];
+	char vendor[ARRAY_SIZE(vendor_c16)];
+	int ret, i;
+	struct xen_platform_op op;
+	union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
+
+	efi = efi_xen;
+	op.cmd = XENPF_firmware_info;
+	op.u.firmware_info.type = XEN_FW_EFI_INFO;
+
+	/*
+	 * Show what we know for posterity
+	 */
+	op.u.firmware_info.index = XEN_FW_EFI_VENDOR;
+	info->vendor.bufsz = sizeof(vendor_c16);
+	set_xen_guest_handle(info->vendor.name, vendor_c16);
+	ret = HYPERVISOR_dom0_op(&op);
+	if (!ret) {
+		for (i = 0; i < sizeof(vendor) - 1 && vendor_c16[i]; ++i)
+			vendor[i] = vendor_c16[i];
+		vendor[i] = '\0';
+	} else
+		pr_err("Could not get the firmware vendor!\n");
+
+	op.u.firmware_info.index = XEN_FW_EFI_VERSION;
+	ret = HYPERVISOR_dom0_op(&op);
+	if (!ret)
+		pr_info("EFI v%u.%.02u by %s\n",
+		       info->version >> 16,
+		       info->version & 0xffff, vendor);
+	else
+		pr_err("Could not get EFI revision!\n");
+
+	op.u.firmware_info.index = XEN_FW_EFI_RT_VERSION;
+	ret = HYPERVISOR_dom0_op(&op);
+	if (!ret)
+		efi.runtime_version = info->version;
+	else
+		pr_warn(PFX "Could not get runtime services revision.\n");
+
+	/*
+	 * Let's see what config tables the firmware passed to us.
+	 */
+	op.u.firmware_info.index = XEN_FW_EFI_CONFIG_TABLE;
+	if (HYPERVISOR_dom0_op(&op))
+		BUG();
+
+	efi.systab->tables = info->cfg.addr;
+	efi.systab->nr_tables = info->cfg.nent;
+
+	if (efi_config_init(arch_tables))
+		return;
+
+	set_bit(EFI_CONFIG_TABLES, &x86_efi_facility);
+
+	x86_platform.get_wallclock = efi_get_time;
+	x86_platform.set_wallclock = efi_set_rtc_mmss;
+
+	set_bit(EFI_RUNTIME_SERVICES, &x86_efi_facility);
+}
+
+static void __init efi_reserve_boot_services_xen(void)
+{
+}
+
+static void __init efi_enter_virtual_mode_xen(void)
+{
+}
+
+/*
+ * Convenience functions to obtain memory types and attributes
+ */
+static u32 efi_mem_type_xen(unsigned long phys_addr)
+{
+	struct xen_platform_op op;
+	union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
+
+	op.cmd = XENPF_firmware_info;
+	op.u.firmware_info.type = XEN_FW_EFI_INFO;
+	op.u.firmware_info.index = XEN_FW_EFI_MEM_INFO;
+	info->mem.addr = phys_addr;
+	info->mem.size = 0;
+	return HYPERVISOR_dom0_op(&op) ? 0 : info->mem.type;
+}
+
+static u64 efi_mem_attributes_xen(unsigned long phys_addr)
+{
+	struct xen_platform_op op;
+	union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
+
+	op.cmd = XENPF_firmware_info;
+	op.u.firmware_info.type = XEN_FW_EFI_INFO;
+	op.u.firmware_info.index = XEN_FW_EFI_MEM_INFO;
+	info->mem.addr = phys_addr;
+	info->mem.size = 0;
+	return HYPERVISOR_dom0_op(&op) ? 0 : info->mem.attr;
+}
+
+void __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
+		}
+	};
+
+	if (!xen_initial_domain() || HYPERVISOR_dom0_op(&op))
+		return;
+
+	/* Here we know that Xen runs on EFI platform. */
+
+	efi_init_ops.efi_init = efi_init_xen;
+	efi_init_ops.efi_reserve_boot_services = efi_reserve_boot_services_xen;
+	efi_init_ops.efi_enter_virtual_mode = efi_enter_virtual_mode_xen;
+	efi_init_ops.efi_mem_type = efi_mem_type_xen;
+	efi_init_ops.efi_mem_attributes = efi_mem_attributes_xen;
+
+	set_bit(EFI_BOOT, &x86_efi_facility);
+	set_bit(EFI_64BIT, &x86_efi_facility);
+}
-- 
1.7.10.4

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

[Matt Fleming]

On Tue, 25 Mar, at 09:57:56PM, Daniel Kiper wrote:
> Put EFI machinery for Xen in place.
> 
> This patch is based on Jan Beulich and Tang Liang work.
> 
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> Signed-off-by: Tang Liang <liang.tang@oracle.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
>  arch/x86/xen/enlighten.c |   10 ++
>  drivers/xen/Kconfig      |    3 +
>  drivers/xen/Makefile     |    1 +
>  drivers/xen/efi.c        |  425 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 439 insertions(+)
>  create mode 100644 drivers/xen/efi.c

[...]

> +static void __init efi_init_xen(void)
> +{
> +	efi_char16_t vendor_c16[100];
> +	char vendor[ARRAY_SIZE(vendor_c16)];
> +	int ret, i;
> +	struct xen_platform_op op;
> +	union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> +
> +	efi = efi_xen;
> +	op.cmd = XENPF_firmware_info;
> +	op.u.firmware_info.type = XEN_FW_EFI_INFO;
> +
> +	/*
> +	 * Show what we know for posterity
> +	 */
> +	op.u.firmware_info.index = XEN_FW_EFI_VENDOR;
> +	info->vendor.bufsz = sizeof(vendor_c16);
> +	set_xen_guest_handle(info->vendor.name, vendor_c16);
> +	ret = HYPERVISOR_dom0_op(&op);
> +	if (!ret) {
> +		for (i = 0; i < sizeof(vendor) - 1 && vendor_c16[i]; ++i)
> +			vendor[i] = vendor_c16[i];
> +		vendor[i] = '\0';
> +	} else
> +		pr_err("Could not get the firmware vendor!\n");
> +

Is there a reason that you can't just populate an efi_system_table_t
object, which could be used by efi_init(), so that we can save you the
trouble of duplicating all of this code?

> +/*
> + * Convenience functions to obtain memory types and attributes
> + */
> +static u32 efi_mem_type_xen(unsigned long phys_addr)
> +{
> +	struct xen_platform_op op;
> +	union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> +
> +	op.cmd = XENPF_firmware_info;
> +	op.u.firmware_info.type = XEN_FW_EFI_INFO;
> +	op.u.firmware_info.index = XEN_FW_EFI_MEM_INFO;
> +	info->mem.addr = phys_addr;
> +	info->mem.size = 0;
> +	return HYPERVISOR_dom0_op(&op) ? 0 : info->mem.type;
> +}

Same idea here. Unless you expect the EFI memory map to change at runtime
(and it's not clear to me whether that wouldn't cause other things to
explode) you'd be better off building a struct efi_memory_map and using
the existing generic functions.

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Jan Beulich]

>>> On 26.03.14 at 14:12, <matt@console-pimps.org> wrote:
> On Tue, 25 Mar, at 09:57:56PM, Daniel Kiper wrote:
>> +static void __init efi_init_xen(void)
>> +{
>> +	efi_char16_t vendor_c16[100];
>> +	char vendor[ARRAY_SIZE(vendor_c16)];
>> +	int ret, i;
>> +	struct xen_platform_op op;
>> +	union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
>> +
>> +	efi = efi_xen;
>> +	op.cmd = XENPF_firmware_info;
>> +	op.u.firmware_info.type = XEN_FW_EFI_INFO;
>> +
>> +	/*
>> +	 * Show what we know for posterity
>> +	 */
>> +	op.u.firmware_info.index = XEN_FW_EFI_VENDOR;
>> +	info->vendor.bufsz = sizeof(vendor_c16);
>> +	set_xen_guest_handle(info->vendor.name, vendor_c16);
>> +	ret = HYPERVISOR_dom0_op(&op);
>> +	if (!ret) {
>> +		for (i = 0; i < sizeof(vendor) - 1 && vendor_c16[i]; ++i)
>> +			vendor[i] = vendor_c16[i];
>> +		vendor[i] = '\0';
>> +	} else
>> +		pr_err("Could not get the firmware vendor!\n");
>> +
> 
> Is there a reason that you can't just populate an efi_system_table_t
> object, which could be used by efi_init(), so that we can save you the
> trouble of duplicating all of this code?

Would the generic function cope with all other fields being NULL (or
equivalent)?

>> +/*
>> + * Convenience functions to obtain memory types and attributes
>> + */
>> +static u32 efi_mem_type_xen(unsigned long phys_addr)
>> +{
>> +	struct xen_platform_op op;
>> +	union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
>> +
>> +	op.cmd = XENPF_firmware_info;
>> +	op.u.firmware_info.type = XEN_FW_EFI_INFO;
>> +	op.u.firmware_info.index = XEN_FW_EFI_MEM_INFO;
>> +	info->mem.addr = phys_addr;
>> +	info->mem.size = 0;
>> +	return HYPERVISOR_dom0_op(&op) ? 0 : info->mem.type;
>> +}
> 
> Same idea here. Unless you expect the EFI memory map to change at runtime
> (and it's not clear to me whether that wouldn't cause other things to
> explode) you'd be better off building a struct efi_memory_map and using
> the existing generic functions.

As said in another reply to this series - the memory map isn't being
(and shouldn't be) exposed to Dom0.

Jan

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

[Matt Fleming]

On Wed, 26 Mar, at 01:31:04PM, Jan Beulich wrote:
> >>> On 26.03.14 at 14:12, <matt@console-pimps.org> wrote:
> > 
> > Is there a reason that you can't just populate an efi_system_table_t
> > object, which could be used by efi_init(), so that we can save you the
> > trouble of duplicating all of this code?
> 
> Would the generic function cope with all other fields being NULL (or
> equivalent)?
 
I've no idea, but it shouldn't be difficult to make that work. And by
making it work in the generic code

> >> +/*
> >> + * Convenience functions to obtain memory types and attributes
> >> + */
> >> +static u32 efi_mem_type_xen(unsigned long phys_addr)
> >> +{
> >> +	struct xen_platform_op op;
> >> +	union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> >> +
> >> +	op.cmd = XENPF_firmware_info;
> >> +	op.u.firmware_info.type = XEN_FW_EFI_INFO;
> >> +	op.u.firmware_info.index = XEN_FW_EFI_MEM_INFO;
> >> +	info->mem.addr = phys_addr;
> >> +	info->mem.size = 0;
> >> +	return HYPERVISOR_dom0_op(&op) ? 0 : info->mem.type;
> >> +}
> > 
> > Same idea here. Unless you expect the EFI memory map to change at runtime
> > (and it's not clear to me whether that wouldn't cause other things to
> > explode) you'd be better off building a struct efi_memory_map and using
> > the existing generic functions.
> 
> As said in another reply to this series - the memory map isn't being
> (and shouldn't be) exposed to Dom0.
 
Right, so you might not necessarily expose the EFI memory map that was
constructed by the firmware, but it's worth looking at exposing *some*
boot memory map, e.g. the map of memory before the kernel took control
in DomU.

Whether the firmware or the kernel builds it doesn't really matter. My
point is that since we've already got a means of looking in the boot
memory map for EFI type and EFI attributes, let's use it.

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Matt Fleming]

On Wed, 26 Mar, at 01:46:17PM, Matt Fleming wrote:
> On Wed, 26 Mar, at 01:31:04PM, Jan Beulich wrote:
> > >>> On 26.03.14 at 14:12, <matt@console-pimps.org> wrote:
> > > 
> > > Is there a reason that you can't just populate an efi_system_table_t
> > > object, which could be used by efi_init(), so that we can save you the
> > > trouble of duplicating all of this code?
> > 
> > Would the generic function cope with all other fields being NULL (or
> > equivalent)?
>  
> I've no idea, but it shouldn't be difficult to make that work. And by
> making it work in the generic code

Guh, hit Send too soon... sorry.

What I was trying to say was that there's a real benefit to keeping all
these idiosyncrasies, such as "This field may be NULL on some platforms,
or this field, or that", in one place, because it gives you an immediate
view of what platforms Linux supports and what assumptions we can make.

It also helps immensely when the time comes to do some refactoring -
because it's all contained in one central place all the use cases are in
front of you.

Obviously this approach doesn't work for super-platform specific stuff,
but it's not clear to me that these patches fall into that category.

-- 
Matt Fleming, Intel Open Source Technology Center