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);
* Stephen Rothwell <[email protected]> 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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Shannon Zhao <[email protected]>
Reviewed-by: Matt Fleming <[email protected]>
Reviewed-by: Stefano Stabellini <[email protected]>
Tested-by: Julien Grall <[email protected]>
---
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;
}
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.
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.
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.
* Stefano Stabellini <[email protected]> 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
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?
On 29 April 2016 at 16:39, Matt Fleming <[email protected]> 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.
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.
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.
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
On 2016年04月29日 22:53, Ard Biesheuvel wrote:
> On 29 April 2016 at 16:39, Matt Fleming <[email protected]> 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
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?
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