* [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-06 8:32 ` Shannon Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Shannon Zhao @ 2016-05-06 8:32 UTC (permalink / raw)
To: linux-arm-kernel, matt
Cc: catalin.marinas, will.deacon, sstabellini, stefano, julien.grall,
ard.biesheuvel, xen-devel, devicetree, linux-efi, linux-kernel,
shannon.zhao, peter.huangpeng
From: Shannon Zhao <shannon.zhao@linaro.org>
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>
---
Drop using of EFI_PARAVIRT. Discussion can be found from [1].
[1] https://lkml.org/lkml/2016/4/29/8
---
arch/arm/include/asm/efi.h | 2 +
arch/arm/xen/enlighten.c | 16 ++++++++
arch/arm64/include/asm/efi.h | 2 +
drivers/firmware/efi/arm-runtime.c | 70 +++++++++++++++++++++++---------
drivers/firmware/efi/efi.c | 81 ++++++++++++++++++++++++++++++--------
5 files changed, 137 insertions(+), 34 deletions(-)
diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index e0eea72..5ba4343 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -52,9 +52,11 @@ static inline void efi_set_pgd(struct mm_struct *mm)
void efi_virtmap_load(void);
void efi_virtmap_unload(void);
+int xen_arm_enable_runtime_services(void);
#else
#define efi_init()
+#define xen_arm_enable_runtime_services() (0)
#endif /* CONFIG_EFI */
/* arch specific definitions used by the stub code */
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 13e3e9f..3362870 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -16,6 +16,7 @@
#include <asm/xen/hypervisor.h>
#include <asm/xen/hypercall.h>
#include <asm/system_misc.h>
+#include <asm/efi.h>
#include <linux/interrupt.h>
#include <linux/irqreturn.h>
#include <linux/module.h>
@@ -261,6 +262,13 @@ 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) &&
+ !efi_runtime_disabled())
+ set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
+ }
+
return 0;
}
@@ -338,6 +346,7 @@ static int __init xen_guest_init(void)
{
struct xen_add_to_physmap xatp;
struct shared_info *shared_info_page = NULL;
+ int ret;
if (!xen_domain())
return 0;
@@ -352,6 +361,13 @@ static int __init xen_guest_init(void)
return -ENODEV;
}
+ if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_BOOT) &&
+ efi_enabled(EFI_RUNTIME_SERVICES)) {
+ ret = xen_arm_enable_runtime_services();
+ if (ret)
+ return ret;
+ }
+
shared_info_page = (struct shared_info *)get_zeroed_page(GFP_KERNEL);
if (!shared_info_page) {
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 8e88a69..574bee5 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -8,8 +8,10 @@
#ifdef CONFIG_EFI
extern void efi_init(void);
+int xen_arm_enable_runtime_services(void);
#else
#define efi_init()
+#define xen_arm_enable_runtime_services() (0)
#endif
int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
index 6ae21e4..211ec10 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;
@@ -39,6 +40,33 @@ static struct mm_struct efi_mm = {
.mmlist = LIST_HEAD_INIT(efi_mm.mmlist),
};
+static int __init efi_remap_init(void)
+{
+ u64 mapsize;
+
+ 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;
+
+ 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");
+ return -ENOMEM;
+ }
+ set_bit(EFI_SYSTEM_TABLES, &efi.flags);
+
+ return 0;
+}
+
static bool __init efi_virtmap_init(void)
{
efi_memory_desc_t *md;
@@ -75,7 +103,7 @@ static bool __init efi_virtmap_init(void)
*/
static int __init arm_enable_runtime_services(void)
{
- u64 mapsize;
+ int ret;
if (!efi_enabled(EFI_BOOT)) {
pr_info("EFI services will not be available.\n");
@@ -87,25 +115,14 @@ static int __init arm_enable_runtime_services(void)
return 0;
}
- 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;
+ if (efi_enabled(EFI_RUNTIME_SERVICES)) {
+ pr_info("EFI runtime services access via paravirt.\n");
+ return 0;
}
- memmap.map_end = memmap.map + mapsize;
- efi.memmap = &memmap;
- 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");
- return -ENOMEM;
- }
- set_bit(EFI_SYSTEM_TABLES, &efi.flags);
+ ret = efi_remap_init();
+ if (ret)
+ return ret;
if (!efi_virtmap_init()) {
pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
@@ -122,6 +139,23 @@ static int __init arm_enable_runtime_services(void)
}
early_initcall(arm_enable_runtime_services);
+int __init xen_arm_enable_runtime_services(void)
+{
+ int ret;
+
+ ret = efi_remap_init();
+ if (ret)
+ return ret;
+
+ if (IS_ENABLED(CONFIG_XEN_EFI)) {
+ /* Set up runtime services function pointers for Xen Dom0 */
+ xen_efi_runtime_setup();
+ efi.runtime_version = efi.systab->hdr.revision;
+ }
+
+ return 0;
+}
+
void efi_virtmap_load(void)
{
preempt_disable();
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 3a69ed5..f586e1e 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,91 @@ 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;
+ const char *subnode;
+ struct params *params;
+} dt_params[] = {
+ { "hypervisor", "uefi", xen_fdt_params },
+ { "chosen", NULL, 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++) {
+ const char *subnode = dt_params[i].subnode;
+
+ if (depth != 1 || strcmp(uname, dt_params[i].uname) != 0) {
+ info->missing = dt_params[i].params[0].name;
+ continue;
+ }
+
+ if (subnode) {
+ node = of_get_flat_dt_subnode_by_name(node, subnode);
+ if (node < 0)
+ return 0;
+ }
+
+ 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 +584,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;
}
--
2.0.4
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-06 8:32 ` Shannon Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Shannon Zhao @ 2016-05-06 8:32 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
matt-mF/unelCI9GS6iBeEJttW/XRex20P6io
Cc: catalin.marinas-5wv7dgnIgG8, will.deacon-5wv7dgnIgG8,
sstabellini-DgEjT+Ai2ygdnm+yROfE0A,
stefano-yd54mjeZNzVBDgjK7y7TUQ, julien.grall-5wv7dgnIgG8,
ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
xen-devel-GuqFBffKawuEi8DpZVb4nw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-efi-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
shannon.zhao-QSEj5FYQhm4dnm+yROfE0A,
peter.huangpeng-hv44wF8Li93QT0dZR+AlfA
From: Shannon Zhao <shannon.zhao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
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-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Signed-off-by: Shannon Zhao <shannon.zhao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
Drop using of EFI_PARAVIRT. Discussion can be found from [1].
[1] https://lkml.org/lkml/2016/4/29/8
---
arch/arm/include/asm/efi.h | 2 +
arch/arm/xen/enlighten.c | 16 ++++++++
arch/arm64/include/asm/efi.h | 2 +
drivers/firmware/efi/arm-runtime.c | 70 +++++++++++++++++++++++---------
drivers/firmware/efi/efi.c | 81 ++++++++++++++++++++++++++++++--------
5 files changed, 137 insertions(+), 34 deletions(-)
diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index e0eea72..5ba4343 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -52,9 +52,11 @@ static inline void efi_set_pgd(struct mm_struct *mm)
void efi_virtmap_load(void);
void efi_virtmap_unload(void);
+int xen_arm_enable_runtime_services(void);
#else
#define efi_init()
+#define xen_arm_enable_runtime_services() (0)
#endif /* CONFIG_EFI */
/* arch specific definitions used by the stub code */
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 13e3e9f..3362870 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -16,6 +16,7 @@
#include <asm/xen/hypervisor.h>
#include <asm/xen/hypercall.h>
#include <asm/system_misc.h>
+#include <asm/efi.h>
#include <linux/interrupt.h>
#include <linux/irqreturn.h>
#include <linux/module.h>
@@ -261,6 +262,13 @@ 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) &&
+ !efi_runtime_disabled())
+ set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
+ }
+
return 0;
}
@@ -338,6 +346,7 @@ static int __init xen_guest_init(void)
{
struct xen_add_to_physmap xatp;
struct shared_info *shared_info_page = NULL;
+ int ret;
if (!xen_domain())
return 0;
@@ -352,6 +361,13 @@ static int __init xen_guest_init(void)
return -ENODEV;
}
+ if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_BOOT) &&
+ efi_enabled(EFI_RUNTIME_SERVICES)) {
+ ret = xen_arm_enable_runtime_services();
+ if (ret)
+ return ret;
+ }
+
shared_info_page = (struct shared_info *)get_zeroed_page(GFP_KERNEL);
if (!shared_info_page) {
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 8e88a69..574bee5 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -8,8 +8,10 @@
#ifdef CONFIG_EFI
extern void efi_init(void);
+int xen_arm_enable_runtime_services(void);
#else
#define efi_init()
+#define xen_arm_enable_runtime_services() (0)
#endif
int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
index 6ae21e4..211ec10 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;
@@ -39,6 +40,33 @@ static struct mm_struct efi_mm = {
.mmlist = LIST_HEAD_INIT(efi_mm.mmlist),
};
+static int __init efi_remap_init(void)
+{
+ u64 mapsize;
+
+ 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;
+
+ 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");
+ return -ENOMEM;
+ }
+ set_bit(EFI_SYSTEM_TABLES, &efi.flags);
+
+ return 0;
+}
+
static bool __init efi_virtmap_init(void)
{
efi_memory_desc_t *md;
@@ -75,7 +103,7 @@ static bool __init efi_virtmap_init(void)
*/
static int __init arm_enable_runtime_services(void)
{
- u64 mapsize;
+ int ret;
if (!efi_enabled(EFI_BOOT)) {
pr_info("EFI services will not be available.\n");
@@ -87,25 +115,14 @@ static int __init arm_enable_runtime_services(void)
return 0;
}
- 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;
+ if (efi_enabled(EFI_RUNTIME_SERVICES)) {
+ pr_info("EFI runtime services access via paravirt.\n");
+ return 0;
}
- memmap.map_end = memmap.map + mapsize;
- efi.memmap = &memmap;
- 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");
- return -ENOMEM;
- }
- set_bit(EFI_SYSTEM_TABLES, &efi.flags);
+ ret = efi_remap_init();
+ if (ret)
+ return ret;
if (!efi_virtmap_init()) {
pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
@@ -122,6 +139,23 @@ static int __init arm_enable_runtime_services(void)
}
early_initcall(arm_enable_runtime_services);
+int __init xen_arm_enable_runtime_services(void)
+{
+ int ret;
+
+ ret = efi_remap_init();
+ if (ret)
+ return ret;
+
+ if (IS_ENABLED(CONFIG_XEN_EFI)) {
+ /* Set up runtime services function pointers for Xen Dom0 */
+ xen_efi_runtime_setup();
+ efi.runtime_version = efi.systab->hdr.revision;
+ }
+
+ return 0;
+}
+
void efi_virtmap_load(void)
{
preempt_disable();
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 3a69ed5..f586e1e 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,91 @@ 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;
+ const char *subnode;
+ struct params *params;
+} dt_params[] = {
+ { "hypervisor", "uefi", xen_fdt_params },
+ { "chosen", NULL, 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++) {
+ const char *subnode = dt_params[i].subnode;
+
+ if (depth != 1 || strcmp(uname, dt_params[i].uname) != 0) {
+ info->missing = dt_params[i].params[0].name;
+ continue;
+ }
+
+ if (subnode) {
+ node = of_get_flat_dt_subnode_by_name(node, subnode);
+ if (node < 0)
+ return 0;
+ }
+
+ 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 +584,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;
}
--
2.0.4
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-06 8:32 ` Shannon Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Shannon Zhao @ 2016-05-06 8:32 UTC (permalink / raw)
To: linux-arm-kernel
From: Shannon Zhao <shannon.zhao@linaro.org>
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>
---
Drop using of EFI_PARAVIRT. Discussion can be found from [1].
[1] https://lkml.org/lkml/2016/4/29/8
---
arch/arm/include/asm/efi.h | 2 +
arch/arm/xen/enlighten.c | 16 ++++++++
arch/arm64/include/asm/efi.h | 2 +
drivers/firmware/efi/arm-runtime.c | 70 +++++++++++++++++++++++---------
drivers/firmware/efi/efi.c | 81 ++++++++++++++++++++++++++++++--------
5 files changed, 137 insertions(+), 34 deletions(-)
diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index e0eea72..5ba4343 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -52,9 +52,11 @@ static inline void efi_set_pgd(struct mm_struct *mm)
void efi_virtmap_load(void);
void efi_virtmap_unload(void);
+int xen_arm_enable_runtime_services(void);
#else
#define efi_init()
+#define xen_arm_enable_runtime_services() (0)
#endif /* CONFIG_EFI */
/* arch specific definitions used by the stub code */
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 13e3e9f..3362870 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -16,6 +16,7 @@
#include <asm/xen/hypervisor.h>
#include <asm/xen/hypercall.h>
#include <asm/system_misc.h>
+#include <asm/efi.h>
#include <linux/interrupt.h>
#include <linux/irqreturn.h>
#include <linux/module.h>
@@ -261,6 +262,13 @@ 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) &&
+ !efi_runtime_disabled())
+ set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
+ }
+
return 0;
}
@@ -338,6 +346,7 @@ static int __init xen_guest_init(void)
{
struct xen_add_to_physmap xatp;
struct shared_info *shared_info_page = NULL;
+ int ret;
if (!xen_domain())
return 0;
@@ -352,6 +361,13 @@ static int __init xen_guest_init(void)
return -ENODEV;
}
+ if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_BOOT) &&
+ efi_enabled(EFI_RUNTIME_SERVICES)) {
+ ret = xen_arm_enable_runtime_services();
+ if (ret)
+ return ret;
+ }
+
shared_info_page = (struct shared_info *)get_zeroed_page(GFP_KERNEL);
if (!shared_info_page) {
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 8e88a69..574bee5 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -8,8 +8,10 @@
#ifdef CONFIG_EFI
extern void efi_init(void);
+int xen_arm_enable_runtime_services(void);
#else
#define efi_init()
+#define xen_arm_enable_runtime_services() (0)
#endif
int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
index 6ae21e4..211ec10 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;
@@ -39,6 +40,33 @@ static struct mm_struct efi_mm = {
.mmlist = LIST_HEAD_INIT(efi_mm.mmlist),
};
+static int __init efi_remap_init(void)
+{
+ u64 mapsize;
+
+ 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;
+
+ 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");
+ return -ENOMEM;
+ }
+ set_bit(EFI_SYSTEM_TABLES, &efi.flags);
+
+ return 0;
+}
+
static bool __init efi_virtmap_init(void)
{
efi_memory_desc_t *md;
@@ -75,7 +103,7 @@ static bool __init efi_virtmap_init(void)
*/
static int __init arm_enable_runtime_services(void)
{
- u64 mapsize;
+ int ret;
if (!efi_enabled(EFI_BOOT)) {
pr_info("EFI services will not be available.\n");
@@ -87,25 +115,14 @@ static int __init arm_enable_runtime_services(void)
return 0;
}
- 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;
+ if (efi_enabled(EFI_RUNTIME_SERVICES)) {
+ pr_info("EFI runtime services access via paravirt.\n");
+ return 0;
}
- memmap.map_end = memmap.map + mapsize;
- efi.memmap = &memmap;
- 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");
- return -ENOMEM;
- }
- set_bit(EFI_SYSTEM_TABLES, &efi.flags);
+ ret = efi_remap_init();
+ if (ret)
+ return ret;
if (!efi_virtmap_init()) {
pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
@@ -122,6 +139,23 @@ static int __init arm_enable_runtime_services(void)
}
early_initcall(arm_enable_runtime_services);
+int __init xen_arm_enable_runtime_services(void)
+{
+ int ret;
+
+ ret = efi_remap_init();
+ if (ret)
+ return ret;
+
+ if (IS_ENABLED(CONFIG_XEN_EFI)) {
+ /* Set up runtime services function pointers for Xen Dom0 */
+ xen_efi_runtime_setup();
+ efi.runtime_version = efi.systab->hdr.revision;
+ }
+
+ return 0;
+}
+
void efi_virtmap_load(void)
{
preempt_disable();
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 3a69ed5..f586e1e 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,91 @@ 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;
+ const char *subnode;
+ struct params *params;
+} dt_params[] = {
+ { "hypervisor", "uefi", xen_fdt_params },
+ { "chosen", NULL, 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++) {
+ const char *subnode = dt_params[i].subnode;
+
+ if (depth != 1 || strcmp(uname, dt_params[i].uname) != 0) {
+ info->missing = dt_params[i].params[0].name;
+ continue;
+ }
+
+ if (subnode) {
+ node = of_get_flat_dt_subnode_by_name(node, subnode);
+ if (node < 0)
+ return 0;
+ }
+
+ 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 +584,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;
}
--
2.0.4
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-06 15:52 ` Mathieu Poirier
0 siblings, 0 replies; 35+ messages in thread
From: Mathieu Poirier @ 2016-05-06 15:52 UTC (permalink / raw)
To: Shannon Zhao
Cc: linux-arm-kernel, matt, Catalin Marinas, Will Deacon,
sstabellini, stefano, julien.grall, ard.biesheuvel, xen-devel,
devicetree, linux-efi, linux-kernel, shannon.zhao,
peter.huangpeng
On 6 May 2016 at 02:32, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> 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>
> ---
> Drop using of EFI_PARAVIRT. Discussion can be found from [1].
> [1] https://lkml.org/lkml/2016/4/29/8
> ---
> arch/arm/include/asm/efi.h | 2 +
> arch/arm/xen/enlighten.c | 16 ++++++++
> arch/arm64/include/asm/efi.h | 2 +
> drivers/firmware/efi/arm-runtime.c | 70 +++++++++++++++++++++++---------
> drivers/firmware/efi/efi.c | 81 ++++++++++++++++++++++++++++++--------
> 5 files changed, 137 insertions(+), 34 deletions(-)
>
> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
> index e0eea72..5ba4343 100644
> --- a/arch/arm/include/asm/efi.h
> +++ b/arch/arm/include/asm/efi.h
> @@ -52,9 +52,11 @@ static inline void efi_set_pgd(struct mm_struct *mm)
>
> void efi_virtmap_load(void);
> void efi_virtmap_unload(void);
> +int xen_arm_enable_runtime_services(void);
>
> #else
> #define efi_init()
> +#define xen_arm_enable_runtime_services() (0)
> #endif /* CONFIG_EFI */
>
> /* arch specific definitions used by the stub code */
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 13e3e9f..3362870 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -16,6 +16,7 @@
> #include <asm/xen/hypervisor.h>
> #include <asm/xen/hypercall.h>
> #include <asm/system_misc.h>
> +#include <asm/efi.h>
> #include <linux/interrupt.h>
> #include <linux/irqreturn.h>
> #include <linux/module.h>
> @@ -261,6 +262,13 @@ 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) &&
> + !efi_runtime_disabled())
> + set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> + }
> +
> return 0;
> }
>
> @@ -338,6 +346,7 @@ static int __init xen_guest_init(void)
> {
> struct xen_add_to_physmap xatp;
> struct shared_info *shared_info_page = NULL;
> + int ret;
>
> if (!xen_domain())
> return 0;
> @@ -352,6 +361,13 @@ static int __init xen_guest_init(void)
> return -ENODEV;
> }
>
> + if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_BOOT) &&
> + efi_enabled(EFI_RUNTIME_SERVICES)) {
> + ret = xen_arm_enable_runtime_services();
> + if (ret)
> + return ret;
> + }
> +
> shared_info_page = (struct shared_info *)get_zeroed_page(GFP_KERNEL);
>
> if (!shared_info_page) {
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index 8e88a69..574bee5 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -8,8 +8,10 @@
>
> #ifdef CONFIG_EFI
> extern void efi_init(void);
> +int xen_arm_enable_runtime_services(void);
> #else
> #define efi_init()
> +#define xen_arm_enable_runtime_services() (0)
> #endif
>
> int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
> diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
> index 6ae21e4..211ec10 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;
>
> @@ -39,6 +40,33 @@ static struct mm_struct efi_mm = {
> .mmlist = LIST_HEAD_INIT(efi_mm.mmlist),
> };
>
> +static int __init efi_remap_init(void)
> +{
> + u64 mapsize;
> +
> + 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;
> +
> + efi.systab = (__force void *)ioremap_cache(efi_system_table,
> + sizeof(efi_system_table_t));
> + if (!efi.systab) {
Is there a reason why memmap.map isn't iounmap() in the error path?
The original code didn't have it either, hence the question.
Thanks,
Mathieu
> + pr_err("Failed to remap EFI System Table\n");
> + return -ENOMEM;
> + }
> + set_bit(EFI_SYSTEM_TABLES, &efi.flags);
> +
> + return 0;
> +}
> +
> static bool __init efi_virtmap_init(void)
> {
> efi_memory_desc_t *md;
> @@ -75,7 +103,7 @@ static bool __init efi_virtmap_init(void)
> */
> static int __init arm_enable_runtime_services(void)
> {
> - u64 mapsize;
> + int ret;
>
> if (!efi_enabled(EFI_BOOT)) {
> pr_info("EFI services will not be available.\n");
> @@ -87,25 +115,14 @@ static int __init arm_enable_runtime_services(void)
> return 0;
> }
>
> - 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;
> + if (efi_enabled(EFI_RUNTIME_SERVICES)) {
> + pr_info("EFI runtime services access via paravirt.\n");
> + return 0;
> }
> - memmap.map_end = memmap.map + mapsize;
> - efi.memmap = &memmap;
>
> - 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");
> - return -ENOMEM;
> - }
> - set_bit(EFI_SYSTEM_TABLES, &efi.flags);
> + ret = efi_remap_init();
> + if (ret)
> + return ret;
>
> if (!efi_virtmap_init()) {
> pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
> @@ -122,6 +139,23 @@ static int __init arm_enable_runtime_services(void)
> }
> early_initcall(arm_enable_runtime_services);
>
> +int __init xen_arm_enable_runtime_services(void)
> +{
> + int ret;
> +
> + ret = efi_remap_init();
> + if (ret)
> + return ret;
> +
> + if (IS_ENABLED(CONFIG_XEN_EFI)) {
> + /* Set up runtime services function pointers for Xen Dom0 */
> + xen_efi_runtime_setup();
> + efi.runtime_version = efi.systab->hdr.revision;
> + }
> +
> + return 0;
> +}
> +
> void efi_virtmap_load(void)
> {
> preempt_disable();
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 3a69ed5..f586e1e 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,91 @@ 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;
> + const char *subnode;
> + struct params *params;
> +} dt_params[] = {
> + { "hypervisor", "uefi", xen_fdt_params },
> + { "chosen", NULL, 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++) {
> + const char *subnode = dt_params[i].subnode;
> +
> + if (depth != 1 || strcmp(uname, dt_params[i].uname) != 0) {
> + info->missing = dt_params[i].params[0].name;
> + continue;
> + }
> +
> + if (subnode) {
> + node = of_get_flat_dt_subnode_by_name(node, subnode);
> + if (node < 0)
> + return 0;
> + }
> +
> + 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 +584,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;
> }
> --
> 2.0.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-06 15:52 ` Mathieu Poirier
0 siblings, 0 replies; 35+ messages in thread
From: Mathieu Poirier @ 2016-05-06 15:52 UTC (permalink / raw)
To: Shannon Zhao
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
matt-mF/unelCI9GS6iBeEJttW/XRex20P6io, Catalin Marinas,
Will Deacon, sstabellini-DgEjT+Ai2ygdnm+yROfE0A,
stefano-yd54mjeZNzVBDgjK7y7TUQ, julien.grall-5wv7dgnIgG8,
ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
xen-devel-GuqFBffKawuEi8DpZVb4nw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-efi-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
shannon.zhao-QSEj5FYQhm4dnm+yROfE0A,
peter.huangpeng-hv44wF8Li93QT0dZR+AlfA
On 6 May 2016 at 02:32, Shannon Zhao <zhaoshenglong-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> wrote:
> From: Shannon Zhao <shannon.zhao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> 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-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> Signed-off-by: Shannon Zhao <shannon.zhao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> Drop using of EFI_PARAVIRT. Discussion can be found from [1].
> [1] https://lkml.org/lkml/2016/4/29/8
> ---
> arch/arm/include/asm/efi.h | 2 +
> arch/arm/xen/enlighten.c | 16 ++++++++
> arch/arm64/include/asm/efi.h | 2 +
> drivers/firmware/efi/arm-runtime.c | 70 +++++++++++++++++++++++---------
> drivers/firmware/efi/efi.c | 81 ++++++++++++++++++++++++++++++--------
> 5 files changed, 137 insertions(+), 34 deletions(-)
>
> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
> index e0eea72..5ba4343 100644
> --- a/arch/arm/include/asm/efi.h
> +++ b/arch/arm/include/asm/efi.h
> @@ -52,9 +52,11 @@ static inline void efi_set_pgd(struct mm_struct *mm)
>
> void efi_virtmap_load(void);
> void efi_virtmap_unload(void);
> +int xen_arm_enable_runtime_services(void);
>
> #else
> #define efi_init()
> +#define xen_arm_enable_runtime_services() (0)
> #endif /* CONFIG_EFI */
>
> /* arch specific definitions used by the stub code */
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 13e3e9f..3362870 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -16,6 +16,7 @@
> #include <asm/xen/hypervisor.h>
> #include <asm/xen/hypercall.h>
> #include <asm/system_misc.h>
> +#include <asm/efi.h>
> #include <linux/interrupt.h>
> #include <linux/irqreturn.h>
> #include <linux/module.h>
> @@ -261,6 +262,13 @@ 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) &&
> + !efi_runtime_disabled())
> + set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> + }
> +
> return 0;
> }
>
> @@ -338,6 +346,7 @@ static int __init xen_guest_init(void)
> {
> struct xen_add_to_physmap xatp;
> struct shared_info *shared_info_page = NULL;
> + int ret;
>
> if (!xen_domain())
> return 0;
> @@ -352,6 +361,13 @@ static int __init xen_guest_init(void)
> return -ENODEV;
> }
>
> + if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_BOOT) &&
> + efi_enabled(EFI_RUNTIME_SERVICES)) {
> + ret = xen_arm_enable_runtime_services();
> + if (ret)
> + return ret;
> + }
> +
> shared_info_page = (struct shared_info *)get_zeroed_page(GFP_KERNEL);
>
> if (!shared_info_page) {
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index 8e88a69..574bee5 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -8,8 +8,10 @@
>
> #ifdef CONFIG_EFI
> extern void efi_init(void);
> +int xen_arm_enable_runtime_services(void);
> #else
> #define efi_init()
> +#define xen_arm_enable_runtime_services() (0)
> #endif
>
> int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
> diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
> index 6ae21e4..211ec10 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;
>
> @@ -39,6 +40,33 @@ static struct mm_struct efi_mm = {
> .mmlist = LIST_HEAD_INIT(efi_mm.mmlist),
> };
>
> +static int __init efi_remap_init(void)
> +{
> + u64 mapsize;
> +
> + 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;
> +
> + efi.systab = (__force void *)ioremap_cache(efi_system_table,
> + sizeof(efi_system_table_t));
> + if (!efi.systab) {
Is there a reason why memmap.map isn't iounmap() in the error path?
The original code didn't have it either, hence the question.
Thanks,
Mathieu
> + pr_err("Failed to remap EFI System Table\n");
> + return -ENOMEM;
> + }
> + set_bit(EFI_SYSTEM_TABLES, &efi.flags);
> +
> + return 0;
> +}
> +
> static bool __init efi_virtmap_init(void)
> {
> efi_memory_desc_t *md;
> @@ -75,7 +103,7 @@ static bool __init efi_virtmap_init(void)
> */
> static int __init arm_enable_runtime_services(void)
> {
> - u64 mapsize;
> + int ret;
>
> if (!efi_enabled(EFI_BOOT)) {
> pr_info("EFI services will not be available.\n");
> @@ -87,25 +115,14 @@ static int __init arm_enable_runtime_services(void)
> return 0;
> }
>
> - 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;
> + if (efi_enabled(EFI_RUNTIME_SERVICES)) {
> + pr_info("EFI runtime services access via paravirt.\n");
> + return 0;
> }
> - memmap.map_end = memmap.map + mapsize;
> - efi.memmap = &memmap;
>
> - 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");
> - return -ENOMEM;
> - }
> - set_bit(EFI_SYSTEM_TABLES, &efi.flags);
> + ret = efi_remap_init();
> + if (ret)
> + return ret;
>
> if (!efi_virtmap_init()) {
> pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
> @@ -122,6 +139,23 @@ static int __init arm_enable_runtime_services(void)
> }
> early_initcall(arm_enable_runtime_services);
>
> +int __init xen_arm_enable_runtime_services(void)
> +{
> + int ret;
> +
> + ret = efi_remap_init();
> + if (ret)
> + return ret;
> +
> + if (IS_ENABLED(CONFIG_XEN_EFI)) {
> + /* Set up runtime services function pointers for Xen Dom0 */
> + xen_efi_runtime_setup();
> + efi.runtime_version = efi.systab->hdr.revision;
> + }
> +
> + return 0;
> +}
> +
> void efi_virtmap_load(void)
> {
> preempt_disable();
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 3a69ed5..f586e1e 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,91 @@ 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;
> + const char *subnode;
> + struct params *params;
> +} dt_params[] = {
> + { "hypervisor", "uefi", xen_fdt_params },
> + { "chosen", NULL, 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++) {
> + const char *subnode = dt_params[i].subnode;
> +
> + if (depth != 1 || strcmp(uname, dt_params[i].uname) != 0) {
> + info->missing = dt_params[i].params[0].name;
> + continue;
> + }
> +
> + if (subnode) {
> + node = of_get_flat_dt_subnode_by_name(node, subnode);
> + if (node < 0)
> + return 0;
> + }
> +
> + 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 +584,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;
> }
> --
> 2.0.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-06 15:52 ` Mathieu Poirier
0 siblings, 0 replies; 35+ messages in thread
From: Mathieu Poirier @ 2016-05-06 15:52 UTC (permalink / raw)
To: linux-arm-kernel
On 6 May 2016 at 02:32, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> 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>
> ---
> Drop using of EFI_PARAVIRT. Discussion can be found from [1].
> [1] https://lkml.org/lkml/2016/4/29/8
> ---
> arch/arm/include/asm/efi.h | 2 +
> arch/arm/xen/enlighten.c | 16 ++++++++
> arch/arm64/include/asm/efi.h | 2 +
> drivers/firmware/efi/arm-runtime.c | 70 +++++++++++++++++++++++---------
> drivers/firmware/efi/efi.c | 81 ++++++++++++++++++++++++++++++--------
> 5 files changed, 137 insertions(+), 34 deletions(-)
>
> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
> index e0eea72..5ba4343 100644
> --- a/arch/arm/include/asm/efi.h
> +++ b/arch/arm/include/asm/efi.h
> @@ -52,9 +52,11 @@ static inline void efi_set_pgd(struct mm_struct *mm)
>
> void efi_virtmap_load(void);
> void efi_virtmap_unload(void);
> +int xen_arm_enable_runtime_services(void);
>
> #else
> #define efi_init()
> +#define xen_arm_enable_runtime_services() (0)
> #endif /* CONFIG_EFI */
>
> /* arch specific definitions used by the stub code */
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 13e3e9f..3362870 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -16,6 +16,7 @@
> #include <asm/xen/hypervisor.h>
> #include <asm/xen/hypercall.h>
> #include <asm/system_misc.h>
> +#include <asm/efi.h>
> #include <linux/interrupt.h>
> #include <linux/irqreturn.h>
> #include <linux/module.h>
> @@ -261,6 +262,13 @@ 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) &&
> + !efi_runtime_disabled())
> + set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> + }
> +
> return 0;
> }
>
> @@ -338,6 +346,7 @@ static int __init xen_guest_init(void)
> {
> struct xen_add_to_physmap xatp;
> struct shared_info *shared_info_page = NULL;
> + int ret;
>
> if (!xen_domain())
> return 0;
> @@ -352,6 +361,13 @@ static int __init xen_guest_init(void)
> return -ENODEV;
> }
>
> + if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_BOOT) &&
> + efi_enabled(EFI_RUNTIME_SERVICES)) {
> + ret = xen_arm_enable_runtime_services();
> + if (ret)
> + return ret;
> + }
> +
> shared_info_page = (struct shared_info *)get_zeroed_page(GFP_KERNEL);
>
> if (!shared_info_page) {
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index 8e88a69..574bee5 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -8,8 +8,10 @@
>
> #ifdef CONFIG_EFI
> extern void efi_init(void);
> +int xen_arm_enable_runtime_services(void);
> #else
> #define efi_init()
> +#define xen_arm_enable_runtime_services() (0)
> #endif
>
> int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
> diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
> index 6ae21e4..211ec10 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;
>
> @@ -39,6 +40,33 @@ static struct mm_struct efi_mm = {
> .mmlist = LIST_HEAD_INIT(efi_mm.mmlist),
> };
>
> +static int __init efi_remap_init(void)
> +{
> + u64 mapsize;
> +
> + 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;
> +
> + efi.systab = (__force void *)ioremap_cache(efi_system_table,
> + sizeof(efi_system_table_t));
> + if (!efi.systab) {
Is there a reason why memmap.map isn't iounmap() in the error path?
The original code didn't have it either, hence the question.
Thanks,
Mathieu
> + pr_err("Failed to remap EFI System Table\n");
> + return -ENOMEM;
> + }
> + set_bit(EFI_SYSTEM_TABLES, &efi.flags);
> +
> + return 0;
> +}
> +
> static bool __init efi_virtmap_init(void)
> {
> efi_memory_desc_t *md;
> @@ -75,7 +103,7 @@ static bool __init efi_virtmap_init(void)
> */
> static int __init arm_enable_runtime_services(void)
> {
> - u64 mapsize;
> + int ret;
>
> if (!efi_enabled(EFI_BOOT)) {
> pr_info("EFI services will not be available.\n");
> @@ -87,25 +115,14 @@ static int __init arm_enable_runtime_services(void)
> return 0;
> }
>
> - 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;
> + if (efi_enabled(EFI_RUNTIME_SERVICES)) {
> + pr_info("EFI runtime services access via paravirt.\n");
> + return 0;
> }
> - memmap.map_end = memmap.map + mapsize;
> - efi.memmap = &memmap;
>
> - 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");
> - return -ENOMEM;
> - }
> - set_bit(EFI_SYSTEM_TABLES, &efi.flags);
> + ret = efi_remap_init();
> + if (ret)
> + return ret;
>
> if (!efi_virtmap_init()) {
> pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
> @@ -122,6 +139,23 @@ static int __init arm_enable_runtime_services(void)
> }
> early_initcall(arm_enable_runtime_services);
>
> +int __init xen_arm_enable_runtime_services(void)
> +{
> + int ret;
> +
> + ret = efi_remap_init();
> + if (ret)
> + return ret;
> +
> + if (IS_ENABLED(CONFIG_XEN_EFI)) {
> + /* Set up runtime services function pointers for Xen Dom0 */
> + xen_efi_runtime_setup();
> + efi.runtime_version = efi.systab->hdr.revision;
> + }
> +
> + return 0;
> +}
> +
> void efi_virtmap_load(void)
> {
> preempt_disable();
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 3a69ed5..f586e1e 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,91 @@ 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;
> + const char *subnode;
> + struct params *params;
> +} dt_params[] = {
> + { "hypervisor", "uefi", xen_fdt_params },
> + { "chosen", NULL, 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++) {
> + const char *subnode = dt_params[i].subnode;
> +
> + if (depth != 1 || strcmp(uname, dt_params[i].uname) != 0) {
> + info->missing = dt_params[i].params[0].name;
> + continue;
> + }
> +
> + if (subnode) {
> + node = of_get_flat_dt_subnode_by_name(node, subnode);
> + if (node < 0)
> + return 0;
> + }
> +
> + 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 +584,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;
> }
> --
> 2.0.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
2016-05-06 8:32 ` Shannon Zhao
` (2 preceding siblings ...)
(?)
@ 2016-05-06 15:52 ` Mathieu Poirier
-1 siblings, 0 replies; 35+ messages in thread
From: Mathieu Poirier @ 2016-05-06 15:52 UTC (permalink / raw)
To: Shannon Zhao
Cc: devicetree, sstabellini, ard.biesheuvel, matt, Catalin Marinas,
Will Deacon, linux-kernel, xen-devel, stefano, julien.grall,
linux-efi, shannon.zhao, peter.huangpeng, linux-arm-kernel
On 6 May 2016 at 02:32, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> 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>
> ---
> Drop using of EFI_PARAVIRT. Discussion can be found from [1].
> [1] https://lkml.org/lkml/2016/4/29/8
> ---
> arch/arm/include/asm/efi.h | 2 +
> arch/arm/xen/enlighten.c | 16 ++++++++
> arch/arm64/include/asm/efi.h | 2 +
> drivers/firmware/efi/arm-runtime.c | 70 +++++++++++++++++++++++---------
> drivers/firmware/efi/efi.c | 81 ++++++++++++++++++++++++++++++--------
> 5 files changed, 137 insertions(+), 34 deletions(-)
>
> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
> index e0eea72..5ba4343 100644
> --- a/arch/arm/include/asm/efi.h
> +++ b/arch/arm/include/asm/efi.h
> @@ -52,9 +52,11 @@ static inline void efi_set_pgd(struct mm_struct *mm)
>
> void efi_virtmap_load(void);
> void efi_virtmap_unload(void);
> +int xen_arm_enable_runtime_services(void);
>
> #else
> #define efi_init()
> +#define xen_arm_enable_runtime_services() (0)
> #endif /* CONFIG_EFI */
>
> /* arch specific definitions used by the stub code */
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 13e3e9f..3362870 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -16,6 +16,7 @@
> #include <asm/xen/hypervisor.h>
> #include <asm/xen/hypercall.h>
> #include <asm/system_misc.h>
> +#include <asm/efi.h>
> #include <linux/interrupt.h>
> #include <linux/irqreturn.h>
> #include <linux/module.h>
> @@ -261,6 +262,13 @@ 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) &&
> + !efi_runtime_disabled())
> + set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> + }
> +
> return 0;
> }
>
> @@ -338,6 +346,7 @@ static int __init xen_guest_init(void)
> {
> struct xen_add_to_physmap xatp;
> struct shared_info *shared_info_page = NULL;
> + int ret;
>
> if (!xen_domain())
> return 0;
> @@ -352,6 +361,13 @@ static int __init xen_guest_init(void)
> return -ENODEV;
> }
>
> + if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_BOOT) &&
> + efi_enabled(EFI_RUNTIME_SERVICES)) {
> + ret = xen_arm_enable_runtime_services();
> + if (ret)
> + return ret;
> + }
> +
> shared_info_page = (struct shared_info *)get_zeroed_page(GFP_KERNEL);
>
> if (!shared_info_page) {
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index 8e88a69..574bee5 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -8,8 +8,10 @@
>
> #ifdef CONFIG_EFI
> extern void efi_init(void);
> +int xen_arm_enable_runtime_services(void);
> #else
> #define efi_init()
> +#define xen_arm_enable_runtime_services() (0)
> #endif
>
> int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
> diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
> index 6ae21e4..211ec10 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;
>
> @@ -39,6 +40,33 @@ static struct mm_struct efi_mm = {
> .mmlist = LIST_HEAD_INIT(efi_mm.mmlist),
> };
>
> +static int __init efi_remap_init(void)
> +{
> + u64 mapsize;
> +
> + 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;
> +
> + efi.systab = (__force void *)ioremap_cache(efi_system_table,
> + sizeof(efi_system_table_t));
> + if (!efi.systab) {
Is there a reason why memmap.map isn't iounmap() in the error path?
The original code didn't have it either, hence the question.
Thanks,
Mathieu
> + pr_err("Failed to remap EFI System Table\n");
> + return -ENOMEM;
> + }
> + set_bit(EFI_SYSTEM_TABLES, &efi.flags);
> +
> + return 0;
> +}
> +
> static bool __init efi_virtmap_init(void)
> {
> efi_memory_desc_t *md;
> @@ -75,7 +103,7 @@ static bool __init efi_virtmap_init(void)
> */
> static int __init arm_enable_runtime_services(void)
> {
> - u64 mapsize;
> + int ret;
>
> if (!efi_enabled(EFI_BOOT)) {
> pr_info("EFI services will not be available.\n");
> @@ -87,25 +115,14 @@ static int __init arm_enable_runtime_services(void)
> return 0;
> }
>
> - 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;
> + if (efi_enabled(EFI_RUNTIME_SERVICES)) {
> + pr_info("EFI runtime services access via paravirt.\n");
> + return 0;
> }
> - memmap.map_end = memmap.map + mapsize;
> - efi.memmap = &memmap;
>
> - 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");
> - return -ENOMEM;
> - }
> - set_bit(EFI_SYSTEM_TABLES, &efi.flags);
> + ret = efi_remap_init();
> + if (ret)
> + return ret;
>
> if (!efi_virtmap_init()) {
> pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
> @@ -122,6 +139,23 @@ static int __init arm_enable_runtime_services(void)
> }
> early_initcall(arm_enable_runtime_services);
>
> +int __init xen_arm_enable_runtime_services(void)
> +{
> + int ret;
> +
> + ret = efi_remap_init();
> + if (ret)
> + return ret;
> +
> + if (IS_ENABLED(CONFIG_XEN_EFI)) {
> + /* Set up runtime services function pointers for Xen Dom0 */
> + xen_efi_runtime_setup();
> + efi.runtime_version = efi.systab->hdr.revision;
> + }
> +
> + return 0;
> +}
> +
> void efi_virtmap_load(void)
> {
> preempt_disable();
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 3a69ed5..f586e1e 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,91 @@ 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;
> + const char *subnode;
> + struct params *params;
> +} dt_params[] = {
> + { "hypervisor", "uefi", xen_fdt_params },
> + { "chosen", NULL, 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++) {
> + const char *subnode = dt_params[i].subnode;
> +
> + if (depth != 1 || strcmp(uname, dt_params[i].uname) != 0) {
> + info->missing = dt_params[i].params[0].name;
> + continue;
> + }
> +
> + if (subnode) {
> + node = of_get_flat_dt_subnode_by_name(node, subnode);
> + if (node < 0)
> + return 0;
> + }
> +
> + 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 +584,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;
> }
> --
> 2.0.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-11 12:27 ` Matt Fleming
0 siblings, 0 replies; 35+ messages in thread
From: Matt Fleming @ 2016-05-11 12:27 UTC (permalink / raw)
To: Shannon Zhao
Cc: linux-arm-kernel, catalin.marinas, will.deacon, sstabellini,
stefano, julien.grall, ard.biesheuvel, xen-devel, devicetree,
linux-efi, linux-kernel, shannon.zhao, peter.huangpeng
On Fri, 06 May, at 04:32:06PM, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> 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.
This commit log would benefit from a little expansion. I'd like to see
information that answers the following questions,
- How do the Xen DT paramters differ from bare metal?
- What existing code is reused with your patch?
- How are Xen runtime services different from bare metal?
- Why is it OK to force enable EFI runtime services for Xen?
I think it would also be good to explicitly state that we do not
expect to find both Xen EFI DT parameters and bare metal EFI DT
parameters when performing the search; the two should be mutually
exclusive.
> CC: Matt Fleming <matt@codeblueprint.co.uk>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
> Drop using of EFI_PARAVIRT. Discussion can be found from [1].
> [1] https://lkml.org/lkml/2016/4/29/8
> ---
> arch/arm/include/asm/efi.h | 2 +
> arch/arm/xen/enlighten.c | 16 ++++++++
> arch/arm64/include/asm/efi.h | 2 +
> drivers/firmware/efi/arm-runtime.c | 70 +++++++++++++++++++++++---------
> drivers/firmware/efi/efi.c | 81 ++++++++++++++++++++++++++++++--------
> 5 files changed, 137 insertions(+), 34 deletions(-)
>
> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
> index e0eea72..5ba4343 100644
> --- a/arch/arm/include/asm/efi.h
> +++ b/arch/arm/include/asm/efi.h
> @@ -52,9 +52,11 @@ static inline void efi_set_pgd(struct mm_struct *mm)
>
> void efi_virtmap_load(void);
> void efi_virtmap_unload(void);
> +int xen_arm_enable_runtime_services(void);
>
> #else
> #define efi_init()
> +#define xen_arm_enable_runtime_services() (0)
> #endif /* CONFIG_EFI */
>
> /* arch specific definitions used by the stub code */
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 13e3e9f..3362870 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -16,6 +16,7 @@
> #include <asm/xen/hypervisor.h>
> #include <asm/xen/hypercall.h>
> #include <asm/system_misc.h>
> +#include <asm/efi.h>
> #include <linux/interrupt.h>
> #include <linux/irqreturn.h>
> #include <linux/module.h>
> @@ -261,6 +262,13 @@ 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) &&
> + !efi_runtime_disabled())
> + set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> + }
> +
> return 0;
> }
>
The above comment could do with including similar information to the
commit log as to why we want to force enable runtime services. For x86
we have this,
*
* When EFI_PARAVIRT is in force then we could not map runtime
* service memory region because we do not have direct access to it.
* However, runtime services are available through proxy functions
* (e.g. in case of Xen dom0 EFI implementation they call special
* hypercall which executes relevant EFI functions) and that is why
* they are always enabled.
*/
We need something similar here.
> @@ -338,6 +346,7 @@ static int __init xen_guest_init(void)
> {
> struct xen_add_to_physmap xatp;
> struct shared_info *shared_info_page = NULL;
> + int ret;
>
> if (!xen_domain())
> return 0;
> @@ -352,6 +361,13 @@ static int __init xen_guest_init(void)
> return -ENODEV;
> }
>
> + if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_BOOT) &&
> + efi_enabled(EFI_RUNTIME_SERVICES)) {
> + ret = xen_arm_enable_runtime_services();
> + if (ret)
> + return ret;
> + }
> +
Is it ever possible to have EFI_RUNTIME_SERVICES set but
efi_enabled(EFI_BOOT) and IS_ENABLED(CONFIG_XEN_EFI) be false inside
this function? If the answer is "no", I'd suggest just reducing this
down to,
/*
* The fdt parsing code will have set EFI_RUNTIME_SERVICES if
* it found Xen EFI parameters. Force enable runtime services.
*/
if (efi_enabled(EFI_RUNTIME_SERVICES)) {
ret = xen_arm_enable_runtime_services();
if (ret)
return ret;
}
But first, see my comments below.
> @@ -39,6 +40,33 @@ static struct mm_struct efi_mm = {
> .mmlist = LIST_HEAD_INIT(efi_mm.mmlist),
> };
>
> +static int __init efi_remap_init(void)
> +{
> + u64 mapsize;
> +
> + 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;
> +
> + 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");
> + return -ENOMEM;
> + }
> + set_bit(EFI_SYSTEM_TABLES, &efi.flags);
> +
> + return 0;
> +}
> +
> static bool __init efi_virtmap_init(void)
> {
> efi_memory_desc_t *md;
> @@ -75,7 +103,7 @@ static bool __init efi_virtmap_init(void)
> */
> static int __init arm_enable_runtime_services(void)
> {
> - u64 mapsize;
> + int ret;
>
> if (!efi_enabled(EFI_BOOT)) {
> pr_info("EFI services will not be available.\n");
> @@ -87,25 +115,14 @@ static int __init arm_enable_runtime_services(void)
> return 0;
> }
>
> - 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;
> + if (efi_enabled(EFI_RUNTIME_SERVICES)) {
> + pr_info("EFI runtime services access via paravirt.\n");
> + return 0;
> }
> - memmap.map_end = memmap.map + mapsize;
> - efi.memmap = &memmap;
>
> - 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");
> - return -ENOMEM;
> - }
> - set_bit(EFI_SYSTEM_TABLES, &efi.flags);
> + ret = efi_remap_init();
> + if (ret)
> + return ret;
>
> if (!efi_virtmap_init()) {
> pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
> @@ -122,6 +139,23 @@ static int __init arm_enable_runtime_services(void)
> }
> early_initcall(arm_enable_runtime_services);
>
> +int __init xen_arm_enable_runtime_services(void)
> +{
> + int ret;
> +
> + ret = efi_remap_init();
> + if (ret)
> + return ret;
> +
> + if (IS_ENABLED(CONFIG_XEN_EFI)) {
> + /* Set up runtime services function pointers for Xen Dom0 */
> + xen_efi_runtime_setup();
> + efi.runtime_version = efi.systab->hdr.revision;
> + }
> +
> + return 0;
> +}
Since you call efi_remap_init() in both of these functions, couldn't
you leave the existing code alone and bail after setting up the memory
map and systab in arm_enable_runtime_services()?
Also, why do you need to setup efi.runtime_version here? Isn't that
done inside uefi_init()?
Here is how I would have expected this patch to look:
- Add FDT code to find hypervisor EFI params
- Force enable EFI_RUNTIME_SERVICES for Xen and call
xen_efi_runtime_setup() inside xen_guest_init()
- Change arm_enable_runtime_services() to handle the case where
EFI_RUNTIME_SERVICES is already set
Am I misunderstanding some ordering issues?
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-11 12:27 ` Matt Fleming
0 siblings, 0 replies; 35+ messages in thread
From: Matt Fleming @ 2016-05-11 12:27 UTC (permalink / raw)
To: Shannon Zhao
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
catalin.marinas-5wv7dgnIgG8, will.deacon-5wv7dgnIgG8,
sstabellini-DgEjT+Ai2ygdnm+yROfE0A,
stefano-yd54mjeZNzVBDgjK7y7TUQ, julien.grall-5wv7dgnIgG8,
ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
xen-devel-GuqFBffKawuEi8DpZVb4nw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-efi-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
shannon.zhao-QSEj5FYQhm4dnm+yROfE0A,
peter.huangpeng-hv44wF8Li93QT0dZR+AlfA
On Fri, 06 May, at 04:32:06PM, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> 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.
This commit log would benefit from a little expansion. I'd like to see
information that answers the following questions,
- How do the Xen DT paramters differ from bare metal?
- What existing code is reused with your patch?
- How are Xen runtime services different from bare metal?
- Why is it OK to force enable EFI runtime services for Xen?
I think it would also be good to explicitly state that we do not
expect to find both Xen EFI DT parameters and bare metal EFI DT
parameters when performing the search; the two should be mutually
exclusive.
> CC: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> Signed-off-by: Shannon Zhao <shannon.zhao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> Drop using of EFI_PARAVIRT. Discussion can be found from [1].
> [1] https://lkml.org/lkml/2016/4/29/8
> ---
> arch/arm/include/asm/efi.h | 2 +
> arch/arm/xen/enlighten.c | 16 ++++++++
> arch/arm64/include/asm/efi.h | 2 +
> drivers/firmware/efi/arm-runtime.c | 70 +++++++++++++++++++++++---------
> drivers/firmware/efi/efi.c | 81 ++++++++++++++++++++++++++++++--------
> 5 files changed, 137 insertions(+), 34 deletions(-)
>
> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
> index e0eea72..5ba4343 100644
> --- a/arch/arm/include/asm/efi.h
> +++ b/arch/arm/include/asm/efi.h
> @@ -52,9 +52,11 @@ static inline void efi_set_pgd(struct mm_struct *mm)
>
> void efi_virtmap_load(void);
> void efi_virtmap_unload(void);
> +int xen_arm_enable_runtime_services(void);
>
> #else
> #define efi_init()
> +#define xen_arm_enable_runtime_services() (0)
> #endif /* CONFIG_EFI */
>
> /* arch specific definitions used by the stub code */
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 13e3e9f..3362870 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -16,6 +16,7 @@
> #include <asm/xen/hypervisor.h>
> #include <asm/xen/hypercall.h>
> #include <asm/system_misc.h>
> +#include <asm/efi.h>
> #include <linux/interrupt.h>
> #include <linux/irqreturn.h>
> #include <linux/module.h>
> @@ -261,6 +262,13 @@ 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) &&
> + !efi_runtime_disabled())
> + set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> + }
> +
> return 0;
> }
>
The above comment could do with including similar information to the
commit log as to why we want to force enable runtime services. For x86
we have this,
*
* When EFI_PARAVIRT is in force then we could not map runtime
* service memory region because we do not have direct access to it.
* However, runtime services are available through proxy functions
* (e.g. in case of Xen dom0 EFI implementation they call special
* hypercall which executes relevant EFI functions) and that is why
* they are always enabled.
*/
We need something similar here.
> @@ -338,6 +346,7 @@ static int __init xen_guest_init(void)
> {
> struct xen_add_to_physmap xatp;
> struct shared_info *shared_info_page = NULL;
> + int ret;
>
> if (!xen_domain())
> return 0;
> @@ -352,6 +361,13 @@ static int __init xen_guest_init(void)
> return -ENODEV;
> }
>
> + if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_BOOT) &&
> + efi_enabled(EFI_RUNTIME_SERVICES)) {
> + ret = xen_arm_enable_runtime_services();
> + if (ret)
> + return ret;
> + }
> +
Is it ever possible to have EFI_RUNTIME_SERVICES set but
efi_enabled(EFI_BOOT) and IS_ENABLED(CONFIG_XEN_EFI) be false inside
this function? If the answer is "no", I'd suggest just reducing this
down to,
/*
* The fdt parsing code will have set EFI_RUNTIME_SERVICES if
* it found Xen EFI parameters. Force enable runtime services.
*/
if (efi_enabled(EFI_RUNTIME_SERVICES)) {
ret = xen_arm_enable_runtime_services();
if (ret)
return ret;
}
But first, see my comments below.
> @@ -39,6 +40,33 @@ static struct mm_struct efi_mm = {
> .mmlist = LIST_HEAD_INIT(efi_mm.mmlist),
> };
>
> +static int __init efi_remap_init(void)
> +{
> + u64 mapsize;
> +
> + 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;
> +
> + 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");
> + return -ENOMEM;
> + }
> + set_bit(EFI_SYSTEM_TABLES, &efi.flags);
> +
> + return 0;
> +}
> +
> static bool __init efi_virtmap_init(void)
> {
> efi_memory_desc_t *md;
> @@ -75,7 +103,7 @@ static bool __init efi_virtmap_init(void)
> */
> static int __init arm_enable_runtime_services(void)
> {
> - u64 mapsize;
> + int ret;
>
> if (!efi_enabled(EFI_BOOT)) {
> pr_info("EFI services will not be available.\n");
> @@ -87,25 +115,14 @@ static int __init arm_enable_runtime_services(void)
> return 0;
> }
>
> - 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;
> + if (efi_enabled(EFI_RUNTIME_SERVICES)) {
> + pr_info("EFI runtime services access via paravirt.\n");
> + return 0;
> }
> - memmap.map_end = memmap.map + mapsize;
> - efi.memmap = &memmap;
>
> - 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");
> - return -ENOMEM;
> - }
> - set_bit(EFI_SYSTEM_TABLES, &efi.flags);
> + ret = efi_remap_init();
> + if (ret)
> + return ret;
>
> if (!efi_virtmap_init()) {
> pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
> @@ -122,6 +139,23 @@ static int __init arm_enable_runtime_services(void)
> }
> early_initcall(arm_enable_runtime_services);
>
> +int __init xen_arm_enable_runtime_services(void)
> +{
> + int ret;
> +
> + ret = efi_remap_init();
> + if (ret)
> + return ret;
> +
> + if (IS_ENABLED(CONFIG_XEN_EFI)) {
> + /* Set up runtime services function pointers for Xen Dom0 */
> + xen_efi_runtime_setup();
> + efi.runtime_version = efi.systab->hdr.revision;
> + }
> +
> + return 0;
> +}
Since you call efi_remap_init() in both of these functions, couldn't
you leave the existing code alone and bail after setting up the memory
map and systab in arm_enable_runtime_services()?
Also, why do you need to setup efi.runtime_version here? Isn't that
done inside uefi_init()?
Here is how I would have expected this patch to look:
- Add FDT code to find hypervisor EFI params
- Force enable EFI_RUNTIME_SERVICES for Xen and call
xen_efi_runtime_setup() inside xen_guest_init()
- Change arm_enable_runtime_services() to handle the case where
EFI_RUNTIME_SERVICES is already set
Am I misunderstanding some ordering issues?
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-11 12:27 ` Matt Fleming
0 siblings, 0 replies; 35+ messages in thread
From: Matt Fleming @ 2016-05-11 12:27 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 06 May, at 04:32:06PM, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> 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.
This commit log would benefit from a little expansion. I'd like to see
information that answers the following questions,
- How do the Xen DT paramters differ from bare metal?
- What existing code is reused with your patch?
- How are Xen runtime services different from bare metal?
- Why is it OK to force enable EFI runtime services for Xen?
I think it would also be good to explicitly state that we do not
expect to find both Xen EFI DT parameters and bare metal EFI DT
parameters when performing the search; the two should be mutually
exclusive.
> CC: Matt Fleming <matt@codeblueprint.co.uk>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
> Drop using of EFI_PARAVIRT. Discussion can be found from [1].
> [1] https://lkml.org/lkml/2016/4/29/8
> ---
> arch/arm/include/asm/efi.h | 2 +
> arch/arm/xen/enlighten.c | 16 ++++++++
> arch/arm64/include/asm/efi.h | 2 +
> drivers/firmware/efi/arm-runtime.c | 70 +++++++++++++++++++++++---------
> drivers/firmware/efi/efi.c | 81 ++++++++++++++++++++++++++++++--------
> 5 files changed, 137 insertions(+), 34 deletions(-)
>
> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
> index e0eea72..5ba4343 100644
> --- a/arch/arm/include/asm/efi.h
> +++ b/arch/arm/include/asm/efi.h
> @@ -52,9 +52,11 @@ static inline void efi_set_pgd(struct mm_struct *mm)
>
> void efi_virtmap_load(void);
> void efi_virtmap_unload(void);
> +int xen_arm_enable_runtime_services(void);
>
> #else
> #define efi_init()
> +#define xen_arm_enable_runtime_services() (0)
> #endif /* CONFIG_EFI */
>
> /* arch specific definitions used by the stub code */
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 13e3e9f..3362870 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -16,6 +16,7 @@
> #include <asm/xen/hypervisor.h>
> #include <asm/xen/hypercall.h>
> #include <asm/system_misc.h>
> +#include <asm/efi.h>
> #include <linux/interrupt.h>
> #include <linux/irqreturn.h>
> #include <linux/module.h>
> @@ -261,6 +262,13 @@ 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) &&
> + !efi_runtime_disabled())
> + set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> + }
> +
> return 0;
> }
>
The above comment could do with including similar information to the
commit log as to why we want to force enable runtime services. For x86
we have this,
*
* When EFI_PARAVIRT is in force then we could not map runtime
* service memory region because we do not have direct access to it.
* However, runtime services are available through proxy functions
* (e.g. in case of Xen dom0 EFI implementation they call special
* hypercall which executes relevant EFI functions) and that is why
* they are always enabled.
*/
We need something similar here.
> @@ -338,6 +346,7 @@ static int __init xen_guest_init(void)
> {
> struct xen_add_to_physmap xatp;
> struct shared_info *shared_info_page = NULL;
> + int ret;
>
> if (!xen_domain())
> return 0;
> @@ -352,6 +361,13 @@ static int __init xen_guest_init(void)
> return -ENODEV;
> }
>
> + if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_BOOT) &&
> + efi_enabled(EFI_RUNTIME_SERVICES)) {
> + ret = xen_arm_enable_runtime_services();
> + if (ret)
> + return ret;
> + }
> +
Is it ever possible to have EFI_RUNTIME_SERVICES set but
efi_enabled(EFI_BOOT) and IS_ENABLED(CONFIG_XEN_EFI) be false inside
this function? If the answer is "no", I'd suggest just reducing this
down to,
/*
* The fdt parsing code will have set EFI_RUNTIME_SERVICES if
* it found Xen EFI parameters. Force enable runtime services.
*/
if (efi_enabled(EFI_RUNTIME_SERVICES)) {
ret = xen_arm_enable_runtime_services();
if (ret)
return ret;
}
But first, see my comments below.
> @@ -39,6 +40,33 @@ static struct mm_struct efi_mm = {
> .mmlist = LIST_HEAD_INIT(efi_mm.mmlist),
> };
>
> +static int __init efi_remap_init(void)
> +{
> + u64 mapsize;
> +
> + 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;
> +
> + 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");
> + return -ENOMEM;
> + }
> + set_bit(EFI_SYSTEM_TABLES, &efi.flags);
> +
> + return 0;
> +}
> +
> static bool __init efi_virtmap_init(void)
> {
> efi_memory_desc_t *md;
> @@ -75,7 +103,7 @@ static bool __init efi_virtmap_init(void)
> */
> static int __init arm_enable_runtime_services(void)
> {
> - u64 mapsize;
> + int ret;
>
> if (!efi_enabled(EFI_BOOT)) {
> pr_info("EFI services will not be available.\n");
> @@ -87,25 +115,14 @@ static int __init arm_enable_runtime_services(void)
> return 0;
> }
>
> - 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;
> + if (efi_enabled(EFI_RUNTIME_SERVICES)) {
> + pr_info("EFI runtime services access via paravirt.\n");
> + return 0;
> }
> - memmap.map_end = memmap.map + mapsize;
> - efi.memmap = &memmap;
>
> - 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");
> - return -ENOMEM;
> - }
> - set_bit(EFI_SYSTEM_TABLES, &efi.flags);
> + ret = efi_remap_init();
> + if (ret)
> + return ret;
>
> if (!efi_virtmap_init()) {
> pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
> @@ -122,6 +139,23 @@ static int __init arm_enable_runtime_services(void)
> }
> early_initcall(arm_enable_runtime_services);
>
> +int __init xen_arm_enable_runtime_services(void)
> +{
> + int ret;
> +
> + ret = efi_remap_init();
> + if (ret)
> + return ret;
> +
> + if (IS_ENABLED(CONFIG_XEN_EFI)) {
> + /* Set up runtime services function pointers for Xen Dom0 */
> + xen_efi_runtime_setup();
> + efi.runtime_version = efi.systab->hdr.revision;
> + }
> +
> + return 0;
> +}
Since you call efi_remap_init() in both of these functions, couldn't
you leave the existing code alone and bail after setting up the memory
map and systab in arm_enable_runtime_services()?
Also, why do you need to setup efi.runtime_version here? Isn't that
done inside uefi_init()?
Here is how I would have expected this patch to look:
- Add FDT code to find hypervisor EFI params
- Force enable EFI_RUNTIME_SERVICES for Xen and call
xen_efi_runtime_setup() inside xen_guest_init()
- Change arm_enable_runtime_services() to handle the case where
EFI_RUNTIME_SERVICES is already set
Am I misunderstanding some ordering issues?
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
2016-05-06 8:32 ` Shannon Zhao
` (3 preceding siblings ...)
(?)
@ 2016-05-11 12:27 ` Matt Fleming
-1 siblings, 0 replies; 35+ messages in thread
From: Matt Fleming @ 2016-05-11 12:27 UTC (permalink / raw)
To: Shannon Zhao
Cc: devicetree, sstabellini, ard.biesheuvel, catalin.marinas,
will.deacon, linux-kernel, xen-devel, stefano, julien.grall,
linux-efi, shannon.zhao, peter.huangpeng, linux-arm-kernel
On Fri, 06 May, at 04:32:06PM, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> 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.
This commit log would benefit from a little expansion. I'd like to see
information that answers the following questions,
- How do the Xen DT paramters differ from bare metal?
- What existing code is reused with your patch?
- How are Xen runtime services different from bare metal?
- Why is it OK to force enable EFI runtime services for Xen?
I think it would also be good to explicitly state that we do not
expect to find both Xen EFI DT parameters and bare metal EFI DT
parameters when performing the search; the two should be mutually
exclusive.
> CC: Matt Fleming <matt@codeblueprint.co.uk>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
> Drop using of EFI_PARAVIRT. Discussion can be found from [1].
> [1] https://lkml.org/lkml/2016/4/29/8
> ---
> arch/arm/include/asm/efi.h | 2 +
> arch/arm/xen/enlighten.c | 16 ++++++++
> arch/arm64/include/asm/efi.h | 2 +
> drivers/firmware/efi/arm-runtime.c | 70 +++++++++++++++++++++++---------
> drivers/firmware/efi/efi.c | 81 ++++++++++++++++++++++++++++++--------
> 5 files changed, 137 insertions(+), 34 deletions(-)
>
> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
> index e0eea72..5ba4343 100644
> --- a/arch/arm/include/asm/efi.h
> +++ b/arch/arm/include/asm/efi.h
> @@ -52,9 +52,11 @@ static inline void efi_set_pgd(struct mm_struct *mm)
>
> void efi_virtmap_load(void);
> void efi_virtmap_unload(void);
> +int xen_arm_enable_runtime_services(void);
>
> #else
> #define efi_init()
> +#define xen_arm_enable_runtime_services() (0)
> #endif /* CONFIG_EFI */
>
> /* arch specific definitions used by the stub code */
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 13e3e9f..3362870 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -16,6 +16,7 @@
> #include <asm/xen/hypervisor.h>
> #include <asm/xen/hypercall.h>
> #include <asm/system_misc.h>
> +#include <asm/efi.h>
> #include <linux/interrupt.h>
> #include <linux/irqreturn.h>
> #include <linux/module.h>
> @@ -261,6 +262,13 @@ 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) &&
> + !efi_runtime_disabled())
> + set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> + }
> +
> return 0;
> }
>
The above comment could do with including similar information to the
commit log as to why we want to force enable runtime services. For x86
we have this,
*
* When EFI_PARAVIRT is in force then we could not map runtime
* service memory region because we do not have direct access to it.
* However, runtime services are available through proxy functions
* (e.g. in case of Xen dom0 EFI implementation they call special
* hypercall which executes relevant EFI functions) and that is why
* they are always enabled.
*/
We need something similar here.
> @@ -338,6 +346,7 @@ static int __init xen_guest_init(void)
> {
> struct xen_add_to_physmap xatp;
> struct shared_info *shared_info_page = NULL;
> + int ret;
>
> if (!xen_domain())
> return 0;
> @@ -352,6 +361,13 @@ static int __init xen_guest_init(void)
> return -ENODEV;
> }
>
> + if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_BOOT) &&
> + efi_enabled(EFI_RUNTIME_SERVICES)) {
> + ret = xen_arm_enable_runtime_services();
> + if (ret)
> + return ret;
> + }
> +
Is it ever possible to have EFI_RUNTIME_SERVICES set but
efi_enabled(EFI_BOOT) and IS_ENABLED(CONFIG_XEN_EFI) be false inside
this function? If the answer is "no", I'd suggest just reducing this
down to,
/*
* The fdt parsing code will have set EFI_RUNTIME_SERVICES if
* it found Xen EFI parameters. Force enable runtime services.
*/
if (efi_enabled(EFI_RUNTIME_SERVICES)) {
ret = xen_arm_enable_runtime_services();
if (ret)
return ret;
}
But first, see my comments below.
> @@ -39,6 +40,33 @@ static struct mm_struct efi_mm = {
> .mmlist = LIST_HEAD_INIT(efi_mm.mmlist),
> };
>
> +static int __init efi_remap_init(void)
> +{
> + u64 mapsize;
> +
> + 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;
> +
> + 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");
> + return -ENOMEM;
> + }
> + set_bit(EFI_SYSTEM_TABLES, &efi.flags);
> +
> + return 0;
> +}
> +
> static bool __init efi_virtmap_init(void)
> {
> efi_memory_desc_t *md;
> @@ -75,7 +103,7 @@ static bool __init efi_virtmap_init(void)
> */
> static int __init arm_enable_runtime_services(void)
> {
> - u64 mapsize;
> + int ret;
>
> if (!efi_enabled(EFI_BOOT)) {
> pr_info("EFI services will not be available.\n");
> @@ -87,25 +115,14 @@ static int __init arm_enable_runtime_services(void)
> return 0;
> }
>
> - 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;
> + if (efi_enabled(EFI_RUNTIME_SERVICES)) {
> + pr_info("EFI runtime services access via paravirt.\n");
> + return 0;
> }
> - memmap.map_end = memmap.map + mapsize;
> - efi.memmap = &memmap;
>
> - 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");
> - return -ENOMEM;
> - }
> - set_bit(EFI_SYSTEM_TABLES, &efi.flags);
> + ret = efi_remap_init();
> + if (ret)
> + return ret;
>
> if (!efi_virtmap_init()) {
> pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
> @@ -122,6 +139,23 @@ static int __init arm_enable_runtime_services(void)
> }
> early_initcall(arm_enable_runtime_services);
>
> +int __init xen_arm_enable_runtime_services(void)
> +{
> + int ret;
> +
> + ret = efi_remap_init();
> + if (ret)
> + return ret;
> +
> + if (IS_ENABLED(CONFIG_XEN_EFI)) {
> + /* Set up runtime services function pointers for Xen Dom0 */
> + xen_efi_runtime_setup();
> + efi.runtime_version = efi.systab->hdr.revision;
> + }
> +
> + return 0;
> +}
Since you call efi_remap_init() in both of these functions, couldn't
you leave the existing code alone and bail after setting up the memory
map and systab in arm_enable_runtime_services()?
Also, why do you need to setup efi.runtime_version here? Isn't that
done inside uefi_init()?
Here is how I would have expected this patch to look:
- Add FDT code to find hypervisor EFI params
- Force enable EFI_RUNTIME_SERVICES for Xen and call
xen_efi_runtime_setup() inside xen_guest_init()
- Change arm_enable_runtime_services() to handle the case where
EFI_RUNTIME_SERVICES is already set
Am I misunderstanding some ordering issues?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-11 12:29 ` Matt Fleming
0 siblings, 0 replies; 35+ messages in thread
From: Matt Fleming @ 2016-05-11 12:29 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Shannon Zhao, linux-arm-kernel, Catalin Marinas, Will Deacon,
sstabellini, stefano, julien.grall, ard.biesheuvel, xen-devel,
devicetree, linux-efi, linux-kernel, shannon.zhao,
peter.huangpeng
On Fri, 06 May, at 09:52:42AM, Mathieu Poirier wrote:
> > +static int __init efi_remap_init(void)
> > +{
> > + u64 mapsize;
> > +
> > + 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;
> > +
> > + efi.systab = (__force void *)ioremap_cache(efi_system_table,
> > + sizeof(efi_system_table_t));
> > + if (!efi.systab) {
>
> Is there a reason why memmap.map isn't iounmap() in the error path?
> The original code didn't have it either, hence the question.
I suspect that is a bug.
In reality, if you can't access the EFI System Table because you fail
to map it I would guess you are going to crash very, very quickly
anyhow.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-11 12:29 ` Matt Fleming
0 siblings, 0 replies; 35+ messages in thread
From: Matt Fleming @ 2016-05-11 12:29 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Shannon Zhao, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Catalin Marinas, Will Deacon, sstabellini-DgEjT+Ai2ygdnm+yROfE0A,
stefano-yd54mjeZNzVBDgjK7y7TUQ, julien.grall-5wv7dgnIgG8,
ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
xen-devel-GuqFBffKawuEi8DpZVb4nw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-efi-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
shannon.zhao-QSEj5FYQhm4dnm+yROfE0A,
peter.huangpeng-hv44wF8Li93QT0dZR+AlfA
On Fri, 06 May, at 09:52:42AM, Mathieu Poirier wrote:
> > +static int __init efi_remap_init(void)
> > +{
> > + u64 mapsize;
> > +
> > + 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;
> > +
> > + efi.systab = (__force void *)ioremap_cache(efi_system_table,
> > + sizeof(efi_system_table_t));
> > + if (!efi.systab) {
>
> Is there a reason why memmap.map isn't iounmap() in the error path?
> The original code didn't have it either, hence the question.
I suspect that is a bug.
In reality, if you can't access the EFI System Table because you fail
to map it I would guess you are going to crash very, very quickly
anyhow.
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-11 12:29 ` Matt Fleming
0 siblings, 0 replies; 35+ messages in thread
From: Matt Fleming @ 2016-05-11 12:29 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 06 May, at 09:52:42AM, Mathieu Poirier wrote:
> > +static int __init efi_remap_init(void)
> > +{
> > + u64 mapsize;
> > +
> > + 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;
> > +
> > + efi.systab = (__force void *)ioremap_cache(efi_system_table,
> > + sizeof(efi_system_table_t));
> > + if (!efi.systab) {
>
> Is there a reason why memmap.map isn't iounmap() in the error path?
> The original code didn't have it either, hence the question.
I suspect that is a bug.
In reality, if you can't access the EFI System Table because you fail
to map it I would guess you are going to crash very, very quickly
anyhow.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
2016-05-06 15:52 ` Mathieu Poirier
(?)
(?)
@ 2016-05-11 12:29 ` Matt Fleming
-1 siblings, 0 replies; 35+ messages in thread
From: Matt Fleming @ 2016-05-11 12:29 UTC (permalink / raw)
To: Mathieu Poirier
Cc: devicetree, sstabellini, ard.biesheuvel, Catalin Marinas,
Will Deacon, linux-kernel, xen-devel, stefano, julien.grall,
linux-efi, shannon.zhao, Shannon Zhao, peter.huangpeng,
linux-arm-kernel
On Fri, 06 May, at 09:52:42AM, Mathieu Poirier wrote:
> > +static int __init efi_remap_init(void)
> > +{
> > + u64 mapsize;
> > +
> > + 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;
> > +
> > + efi.systab = (__force void *)ioremap_cache(efi_system_table,
> > + sizeof(efi_system_table_t));
> > + if (!efi.systab) {
>
> Is there a reason why memmap.map isn't iounmap() in the error path?
> The original code didn't have it either, hence the question.
I suspect that is a bug.
In reality, if you can't access the EFI System Table because you fail
to map it I would guess you are going to crash very, very quickly
anyhow.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-11 13:35 ` Shannon Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Shannon Zhao @ 2016-05-11 13:35 UTC (permalink / raw)
To: Matt Fleming, Shannon Zhao
Cc: linux-arm-kernel, catalin.marinas, will.deacon, sstabellini,
stefano, julien.grall, ard.biesheuvel, xen-devel, devicetree,
linux-efi, linux-kernel, peter.huangpeng
On 2016年05月11日 20:27, Matt Fleming wrote:
> On Fri, 06 May, at 04:32:06PM, Shannon Zhao wrote:
>> > From: Shannon Zhao <shannon.zhao@linaro.org>
>> >
>> > 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.
>
> This commit log would benefit from a little expansion. I'd like to see
> information that answers the following questions,
>
> - How do the Xen DT paramters differ from bare metal?
> - What existing code is reused with your patch?
> - How are Xen runtime services different from bare metal?
> - Why is it OK to force enable EFI runtime services for Xen?
>
> I think it would also be good to explicitly state that we do not
> expect to find both Xen EFI DT parameters and bare metal EFI DT
> parameters when performing the search; the two should be mutually
> exclusive.
>
>> > CC: Matt Fleming <matt@codeblueprint.co.uk>
>> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> > ---
>> > Drop using of EFI_PARAVIRT. Discussion can be found from [1].
>> > [1] https://lkml.org/lkml/2016/4/29/8
>> > ---
>> > arch/arm/include/asm/efi.h | 2 +
>> > arch/arm/xen/enlighten.c | 16 ++++++++
>> > arch/arm64/include/asm/efi.h | 2 +
>> > drivers/firmware/efi/arm-runtime.c | 70 +++++++++++++++++++++++---------
>> > drivers/firmware/efi/efi.c | 81 ++++++++++++++++++++++++++++++--------
>> > 5 files changed, 137 insertions(+), 34 deletions(-)
>> >
>> > diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
>> > index e0eea72..5ba4343 100644
>> > --- a/arch/arm/include/asm/efi.h
>> > +++ b/arch/arm/include/asm/efi.h
>> > @@ -52,9 +52,11 @@ static inline void efi_set_pgd(struct mm_struct *mm)
>> >
>> > void efi_virtmap_load(void);
>> > void efi_virtmap_unload(void);
>> > +int xen_arm_enable_runtime_services(void);
>> >
>> > #else
>> > #define efi_init()
>> > +#define xen_arm_enable_runtime_services() (0)
>> > #endif /* CONFIG_EFI */
>> >
>> > /* arch specific definitions used by the stub code */
>> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
>> > index 13e3e9f..3362870 100644
>> > --- a/arch/arm/xen/enlighten.c
>> > +++ b/arch/arm/xen/enlighten.c
>> > @@ -16,6 +16,7 @@
>> > #include <asm/xen/hypervisor.h>
>> > #include <asm/xen/hypercall.h>
>> > #include <asm/system_misc.h>
>> > +#include <asm/efi.h>
>> > #include <linux/interrupt.h>
>> > #include <linux/irqreturn.h>
>> > #include <linux/module.h>
>> > @@ -261,6 +262,13 @@ 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) &&
>> > + !efi_runtime_disabled())
>> > + set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
>> > + }
>> > +
>> > return 0;
>> > }
>> >
> The above comment could do with including similar information to the
> commit log as to why we want to force enable runtime services. For x86
> we have this,
>
> *
> * When EFI_PARAVIRT is in force then we could not map runtime
> * service memory region because we do not have direct access to it.
> * However, runtime services are available through proxy functions
> * (e.g. in case of Xen dom0 EFI implementation they call special
> * hypercall which executes relevant EFI functions) and that is why
> * they are always enabled.
> */
>
> We need something similar here.
>
>> > @@ -338,6 +346,7 @@ static int __init xen_guest_init(void)
>> > {
>> > struct xen_add_to_physmap xatp;
>> > struct shared_info *shared_info_page = NULL;
>> > + int ret;
>> >
>> > if (!xen_domain())
>> > return 0;
>> > @@ -352,6 +361,13 @@ static int __init xen_guest_init(void)
>> > return -ENODEV;
>> > }
>> >
>> > + if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_BOOT) &&
>> > + efi_enabled(EFI_RUNTIME_SERVICES)) {
>> > + ret = xen_arm_enable_runtime_services();
>> > + if (ret)
>> > + return ret;
>> > + }
>> > +
> Is it ever possible to have EFI_RUNTIME_SERVICES set but
> efi_enabled(EFI_BOOT) and IS_ENABLED(CONFIG_XEN_EFI) be false inside
> this function? If the answer is "no", I'd suggest just reducing this
> down to,
>
> /*
> * The fdt parsing code will have set EFI_RUNTIME_SERVICES if
> * it found Xen EFI parameters. Force enable runtime services.
> */
> if (efi_enabled(EFI_RUNTIME_SERVICES)) {
> ret = xen_arm_enable_runtime_services();
> if (ret)
> return ret;
> }
>
> But first, see my comments below.
>
>> > @@ -39,6 +40,33 @@ static struct mm_struct efi_mm = {
>> > .mmlist = LIST_HEAD_INIT(efi_mm.mmlist),
>> > };
>> >
>> > +static int __init efi_remap_init(void)
>> > +{
>> > + u64 mapsize;
>> > +
>> > + 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;
>> > +
>> > + 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");
>> > + return -ENOMEM;
>> > + }
>> > + set_bit(EFI_SYSTEM_TABLES, &efi.flags);
>> > +
>> > + return 0;
>> > +}
>> > +
>> > static bool __init efi_virtmap_init(void)
>> > {
>> > efi_memory_desc_t *md;
>> > @@ -75,7 +103,7 @@ static bool __init efi_virtmap_init(void)
>> > */
>> > static int __init arm_enable_runtime_services(void)
>> > {
>> > - u64 mapsize;
>> > + int ret;
>> >
>> > if (!efi_enabled(EFI_BOOT)) {
>> > pr_info("EFI services will not be available.\n");
>> > @@ -87,25 +115,14 @@ static int __init arm_enable_runtime_services(void)
>> > return 0;
>> > }
>> >
>> > - 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;
>> > + if (efi_enabled(EFI_RUNTIME_SERVICES)) {
>> > + pr_info("EFI runtime services access via paravirt.\n");
>> > + return 0;
>> > }
>> > - memmap.map_end = memmap.map + mapsize;
>> > - efi.memmap = &memmap;
>> >
>> > - 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");
>> > - return -ENOMEM;
>> > - }
>> > - set_bit(EFI_SYSTEM_TABLES, &efi.flags);
>> > + ret = efi_remap_init();
>> > + if (ret)
>> > + return ret;
>> >
>> > if (!efi_virtmap_init()) {
>> > pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
>> > @@ -122,6 +139,23 @@ static int __init arm_enable_runtime_services(void)
>> > }
>> > early_initcall(arm_enable_runtime_services);
>> >
>> > +int __init xen_arm_enable_runtime_services(void)
>> > +{
>> > + int ret;
>> > +
>> > + ret = efi_remap_init();
>> > + if (ret)
>> > + return ret;
>> > +
>> > + if (IS_ENABLED(CONFIG_XEN_EFI)) {
>> > + /* Set up runtime services function pointers for Xen Dom0 */
>> > + xen_efi_runtime_setup();
>> > + efi.runtime_version = efi.systab->hdr.revision;
>> > + }
>> > +
>> > + return 0;
>> > +}
> Since you call efi_remap_init() in both of these functions, couldn't
> you leave the existing code alone and bail after setting up the memory
> map and systab in arm_enable_runtime_services()?
>
> Also, why do you need to setup efi.runtime_version here? Isn't that
> done inside uefi_init()?
>
I don't see any codes which setup efi.runtime_version in uefi_init().
> Here is how I would have expected this patch to look:
>
> - Add FDT code to find hypervisor EFI params
>
> - Force enable EFI_RUNTIME_SERVICES for Xen and call
> xen_efi_runtime_setup() inside xen_guest_init()
>
> - Change arm_enable_runtime_services() to handle the case where
> EFI_RUNTIME_SERVICES is already set
>
> Am I misunderstanding some ordering issues?
Since xen_guest_init() and arm_enable_runtime_services() are both
early_initcall and the calling order is random I think. And when we call
xen_efi_runtime_setup() and setup efi.runtime_version in
xen_guest_init(), the efi.systab might be NULL. So it needs to map the
systanle explicitly before we use efi.systab.
Thanks,
--
Shannon
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-11 13:35 ` Shannon Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Shannon Zhao @ 2016-05-11 13:35 UTC (permalink / raw)
To: Matt Fleming, Shannon Zhao
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
catalin.marinas-5wv7dgnIgG8, will.deacon-5wv7dgnIgG8,
sstabellini-DgEjT+Ai2ygdnm+yROfE0A,
stefano-yd54mjeZNzVBDgjK7y7TUQ, julien.grall-5wv7dgnIgG8,
ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
xen-devel-GuqFBffKawuEi8DpZVb4nw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-efi-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
peter.huangpeng-hv44wF8Li93QT0dZR+AlfA
On 2016年05月11日 20:27, Matt Fleming wrote:
> On Fri, 06 May, at 04:32:06PM, Shannon Zhao wrote:
>> > From: Shannon Zhao <shannon.zhao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> >
>> > 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.
>
> This commit log would benefit from a little expansion. I'd like to see
> information that answers the following questions,
>
> - How do the Xen DT paramters differ from bare metal?
> - What existing code is reused with your patch?
> - How are Xen runtime services different from bare metal?
> - Why is it OK to force enable EFI runtime services for Xen?
>
> I think it would also be good to explicitly state that we do not
> expect to find both Xen EFI DT parameters and bare metal EFI DT
> parameters when performing the search; the two should be mutually
> exclusive.
>
>> > CC: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
>> > Signed-off-by: Shannon Zhao <shannon.zhao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> > ---
>> > Drop using of EFI_PARAVIRT. Discussion can be found from [1].
>> > [1] https://lkml.org/lkml/2016/4/29/8
>> > ---
>> > arch/arm/include/asm/efi.h | 2 +
>> > arch/arm/xen/enlighten.c | 16 ++++++++
>> > arch/arm64/include/asm/efi.h | 2 +
>> > drivers/firmware/efi/arm-runtime.c | 70 +++++++++++++++++++++++---------
>> > drivers/firmware/efi/efi.c | 81 ++++++++++++++++++++++++++++++--------
>> > 5 files changed, 137 insertions(+), 34 deletions(-)
>> >
>> > diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
>> > index e0eea72..5ba4343 100644
>> > --- a/arch/arm/include/asm/efi.h
>> > +++ b/arch/arm/include/asm/efi.h
>> > @@ -52,9 +52,11 @@ static inline void efi_set_pgd(struct mm_struct *mm)
>> >
>> > void efi_virtmap_load(void);
>> > void efi_virtmap_unload(void);
>> > +int xen_arm_enable_runtime_services(void);
>> >
>> > #else
>> > #define efi_init()
>> > +#define xen_arm_enable_runtime_services() (0)
>> > #endif /* CONFIG_EFI */
>> >
>> > /* arch specific definitions used by the stub code */
>> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
>> > index 13e3e9f..3362870 100644
>> > --- a/arch/arm/xen/enlighten.c
>> > +++ b/arch/arm/xen/enlighten.c
>> > @@ -16,6 +16,7 @@
>> > #include <asm/xen/hypervisor.h>
>> > #include <asm/xen/hypercall.h>
>> > #include <asm/system_misc.h>
>> > +#include <asm/efi.h>
>> > #include <linux/interrupt.h>
>> > #include <linux/irqreturn.h>
>> > #include <linux/module.h>
>> > @@ -261,6 +262,13 @@ 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) &&
>> > + !efi_runtime_disabled())
>> > + set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
>> > + }
>> > +
>> > return 0;
>> > }
>> >
> The above comment could do with including similar information to the
> commit log as to why we want to force enable runtime services. For x86
> we have this,
>
> *
> * When EFI_PARAVIRT is in force then we could not map runtime
> * service memory region because we do not have direct access to it.
> * However, runtime services are available through proxy functions
> * (e.g. in case of Xen dom0 EFI implementation they call special
> * hypercall which executes relevant EFI functions) and that is why
> * they are always enabled.
> */
>
> We need something similar here.
>
>> > @@ -338,6 +346,7 @@ static int __init xen_guest_init(void)
>> > {
>> > struct xen_add_to_physmap xatp;
>> > struct shared_info *shared_info_page = NULL;
>> > + int ret;
>> >
>> > if (!xen_domain())
>> > return 0;
>> > @@ -352,6 +361,13 @@ static int __init xen_guest_init(void)
>> > return -ENODEV;
>> > }
>> >
>> > + if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_BOOT) &&
>> > + efi_enabled(EFI_RUNTIME_SERVICES)) {
>> > + ret = xen_arm_enable_runtime_services();
>> > + if (ret)
>> > + return ret;
>> > + }
>> > +
> Is it ever possible to have EFI_RUNTIME_SERVICES set but
> efi_enabled(EFI_BOOT) and IS_ENABLED(CONFIG_XEN_EFI) be false inside
> this function? If the answer is "no", I'd suggest just reducing this
> down to,
>
> /*
> * The fdt parsing code will have set EFI_RUNTIME_SERVICES if
> * it found Xen EFI parameters. Force enable runtime services.
> */
> if (efi_enabled(EFI_RUNTIME_SERVICES)) {
> ret = xen_arm_enable_runtime_services();
> if (ret)
> return ret;
> }
>
> But first, see my comments below.
>
>> > @@ -39,6 +40,33 @@ static struct mm_struct efi_mm = {
>> > .mmlist = LIST_HEAD_INIT(efi_mm.mmlist),
>> > };
>> >
>> > +static int __init efi_remap_init(void)
>> > +{
>> > + u64 mapsize;
>> > +
>> > + 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;
>> > +
>> > + 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");
>> > + return -ENOMEM;
>> > + }
>> > + set_bit(EFI_SYSTEM_TABLES, &efi.flags);
>> > +
>> > + return 0;
>> > +}
>> > +
>> > static bool __init efi_virtmap_init(void)
>> > {
>> > efi_memory_desc_t *md;
>> > @@ -75,7 +103,7 @@ static bool __init efi_virtmap_init(void)
>> > */
>> > static int __init arm_enable_runtime_services(void)
>> > {
>> > - u64 mapsize;
>> > + int ret;
>> >
>> > if (!efi_enabled(EFI_BOOT)) {
>> > pr_info("EFI services will not be available.\n");
>> > @@ -87,25 +115,14 @@ static int __init arm_enable_runtime_services(void)
>> > return 0;
>> > }
>> >
>> > - 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;
>> > + if (efi_enabled(EFI_RUNTIME_SERVICES)) {
>> > + pr_info("EFI runtime services access via paravirt.\n");
>> > + return 0;
>> > }
>> > - memmap.map_end = memmap.map + mapsize;
>> > - efi.memmap = &memmap;
>> >
>> > - 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");
>> > - return -ENOMEM;
>> > - }
>> > - set_bit(EFI_SYSTEM_TABLES, &efi.flags);
>> > + ret = efi_remap_init();
>> > + if (ret)
>> > + return ret;
>> >
>> > if (!efi_virtmap_init()) {
>> > pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
>> > @@ -122,6 +139,23 @@ static int __init arm_enable_runtime_services(void)
>> > }
>> > early_initcall(arm_enable_runtime_services);
>> >
>> > +int __init xen_arm_enable_runtime_services(void)
>> > +{
>> > + int ret;
>> > +
>> > + ret = efi_remap_init();
>> > + if (ret)
>> > + return ret;
>> > +
>> > + if (IS_ENABLED(CONFIG_XEN_EFI)) {
>> > + /* Set up runtime services function pointers for Xen Dom0 */
>> > + xen_efi_runtime_setup();
>> > + efi.runtime_version = efi.systab->hdr.revision;
>> > + }
>> > +
>> > + return 0;
>> > +}
> Since you call efi_remap_init() in both of these functions, couldn't
> you leave the existing code alone and bail after setting up the memory
> map and systab in arm_enable_runtime_services()?
>
> Also, why do you need to setup efi.runtime_version here? Isn't that
> done inside uefi_init()?
>
I don't see any codes which setup efi.runtime_version in uefi_init().
> Here is how I would have expected this patch to look:
>
> - Add FDT code to find hypervisor EFI params
>
> - Force enable EFI_RUNTIME_SERVICES for Xen and call
> xen_efi_runtime_setup() inside xen_guest_init()
>
> - Change arm_enable_runtime_services() to handle the case where
> EFI_RUNTIME_SERVICES is already set
>
> Am I misunderstanding some ordering issues?
Since xen_guest_init() and arm_enable_runtime_services() are both
early_initcall and the calling order is random I think. And when we call
xen_efi_runtime_setup() and setup efi.runtime_version in
xen_guest_init(), the efi.systab might be NULL. So it needs to map the
systanle explicitly before we use efi.systab.
Thanks,
--
Shannon
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-11 13:35 ` Shannon Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Shannon Zhao @ 2016-05-11 13:35 UTC (permalink / raw)
To: linux-arm-kernel
On 2016?05?11? 20:27, Matt Fleming wrote:
> On Fri, 06 May, at 04:32:06PM, Shannon Zhao wrote:
>> > From: Shannon Zhao <shannon.zhao@linaro.org>
>> >
>> > 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.
>
> This commit log would benefit from a little expansion. I'd like to see
> information that answers the following questions,
>
> - How do the Xen DT paramters differ from bare metal?
> - What existing code is reused with your patch?
> - How are Xen runtime services different from bare metal?
> - Why is it OK to force enable EFI runtime services for Xen?
>
> I think it would also be good to explicitly state that we do not
> expect to find both Xen EFI DT parameters and bare metal EFI DT
> parameters when performing the search; the two should be mutually
> exclusive.
>
>> > CC: Matt Fleming <matt@codeblueprint.co.uk>
>> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> > ---
>> > Drop using of EFI_PARAVIRT. Discussion can be found from [1].
>> > [1] https://lkml.org/lkml/2016/4/29/8
>> > ---
>> > arch/arm/include/asm/efi.h | 2 +
>> > arch/arm/xen/enlighten.c | 16 ++++++++
>> > arch/arm64/include/asm/efi.h | 2 +
>> > drivers/firmware/efi/arm-runtime.c | 70 +++++++++++++++++++++++---------
>> > drivers/firmware/efi/efi.c | 81 ++++++++++++++++++++++++++++++--------
>> > 5 files changed, 137 insertions(+), 34 deletions(-)
>> >
>> > diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
>> > index e0eea72..5ba4343 100644
>> > --- a/arch/arm/include/asm/efi.h
>> > +++ b/arch/arm/include/asm/efi.h
>> > @@ -52,9 +52,11 @@ static inline void efi_set_pgd(struct mm_struct *mm)
>> >
>> > void efi_virtmap_load(void);
>> > void efi_virtmap_unload(void);
>> > +int xen_arm_enable_runtime_services(void);
>> >
>> > #else
>> > #define efi_init()
>> > +#define xen_arm_enable_runtime_services() (0)
>> > #endif /* CONFIG_EFI */
>> >
>> > /* arch specific definitions used by the stub code */
>> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
>> > index 13e3e9f..3362870 100644
>> > --- a/arch/arm/xen/enlighten.c
>> > +++ b/arch/arm/xen/enlighten.c
>> > @@ -16,6 +16,7 @@
>> > #include <asm/xen/hypervisor.h>
>> > #include <asm/xen/hypercall.h>
>> > #include <asm/system_misc.h>
>> > +#include <asm/efi.h>
>> > #include <linux/interrupt.h>
>> > #include <linux/irqreturn.h>
>> > #include <linux/module.h>
>> > @@ -261,6 +262,13 @@ 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) &&
>> > + !efi_runtime_disabled())
>> > + set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
>> > + }
>> > +
>> > return 0;
>> > }
>> >
> The above comment could do with including similar information to the
> commit log as to why we want to force enable runtime services. For x86
> we have this,
>
> *
> * When EFI_PARAVIRT is in force then we could not map runtime
> * service memory region because we do not have direct access to it.
> * However, runtime services are available through proxy functions
> * (e.g. in case of Xen dom0 EFI implementation they call special
> * hypercall which executes relevant EFI functions) and that is why
> * they are always enabled.
> */
>
> We need something similar here.
>
>> > @@ -338,6 +346,7 @@ static int __init xen_guest_init(void)
>> > {
>> > struct xen_add_to_physmap xatp;
>> > struct shared_info *shared_info_page = NULL;
>> > + int ret;
>> >
>> > if (!xen_domain())
>> > return 0;
>> > @@ -352,6 +361,13 @@ static int __init xen_guest_init(void)
>> > return -ENODEV;
>> > }
>> >
>> > + if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_BOOT) &&
>> > + efi_enabled(EFI_RUNTIME_SERVICES)) {
>> > + ret = xen_arm_enable_runtime_services();
>> > + if (ret)
>> > + return ret;
>> > + }
>> > +
> Is it ever possible to have EFI_RUNTIME_SERVICES set but
> efi_enabled(EFI_BOOT) and IS_ENABLED(CONFIG_XEN_EFI) be false inside
> this function? If the answer is "no", I'd suggest just reducing this
> down to,
>
> /*
> * The fdt parsing code will have set EFI_RUNTIME_SERVICES if
> * it found Xen EFI parameters. Force enable runtime services.
> */
> if (efi_enabled(EFI_RUNTIME_SERVICES)) {
> ret = xen_arm_enable_runtime_services();
> if (ret)
> return ret;
> }
>
> But first, see my comments below.
>
>> > @@ -39,6 +40,33 @@ static struct mm_struct efi_mm = {
>> > .mmlist = LIST_HEAD_INIT(efi_mm.mmlist),
>> > };
>> >
>> > +static int __init efi_remap_init(void)
>> > +{
>> > + u64 mapsize;
>> > +
>> > + 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;
>> > +
>> > + 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");
>> > + return -ENOMEM;
>> > + }
>> > + set_bit(EFI_SYSTEM_TABLES, &efi.flags);
>> > +
>> > + return 0;
>> > +}
>> > +
>> > static bool __init efi_virtmap_init(void)
>> > {
>> > efi_memory_desc_t *md;
>> > @@ -75,7 +103,7 @@ static bool __init efi_virtmap_init(void)
>> > */
>> > static int __init arm_enable_runtime_services(void)
>> > {
>> > - u64 mapsize;
>> > + int ret;
>> >
>> > if (!efi_enabled(EFI_BOOT)) {
>> > pr_info("EFI services will not be available.\n");
>> > @@ -87,25 +115,14 @@ static int __init arm_enable_runtime_services(void)
>> > return 0;
>> > }
>> >
>> > - 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;
>> > + if (efi_enabled(EFI_RUNTIME_SERVICES)) {
>> > + pr_info("EFI runtime services access via paravirt.\n");
>> > + return 0;
>> > }
>> > - memmap.map_end = memmap.map + mapsize;
>> > - efi.memmap = &memmap;
>> >
>> > - 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");
>> > - return -ENOMEM;
>> > - }
>> > - set_bit(EFI_SYSTEM_TABLES, &efi.flags);
>> > + ret = efi_remap_init();
>> > + if (ret)
>> > + return ret;
>> >
>> > if (!efi_virtmap_init()) {
>> > pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
>> > @@ -122,6 +139,23 @@ static int __init arm_enable_runtime_services(void)
>> > }
>> > early_initcall(arm_enable_runtime_services);
>> >
>> > +int __init xen_arm_enable_runtime_services(void)
>> > +{
>> > + int ret;
>> > +
>> > + ret = efi_remap_init();
>> > + if (ret)
>> > + return ret;
>> > +
>> > + if (IS_ENABLED(CONFIG_XEN_EFI)) {
>> > + /* Set up runtime services function pointers for Xen Dom0 */
>> > + xen_efi_runtime_setup();
>> > + efi.runtime_version = efi.systab->hdr.revision;
>> > + }
>> > +
>> > + return 0;
>> > +}
> Since you call efi_remap_init() in both of these functions, couldn't
> you leave the existing code alone and bail after setting up the memory
> map and systab in arm_enable_runtime_services()?
>
> Also, why do you need to setup efi.runtime_version here? Isn't that
> done inside uefi_init()?
>
I don't see any codes which setup efi.runtime_version in uefi_init().
> Here is how I would have expected this patch to look:
>
> - Add FDT code to find hypervisor EFI params
>
> - Force enable EFI_RUNTIME_SERVICES for Xen and call
> xen_efi_runtime_setup() inside xen_guest_init()
>
> - Change arm_enable_runtime_services() to handle the case where
> EFI_RUNTIME_SERVICES is already set
>
> Am I misunderstanding some ordering issues?
Since xen_guest_init() and arm_enable_runtime_services() are both
early_initcall and the calling order is random I think. And when we call
xen_efi_runtime_setup() and setup efi.runtime_version in
xen_guest_init(), the efi.systab might be NULL. So it needs to map the
systanle explicitly before we use efi.systab.
Thanks,
--
Shannon
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
2016-05-11 12:27 ` Matt Fleming
(?)
(?)
@ 2016-05-11 13:35 ` Shannon Zhao
-1 siblings, 0 replies; 35+ messages in thread
From: Shannon Zhao @ 2016-05-11 13:35 UTC (permalink / raw)
To: Matt Fleming, Shannon Zhao
Cc: devicetree, sstabellini, ard.biesheuvel, catalin.marinas,
will.deacon, linux-kernel, xen-devel, stefano, julien.grall,
linux-efi, peter.huangpeng, linux-arm-kernel
On 2016年05月11日 20:27, Matt Fleming wrote:
> On Fri, 06 May, at 04:32:06PM, Shannon Zhao wrote:
>> > From: Shannon Zhao <shannon.zhao@linaro.org>
>> >
>> > 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.
>
> This commit log would benefit from a little expansion. I'd like to see
> information that answers the following questions,
>
> - How do the Xen DT paramters differ from bare metal?
> - What existing code is reused with your patch?
> - How are Xen runtime services different from bare metal?
> - Why is it OK to force enable EFI runtime services for Xen?
>
> I think it would also be good to explicitly state that we do not
> expect to find both Xen EFI DT parameters and bare metal EFI DT
> parameters when performing the search; the two should be mutually
> exclusive.
>
>> > CC: Matt Fleming <matt@codeblueprint.co.uk>
>> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> > ---
>> > Drop using of EFI_PARAVIRT. Discussion can be found from [1].
>> > [1] https://lkml.org/lkml/2016/4/29/8
>> > ---
>> > arch/arm/include/asm/efi.h | 2 +
>> > arch/arm/xen/enlighten.c | 16 ++++++++
>> > arch/arm64/include/asm/efi.h | 2 +
>> > drivers/firmware/efi/arm-runtime.c | 70 +++++++++++++++++++++++---------
>> > drivers/firmware/efi/efi.c | 81 ++++++++++++++++++++++++++++++--------
>> > 5 files changed, 137 insertions(+), 34 deletions(-)
>> >
>> > diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
>> > index e0eea72..5ba4343 100644
>> > --- a/arch/arm/include/asm/efi.h
>> > +++ b/arch/arm/include/asm/efi.h
>> > @@ -52,9 +52,11 @@ static inline void efi_set_pgd(struct mm_struct *mm)
>> >
>> > void efi_virtmap_load(void);
>> > void efi_virtmap_unload(void);
>> > +int xen_arm_enable_runtime_services(void);
>> >
>> > #else
>> > #define efi_init()
>> > +#define xen_arm_enable_runtime_services() (0)
>> > #endif /* CONFIG_EFI */
>> >
>> > /* arch specific definitions used by the stub code */
>> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
>> > index 13e3e9f..3362870 100644
>> > --- a/arch/arm/xen/enlighten.c
>> > +++ b/arch/arm/xen/enlighten.c
>> > @@ -16,6 +16,7 @@
>> > #include <asm/xen/hypervisor.h>
>> > #include <asm/xen/hypercall.h>
>> > #include <asm/system_misc.h>
>> > +#include <asm/efi.h>
>> > #include <linux/interrupt.h>
>> > #include <linux/irqreturn.h>
>> > #include <linux/module.h>
>> > @@ -261,6 +262,13 @@ 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) &&
>> > + !efi_runtime_disabled())
>> > + set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
>> > + }
>> > +
>> > return 0;
>> > }
>> >
> The above comment could do with including similar information to the
> commit log as to why we want to force enable runtime services. For x86
> we have this,
>
> *
> * When EFI_PARAVIRT is in force then we could not map runtime
> * service memory region because we do not have direct access to it.
> * However, runtime services are available through proxy functions
> * (e.g. in case of Xen dom0 EFI implementation they call special
> * hypercall which executes relevant EFI functions) and that is why
> * they are always enabled.
> */
>
> We need something similar here.
>
>> > @@ -338,6 +346,7 @@ static int __init xen_guest_init(void)
>> > {
>> > struct xen_add_to_physmap xatp;
>> > struct shared_info *shared_info_page = NULL;
>> > + int ret;
>> >
>> > if (!xen_domain())
>> > return 0;
>> > @@ -352,6 +361,13 @@ static int __init xen_guest_init(void)
>> > return -ENODEV;
>> > }
>> >
>> > + if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_BOOT) &&
>> > + efi_enabled(EFI_RUNTIME_SERVICES)) {
>> > + ret = xen_arm_enable_runtime_services();
>> > + if (ret)
>> > + return ret;
>> > + }
>> > +
> Is it ever possible to have EFI_RUNTIME_SERVICES set but
> efi_enabled(EFI_BOOT) and IS_ENABLED(CONFIG_XEN_EFI) be false inside
> this function? If the answer is "no", I'd suggest just reducing this
> down to,
>
> /*
> * The fdt parsing code will have set EFI_RUNTIME_SERVICES if
> * it found Xen EFI parameters. Force enable runtime services.
> */
> if (efi_enabled(EFI_RUNTIME_SERVICES)) {
> ret = xen_arm_enable_runtime_services();
> if (ret)
> return ret;
> }
>
> But first, see my comments below.
>
>> > @@ -39,6 +40,33 @@ static struct mm_struct efi_mm = {
>> > .mmlist = LIST_HEAD_INIT(efi_mm.mmlist),
>> > };
>> >
>> > +static int __init efi_remap_init(void)
>> > +{
>> > + u64 mapsize;
>> > +
>> > + 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;
>> > +
>> > + 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");
>> > + return -ENOMEM;
>> > + }
>> > + set_bit(EFI_SYSTEM_TABLES, &efi.flags);
>> > +
>> > + return 0;
>> > +}
>> > +
>> > static bool __init efi_virtmap_init(void)
>> > {
>> > efi_memory_desc_t *md;
>> > @@ -75,7 +103,7 @@ static bool __init efi_virtmap_init(void)
>> > */
>> > static int __init arm_enable_runtime_services(void)
>> > {
>> > - u64 mapsize;
>> > + int ret;
>> >
>> > if (!efi_enabled(EFI_BOOT)) {
>> > pr_info("EFI services will not be available.\n");
>> > @@ -87,25 +115,14 @@ static int __init arm_enable_runtime_services(void)
>> > return 0;
>> > }
>> >
>> > - 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;
>> > + if (efi_enabled(EFI_RUNTIME_SERVICES)) {
>> > + pr_info("EFI runtime services access via paravirt.\n");
>> > + return 0;
>> > }
>> > - memmap.map_end = memmap.map + mapsize;
>> > - efi.memmap = &memmap;
>> >
>> > - 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");
>> > - return -ENOMEM;
>> > - }
>> > - set_bit(EFI_SYSTEM_TABLES, &efi.flags);
>> > + ret = efi_remap_init();
>> > + if (ret)
>> > + return ret;
>> >
>> > if (!efi_virtmap_init()) {
>> > pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
>> > @@ -122,6 +139,23 @@ static int __init arm_enable_runtime_services(void)
>> > }
>> > early_initcall(arm_enable_runtime_services);
>> >
>> > +int __init xen_arm_enable_runtime_services(void)
>> > +{
>> > + int ret;
>> > +
>> > + ret = efi_remap_init();
>> > + if (ret)
>> > + return ret;
>> > +
>> > + if (IS_ENABLED(CONFIG_XEN_EFI)) {
>> > + /* Set up runtime services function pointers for Xen Dom0 */
>> > + xen_efi_runtime_setup();
>> > + efi.runtime_version = efi.systab->hdr.revision;
>> > + }
>> > +
>> > + return 0;
>> > +}
> Since you call efi_remap_init() in both of these functions, couldn't
> you leave the existing code alone and bail after setting up the memory
> map and systab in arm_enable_runtime_services()?
>
> Also, why do you need to setup efi.runtime_version here? Isn't that
> done inside uefi_init()?
>
I don't see any codes which setup efi.runtime_version in uefi_init().
> Here is how I would have expected this patch to look:
>
> - Add FDT code to find hypervisor EFI params
>
> - Force enable EFI_RUNTIME_SERVICES for Xen and call
> xen_efi_runtime_setup() inside xen_guest_init()
>
> - Change arm_enable_runtime_services() to handle the case where
> EFI_RUNTIME_SERVICES is already set
>
> Am I misunderstanding some ordering issues?
Since xen_guest_init() and arm_enable_runtime_services() are both
early_initcall and the calling order is random I think. And when we call
xen_efi_runtime_setup() and setup efi.runtime_version in
xen_guest_init(), the efi.systab might be NULL. So it needs to map the
systanle explicitly before we use efi.systab.
Thanks,
--
Shannon
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
2016-05-11 13:35 ` Shannon Zhao
@ 2016-05-11 23:24 ` Matt Fleming
-1 siblings, 0 replies; 35+ messages in thread
From: Matt Fleming @ 2016-05-11 23:24 UTC (permalink / raw)
To: Shannon Zhao
Cc: Shannon Zhao, linux-arm-kernel, catalin.marinas, will.deacon,
sstabellini, stefano, julien.grall, ard.biesheuvel, xen-devel,
devicetree, linux-efi, linux-kernel, peter.huangpeng
On Wed, 11 May, at 09:35:47PM, Shannon Zhao wrote:
> >
> > Also, why do you need to setup efi.runtime_version here? Isn't that
> > done inside uefi_init()?
> >
> I don't see any codes which setup efi.runtime_version in uefi_init().
Look in the EFI tree,
https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/tree/drivers/firmware/efi/arm-init.c?h=next#n120
> > Here is how I would have expected this patch to look:
> >
> > - Add FDT code to find hypervisor EFI params
> >
> > - Force enable EFI_RUNTIME_SERVICES for Xen and call
> > xen_efi_runtime_setup() inside xen_guest_init()
> >
> > - Change arm_enable_runtime_services() to handle the case where
> > EFI_RUNTIME_SERVICES is already set
> >
> > Am I misunderstanding some ordering issues?
>
> Since xen_guest_init() and arm_enable_runtime_services() are both
> early_initcall and the calling order is random I think.
Urgh, right, I missed that.
> And when we call xen_efi_runtime_setup() and setup
> efi.runtime_version in xen_guest_init(), the efi.systab might be
> NULL. So it needs to map the systanle explicitly before we use
> efi.systab.
Could you explain why you need to copy efi.runtime_version instead of
letting the existing code in uefi_init() set it up?
Also, efi.systab isn't used by xen_efi_runtime_setup() as far I can
see, not sure why you need that either?
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-11 23:24 ` Matt Fleming
0 siblings, 0 replies; 35+ messages in thread
From: Matt Fleming @ 2016-05-11 23:24 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, 11 May, at 09:35:47PM, Shannon Zhao wrote:
> >
> > Also, why do you need to setup efi.runtime_version here? Isn't that
> > done inside uefi_init()?
> >
> I don't see any codes which setup efi.runtime_version in uefi_init().
Look in the EFI tree,
https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/tree/drivers/firmware/efi/arm-init.c?h=next#n120
> > Here is how I would have expected this patch to look:
> >
> > - Add FDT code to find hypervisor EFI params
> >
> > - Force enable EFI_RUNTIME_SERVICES for Xen and call
> > xen_efi_runtime_setup() inside xen_guest_init()
> >
> > - Change arm_enable_runtime_services() to handle the case where
> > EFI_RUNTIME_SERVICES is already set
> >
> > Am I misunderstanding some ordering issues?
>
> Since xen_guest_init() and arm_enable_runtime_services() are both
> early_initcall and the calling order is random I think.
Urgh, right, I missed that.
> And when we call xen_efi_runtime_setup() and setup
> efi.runtime_version in xen_guest_init(), the efi.systab might be
> NULL. So it needs to map the systanle explicitly before we use
> efi.systab.
Could you explain why you need to copy efi.runtime_version instead of
letting the existing code in uefi_init() set it up?
Also, efi.systab isn't used by xen_efi_runtime_setup() as far I can
see, not sure why you need that either?
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
2016-05-11 13:35 ` Shannon Zhao
` (2 preceding siblings ...)
(?)
@ 2016-05-11 23:24 ` Matt Fleming
-1 siblings, 0 replies; 35+ messages in thread
From: Matt Fleming @ 2016-05-11 23:24 UTC (permalink / raw)
To: Shannon Zhao
Cc: devicetree, sstabellini, ard.biesheuvel, catalin.marinas,
will.deacon, linux-kernel, xen-devel, stefano, julien.grall,
linux-efi, Shannon Zhao, peter.huangpeng, linux-arm-kernel
On Wed, 11 May, at 09:35:47PM, Shannon Zhao wrote:
> >
> > Also, why do you need to setup efi.runtime_version here? Isn't that
> > done inside uefi_init()?
> >
> I don't see any codes which setup efi.runtime_version in uefi_init().
Look in the EFI tree,
https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/tree/drivers/firmware/efi/arm-init.c?h=next#n120
> > Here is how I would have expected this patch to look:
> >
> > - Add FDT code to find hypervisor EFI params
> >
> > - Force enable EFI_RUNTIME_SERVICES for Xen and call
> > xen_efi_runtime_setup() inside xen_guest_init()
> >
> > - Change arm_enable_runtime_services() to handle the case where
> > EFI_RUNTIME_SERVICES is already set
> >
> > Am I misunderstanding some ordering issues?
>
> Since xen_guest_init() and arm_enable_runtime_services() are both
> early_initcall and the calling order is random I think.
Urgh, right, I missed that.
> And when we call xen_efi_runtime_setup() and setup
> efi.runtime_version in xen_guest_init(), the efi.systab might be
> NULL. So it needs to map the systanle explicitly before we use
> efi.systab.
Could you explain why you need to copy efi.runtime_version instead of
letting the existing code in uefi_init() set it up?
Also, efi.systab isn't used by xen_efi_runtime_setup() as far I can
see, not sure why you need that either?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-12 2:22 ` Shannon Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Shannon Zhao @ 2016-05-12 2:22 UTC (permalink / raw)
To: Matt Fleming, Shannon Zhao
Cc: linux-arm-kernel, catalin.marinas, will.deacon, sstabellini,
stefano, julien.grall, ard.biesheuvel, xen-devel, devicetree,
linux-efi, linux-kernel, peter.huangpeng
On 2016/5/12 7:24, Matt Fleming wrote:
> On Wed, 11 May, at 09:35:47PM, Shannon Zhao wrote:
>>> > >
>>> > > Also, why do you need to setup efi.runtime_version here? Isn't that
>>> > > done inside uefi_init()?
>>> > >
>> > I don't see any codes which setup efi.runtime_version in uefi_init().
>
> Look in the EFI tree,
>
> https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/tree/drivers/firmware/efi/arm-init.c?h=next#n120
>
Ah, there are some patches in EFI tree introducing this change, but they
are not in kernel matser but I didn't notice this, sorry.
>>> > > Here is how I would have expected this patch to look:
>>> > >
>>> > > - Add FDT code to find hypervisor EFI params
>>> > >
>>> > > - Force enable EFI_RUNTIME_SERVICES for Xen and call
>>> > > xen_efi_runtime_setup() inside xen_guest_init()
>>> > >
>>> > > - Change arm_enable_runtime_services() to handle the case where
>>> > > EFI_RUNTIME_SERVICES is already set
>>> > >
>>> > > Am I misunderstanding some ordering issues?
>> >
>> > Since xen_guest_init() and arm_enable_runtime_services() are both
>> > early_initcall and the calling order is random I think.
> Urgh, right, I missed that.
>
>> > And when we call xen_efi_runtime_setup() and setup
>> > efi.runtime_version in xen_guest_init(), the efi.systab might be
>> > NULL. So it needs to map the systanle explicitly before we use
>> > efi.systab.
> Could you explain why you need to copy efi.runtime_version instead of
> letting the existing code in uefi_init() set it up?
>
> Also, efi.systab isn't used by xen_efi_runtime_setup() as far I can
> see, not sure why you need that either?
As said above, I will rebase this patch on top of the EFI next branch.
Thanks,
--
Shannon
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-12 2:22 ` Shannon Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Shannon Zhao @ 2016-05-12 2:22 UTC (permalink / raw)
To: Matt Fleming, Shannon Zhao
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
catalin.marinas-5wv7dgnIgG8, will.deacon-5wv7dgnIgG8,
sstabellini-DgEjT+Ai2ygdnm+yROfE0A,
stefano-yd54mjeZNzVBDgjK7y7TUQ, julien.grall-5wv7dgnIgG8,
ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
xen-devel-GuqFBffKawuEi8DpZVb4nw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-efi-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
peter.huangpeng-hv44wF8Li93QT0dZR+AlfA
On 2016/5/12 7:24, Matt Fleming wrote:
> On Wed, 11 May, at 09:35:47PM, Shannon Zhao wrote:
>>> > >
>>> > > Also, why do you need to setup efi.runtime_version here? Isn't that
>>> > > done inside uefi_init()?
>>> > >
>> > I don't see any codes which setup efi.runtime_version in uefi_init().
>
> Look in the EFI tree,
>
> https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/tree/drivers/firmware/efi/arm-init.c?h=next#n120
>
Ah, there are some patches in EFI tree introducing this change, but they
are not in kernel matser but I didn't notice this, sorry.
>>> > > Here is how I would have expected this patch to look:
>>> > >
>>> > > - Add FDT code to find hypervisor EFI params
>>> > >
>>> > > - Force enable EFI_RUNTIME_SERVICES for Xen and call
>>> > > xen_efi_runtime_setup() inside xen_guest_init()
>>> > >
>>> > > - Change arm_enable_runtime_services() to handle the case where
>>> > > EFI_RUNTIME_SERVICES is already set
>>> > >
>>> > > Am I misunderstanding some ordering issues?
>> >
>> > Since xen_guest_init() and arm_enable_runtime_services() are both
>> > early_initcall and the calling order is random I think.
> Urgh, right, I missed that.
>
>> > And when we call xen_efi_runtime_setup() and setup
>> > efi.runtime_version in xen_guest_init(), the efi.systab might be
>> > NULL. So it needs to map the systanle explicitly before we use
>> > efi.systab.
> Could you explain why you need to copy efi.runtime_version instead of
> letting the existing code in uefi_init() set it up?
>
> Also, efi.systab isn't used by xen_efi_runtime_setup() as far I can
> see, not sure why you need that either?
As said above, I will rebase this patch on top of the EFI next branch.
Thanks,
--
Shannon
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-12 2:22 ` Shannon Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Shannon Zhao @ 2016-05-12 2:22 UTC (permalink / raw)
To: linux-arm-kernel
On 2016/5/12 7:24, Matt Fleming wrote:
> On Wed, 11 May, at 09:35:47PM, Shannon Zhao wrote:
>>> > >
>>> > > Also, why do you need to setup efi.runtime_version here? Isn't that
>>> > > done inside uefi_init()?
>>> > >
>> > I don't see any codes which setup efi.runtime_version in uefi_init().
>
> Look in the EFI tree,
>
> https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/tree/drivers/firmware/efi/arm-init.c?h=next#n120
>
Ah, there are some patches in EFI tree introducing this change, but they
are not in kernel matser but I didn't notice this, sorry.
>>> > > Here is how I would have expected this patch to look:
>>> > >
>>> > > - Add FDT code to find hypervisor EFI params
>>> > >
>>> > > - Force enable EFI_RUNTIME_SERVICES for Xen and call
>>> > > xen_efi_runtime_setup() inside xen_guest_init()
>>> > >
>>> > > - Change arm_enable_runtime_services() to handle the case where
>>> > > EFI_RUNTIME_SERVICES is already set
>>> > >
>>> > > Am I misunderstanding some ordering issues?
>> >
>> > Since xen_guest_init() and arm_enable_runtime_services() are both
>> > early_initcall and the calling order is random I think.
> Urgh, right, I missed that.
>
>> > And when we call xen_efi_runtime_setup() and setup
>> > efi.runtime_version in xen_guest_init(), the efi.systab might be
>> > NULL. So it needs to map the systanle explicitly before we use
>> > efi.systab.
> Could you explain why you need to copy efi.runtime_version instead of
> letting the existing code in uefi_init() set it up?
>
> Also, efi.systab isn't used by xen_efi_runtime_setup() as far I can
> see, not sure why you need that either?
As said above, I will rebase this patch on top of the EFI next branch.
Thanks,
--
Shannon
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
2016-05-11 23:24 ` Matt Fleming
(?)
(?)
@ 2016-05-12 2:22 ` Shannon Zhao
-1 siblings, 0 replies; 35+ messages in thread
From: Shannon Zhao @ 2016-05-12 2:22 UTC (permalink / raw)
To: Matt Fleming, Shannon Zhao
Cc: devicetree, sstabellini, ard.biesheuvel, catalin.marinas,
will.deacon, linux-kernel, xen-devel, stefano, julien.grall,
linux-efi, peter.huangpeng, linux-arm-kernel
On 2016/5/12 7:24, Matt Fleming wrote:
> On Wed, 11 May, at 09:35:47PM, Shannon Zhao wrote:
>>> > >
>>> > > Also, why do you need to setup efi.runtime_version here? Isn't that
>>> > > done inside uefi_init()?
>>> > >
>> > I don't see any codes which setup efi.runtime_version in uefi_init().
>
> Look in the EFI tree,
>
> https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/tree/drivers/firmware/efi/arm-init.c?h=next#n120
>
Ah, there are some patches in EFI tree introducing this change, but they
are not in kernel matser but I didn't notice this, sorry.
>>> > > Here is how I would have expected this patch to look:
>>> > >
>>> > > - Add FDT code to find hypervisor EFI params
>>> > >
>>> > > - Force enable EFI_RUNTIME_SERVICES for Xen and call
>>> > > xen_efi_runtime_setup() inside xen_guest_init()
>>> > >
>>> > > - Change arm_enable_runtime_services() to handle the case where
>>> > > EFI_RUNTIME_SERVICES is already set
>>> > >
>>> > > Am I misunderstanding some ordering issues?
>> >
>> > Since xen_guest_init() and arm_enable_runtime_services() are both
>> > early_initcall and the calling order is random I think.
> Urgh, right, I missed that.
>
>> > And when we call xen_efi_runtime_setup() and setup
>> > efi.runtime_version in xen_guest_init(), the efi.systab might be
>> > NULL. So it needs to map the systanle explicitly before we use
>> > efi.systab.
> Could you explain why you need to copy efi.runtime_version instead of
> letting the existing code in uefi_init() set it up?
>
> Also, efi.systab isn't used by xen_efi_runtime_setup() as far I can
> see, not sure why you need that either?
As said above, I will rebase this patch on top of the EFI next branch.
Thanks,
--
Shannon
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-12 10:04 ` Matt Fleming
0 siblings, 0 replies; 35+ messages in thread
From: Matt Fleming @ 2016-05-12 10:04 UTC (permalink / raw)
To: Shannon Zhao
Cc: Shannon Zhao, linux-arm-kernel, catalin.marinas, will.deacon,
sstabellini, stefano, julien.grall, ard.biesheuvel, xen-devel,
devicetree, linux-efi, linux-kernel, peter.huangpeng,
Ingo Molnar, Thomas Gleixner, H. Peter Anvin, Linus Torvalds
On Thu, 12 May, at 10:22:07AM, Shannon Zhao wrote:
>
> As said above, I will rebase this patch on top of the EFI next branch.
OK thanks.
Note that it is not possible to escape merge conflicts, since there
are changes in the xen tip tree that are not in the EFI next branch
and vice versa.
For example these commits from xen/linux-next look relevant,
8e147fcc3ffa ("FDT: Add a helper to get the subnode by given name")
37060935dc04 ("ARM64: XEN: Add a function to initialize Xen specific UEFI runtime services")
acb2c923a860 ("XEN: EFI: Move x86 specific codes to architecture directory")
055be2d42408 ("ARM: Xen: Document UEFI support on Xen ARM virtual platforms")
3915fea959b6 ("ARM: XEN: Move xen_early_init() before efi_init()")
Linus, Stefano, tip-folks: I'm soliciting opinions on the correct way
to handle this inter-tree dependency where we've got changes to EFI
code in two separate trees and Shannon wants to write patches on top
of both.
I'm guessing the best solution is to merge xen/linux-next and efi/next
into a topic branch either in the EFI tree or Xen tree, but I want to
be cautious of the branch history that will create.
(In hindsight all of these change should have probably gone via the
EFI tree.)
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-12 10:04 ` Matt Fleming
0 siblings, 0 replies; 35+ messages in thread
From: Matt Fleming @ 2016-05-12 10:04 UTC (permalink / raw)
To: Shannon Zhao
Cc: Shannon Zhao, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
catalin.marinas-5wv7dgnIgG8, will.deacon-5wv7dgnIgG8,
sstabellini-DgEjT+Ai2ygdnm+yROfE0A,
stefano-yd54mjeZNzVBDgjK7y7TUQ, julien.grall-5wv7dgnIgG8,
ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
xen-devel-GuqFBffKawuEi8DpZVb4nw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-efi-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
peter.huangpeng-hv44wF8Li93QT0dZR+AlfA, Ingo Molnar,
Thomas Gleixner, H. Peter Anvin, Linus Torvalds
On Thu, 12 May, at 10:22:07AM, Shannon Zhao wrote:
>
> As said above, I will rebase this patch on top of the EFI next branch.
OK thanks.
Note that it is not possible to escape merge conflicts, since there
are changes in the xen tip tree that are not in the EFI next branch
and vice versa.
For example these commits from xen/linux-next look relevant,
8e147fcc3ffa ("FDT: Add a helper to get the subnode by given name")
37060935dc04 ("ARM64: XEN: Add a function to initialize Xen specific UEFI runtime services")
acb2c923a860 ("XEN: EFI: Move x86 specific codes to architecture directory")
055be2d42408 ("ARM: Xen: Document UEFI support on Xen ARM virtual platforms")
3915fea959b6 ("ARM: XEN: Move xen_early_init() before efi_init()")
Linus, Stefano, tip-folks: I'm soliciting opinions on the correct way
to handle this inter-tree dependency where we've got changes to EFI
code in two separate trees and Shannon wants to write patches on top
of both.
I'm guessing the best solution is to merge xen/linux-next and efi/next
into a topic branch either in the EFI tree or Xen tree, but I want to
be cautious of the branch history that will create.
(In hindsight all of these change should have probably gone via the
EFI tree.)
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-12 10:04 ` Matt Fleming
0 siblings, 0 replies; 35+ messages in thread
From: Matt Fleming @ 2016-05-12 10:04 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 12 May, at 10:22:07AM, Shannon Zhao wrote:
>
> As said above, I will rebase this patch on top of the EFI next branch.
OK thanks.
Note that it is not possible to escape merge conflicts, since there
are changes in the xen tip tree that are not in the EFI next branch
and vice versa.
For example these commits from xen/linux-next look relevant,
8e147fcc3ffa ("FDT: Add a helper to get the subnode by given name")
37060935dc04 ("ARM64: XEN: Add a function to initialize Xen specific UEFI runtime services")
acb2c923a860 ("XEN: EFI: Move x86 specific codes to architecture directory")
055be2d42408 ("ARM: Xen: Document UEFI support on Xen ARM virtual platforms")
3915fea959b6 ("ARM: XEN: Move xen_early_init() before efi_init()")
Linus, Stefano, tip-folks: I'm soliciting opinions on the correct way
to handle this inter-tree dependency where we've got changes to EFI
code in two separate trees and Shannon wants to write patches on top
of both.
I'm guessing the best solution is to merge xen/linux-next and efi/next
into a topic branch either in the EFI tree or Xen tree, but I want to
be cautious of the branch history that will create.
(In hindsight all of these change should have probably gone via the
EFI tree.)
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
2016-05-12 2:22 ` Shannon Zhao
` (2 preceding siblings ...)
(?)
@ 2016-05-12 10:04 ` Matt Fleming
-1 siblings, 0 replies; 35+ messages in thread
From: Matt Fleming @ 2016-05-12 10:04 UTC (permalink / raw)
To: Shannon Zhao
Cc: devicetree, sstabellini, ard.biesheuvel, catalin.marinas,
will.deacon, linux-kernel, xen-devel, stefano, julien.grall,
linux-efi, Thomas Gleixner, Shannon Zhao, H. Peter Anvin,
peter.huangpeng, Linus Torvalds, Ingo Molnar, linux-arm-kernel
On Thu, 12 May, at 10:22:07AM, Shannon Zhao wrote:
>
> As said above, I will rebase this patch on top of the EFI next branch.
OK thanks.
Note that it is not possible to escape merge conflicts, since there
are changes in the xen tip tree that are not in the EFI next branch
and vice versa.
For example these commits from xen/linux-next look relevant,
8e147fcc3ffa ("FDT: Add a helper to get the subnode by given name")
37060935dc04 ("ARM64: XEN: Add a function to initialize Xen specific UEFI runtime services")
acb2c923a860 ("XEN: EFI: Move x86 specific codes to architecture directory")
055be2d42408 ("ARM: Xen: Document UEFI support on Xen ARM virtual platforms")
3915fea959b6 ("ARM: XEN: Move xen_early_init() before efi_init()")
Linus, Stefano, tip-folks: I'm soliciting opinions on the correct way
to handle this inter-tree dependency where we've got changes to EFI
code in two separate trees and Shannon wants to write patches on top
of both.
I'm guessing the best solution is to merge xen/linux-next and efi/next
into a topic branch either in the EFI tree or Xen tree, but I want to
be cautious of the branch history that will create.
(In hindsight all of these change should have probably gone via the
EFI tree.)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-12 10:58 ` Stefano Stabellini
0 siblings, 0 replies; 35+ messages in thread
From: Stefano Stabellini @ 2016-05-12 10:58 UTC (permalink / raw)
To: Matt Fleming
Cc: Shannon Zhao, Shannon Zhao, linux-arm-kernel, catalin.marinas,
will.deacon, sstabellini, stefano, julien.grall, ard.biesheuvel,
xen-devel, devicetree, linux-efi, linux-kernel, peter.huangpeng,
Ingo Molnar, Thomas Gleixner, H. Peter Anvin, Linus Torvalds
On Thu, 12 May 2016, Matt Fleming wrote:
> On Thu, 12 May, at 10:22:07AM, Shannon Zhao wrote:
> >
> > As said above, I will rebase this patch on top of the EFI next branch.
>
> OK thanks.
>
> Note that it is not possible to escape merge conflicts, since there
> are changes in the xen tip tree that are not in the EFI next branch
> and vice versa.
>
> For example these commits from xen/linux-next look relevant,
>
> 8e147fcc3ffa ("FDT: Add a helper to get the subnode by given name")
> 37060935dc04 ("ARM64: XEN: Add a function to initialize Xen specific UEFI runtime services")
> acb2c923a860 ("XEN: EFI: Move x86 specific codes to architecture directory")
> 055be2d42408 ("ARM: Xen: Document UEFI support on Xen ARM virtual platforms")
> 3915fea959b6 ("ARM: XEN: Move xen_early_init() before efi_init()")
>From a diffstat perspective, the changes introduced by these commits
affect drivers/of/fdt.c, arch/arm/xen, arch/x86/xen, drivers/xen and
little else. I don't think they should cause merge troubles.
> Linus, Stefano, tip-folks: I'm soliciting opinions on the correct way
> to handle this inter-tree dependency where we've got changes to EFI
> code in two separate trees and Shannon wants to write patches on top
> of both.
>
> I'm guessing the best solution is to merge xen/linux-next and efi/next
> into a topic branch either in the EFI tree or Xen tree, but I want to
> be cautious of the branch history that will create.
I am OK with that. You and I will have to be careful with the pull
requests.
> (In hindsight all of these change should have probably gone via the
> EFI tree.)
That is still possible if deemed best.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-12 10:58 ` Stefano Stabellini
0 siblings, 0 replies; 35+ messages in thread
From: Stefano Stabellini @ 2016-05-12 10:58 UTC (permalink / raw)
To: Matt Fleming
Cc: Shannon Zhao, Shannon Zhao,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
catalin.marinas-5wv7dgnIgG8, will.deacon-5wv7dgnIgG8,
sstabellini-DgEjT+Ai2ygdnm+yROfE0A,
stefano-yd54mjeZNzVBDgjK7y7TUQ, julien.grall-5wv7dgnIgG8,
ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A,
xen-devel-GuqFBffKawuEi8DpZVb4nw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-efi-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
peter.huangpeng-hv44wF8Li93QT0dZR+AlfA, Ingo Molnar,
Thomas Gleixner, H. Peter Anvin, Linus Torvalds
On Thu, 12 May 2016, Matt Fleming wrote:
> On Thu, 12 May, at 10:22:07AM, Shannon Zhao wrote:
> >
> > As said above, I will rebase this patch on top of the EFI next branch.
>
> OK thanks.
>
> Note that it is not possible to escape merge conflicts, since there
> are changes in the xen tip tree that are not in the EFI next branch
> and vice versa.
>
> For example these commits from xen/linux-next look relevant,
>
> 8e147fcc3ffa ("FDT: Add a helper to get the subnode by given name")
> 37060935dc04 ("ARM64: XEN: Add a function to initialize Xen specific UEFI runtime services")
> acb2c923a860 ("XEN: EFI: Move x86 specific codes to architecture directory")
> 055be2d42408 ("ARM: Xen: Document UEFI support on Xen ARM virtual platforms")
> 3915fea959b6 ("ARM: XEN: Move xen_early_init() before efi_init()")
>From a diffstat perspective, the changes introduced by these commits
affect drivers/of/fdt.c, arch/arm/xen, arch/x86/xen, drivers/xen and
little else. I don't think they should cause merge troubles.
> Linus, Stefano, tip-folks: I'm soliciting opinions on the correct way
> to handle this inter-tree dependency where we've got changes to EFI
> code in two separate trees and Shannon wants to write patches on top
> of both.
>
> I'm guessing the best solution is to merge xen/linux-next and efi/next
> into a topic branch either in the EFI tree or Xen tree, but I want to
> be cautious of the branch history that will create.
I am OK with that. You and I will have to be careful with the pull
requests.
> (In hindsight all of these change should have probably gone via the
> EFI tree.)
That is still possible if deemed best.
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-12 10:58 ` Stefano Stabellini
0 siblings, 0 replies; 35+ messages in thread
From: Stefano Stabellini @ 2016-05-12 10:58 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 12 May 2016, Matt Fleming wrote:
> On Thu, 12 May, at 10:22:07AM, Shannon Zhao wrote:
> >
> > As said above, I will rebase this patch on top of the EFI next branch.
>
> OK thanks.
>
> Note that it is not possible to escape merge conflicts, since there
> are changes in the xen tip tree that are not in the EFI next branch
> and vice versa.
>
> For example these commits from xen/linux-next look relevant,
>
> 8e147fcc3ffa ("FDT: Add a helper to get the subnode by given name")
> 37060935dc04 ("ARM64: XEN: Add a function to initialize Xen specific UEFI runtime services")
> acb2c923a860 ("XEN: EFI: Move x86 specific codes to architecture directory")
> 055be2d42408 ("ARM: Xen: Document UEFI support on Xen ARM virtual platforms")
> 3915fea959b6 ("ARM: XEN: Move xen_early_init() before efi_init()")
>From a diffstat perspective, the changes introduced by these commits
affect drivers/of/fdt.c, arch/arm/xen, arch/x86/xen, drivers/xen and
little else. I don't think they should cause merge troubles.
> Linus, Stefano, tip-folks: I'm soliciting opinions on the correct way
> to handle this inter-tree dependency where we've got changes to EFI
> code in two separate trees and Shannon wants to write patches on top
> of both.
>
> I'm guessing the best solution is to merge xen/linux-next and efi/next
> into a topic branch either in the EFI tree or Xen tree, but I want to
> be cautious of the branch history that will create.
I am OK with that. You and I will have to be careful with the pull
requests.
> (In hindsight all of these change should have probably gone via the
> EFI tree.)
That is still possible if deemed best.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
2016-05-12 10:04 ` Matt Fleming
` (2 preceding siblings ...)
(?)
@ 2016-05-12 10:58 ` Stefano Stabellini
-1 siblings, 0 replies; 35+ messages in thread
From: Stefano Stabellini @ 2016-05-12 10:58 UTC (permalink / raw)
To: Matt Fleming
Cc: devicetree, sstabellini, ard.biesheuvel, catalin.marinas,
H. Peter Anvin, will.deacon, linux-kernel, xen-devel, stefano,
julien.grall, linux-efi, Thomas Gleixner, Shannon Zhao,
Shannon Zhao, peter.huangpeng, Linus Torvalds, Ingo Molnar,
linux-arm-kernel
On Thu, 12 May 2016, Matt Fleming wrote:
> On Thu, 12 May, at 10:22:07AM, Shannon Zhao wrote:
> >
> > As said above, I will rebase this patch on top of the EFI next branch.
>
> OK thanks.
>
> Note that it is not possible to escape merge conflicts, since there
> are changes in the xen tip tree that are not in the EFI next branch
> and vice versa.
>
> For example these commits from xen/linux-next look relevant,
>
> 8e147fcc3ffa ("FDT: Add a helper to get the subnode by given name")
> 37060935dc04 ("ARM64: XEN: Add a function to initialize Xen specific UEFI runtime services")
> acb2c923a860 ("XEN: EFI: Move x86 specific codes to architecture directory")
> 055be2d42408 ("ARM: Xen: Document UEFI support on Xen ARM virtual platforms")
> 3915fea959b6 ("ARM: XEN: Move xen_early_init() before efi_init()")
From a diffstat perspective, the changes introduced by these commits
affect drivers/of/fdt.c, arch/arm/xen, arch/x86/xen, drivers/xen and
little else. I don't think they should cause merge troubles.
> Linus, Stefano, tip-folks: I'm soliciting opinions on the correct way
> to handle this inter-tree dependency where we've got changes to EFI
> code in two separate trees and Shannon wants to write patches on top
> of both.
>
> I'm guessing the best solution is to merge xen/linux-next and efi/next
> into a topic branch either in the EFI tree or Xen tree, but I want to
> be cautious of the branch history that will create.
I am OK with that. You and I will have to be careful with the pull
requests.
> (In hindsight all of these change should have probably gone via the
> EFI tree.)
That is still possible if deemed best.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI
@ 2016-05-06 8:32 Shannon Zhao
0 siblings, 0 replies; 35+ messages in thread
From: Shannon Zhao @ 2016-05-06 8:32 UTC (permalink / raw)
To: linux-arm-kernel, matt
Cc: devicetree, sstabellini, ard.biesheuvel, catalin.marinas,
will.deacon, linux-kernel, xen-devel, stefano, julien.grall,
linux-efi, shannon.zhao, peter.huangpeng
From: Shannon Zhao <shannon.zhao@linaro.org>
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>
---
Drop using of EFI_PARAVIRT. Discussion can be found from [1].
[1] https://lkml.org/lkml/2016/4/29/8
---
arch/arm/include/asm/efi.h | 2 +
arch/arm/xen/enlighten.c | 16 ++++++++
arch/arm64/include/asm/efi.h | 2 +
drivers/firmware/efi/arm-runtime.c | 70 +++++++++++++++++++++++---------
drivers/firmware/efi/efi.c | 81 ++++++++++++++++++++++++++++++--------
5 files changed, 137 insertions(+), 34 deletions(-)
diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index e0eea72..5ba4343 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -52,9 +52,11 @@ static inline void efi_set_pgd(struct mm_struct *mm)
void efi_virtmap_load(void);
void efi_virtmap_unload(void);
+int xen_arm_enable_runtime_services(void);
#else
#define efi_init()
+#define xen_arm_enable_runtime_services() (0)
#endif /* CONFIG_EFI */
/* arch specific definitions used by the stub code */
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 13e3e9f..3362870 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -16,6 +16,7 @@
#include <asm/xen/hypervisor.h>
#include <asm/xen/hypercall.h>
#include <asm/system_misc.h>
+#include <asm/efi.h>
#include <linux/interrupt.h>
#include <linux/irqreturn.h>
#include <linux/module.h>
@@ -261,6 +262,13 @@ 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) &&
+ !efi_runtime_disabled())
+ set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
+ }
+
return 0;
}
@@ -338,6 +346,7 @@ static int __init xen_guest_init(void)
{
struct xen_add_to_physmap xatp;
struct shared_info *shared_info_page = NULL;
+ int ret;
if (!xen_domain())
return 0;
@@ -352,6 +361,13 @@ static int __init xen_guest_init(void)
return -ENODEV;
}
+ if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_BOOT) &&
+ efi_enabled(EFI_RUNTIME_SERVICES)) {
+ ret = xen_arm_enable_runtime_services();
+ if (ret)
+ return ret;
+ }
+
shared_info_page = (struct shared_info *)get_zeroed_page(GFP_KERNEL);
if (!shared_info_page) {
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 8e88a69..574bee5 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -8,8 +8,10 @@
#ifdef CONFIG_EFI
extern void efi_init(void);
+int xen_arm_enable_runtime_services(void);
#else
#define efi_init()
+#define xen_arm_enable_runtime_services() (0)
#endif
int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
index 6ae21e4..211ec10 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;
@@ -39,6 +40,33 @@ static struct mm_struct efi_mm = {
.mmlist = LIST_HEAD_INIT(efi_mm.mmlist),
};
+static int __init efi_remap_init(void)
+{
+ u64 mapsize;
+
+ 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;
+
+ 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");
+ return -ENOMEM;
+ }
+ set_bit(EFI_SYSTEM_TABLES, &efi.flags);
+
+ return 0;
+}
+
static bool __init efi_virtmap_init(void)
{
efi_memory_desc_t *md;
@@ -75,7 +103,7 @@ static bool __init efi_virtmap_init(void)
*/
static int __init arm_enable_runtime_services(void)
{
- u64 mapsize;
+ int ret;
if (!efi_enabled(EFI_BOOT)) {
pr_info("EFI services will not be available.\n");
@@ -87,25 +115,14 @@ static int __init arm_enable_runtime_services(void)
return 0;
}
- 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;
+ if (efi_enabled(EFI_RUNTIME_SERVICES)) {
+ pr_info("EFI runtime services access via paravirt.\n");
+ return 0;
}
- memmap.map_end = memmap.map + mapsize;
- efi.memmap = &memmap;
- 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");
- return -ENOMEM;
- }
- set_bit(EFI_SYSTEM_TABLES, &efi.flags);
+ ret = efi_remap_init();
+ if (ret)
+ return ret;
if (!efi_virtmap_init()) {
pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
@@ -122,6 +139,23 @@ static int __init arm_enable_runtime_services(void)
}
early_initcall(arm_enable_runtime_services);
+int __init xen_arm_enable_runtime_services(void)
+{
+ int ret;
+
+ ret = efi_remap_init();
+ if (ret)
+ return ret;
+
+ if (IS_ENABLED(CONFIG_XEN_EFI)) {
+ /* Set up runtime services function pointers for Xen Dom0 */
+ xen_efi_runtime_setup();
+ efi.runtime_version = efi.systab->hdr.revision;
+ }
+
+ return 0;
+}
+
void efi_virtmap_load(void)
{
preempt_disable();
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 3a69ed5..f586e1e 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,91 @@ 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;
+ const char *subnode;
+ struct params *params;
+} dt_params[] = {
+ { "hypervisor", "uefi", xen_fdt_params },
+ { "chosen", NULL, 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++) {
+ const char *subnode = dt_params[i].subnode;
+
+ if (depth != 1 || strcmp(uname, dt_params[i].uname) != 0) {
+ info->missing = dt_params[i].params[0].name;
+ continue;
+ }
+
+ if (subnode) {
+ node = of_get_flat_dt_subnode_by_name(node, subnode);
+ if (node < 0)
+ return 0;
+ }
+
+ 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 +584,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;
}
--
2.0.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread
end of thread, other threads:[~2016-05-12 10:58 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-06 8:32 [PATCH] Xen: EFI: Parse DT parameters for Xen specific UEFI Shannon Zhao
2016-05-06 8:32 ` Shannon Zhao
2016-05-06 8:32 ` Shannon Zhao
2016-05-06 15:52 ` Mathieu Poirier
2016-05-06 15:52 ` Mathieu Poirier
2016-05-06 15:52 ` Mathieu Poirier
2016-05-11 12:29 ` Matt Fleming
2016-05-11 12:29 ` Matt Fleming
2016-05-11 12:29 ` Matt Fleming
2016-05-11 12:29 ` Matt Fleming
2016-05-06 15:52 ` Mathieu Poirier
2016-05-11 12:27 ` Matt Fleming
2016-05-11 12:27 ` Matt Fleming
2016-05-11 12:27 ` Matt Fleming
2016-05-11 12:27 ` Matt Fleming
2016-05-11 13:35 ` Shannon Zhao
2016-05-11 13:35 ` Shannon Zhao
2016-05-11 13:35 ` Shannon Zhao
2016-05-11 13:35 ` Shannon Zhao
2016-05-11 23:24 ` Matt Fleming
2016-05-11 23:24 ` Matt Fleming
2016-05-12 2:22 ` Shannon Zhao
2016-05-12 2:22 ` Shannon Zhao
2016-05-12 2:22 ` Shannon Zhao
2016-05-12 10:04 ` Matt Fleming
2016-05-12 10:04 ` Matt Fleming
2016-05-12 10:04 ` Matt Fleming
2016-05-12 10:58 ` Stefano Stabellini
2016-05-12 10:58 ` Stefano Stabellini
2016-05-12 10:58 ` Stefano Stabellini
2016-05-12 10:58 ` Stefano Stabellini
2016-05-12 10:04 ` Matt Fleming
2016-05-12 2:22 ` Shannon Zhao
2016-05-11 23:24 ` Matt Fleming
-- strict thread matches above, loose matches on Subject: below --
2016-05-06 8:32 Shannon Zhao
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.