linux-next: manual merge of the xen-tip tree with the tip tree

linux-next: manual merge of the xen-tip tree with the tip tree

[Stephen Rothwell]

Hi all,

Today's linux-next merge of the xen-tip tree got a conflict in:

  drivers/firmware/efi/arm-runtime.c

between commit:

  14c43be60166 ("efi/arm*: Drop writable mapping of the UEFI System table")

from the tip tree and commit:

  21c8dfaa2327 ("Xen: EFI: Parse DT parameters for Xen specific UEFI")

from the xen-tip tree.

I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging.  You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.

-- 
Cheers,
Stephen Rothwell

diff --cc drivers/firmware/efi/arm-runtime.c
index 17ccf0a8787a,ac609b9f0b99..000000000000
--- a/drivers/firmware/efi/arm-runtime.c
+++ b/drivers/firmware/efi/arm-runtime.c
@@@ -109,24 -90,41 +110,30 @@@ static int __init arm_enable_runtime_se
  
  	pr_info("Remapping and enabling EFI services.\n");
  
 -	mapsize = memmap.map_end - memmap.map;
 -	memmap.map = (__force void *)ioremap_cache(memmap.phys_map,
 -						   mapsize);
 -	if (!memmap.map) {
 -		pr_err("Failed to remap EFI memory map\n");
 -		return -ENOMEM;
 -	}
 -	memmap.map_end = memmap.map + mapsize;
 -	efi.memmap = &memmap;
 +	mapsize = efi.memmap.map_end - efi.memmap.map;
  
 -	efi.systab = (__force void *)ioremap_cache(efi_system_table,
 -						   sizeof(efi_system_table_t));
 -	if (!efi.systab) {
 -		pr_err("Failed to remap EFI System Table\n");
 +	efi.memmap.map = memremap(efi.memmap.phys_map, mapsize, MEMREMAP_WB);
 +	if (!efi.memmap.map) {
 +		pr_err("Failed to remap EFI memory map\n");
  		return -ENOMEM;
  	}
 -	set_bit(EFI_SYSTEM_TABLES, &efi.flags);
 +	efi.memmap.map_end = efi.memmap.map + mapsize;
  
- 	if (!efi_virtmap_init()) {
- 		pr_err("UEFI virtual mapping missing or invalid -- runtime services will not be available\n");
- 		return -ENOMEM;
+ 	if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_PARAVIRT)) {
+ 		/* Set up runtime services function pointers for Xen Dom0 */
+ 		xen_efi_runtime_setup();
+ 	} else {
+ 		if (!efi_virtmap_init()) {
 -			pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
++			pr_err("UEFI virtual mapping missing or invalid -- runtime services will not be available\n");
+ 			return -ENOMEM;
+ 		}
+ 
+ 		/* Set up runtime services function pointers */
+ 		efi_native_runtime_setup();
  	}
  
- 	/* Set up runtime services function pointers */
- 	efi_native_runtime_setup();
  	set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
  
 -	efi.runtime_version = efi.systab->hdr.revision;
 -
  	return 0;
  }
  early_initcall(arm_enable_runtime_services);

Re: efi_enabled(EFI_PARAVIRT) use

[Ingo Molnar]

* Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> Hi all,
> 
> Today's linux-next merge of the xen-tip tree got a conflict in:
> 
>   drivers/firmware/efi/arm-runtime.c
> 
> between commit:
> 
>   14c43be60166 ("efi/arm*: Drop writable mapping of the UEFI System table")
> 
> from the tip tree and commit:
> 
>   21c8dfaa2327 ("Xen: EFI: Parse DT parameters for Xen specific UEFI")
> 
> from the xen-tip tree.

(I've attached 21c8dfaa2327 below, for reference.)

Argh:

With considerable pain we just got rid of paravirt_enabled() in the x86 tree, and 
Xen is now reintroducing it in the EFI code. Please don't: if you have to do 
capability flags then name the flag accordingly to what it does, don't use some 
generic catch-all naming that will inevitably cause the kind of problems 
paravirt_enabled() caused...

So: NAKed-by: Ingo Molnar <mingo@kernel.org>

Also, it would be nice to have all things EFI in a single tree, the conflicts are 
going to be painful! There's very little reason not to carry this kind of commit:

 arch/arm/xen/enlighten.c           |  6 +++++
 drivers/firmware/efi/arm-runtime.c | 17 +++++++++-----
 drivers/firmware/efi/efi.c         | 45 ++++++++++++++++++++++++++++++++------
 3 files changed, 56 insertions(+), 12 deletions(-)

in the EFI tree.

Thanks,

	Ingo

=======================>
>From 21c8dfaa23276be2ae6d580331d8d252cc41e8d9 Mon Sep 17 00:00:00 2001
From: Shannon Zhao <shannon.zhao@linaro.org>
Date: Thu, 7 Apr 2016 20:03:34 +0800
Subject: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI

Add a new function to parse DT parameters for Xen specific UEFI just
like the way for normal UEFI. Then it could reuse the existing codes.

If Xen supports EFI, initialize runtime services.

CC: Matt Fleming <matt@codeblueprint.co.uk>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Tested-by: Julien Grall <julien.grall@arm.com>
---
 arch/arm/xen/enlighten.c           |  6 +++++
 drivers/firmware/efi/arm-runtime.c | 17 +++++++++-----
 drivers/firmware/efi/efi.c         | 45 ++++++++++++++++++++++++++++++++------
 3 files changed, 56 insertions(+), 12 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 13e3e9f9b094..e130562d3283 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -261,6 +261,12 @@ static int __init fdt_find_hyper_node(unsigned long node, const char *uname,
 	    !strncmp(hyper_node.prefix, s, strlen(hyper_node.prefix)))
 		hyper_node.version = s + strlen(hyper_node.prefix);
 
+	if (IS_ENABLED(CONFIG_XEN_EFI)) {
+		/* Check if Xen supports EFI */
+		if (of_get_flat_dt_subnode_by_name(node, "uefi") > 0)
+			set_bit(EFI_PARAVIRT, &efi.flags);
+	}
+
 	return 0;
 }
 
diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
index 6ae21e41a429..ac609b9f0b99 100644
--- a/drivers/firmware/efi/arm-runtime.c
+++ b/drivers/firmware/efi/arm-runtime.c
@@ -27,6 +27,7 @@
 #include <asm/mmu.h>
 #include <asm/pgalloc.h>
 #include <asm/pgtable.h>
+#include <asm/xen/xen-ops.h>
 
 extern u64 efi_system_table;
 
@@ -107,13 +108,19 @@ static int __init arm_enable_runtime_services(void)
 	}
 	set_bit(EFI_SYSTEM_TABLES, &efi.flags);
 
-	if (!efi_virtmap_init()) {
-		pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
-		return -ENOMEM;
+	if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_PARAVIRT)) {
+		/* Set up runtime services function pointers for Xen Dom0 */
+		xen_efi_runtime_setup();
+	} else {
+		if (!efi_virtmap_init()) {
+			pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
+			return -ENOMEM;
+		}
+
+		/* Set up runtime services function pointers */
+		efi_native_runtime_setup();
 	}
 
-	/* Set up runtime services function pointers */
-	efi_native_runtime_setup();
 	set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
 
 	efi.runtime_version = efi.systab->hdr.revision;
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 3a69ed5ecfcb..519c096a7c33 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -469,12 +469,14 @@ device_initcall(efi_load_efivars);
 		FIELD_SIZEOF(struct efi_fdt_params, field) \
 	}
 
-static __initdata struct {
+struct params {
 	const char name[32];
 	const char propname[32];
 	int offset;
 	int size;
-} dt_params[] = {
+};
+
+static struct params fdt_params[] __initdata = {
 	UEFI_PARAM("System Table", "linux,uefi-system-table", system_table),
 	UEFI_PARAM("MemMap Address", "linux,uefi-mmap-start", mmap),
 	UEFI_PARAM("MemMap Size", "linux,uefi-mmap-size", mmap_size),
@@ -482,24 +484,45 @@ static __initdata struct {
 	UEFI_PARAM("MemMap Desc. Version", "linux,uefi-mmap-desc-ver", desc_ver)
 };
 
+static struct params xen_fdt_params[] __initdata = {
+	UEFI_PARAM("System Table", "xen,uefi-system-table", system_table),
+	UEFI_PARAM("MemMap Address", "xen,uefi-mmap-start", mmap),
+	UEFI_PARAM("MemMap Size", "xen,uefi-mmap-size", mmap_size),
+	UEFI_PARAM("MemMap Desc. Size", "xen,uefi-mmap-desc-size", desc_size),
+	UEFI_PARAM("MemMap Desc. Version", "xen,uefi-mmap-desc-ver", desc_ver)
+};
+
 struct param_info {
 	int found;
 	void *params;
+	struct params *dt_params;
+	int size;
 };
 
 static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
 				       int depth, void *data)
 {
 	struct param_info *info = data;
+	struct params *dt_params = info->dt_params;
 	const void *prop;
 	void *dest;
 	u64 val;
-	int i, len;
+	int i, len, offset;
 
-	if (depth != 1 || strcmp(uname, "chosen") != 0)
-		return 0;
+	if (efi_enabled(EFI_PARAVIRT)) {
+		if (depth != 1 || strcmp(uname, "hypervisor") != 0)
+			return 0;
 
-	for (i = 0; i < ARRAY_SIZE(dt_params); i++) {
+		offset = of_get_flat_dt_subnode_by_name(node, "uefi");
+		if (offset < 0)
+			return 0;
+		node = offset;
+	} else {
+		if (depth != 1 || strcmp(uname, "chosen") != 0)
+			return 0;
+	}
+
+	for (i = 0; i < info->size; i++) {
 		prop = of_get_flat_dt_prop(node, dt_params[i].propname, &len);
 		if (!prop)
 			return 0;
@@ -530,12 +553,20 @@ int __init efi_get_fdt_params(struct efi_fdt_params *params)
 	info.found = 0;
 	info.params = params;
 
+	if (efi_enabled(EFI_PARAVIRT)) {
+		info.dt_params = xen_fdt_params;
+		info.size = ARRAY_SIZE(xen_fdt_params);
+	} else {
+		info.dt_params = fdt_params;
+		info.size = ARRAY_SIZE(fdt_params);
+	}
+
 	ret = of_scan_flat_dt(fdt_find_uefi_params, &info);
 	if (!info.found)
 		pr_info("UEFI not found.\n");
 	else if (!ret)
 		pr_err("Can't find '%s' in device tree!\n",
-		       dt_params[info.found].name);
+		       info.dt_params[info.found].name);
 
 	return ret;
 }

Re: efi_enabled(EFI_PARAVIRT) use

[Borislav Petkov]

On Fri, Apr 29, 2016 at 08:39:36AM +0200, Ingo Molnar wrote:
> With considerable pain we just got rid of paravirt_enabled() in the
> x86 tree, and Xen is now reintroducing it in the EFI code.

I think Matt is working towards removing EFI_PARAVIRT but he'll comment
himself when he wakes up... :)

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: efi_enabled(EFI_PARAVIRT) use

[Matt Fleming]

On Fri, 29 Apr, at 10:25:02AM, Borislav Petkov wrote:
> On Fri, Apr 29, 2016 at 08:39:36AM +0200, Ingo Molnar wrote:
> > With considerable pain we just got rid of paravirt_enabled() in the
> > x86 tree, and Xen is now reintroducing it in the EFI code.
> 
> I think Matt is working towards removing EFI_PARAVIRT but he'll comment
> himself when he wakes up... :)

Yeah, I haven't actually got around to dropping EFI_PARAVIRT yet but
since it's basically used to skip certain initialisation operations on
boot I figured we could just provide empty stub functions as part of
struct efi (probably).

The concerns Ingo voiced about EFI_PARAVIRT being a catch-all flag are
very true.

Incidentally kexec and arm64 would need a similar stub functions if we
move more EFI runtime setup code to drivers/firmware/efi, which is my
long-term plan, since neither can call SetVirtualAddressMap().

On x86, I think EFI_PARAVIRT is code for,

  1. Has no EFI memory map
  2. Runtime regions do not need to be mapped
  3. Cannot call SetVirtualAddressMap()
  4. /sys/firmware/efi/fw_vendor is invisible

1. and 2. should be covered by never setting EFI_MEMMAP and
EFI_RUNTIME_SERVICES in efi.flags. We have no bits for 3. and 4. yet.

Re: efi_enabled(EFI_PARAVIRT) use

[Stefano Stabellini]

On Fri, 29 Apr 2016, Ingo Molnar wrote:
> Also, it would be nice to have all things EFI in a single tree, the conflicts are 
> going to be painful! There's very little reason not to carry this kind of commit:
> 
>  arch/arm/xen/enlighten.c           |  6 +++++
>  drivers/firmware/efi/arm-runtime.c | 17 +++++++++-----
>  drivers/firmware/efi/efi.c         | 45 ++++++++++++++++++++++++++++++++------
>  3 files changed, 56 insertions(+), 12 deletions(-)
> 
> in the EFI tree.

That's true. I'll drop this commit from xentip and let Matt pick it up
or request changes as he sees fit.

Re: efi_enabled(EFI_PARAVIRT) use

[Ingo Molnar]

* Stefano Stabellini <sstabellini@kernel.org> wrote:

> On Fri, 29 Apr 2016, Ingo Molnar wrote:
> > Also, it would be nice to have all things EFI in a single tree, the conflicts are 
> > going to be painful! There's very little reason not to carry this kind of commit:
> > 
> >  arch/arm/xen/enlighten.c           |  6 +++++
> >  drivers/firmware/efi/arm-runtime.c | 17 +++++++++-----
> >  drivers/firmware/efi/efi.c         | 45 ++++++++++++++++++++++++++++++++------
> >  3 files changed, 56 insertions(+), 12 deletions(-)
> > 
> > in the EFI tree.
> 
> That's true. I'll drop this commit from xentip and let Matt pick it up
> or request changes as he sees fit.

Thanks!

	Ingo

Re: efi_enabled(EFI_PARAVIRT) use

[Matt Fleming]

On Fri, 29 Apr, at 11:34:45AM, Stefano Stabellini wrote:
> On Fri, 29 Apr 2016, Ingo Molnar wrote:
> > Also, it would be nice to have all things EFI in a single tree, the conflicts are 
> > going to be painful! There's very little reason not to carry this kind of commit:
> > 
> >  arch/arm/xen/enlighten.c           |  6 +++++
> >  drivers/firmware/efi/arm-runtime.c | 17 +++++++++-----
> >  drivers/firmware/efi/efi.c         | 45 ++++++++++++++++++++++++++++++++------
> >  3 files changed, 56 insertions(+), 12 deletions(-)
> > 
> > in the EFI tree.
> 
> That's true. I'll drop this commit from xentip and let Matt pick it up
> or request changes as he sees fit.

One small change I think would be sensible to make is to expand
EFI_PARAVIRT into a few more bits to clearly indicate the quirks on
Xen, and in the process, to delete EFI_PARAVIRT.

That should address Ingo's major concern, and also make it much easier
to rework the code in a piecemeal fashion.

Could somebody enumerate the things that make Xen (dom0) different on
arm* compared with bare metal EFI boot? The list I made for x86 was,

  1. Has no EFI memory map
  2. Runtime regions do not need to be mapped
  3. Cannot call SetVirtualAddressMap()
  4. /sys/firmware/efi/fw_vendor is invisible

The first maps to not setting EFI_MEMMAP, the second to not setting
EFI_RUNTIME. If we add EFI_ALREADY_VIRTUAL and EFI_FW_VENDOR_INVISIBLE
to efi.flags that should cover everything on x86. Does arm* require
anything else?

Re: efi_enabled(EFI_PARAVIRT) use

[Ard Biesheuvel]

On 29 April 2016 at 16:39, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Fri, 29 Apr, at 11:34:45AM, Stefano Stabellini wrote:
>> On Fri, 29 Apr 2016, Ingo Molnar wrote:
>> > Also, it would be nice to have all things EFI in a single tree, the conflicts are
>> > going to be painful! There's very little reason not to carry this kind of commit:
>> >
>> >  arch/arm/xen/enlighten.c           |  6 +++++
>> >  drivers/firmware/efi/arm-runtime.c | 17 +++++++++-----
>> >  drivers/firmware/efi/efi.c         | 45 ++++++++++++++++++++++++++++++++------
>> >  3 files changed, 56 insertions(+), 12 deletions(-)
>> >
>> > in the EFI tree.
>>
>> That's true. I'll drop this commit from xentip and let Matt pick it up
>> or request changes as he sees fit.
>
> One small change I think would be sensible to make is to expand
> EFI_PARAVIRT into a few more bits to clearly indicate the quirks on
> Xen, and in the process, to delete EFI_PARAVIRT.
>
> That should address Ingo's major concern, and also make it much easier
> to rework the code in a piecemeal fashion.
>
> Could somebody enumerate the things that make Xen (dom0) different on
> arm* compared with bare metal EFI boot? The list I made for x86 was,
>
>   1. Has no EFI memory map
>   2. Runtime regions do not need to be mapped
>   3. Cannot call SetVirtualAddressMap()
>   4. /sys/firmware/efi/fw_vendor is invisible
>
> The first maps to not setting EFI_MEMMAP, the second to not setting
> EFI_RUNTIME. If we add EFI_ALREADY_VIRTUAL and EFI_FW_VENDOR_INVISIBLE
> to efi.flags that should cover everything on x86. Does arm* require
> anything else?

I already proposed when this patch was first under review to make the
arm_enable_runtime_services() function bail early without error if the
EFI_RUNTIME_SERVICES flag is already set, and the xen code could set
that bit as well when it installs its paravirtualized alternatives. I
don't remember exactly why that was shot down, though, but I think it
is the only reason this code introduces references to EFI_PARAVIRT in
the first place.

Re: efi_enabled(EFI_PARAVIRT) use

[Stefano Stabellini]

On Fri, 29 Apr 2016, Matt Fleming wrote:
> On Fri, 29 Apr, at 11:34:45AM, Stefano Stabellini wrote:
> > On Fri, 29 Apr 2016, Ingo Molnar wrote:
> > > Also, it would be nice to have all things EFI in a single tree, the conflicts are 
> > > going to be painful! There's very little reason not to carry this kind of commit:
> > > 
> > >  arch/arm/xen/enlighten.c           |  6 +++++
> > >  drivers/firmware/efi/arm-runtime.c | 17 +++++++++-----
> > >  drivers/firmware/efi/efi.c         | 45 ++++++++++++++++++++++++++++++++------
> > >  3 files changed, 56 insertions(+), 12 deletions(-)
> > > 
> > > in the EFI tree.
> > 
> > That's true. I'll drop this commit from xentip and let Matt pick it up
> > or request changes as he sees fit.
> 
> One small change I think would be sensible to make is to expand
> EFI_PARAVIRT into a few more bits to clearly indicate the quirks on
> Xen, and in the process, to delete EFI_PARAVIRT.
> 
> That should address Ingo's major concern, and also make it much easier
> to rework the code in a piecemeal fashion.
> 
> Could somebody enumerate the things that make Xen (dom0) different on
> arm* compared with bare metal EFI boot? The list I made for x86 was,
> 
>   1. Has no EFI memory map
>   2. Runtime regions do not need to be mapped
>   3. Cannot call SetVirtualAddressMap()
>   4. /sys/firmware/efi/fw_vendor is invisible
> 
> The first maps to not setting EFI_MEMMAP, the second to not setting
> EFI_RUNTIME. If we add EFI_ALREADY_VIRTUAL and EFI_FW_VENDOR_INVISIBLE
> to efi.flags that should cover everything on x86. Does arm* require
> anything else?

Xen on ARM is different, the impact should be limited:

- there are no BootServices (ExitBootServices has already been called)
- RuntimeServices go via hypercalls

The UEFI memory map is still available at an address specified on device
tree like on native, but the compatibility string is different
("xen,uefi-mmap-start") to clarify that we are booting on Xen rather
than native.

That's pretty much it, Shannon please confirm.

Re: efi_enabled(EFI_PARAVIRT) use

[Stefano Stabellini]

On Fri, 29 Apr 2016, Stefano Stabellini wrote:
> On Fri, 29 Apr 2016, Matt Fleming wrote:
> > On Fri, 29 Apr, at 11:34:45AM, Stefano Stabellini wrote:
> > > On Fri, 29 Apr 2016, Ingo Molnar wrote:
> > > > Also, it would be nice to have all things EFI in a single tree, the conflicts are 
> > > > going to be painful! There's very little reason not to carry this kind of commit:
> > > > 
> > > >  arch/arm/xen/enlighten.c           |  6 +++++
> > > >  drivers/firmware/efi/arm-runtime.c | 17 +++++++++-----
> > > >  drivers/firmware/efi/efi.c         | 45 ++++++++++++++++++++++++++++++++------
> > > >  3 files changed, 56 insertions(+), 12 deletions(-)
> > > > 
> > > > in the EFI tree.
> > > 
> > > That's true. I'll drop this commit from xentip and let Matt pick it up
> > > or request changes as he sees fit.
> > 
> > One small change I think would be sensible to make is to expand
> > EFI_PARAVIRT into a few more bits to clearly indicate the quirks on
> > Xen, and in the process, to delete EFI_PARAVIRT.
> > 
> > That should address Ingo's major concern, and also make it much easier
> > to rework the code in a piecemeal fashion.
> > 
> > Could somebody enumerate the things that make Xen (dom0) different on
> > arm* compared with bare metal EFI boot? The list I made for x86 was,
> > 
> >   1. Has no EFI memory map
> >   2. Runtime regions do not need to be mapped
> >   3. Cannot call SetVirtualAddressMap()
> >   4. /sys/firmware/efi/fw_vendor is invisible
> > 
> > The first maps to not setting EFI_MEMMAP, the second to not setting
> > EFI_RUNTIME. If we add EFI_ALREADY_VIRTUAL and EFI_FW_VENDOR_INVISIBLE
> > to efi.flags that should cover everything on x86. Does arm* require
> > anything else?
> 
> Xen on ARM is different, the impact should be limited:
> 
> - there are no BootServices (ExitBootServices has already been called)
> - RuntimeServices go via hypercalls
> 
> The UEFI memory map is still available at an address specified on device
> tree like on native, but the compatibility string is different
> ("xen,uefi-mmap-start") to clarify that we are booting on Xen rather
> than native.
> 
> That's pretty much it, Shannon please confirm.

This is to say that Xen on ARM might only need EFI_RUNTIME.

Re: efi_enabled(EFI_PARAVIRT) use

[Shannon Zhao]

On 2016年04月29日 23:37, Stefano Stabellini wrote:
> On Fri, 29 Apr 2016, Stefano Stabellini wrote:
>> On Fri, 29 Apr 2016, Matt Fleming wrote:
>>> On Fri, 29 Apr, at 11:34:45AM, Stefano Stabellini wrote:
>>>> On Fri, 29 Apr 2016, Ingo Molnar wrote:
>>>>> Also, it would be nice to have all things EFI in a single tree, the conflicts are 
>>>>> going to be painful! There's very little reason not to carry this kind of commit:
>>>>>
>>>>>  arch/arm/xen/enlighten.c           |  6 +++++
>>>>>  drivers/firmware/efi/arm-runtime.c | 17 +++++++++-----
>>>>>  drivers/firmware/efi/efi.c         | 45 ++++++++++++++++++++++++++++++++------
>>>>>  3 files changed, 56 insertions(+), 12 deletions(-)
>>>>>
>>>>> in the EFI tree.
>>>>
>>>> That's true. I'll drop this commit from xentip and let Matt pick it up
>>>> or request changes as he sees fit.
>>>
>>> One small change I think would be sensible to make is to expand
>>> EFI_PARAVIRT into a few more bits to clearly indicate the quirks on
>>> Xen, and in the process, to delete EFI_PARAVIRT.
>>>
>>> That should address Ingo's major concern, and also make it much easier
>>> to rework the code in a piecemeal fashion.
>>>
>>> Could somebody enumerate the things that make Xen (dom0) different on
>>> arm* compared with bare metal EFI boot? The list I made for x86 was,
>>>
>>>   1. Has no EFI memory map
>>>   2. Runtime regions do not need to be mapped
>>>   3. Cannot call SetVirtualAddressMap()
>>>   4. /sys/firmware/efi/fw_vendor is invisible
>>>
>>> The first maps to not setting EFI_MEMMAP, the second to not setting
>>> EFI_RUNTIME. If we add EFI_ALREADY_VIRTUAL and EFI_FW_VENDOR_INVISIBLE
>>> to efi.flags that should cover everything on x86. Does arm* require
>>> anything else?
>>
>> Xen on ARM is different, the impact should be limited:
>>
>> - there are no BootServices (ExitBootServices has already been called)
>> - RuntimeServices go via hypercalls
>>
>> The UEFI memory map is still available at an address specified on device
>> tree like on native, but the compatibility string is different
>> ("xen,uefi-mmap-start") to clarify that we are booting on Xen rather
>> than native.
>>
>> That's pretty much it, Shannon please confirm.
> 
> This is to say that Xen on ARM might only need EFI_RUNTIME.
> 
Yes, it needs EFI_RUNTIME_SERVICES.

Thanks,
-- 
Shannon

Re: efi_enabled(EFI_PARAVIRT) use

[Shannon Zhao]

On 2016年04月29日 22:53, Ard Biesheuvel wrote:
> On 29 April 2016 at 16:39, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>> On Fri, 29 Apr, at 11:34:45AM, Stefano Stabellini wrote:
>>> On Fri, 29 Apr 2016, Ingo Molnar wrote:
>>>> Also, it would be nice to have all things EFI in a single tree, the conflicts are
>>>> going to be painful! There's very little reason not to carry this kind of commit:
>>>>
>>>>  arch/arm/xen/enlighten.c           |  6 +++++
>>>>  drivers/firmware/efi/arm-runtime.c | 17 +++++++++-----
>>>>  drivers/firmware/efi/efi.c         | 45 ++++++++++++++++++++++++++++++++------
>>>>  3 files changed, 56 insertions(+), 12 deletions(-)
>>>>
>>>> in the EFI tree.
>>>
>>> That's true. I'll drop this commit from xentip and let Matt pick it up
>>> or request changes as he sees fit.
>>
>> One small change I think would be sensible to make is to expand
>> EFI_PARAVIRT into a few more bits to clearly indicate the quirks on
>> Xen, and in the process, to delete EFI_PARAVIRT.
>>
Sure. How should I add this change? Rework this patch or add new one on
top of it?

>> That should address Ingo's major concern, and also make it much easier
>> to rework the code in a piecemeal fashion.
>>
>> Could somebody enumerate the things that make Xen (dom0) different on
>> arm* compared with bare metal EFI boot? The list I made for x86 was,
>>
>>   1. Has no EFI memory map
>>   2. Runtime regions do not need to be mapped
>>   3. Cannot call SetVirtualAddressMap()
>>   4. /sys/firmware/efi/fw_vendor is invisible
>>
>> The first maps to not setting EFI_MEMMAP, the second to not setting
>> EFI_RUNTIME. If we add EFI_ALREADY_VIRTUAL and EFI_FW_VENDOR_INVISIBLE
>> to efi.flags that should cover everything on x86. Does arm* require
>> anything else?
> 
> I already proposed when this patch was first under review to make the
> arm_enable_runtime_services() function bail early without error if the
> EFI_RUNTIME_SERVICES flag is already set, and the xen code could set
> that bit as well when it installs its paravirtualized alternatives. I
> don't remember exactly why that was shot down, though, but I think it
> is the only reason this code introduces references to EFI_PARAVIRT in
> the first place.
> 
Yes, in this patch we could set EFI_RUNTIME_SERVICES flag in
fdt_find_hyper_node instead of setting EFI_PARAVIRT flag, and then bail
out early in arm_enable_runtime_services() as you said. Then call
xen_efi_runtime_setup() in xen_guest_init().

While I still have a question, in this patch we use
efi_enabled(EFI_PARAVIRT) as a condition to make fdt_find_uefi_params()
and efi_get_fdt_params() execute different ways. So it needs to find a
new condition for that if we need to get rid of EFI_PARAVIRT. One I
think is that xen_initial_domain() check. Is that fine?

Thanks,
-- 
Shannon

Re: efi_enabled(EFI_PARAVIRT) use

[Matt Fleming]

On Sat, 30 Apr, at 10:14:42PM, Shannon Zhao wrote:
> Sure. How should I add this change? Rework this patch or add new one on
> top of it?
 
Rework this patch, please.

> Yes, in this patch we could set EFI_RUNTIME_SERVICES flag in
> fdt_find_hyper_node instead of setting EFI_PARAVIRT flag, and then bail
> out early in arm_enable_runtime_services() as you said. Then call
> xen_efi_runtime_setup() in xen_guest_init().
 
Sounds good.

> While I still have a question, in this patch we use
> efi_enabled(EFI_PARAVIRT) as a condition to make fdt_find_uefi_params()
> and efi_get_fdt_params() execute different ways. So it needs to find a
> new condition for that if we need to get rid of EFI_PARAVIRT. One I
> think is that xen_initial_domain() check. Is that fine?

Hmm... why do you actually need to check whether you're running on a
PV machine in fdt_find_uefi_params()? Can't you infer that from the DT
params you discover?

I could understand maybe only accepting the "xen,uefi-system-table"
property if IS_ENABLED(CONFIG_XEN) but surely you don't also need to
filter based on whether you're booting a PV kernel?

Let me put it this way: when would you see "xen,uefi-system-table" and
*not* be booting a PV kernel?

Re: efi_enabled(EFI_PARAVIRT) use

[Shannon Zhao]

On 2016年05月01日 04:44, Matt Fleming wrote:
>> While I still have a question, in this patch we use
>> > efi_enabled(EFI_PARAVIRT) as a condition to make fdt_find_uefi_params()
>> > and efi_get_fdt_params() execute different ways. So it needs to find a
>> > new condition for that if we need to get rid of EFI_PARAVIRT. One I
>> > think is that xen_initial_domain() check. Is that fine?
> Hmm... why do you actually need to check whether you're running on a
> PV machine in fdt_find_uefi_params()?
Because the UEFI params for Dom0 are located under /hypervisor/uefi node
instead of /chosen. So it needs to check whether it's a Dom0 then search
and parse different node with different params arrays.

> Can't you infer that from the DT
> params you discover?
> 

> I could understand maybe only accepting the "xen,uefi-system-table"
> property if IS_ENABLED(CONFIG_XEN) but surely you don't also need to
> filter based on whether you're booting a PV kernel?
> 
> Let me put it this way: when would you see "xen,uefi-system-table" and
> *not* be booting a PV kernel?
So it still needs add another check to firstly parse the fdt to see if
there is "xen,uefi-system-table" under /hypervisor/uefi node, right? I
think it's a bit redundant compared with xen_initial_domain().

Thanks,
-- 
Shannon

Re: efi_enabled(EFI_PARAVIRT) use

[Matt Fleming]

On Sun, 01 May, at 11:24:18AM, Shannon Zhao wrote:
> Because the UEFI params for Dom0 are located under /hypervisor/uefi node
> instead of /chosen. So it needs to check whether it's a Dom0 then search
> and parse different node with different params arrays.
 
Why can't you search both nodes? Would you ever expect to see both for
a Dom0 kernel?

> So it still needs add another check to firstly parse the fdt to see if
> there is "xen,uefi-system-table" under /hypervisor/uefi node, right? I
> think it's a bit redundant compared with xen_initial_domain().

Sometimes you really do need to check xen_initial_domain(), but I do
not think this is such a case. And if we can get by without adding
that check, the code will be better for it.

Re: efi_enabled(EFI_PARAVIRT) use

[Shannon Zhao]

On 2016年05月01日 21:26, Matt Fleming wrote:
> On Sun, 01 May, at 11:24:18AM, Shannon Zhao wrote:
>> Because the UEFI params for Dom0 are located under /hypervisor/uefi node
>> instead of /chosen. So it needs to check whether it's a Dom0 then search
>> and parse different node with different params arrays.
>  
> Why can't you search both nodes? 
Before searching, it needs to decide which uefi params array should be
passed to of_scan_flat_dt().

> Would you ever expect to see both for
> a Dom0 kernel?
> 
>> So it still needs add another check to firstly parse the fdt to see if
>> there is "xen,uefi-system-table" under /hypervisor/uefi node, right? I
>> think it's a bit redundant compared with xen_initial_domain().
> 
> Sometimes you really do need to check xen_initial_domain(), but I do
> not think this is such a case. And if we can get by without adding
> that check, the code will be better for it.
> 
So is there any other way you suggest?

Thanks,
-- 
Shannon

Re: efi_enabled(EFI_PARAVIRT) use

[Matt Fleming]

On Sun, 01 May, at 10:36:51PM, Shannon Zhao wrote:
> So is there any other way you suggest?

Would this work (compile tested but not runtime tested)?

---

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 3a69ed5ecfcb..13d8be16447a 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -469,12 +469,14 @@ device_initcall(efi_load_efivars);
 		FIELD_SIZEOF(struct efi_fdt_params, field) \
 	}
 
-static __initdata struct {
+struct params {
 	const char name[32];
 	const char propname[32];
 	int offset;
 	int size;
-} dt_params[] = {
+};
+
+static __initdata struct params fdt_params[] = {
 	UEFI_PARAM("System Table", "linux,uefi-system-table", system_table),
 	UEFI_PARAM("MemMap Address", "linux,uefi-mmap-start", mmap),
 	UEFI_PARAM("MemMap Size", "linux,uefi-mmap-size", mmap_size),
@@ -482,44 +484,83 @@ static __initdata struct {
 	UEFI_PARAM("MemMap Desc. Version", "linux,uefi-mmap-desc-ver", desc_ver)
 };
 
+static __initdata struct params xen_fdt_params[] = {
+	UEFI_PARAM("System Table", "xen,uefi-system-table", system_table),
+	UEFI_PARAM("MemMap Address", "xen,uefi-mmap-start", mmap),
+	UEFI_PARAM("MemMap Size", "xen,uefi-mmap-size", mmap_size),
+	UEFI_PARAM("MemMap Desc. Size", "xen,uefi-mmap-desc-size", desc_size),
+	UEFI_PARAM("MemMap Desc. Version", "xen,uefi-mmap-desc-ver", desc_ver)
+};
+
+#define EFI_FDT_PARAMS_SIZE	ARRAY_SIZE(fdt_params)
+
+static __initdata struct {
+	const char *uname;
+	struct params *params;
+} dt_params[] = {
+	{ "hypervisor", xen_fdt_params },
+	{ "chosen", fdt_params },
+};
+
 struct param_info {
 	int found;
 	void *params;
+	const char *missing;
 };
 
-static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
-				       int depth, void *data)
+static int __init __find_uefi_params(unsigned long node,
+				     struct param_info *info,
+				     struct params *params)
 {
-	struct param_info *info = data;
 	const void *prop;
 	void *dest;
 	u64 val;
 	int i, len;
 
-	if (depth != 1 || strcmp(uname, "chosen") != 0)
-		return 0;
-
-	for (i = 0; i < ARRAY_SIZE(dt_params); i++) {
-		prop = of_get_flat_dt_prop(node, dt_params[i].propname, &len);
-		if (!prop)
+	for (i = 0; i < EFI_FDT_PARAMS_SIZE; i++) {
+		prop = of_get_flat_dt_prop(node, params[i].propname, &len);
+		if (!prop) {
+			info->missing = params[i].name;
 			return 0;
-		dest = info->params + dt_params[i].offset;
+		}
+
+		dest = info->params + params[i].offset;
 		info->found++;
 
 		val = of_read_number(prop, len / sizeof(u32));
 
-		if (dt_params[i].size == sizeof(u32))
+		if (params[i].size == sizeof(u32))
 			*(u32 *)dest = val;
 		else
 			*(u64 *)dest = val;
 
 		if (efi_enabled(EFI_DBG))
-			pr_info("  %s: 0x%0*llx\n", dt_params[i].name,
-				dt_params[i].size * 2, val);
+			pr_info("  %s: 0x%0*llx\n", params[i].name,
+				params[i].size * 2, val);
 	}
+
 	return 1;
 }
 
+static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
+				       int depth, void *data)
+{
+	struct param_info *info = data;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(dt_params); i++) {
+
+		if (depth != 1 || strcmp(uname, dt_params[i].uname) != 0) {
+			info->missing = dt_params[i].params[0].name;
+			continue;
+		}
+
+		return __find_uefi_params(node, info, dt_params[i].params);
+	}
+
+	return 0;
+}
+
 int __init efi_get_fdt_params(struct efi_fdt_params *params)
 {
 	struct param_info info;
@@ -535,7 +576,7 @@ int __init efi_get_fdt_params(struct efi_fdt_params *params)
 		pr_info("UEFI not found.\n");
 	else if (!ret)
 		pr_err("Can't find '%s' in device tree!\n",
-		       dt_params[info.found].name);
+		       info.missing);
 
 	return ret;
 }

Re: [Xen-devel] efi_enabled(EFI_PARAVIRT) use

[Shannon Zhao]

On 2016/5/2 18:45, Matt Fleming wrote:
> On Sun, 01 May, at 10:36:51PM, Shannon Zhao wrote:
>> So is there any other way you suggest?
> 
> Would this work (compile tested but not runtime tested)?
> 
> ---
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 3a69ed5ecfcb..13d8be16447a 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -469,12 +469,14 @@ device_initcall(efi_load_efivars);
>  		FIELD_SIZEOF(struct efi_fdt_params, field) \
>  	}
>  
> -static __initdata struct {
> +struct params {
>  	const char name[32];
>  	const char propname[32];
>  	int offset;
>  	int size;
> -} dt_params[] = {
> +};
> +
> +static __initdata struct params fdt_params[] = {
>  	UEFI_PARAM("System Table", "linux,uefi-system-table", system_table),
>  	UEFI_PARAM("MemMap Address", "linux,uefi-mmap-start", mmap),
>  	UEFI_PARAM("MemMap Size", "linux,uefi-mmap-size", mmap_size),
> @@ -482,44 +484,83 @@ static __initdata struct {
>  	UEFI_PARAM("MemMap Desc. Version", "linux,uefi-mmap-desc-ver", desc_ver)
>  };
>  
> +static __initdata struct params xen_fdt_params[] = {
> +	UEFI_PARAM("System Table", "xen,uefi-system-table", system_table),
> +	UEFI_PARAM("MemMap Address", "xen,uefi-mmap-start", mmap),
> +	UEFI_PARAM("MemMap Size", "xen,uefi-mmap-size", mmap_size),
> +	UEFI_PARAM("MemMap Desc. Size", "xen,uefi-mmap-desc-size", desc_size),
> +	UEFI_PARAM("MemMap Desc. Version", "xen,uefi-mmap-desc-ver", desc_ver)
> +};
> +
> +#define EFI_FDT_PARAMS_SIZE	ARRAY_SIZE(fdt_params)
> +
> +static __initdata struct {
> +	const char *uname;
> +	struct params *params;
> +} dt_params[] = {
> +	{ "hypervisor", xen_fdt_params },
While the uefi params are located under /hypervisor/uefi node not
/hypervisor node.

> +	{ "chosen", fdt_params },
> +};
> +
>  struct param_info {
>  	int found;
>  	void *params;
> +	const char *missing;
>  };
>  
> -static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
> -				       int depth, void *data)
> +static int __init __find_uefi_params(unsigned long node,
> +				     struct param_info *info,
> +				     struct params *params)
>  {
> -	struct param_info *info = data;
>  	const void *prop;
>  	void *dest;
>  	u64 val;
>  	int i, len;
>  
> -	if (depth != 1 || strcmp(uname, "chosen") != 0)
> -		return 0;
> -
> -	for (i = 0; i < ARRAY_SIZE(dt_params); i++) {
> -		prop = of_get_flat_dt_prop(node, dt_params[i].propname, &len);
> -		if (!prop)
> +	for (i = 0; i < EFI_FDT_PARAMS_SIZE; i++) {
> +		prop = of_get_flat_dt_prop(node, params[i].propname, &len);
> +		if (!prop) {
> +			info->missing = params[i].name;
>  			return 0;
> -		dest = info->params + dt_params[i].offset;
> +		}
> +
> +		dest = info->params + params[i].offset;
>  		info->found++;
>  
>  		val = of_read_number(prop, len / sizeof(u32));
>  
> -		if (dt_params[i].size == sizeof(u32))
> +		if (params[i].size == sizeof(u32))
>  			*(u32 *)dest = val;
>  		else
>  			*(u64 *)dest = val;
>  
>  		if (efi_enabled(EFI_DBG))
> -			pr_info("  %s: 0x%0*llx\n", dt_params[i].name,
> -				dt_params[i].size * 2, val);
> +			pr_info("  %s: 0x%0*llx\n", params[i].name,
> +				params[i].size * 2, val);
>  	}
> +
>  	return 1;
>  }
>  
> +static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
> +				       int depth, void *data)
> +{
> +	struct param_info *info = data;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(dt_params); i++) {
> +
> +		if (depth != 1 || strcmp(uname, dt_params[i].uname) != 0) {
> +			info->missing = dt_params[i].params[0].name;
> +			continue;
> +		}
> +
So here it needs to check whether the node is /hypervisor. If so, get
the subnode "uefi". Like below:
if (strcmp(uname, "hypervisor") == 0) {
	offset = of_get_flat_dt_subnode_by_name(node, "uefi");
	if (offset < 0)
		return 0;
	node = offset;
}

> +		return __find_uefi_params(node, info, dt_params[i].params);
> +	}
> +
> +	return 0;
> +}
> +
>  int __init efi_get_fdt_params(struct efi_fdt_params *params)
>  {
>  	struct param_info info;
> @@ -535,7 +576,7 @@ int __init efi_get_fdt_params(struct efi_fdt_params *params)
>  		pr_info("UEFI not found.\n");
>  	else if (!ret)
>  		pr_err("Can't find '%s' in device tree!\n",
> -		       dt_params[info.found].name);
> +		       info.missing);
>  
>  	return ret;
>  }
> 

-- 
Shannon

Re: [Xen-devel] efi_enabled(EFI_PARAVIRT) use

[Shannon Zhao]

On 2016/4/30 22:14, Shannon Zhao wrote:
>> I already proposed when this patch was first under review to make the
>> > arm_enable_runtime_services() function bail early without error if the
>> > EFI_RUNTIME_SERVICES flag is already set, and the xen code could set
>> > that bit as well when it installs its paravirtualized alternatives. I
>> > don't remember exactly why that was shot down, though, but I think it
>> > is the only reason this code introduces references to EFI_PARAVIRT in
>> > the first place.
>> > 
> Yes, in this patch we could set EFI_RUNTIME_SERVICES flag in
> fdt_find_hyper_node instead of setting EFI_PARAVIRT flag, and then bail
> out early in arm_enable_runtime_services() as you said. Then call
> xen_efi_runtime_setup() in xen_guest_init().

Hi Ard,

If it sets EFI_RUNTIME_SERVICES flag in fdt_find_hyper_node and in
arm_enable_runtime_services() it checks whether it's a Dom0 through
xen_init_domain() and the EFI_RUNTIME_SERVICES flag is set, then call
xen_efi_runtime_setup(). Is it ok?

Thanks,
-- 
Shannon

Re: [Xen-devel] efi_enabled(EFI_PARAVIRT) use

[Matt Fleming]

On Tue, 03 May, at 09:45:22AM, Shannon Zhao wrote:
> > +static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
> > +				       int depth, void *data)
> > +{
> > +	struct param_info *info = data;
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(dt_params); i++) {
> > +
> > +		if (depth != 1 || strcmp(uname, dt_params[i].uname) != 0) {
> > +			info->missing = dt_params[i].params[0].name;
> > +			continue;
> > +		}
> > +
> So here it needs to check whether the node is /hypervisor. If so, get
> the subnode "uefi". Like below:
> if (strcmp(uname, "hypervisor") == 0) {
> 	offset = of_get_flat_dt_subnode_by_name(node, "uefi");
> 	if (offset < 0)
> 		return 0;
> 	node = offset;
> }

Urgh, right.

How about giving dt_params a const char *subnode field and doing,

	for (i = 0; i < ARRAY_SIZE(dt_params); i++) {
		const char *subnode = dt_params[i].sub_node;

		if (depth != 1 || strcmp(uname, dt_params[i].uname) != 0) {
			info->missing = dt_params[i].params[0].name;
			continue;
		}

		if (subnode) {
			offset = of_get_flat_dt_subnode_by_name(node, subnode);
			if (offset < 0)
				return 0;
			node = offset;
		}

	...