Hey,
This patch series adds EFI support for Xen dom0 guests.
It is based on Jan Beulich and Tang Liang work. I was
trying to take into account all previous comments,
however, if I missed something sorry for that.
Daniel
arch/x86/kernel/setup.c | 4 +-
arch/x86/platform/efi/efi.c | 77 ++++++++++-------
arch/x86/xen/enlighten.c | 24 ++++++
drivers/firmware/efi/efi.c | 26 +++---
drivers/xen/Kconfig | 4 +
drivers/xen/Makefile | 3 +
drivers/xen/efi.c | 374 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/efi.h | 3 +-
include/xen/interface/platform.h | 123 +++++++++++++++++++++++++++
9 files changed, 595 insertions(+), 43 deletions(-)
Daniel Kiper (7):
efi: Use early_mem*() instead of early_io*()
efi: Introduce EFI_NO_DIRECT flag
xen: Define EFI related stuff
xen: Put EFI machinery in place
arch/x86: Replace plain strings with constants
arch/x86: Remove redundant set_bit() call
arch/x86: Comment out efi_set_rtc_mmss()
Define constants and structures which are needed to properly
execute EFI related hypercall in Xen dom0.
This patch is based on Jan Beulich and Tang Liang work.
v5 - suggestions/fixes:
- improve commit message
(suggested by David Vrabel).
v4 - suggestions/fixes:
- change some types from generic to Xen specific ones
(suggested by Stefano Stabellini),
- do some formating changes
(suggested by Jan Beulich).
Signed-off-by: Jan Beulich <[email protected]>
Signed-off-by: Tang Liang <[email protected]>
Signed-off-by: Daniel Kiper <[email protected]>
---
include/xen/interface/platform.h | 123 ++++++++++++++++++++++++++++++++++++++
1 file changed, 123 insertions(+)
diff --git a/include/xen/interface/platform.h b/include/xen/interface/platform.h
index f1331e3..55deeb7 100644
--- a/include/xen/interface/platform.h
+++ b/include/xen/interface/platform.h
@@ -108,11 +108,113 @@ struct xenpf_platform_quirk {
};
DEFINE_GUEST_HANDLE_STRUCT(xenpf_platform_quirk_t);
+#define XENPF_efi_runtime_call 49
+#define XEN_EFI_get_time 1
+#define XEN_EFI_set_time 2
+#define XEN_EFI_get_wakeup_time 3
+#define XEN_EFI_set_wakeup_time 4
+#define XEN_EFI_get_next_high_monotonic_count 5
+#define XEN_EFI_get_variable 6
+#define XEN_EFI_set_variable 7
+#define XEN_EFI_get_next_variable_name 8
+#define XEN_EFI_query_variable_info 9
+#define XEN_EFI_query_capsule_capabilities 10
+#define XEN_EFI_update_capsule 11
+
+struct xenpf_efi_runtime_call {
+ uint32_t function;
+ /*
+ * This field is generally used for per sub-function flags (defined
+ * below), except for the XEN_EFI_get_next_high_monotonic_count case,
+ * where it holds the single returned value.
+ */
+ uint32_t misc;
+ xen_ulong_t status;
+ union {
+#define XEN_EFI_GET_TIME_SET_CLEARS_NS 0x00000001
+ struct {
+ struct xenpf_efi_time {
+ uint16_t year;
+ uint8_t month;
+ uint8_t day;
+ uint8_t hour;
+ uint8_t min;
+ uint8_t sec;
+ uint32_t ns;
+ int16_t tz;
+ uint8_t daylight;
+ } time;
+ uint32_t resolution;
+ uint32_t accuracy;
+ } get_time;
+
+ struct xenpf_efi_time set_time;
+
+#define XEN_EFI_GET_WAKEUP_TIME_ENABLED 0x00000001
+#define XEN_EFI_GET_WAKEUP_TIME_PENDING 0x00000002
+ struct xenpf_efi_time get_wakeup_time;
+
+#define XEN_EFI_SET_WAKEUP_TIME_ENABLE 0x00000001
+#define XEN_EFI_SET_WAKEUP_TIME_ENABLE_ONLY 0x00000002
+ struct xenpf_efi_time set_wakeup_time;
+
+#define XEN_EFI_VARIABLE_NON_VOLATILE 0x00000001
+#define XEN_EFI_VARIABLE_BOOTSERVICE_ACCESS 0x00000002
+#define XEN_EFI_VARIABLE_RUNTIME_ACCESS 0x00000004
+ struct {
+ GUEST_HANDLE(void) name; /* UCS-2/UTF-16 string */
+ xen_ulong_t size;
+ GUEST_HANDLE(void) data;
+ struct xenpf_efi_guid {
+ uint32_t data1;
+ uint16_t data2;
+ uint16_t data3;
+ uint8_t data4[8];
+ } vendor_guid;
+ } get_variable, set_variable;
+
+ struct {
+ xen_ulong_t size;
+ GUEST_HANDLE(void) name; /* UCS-2/UTF-16 string */
+ struct xenpf_efi_guid vendor_guid;
+ } get_next_variable_name;
+
+ struct {
+ uint32_t attr;
+ uint64_t max_store_size;
+ uint64_t remain_store_size;
+ uint64_t max_size;
+ } query_variable_info;
+
+ struct {
+ GUEST_HANDLE(void) capsule_header_array;
+ xen_ulong_t capsule_count;
+ uint64_t max_capsule_size;
+ uint32_t reset_type;
+ } query_capsule_capabilities;
+
+ struct {
+ GUEST_HANDLE(void) capsule_header_array;
+ xen_ulong_t capsule_count;
+ uint64_t sg_list; /* machine address */
+ } update_capsule;
+ } u;
+};
+DEFINE_GUEST_HANDLE_STRUCT(xenpf_efi_runtime_call);
+
+#define XEN_FW_EFI_VERSION 0
+#define XEN_FW_EFI_CONFIG_TABLE 1
+#define XEN_FW_EFI_VENDOR 2
+#define XEN_FW_EFI_MEM_INFO 3
+#define XEN_FW_EFI_RT_VERSION 4
+
#define XENPF_firmware_info 50
#define XEN_FW_DISK_INFO 1 /* from int 13 AH=08/41/48 */
#define XEN_FW_DISK_MBR_SIGNATURE 2 /* from MBR offset 0x1b8 */
#define XEN_FW_VBEDDC_INFO 3 /* from int 10 AX=4f15 */
+#define XEN_FW_EFI_INFO 4 /* from EFI */
#define XEN_FW_KBD_SHIFT_FLAGS 5 /* Int16, Fn02: Get keyboard shift flags. */
+
struct xenpf_firmware_info {
/* IN variables. */
uint32_t type;
@@ -144,6 +246,26 @@ struct xenpf_firmware_info {
GUEST_HANDLE(uchar) edid;
} vbeddc_info; /* XEN_FW_VBEDDC_INFO */
+ union xenpf_efi_info {
+ uint32_t version;
+ struct {
+ uint64_t addr; /* EFI_CONFIGURATION_TABLE */
+ uint32_t nent;
+ } cfg;
+ struct {
+ uint32_t revision;
+ uint32_t bufsz; /* input, in bytes */
+ GUEST_HANDLE(void) name;
+ /* UCS-2/UTF-16 string */
+ } vendor;
+ struct {
+ uint64_t addr;
+ uint64_t size;
+ uint64_t attr;
+ uint32_t type;
+ } mem;
+ } efi_info; /* XEN_FW_EFI_INFO */
+
uint8_t kbd_shift_flags; /* XEN_FW_KBD_SHIFT_FLAGS */
} u;
};
@@ -362,6 +484,7 @@ struct xen_platform_op {
struct xenpf_read_memtype read_memtype;
struct xenpf_microcode_update microcode;
struct xenpf_platform_quirk platform_quirk;
+ struct xenpf_efi_runtime_call efi_runtime_call;
struct xenpf_firmware_info firmware_info;
struct xenpf_enter_acpi_sleep enter_acpi_sleep;
struct xenpf_change_freq change_freq;
--
1.7.10.4
v5 - suggestions/fixes:
- do not change indentation
(suggested by Matt Fleming).
Signed-off-by: Daniel Kiper <[email protected]>
---
arch/x86/kernel/setup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 78a0e62..41ead8d 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -924,10 +924,10 @@ void __init setup_arch(char **cmdline_p)
#endif
#ifdef CONFIG_EFI
if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature,
- "EL32", 4)) {
+ EFI32_LOADER_SIGNATURE, 4)) {
set_bit(EFI_BOOT, &efi.flags);
} else if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature,
- "EL64", 4)) {
+ EFI64_LOADER_SIGNATURE, 4)) {
set_bit(EFI_BOOT, &efi.flags);
set_bit(EFI_64BIT, &efi.flags);
}
--
1.7.10.4
This patch enables EFI usage under Xen dom0. Standard EFI Linux
Kernel infrastructure cannot be used because it requires direct
access to EFI data and code. However, in dom0 case it is not possible
because above mentioned EFI stuff is fully owned and controlled
by Xen hypervisor. In this case all calls from dom0 to EFI must
be requested via special hypercall which in turn executes relevant
EFI code in behalf of dom0.
When dom0 kernel boots it checks for EFI availability on a machine.
If it is detected then artificial EFI system table is filled.
Native EFI callas are replaced by functions which mimics them
by calling relevant hypercall. Later pointer to EFI system table
is passed to standard EFI machinery and it continues EFI subsystem
initialization taking into account that there is no direct access
to EFI boot services, runtime, tables, structures, etc. After that
system runs as usual.
This patch is based on Jan Beulich and Tang Liang work.
v5 - suggestions/fixes:
- improve macro usage readability
(suggested by Andrew Cooper and David Vrabel),
- conditions cleanup
(suggested by David Vrabel),
- use -fshort-wchar option
(suggested by Jan Beulich),
- Kconfig rule cleanup
(suggested by Jan Beulich),
- forward port fixes from SUSE kernel
(suggested by Jan Beulich),
- improve commit message
(suggested by David Vrabel).
v4 - suggestions/fixes:
- "just populate an efi_system_table_t object"
(suggested by Matt Fleming).
Signed-off-by: Jan Beulich <[email protected]>
Signed-off-by: Tang Liang <[email protected]>
Signed-off-by: Daniel Kiper <[email protected]>
---
arch/x86/xen/enlighten.c | 24 +++
drivers/xen/Kconfig | 4 +
drivers/xen/Makefile | 3 +
drivers/xen/efi.c | 374 ++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 405 insertions(+)
create mode 100644 drivers/xen/efi.c
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index f17b292..5dad11c 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -32,6 +32,7 @@
#include <linux/gfp.h>
#include <linux/memblock.h>
#include <linux/edd.h>
+#include <linux/efi.h>
#include <xen/xen.h>
#include <xen/events.h>
@@ -150,6 +151,15 @@ struct shared_info *HYPERVISOR_shared_info = &xen_dummy_shared_info;
*/
static int have_vcpu_info_placement = 1;
+#ifdef CONFIG_XEN_EFI
+extern efi_system_table_t __init *xen_efi_probe(void);
+#else
+static efi_system_table_t __init *xen_efi_probe(void)
+{
+ return NULL;
+}
+#endif
+
struct tls_descs {
struct desc_struct desc[3];
};
@@ -1520,6 +1530,7 @@ asmlinkage __visible void __init xen_start_kernel(void)
{
struct physdev_set_iopl set_iopl;
int rc;
+ efi_system_table_t *efi_systab_xen;
if (!xen_start_info)
return;
@@ -1715,6 +1726,19 @@ asmlinkage __visible void __init xen_start_kernel(void)
xen_setup_runstate_info(0);
+ efi_systab_xen = xen_efi_probe();
+
+ if (efi_systab_xen) {
+ strncpy((char *)&boot_params.efi_info.efi_loader_signature, "Xen",
+ sizeof(boot_params.efi_info.efi_loader_signature));
+ boot_params.efi_info.efi_systab = (__u32)__pa(efi_systab_xen);
+ boot_params.efi_info.efi_systab_hi = (__u32)(__pa(efi_systab_xen) >> 32);
+
+ set_bit(EFI_BOOT, &efi.flags);
+ set_bit(EFI_NO_DIRECT, &efi.flags);
+ set_bit(EFI_64BIT, &efi.flags);
+ }
+
/* Start the world */
#ifdef CONFIG_X86_32
i386_start_kernel();
diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index 38fb36e..8bc0183 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -240,4 +240,8 @@ config XEN_MCE_LOG
config XEN_HAVE_PVMMU
bool
+config XEN_EFI
+ def_bool y
+ depends on X86_64 && EFI
+
endmenu
diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
index 45e00af..84044b5 100644
--- a/drivers/xen/Makefile
+++ b/drivers/xen/Makefile
@@ -9,6 +9,8 @@ obj-y += xenbus/
nostackp := $(call cc-option, -fno-stack-protector)
CFLAGS_features.o := $(nostackp)
+CFLAGS_efi.o += -fshort-wchar
+
dom0-$(CONFIG_PCI) += pci.o
dom0-$(CONFIG_USB_SUPPORT) += dbgp.o
dom0-$(CONFIG_ACPI) += acpi.o $(xen-pad-y)
@@ -33,6 +35,7 @@ obj-$(CONFIG_XEN_STUB) += xen-stub.o
obj-$(CONFIG_XEN_ACPI_HOTPLUG_MEMORY) += xen-acpi-memhotplug.o
obj-$(CONFIG_XEN_ACPI_HOTPLUG_CPU) += xen-acpi-cpuhotplug.o
obj-$(CONFIG_XEN_ACPI_PROCESSOR) += xen-acpi-processor.o
+obj-$(CONFIG_XEN_EFI) += efi.o
xen-evtchn-y := evtchn.o
xen-gntdev-y := gntdev.o
xen-gntalloc-y := gntalloc.o
diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
new file mode 100644
index 0000000..e3d6618
--- /dev/null
+++ b/drivers/xen/efi.c
@@ -0,0 +1,374 @@
+/*
+ * EFI support for Xen.
+ *
+ * Copyright (C) 1999 VA Linux Systems
+ * Copyright (C) 1999 Walt Drummond <[email protected]>
+ * Copyright (C) 1999-2002 Hewlett-Packard Co.
+ * David Mosberger-Tang <[email protected]>
+ * Stephane Eranian <[email protected]>
+ * Copyright (C) 2005-2008 Intel Co.
+ * Fenghua Yu <[email protected]>
+ * Bibo Mao <[email protected]>
+ * Chandramouli Narayanan <[email protected]>
+ * Huang Ying <[email protected]>
+ * Copyright (C) 2011 Novell Co.
+ * Jan Beulich <[email protected]>
+ * Copyright (C) 2011-2012 Oracle Co.
+ * Liang Tang <[email protected]>
+ * Copyright (c) 2014 Oracle Corporation, Daniel Kiper
+ */
+
+#include <linux/bug.h>
+#include <linux/efi.h>
+#include <linux/init.h>
+#include <linux/string.h>
+
+#include <xen/interface/xen.h>
+#include <xen/interface/platform.h>
+
+#include <asm/xen/hypercall.h>
+
+#define INIT_EFI_OP(name) \
+ {.cmd = XENPF_efi_runtime_call, \
+ .u.efi_runtime_call.function = XEN_EFI_##name, \
+ .u.efi_runtime_call.misc = 0}
+
+#define efi_data(op) (op.u.efi_runtime_call)
+
+static efi_status_t xen_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
+{
+ struct xen_platform_op op = INIT_EFI_OP(get_time);
+
+ if (HYPERVISOR_dom0_op(&op) < 0)
+ return EFI_UNSUPPORTED;
+
+ if (tm) {
+ BUILD_BUG_ON(sizeof(*tm) != sizeof(efi_data(op).u.get_time.time));
+ memcpy(tm, &efi_data(op).u.get_time.time, sizeof(*tm));
+ }
+
+ if (tc) {
+ tc->resolution = efi_data(op).u.get_time.resolution;
+ tc->accuracy = efi_data(op).u.get_time.accuracy;
+ tc->sets_to_zero = !!(efi_data(op).misc &
+ XEN_EFI_GET_TIME_SET_CLEARS_NS);
+ }
+
+ return efi_data(op).status;
+}
+
+static efi_status_t xen_efi_set_time(efi_time_t *tm)
+{
+ struct xen_platform_op op = INIT_EFI_OP(set_time);
+
+ BUILD_BUG_ON(sizeof(*tm) != sizeof(efi_data(op).u.set_time));
+ memcpy(&efi_data(op).u.set_time, tm, sizeof(*tm));
+
+ if (HYPERVISOR_dom0_op(&op) < 0)
+ return EFI_UNSUPPORTED;
+
+ return efi_data(op).status;
+}
+
+static efi_status_t xen_efi_get_wakeup_time(efi_bool_t *enabled,
+ efi_bool_t *pending,
+ efi_time_t *tm)
+{
+ struct xen_platform_op op = INIT_EFI_OP(get_wakeup_time);
+
+ if (HYPERVISOR_dom0_op(&op) < 0)
+ return EFI_UNSUPPORTED;
+
+ if (tm) {
+ BUILD_BUG_ON(sizeof(*tm) != sizeof(efi_data(op).u.get_wakeup_time));
+ memcpy(tm, &efi_data(op).u.get_wakeup_time, sizeof(*tm));
+ }
+
+ if (enabled)
+ *enabled = !!(efi_data(op).misc & XEN_EFI_GET_WAKEUP_TIME_ENABLED);
+
+ if (pending)
+ *pending = !!(efi_data(op).misc & XEN_EFI_GET_WAKEUP_TIME_PENDING);
+
+ return efi_data(op).status;
+}
+
+static efi_status_t xen_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
+{
+ struct xen_platform_op op = INIT_EFI_OP(set_wakeup_time);
+
+ BUILD_BUG_ON(sizeof(*tm) != sizeof(efi_data(op).u.set_wakeup_time));
+ if (enabled)
+ efi_data(op).misc = XEN_EFI_SET_WAKEUP_TIME_ENABLE;
+ if (tm)
+ memcpy(&efi_data(op).u.set_wakeup_time, tm, sizeof(*tm));
+ else
+ efi_data(op).misc |= XEN_EFI_SET_WAKEUP_TIME_ENABLE_ONLY;
+
+ if (HYPERVISOR_dom0_op(&op) < 0)
+ return EFI_UNSUPPORTED;
+
+ return efi_data(op).status;
+}
+
+static efi_status_t xen_efi_get_variable(efi_char16_t *name,
+ efi_guid_t *vendor,
+ u32 *attr,
+ unsigned long *data_size,
+ void *data)
+{
+ struct xen_platform_op op = INIT_EFI_OP(get_variable);
+
+ set_xen_guest_handle(efi_data(op).u.get_variable.name, name);
+ BUILD_BUG_ON(sizeof(*vendor) !=
+ sizeof(efi_data(op).u.get_variable.vendor_guid));
+ memcpy(&efi_data(op).u.get_variable.vendor_guid, vendor, sizeof(*vendor));
+ efi_data(op).u.get_variable.size = *data_size;
+ set_xen_guest_handle(efi_data(op).u.get_variable.data, data);
+
+ if (HYPERVISOR_dom0_op(&op) < 0)
+ return EFI_UNSUPPORTED;
+
+ *data_size = efi_data(op).u.get_variable.size;
+ if (attr)
+ *attr = efi_data(op).misc;
+
+ return efi_data(op).status;
+}
+
+static efi_status_t xen_efi_get_next_variable(unsigned long *name_size,
+ efi_char16_t *name,
+ efi_guid_t *vendor)
+{
+ struct xen_platform_op op = INIT_EFI_OP(get_next_variable_name);
+
+ efi_data(op).u.get_next_variable_name.size = *name_size;
+ set_xen_guest_handle(efi_data(op).u.get_next_variable_name.name, name);
+ BUILD_BUG_ON(sizeof(*vendor) !=
+ sizeof(efi_data(op).u.get_next_variable_name.vendor_guid));
+ memcpy(&efi_data(op).u.get_next_variable_name.vendor_guid, vendor,
+ sizeof(*vendor));
+
+ if (HYPERVISOR_dom0_op(&op) < 0)
+ return EFI_UNSUPPORTED;
+
+ *name_size = efi_data(op).u.get_next_variable_name.size;
+ memcpy(vendor, &efi_data(op).u.get_next_variable_name.vendor_guid,
+ sizeof(*vendor));
+
+ return efi_data(op).status;
+}
+
+static efi_status_t xen_efi_set_variable(efi_char16_t *name,
+ efi_guid_t *vendor,
+ u32 attr,
+ unsigned long data_size,
+ void *data)
+{
+ struct xen_platform_op op = INIT_EFI_OP(set_variable);
+
+ set_xen_guest_handle(efi_data(op).u.set_variable.name, name);
+ efi_data(op).misc = attr;
+ BUILD_BUG_ON(sizeof(*vendor) !=
+ sizeof(efi_data(op).u.set_variable.vendor_guid));
+ memcpy(&efi_data(op).u.set_variable.vendor_guid, vendor, sizeof(*vendor));
+ efi_data(op).u.set_variable.size = data_size;
+ set_xen_guest_handle(efi_data(op).u.set_variable.data, data);
+
+ if (HYPERVISOR_dom0_op(&op) < 0)
+ return EFI_UNSUPPORTED;
+
+ return efi_data(op).status;
+}
+
+static efi_status_t xen_efi_query_variable_info(u32 attr,
+ u64 *storage_space,
+ u64 *remaining_space,
+ u64 *max_variable_size)
+{
+ struct xen_platform_op op = INIT_EFI_OP(query_variable_info);
+
+ if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+ return EFI_UNSUPPORTED;
+
+ efi_data(op).u.query_variable_info.attr = attr;
+
+ if (HYPERVISOR_dom0_op(&op) < 0)
+ return EFI_UNSUPPORTED;
+
+ *storage_space = efi_data(op).u.query_variable_info.max_store_size;
+ *remaining_space = efi_data(op).u.query_variable_info.remain_store_size;
+ *max_variable_size = efi_data(op).u.query_variable_info.max_size;
+
+ return efi_data(op).status;
+}
+
+static efi_status_t xen_efi_get_next_high_mono_count(u32 *count)
+{
+ struct xen_platform_op op = INIT_EFI_OP(get_next_high_monotonic_count);
+
+ if (HYPERVISOR_dom0_op(&op) < 0)
+ return EFI_UNSUPPORTED;
+
+ *count = efi_data(op).misc;
+
+ return efi_data(op).status;
+}
+
+static efi_status_t xen_efi_update_capsule(efi_capsule_header_t **capsules,
+ unsigned long count,
+ unsigned long sg_list)
+{
+ struct xen_platform_op op = INIT_EFI_OP(update_capsule);
+
+ if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+ return EFI_UNSUPPORTED;
+
+ set_xen_guest_handle(efi_data(op).u.update_capsule.capsule_header_array,
+ capsules);
+ efi_data(op).u.update_capsule.capsule_count = count;
+ efi_data(op).u.update_capsule.sg_list = sg_list;
+
+ if (HYPERVISOR_dom0_op(&op) < 0)
+ return EFI_UNSUPPORTED;
+
+ return efi_data(op).status;
+}
+
+static efi_status_t xen_efi_query_capsule_caps(efi_capsule_header_t **capsules,
+ unsigned long count,
+ u64 *max_size,
+ int *reset_type)
+{
+ struct xen_platform_op op = INIT_EFI_OP(query_capsule_capabilities);
+
+ if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+ return EFI_UNSUPPORTED;
+
+ set_xen_guest_handle(efi_data(op).u.query_capsule_capabilities.capsule_header_array,
+ capsules);
+ efi_data(op).u.query_capsule_capabilities.capsule_count = count;
+
+ if (HYPERVISOR_dom0_op(&op) < 0)
+ return EFI_UNSUPPORTED;
+
+ *max_size = efi_data(op).u.query_capsule_capabilities.max_capsule_size;
+ *reset_type = efi_data(op).u.query_capsule_capabilities.reset_type;
+
+ return efi_data(op).status;
+}
+
+static efi_char16_t vendor[100] __initdata;
+
+static efi_system_table_t efi_systab_xen __initdata = {
+ .hdr = {
+ .signature = EFI_SYSTEM_TABLE_SIGNATURE,
+ .revision = 0, /* Initialized later. */
+ .headersize = 0, /* Ignored by Linux Kernel. */
+ .crc32 = 0, /* Ignored by Linux Kernel. */
+ .reserved = 0
+ },
+ .fw_vendor = EFI_INVALID_TABLE_ADDR, /* Initialized later. */
+ .fw_revision = 0, /* Initialized later. */
+ .con_in_handle = EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
+ .con_in = EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
+ .con_out_handle = EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
+ .con_out = EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
+ .stderr_handle = EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
+ .stderr = EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
+ .runtime = (efi_runtime_services_t *)EFI_INVALID_TABLE_ADDR,
+ /* Not used under Xen. */
+ .boottime = (efi_boot_services_t *)EFI_INVALID_TABLE_ADDR,
+ /* Not used under Xen. */
+ .nr_tables = 0, /* Initialized later. */
+ .tables = EFI_INVALID_TABLE_ADDR /* Initialized later. */
+};
+
+static const struct efi efi_xen __initconst = {
+ .systab = NULL, /* Initialized later. */
+ .runtime_version = 0, /* Initialized later. */
+ .mps = EFI_INVALID_TABLE_ADDR,
+ .acpi = EFI_INVALID_TABLE_ADDR,
+ .acpi20 = EFI_INVALID_TABLE_ADDR,
+ .smbios = EFI_INVALID_TABLE_ADDR,
+ .sal_systab = EFI_INVALID_TABLE_ADDR,
+ .boot_info = EFI_INVALID_TABLE_ADDR,
+ .hcdp = EFI_INVALID_TABLE_ADDR,
+ .uga = EFI_INVALID_TABLE_ADDR,
+ .uv_systab = EFI_INVALID_TABLE_ADDR,
+ .fw_vendor = EFI_INVALID_TABLE_ADDR,
+ .runtime = EFI_INVALID_TABLE_ADDR,
+ .config_table = EFI_INVALID_TABLE_ADDR,
+ .get_time = xen_efi_get_time,
+ .set_time = xen_efi_set_time,
+ .get_wakeup_time = xen_efi_get_wakeup_time,
+ .set_wakeup_time = xen_efi_set_wakeup_time,
+ .get_variable = xen_efi_get_variable,
+ .get_next_variable = xen_efi_get_next_variable,
+ .set_variable = xen_efi_set_variable,
+ .query_variable_info = xen_efi_query_variable_info,
+ .update_capsule = xen_efi_update_capsule,
+ .query_capsule_caps = xen_efi_query_capsule_caps,
+ .get_next_high_mono_count = xen_efi_get_next_high_mono_count,
+ .reset_system = NULL, /* Functionality provided by Xen. */
+ .set_virtual_address_map = NULL, /* Not used under Xen. */
+ .memmap = NULL, /* Not used under Xen. */
+ .flags = 0 /* Initialized later. */
+};
+
+efi_system_table_t __init *xen_efi_probe(void)
+{
+ struct xen_platform_op op = {
+ .cmd = XENPF_firmware_info,
+ .u.firmware_info = {
+ .type = XEN_FW_EFI_INFO,
+ .index = XEN_FW_EFI_CONFIG_TABLE
+ }
+ };
+ union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
+
+ if (!xen_initial_domain() || HYPERVISOR_dom0_op(&op) < 0)
+ return NULL;
+
+ /* Here we know that Xen runs on EFI platform. */
+
+ efi = efi_xen;
+
+ op.cmd = XENPF_firmware_info;
+ op.u.firmware_info.type = XEN_FW_EFI_INFO;
+ op.u.firmware_info.index = XEN_FW_EFI_VENDOR;
+ info->vendor.bufsz = sizeof(vendor);
+ set_xen_guest_handle(info->vendor.name, vendor);
+
+ if (HYPERVISOR_dom0_op(&op) == 0) {
+ efi_systab_xen.fw_vendor = __pa_symbol(vendor);
+ efi_systab_xen.fw_revision = info->vendor.revision;
+ } else
+ efi_systab_xen.fw_vendor = __pa_symbol(L"UNKNOWN");
+
+ op.cmd = XENPF_firmware_info;
+ op.u.firmware_info.type = XEN_FW_EFI_INFO;
+ op.u.firmware_info.index = XEN_FW_EFI_VERSION;
+
+ if (HYPERVISOR_dom0_op(&op) == 0)
+ efi_systab_xen.hdr.revision = info->version;
+
+ op.cmd = XENPF_firmware_info;
+ op.u.firmware_info.type = XEN_FW_EFI_INFO;
+ op.u.firmware_info.index = XEN_FW_EFI_RT_VERSION;
+
+ if (HYPERVISOR_dom0_op(&op) == 0)
+ efi.runtime_version = info->version;
+
+ op.cmd = XENPF_firmware_info;
+ op.u.firmware_info.type = XEN_FW_EFI_INFO;
+ op.u.firmware_info.index = XEN_FW_EFI_CONFIG_TABLE;
+
+ if (HYPERVISOR_dom0_op(&op) < 0)
+ BUG();
+
+ efi_systab_xen.tables = info->cfg.addr;
+ efi_systab_xen.nr_tables = info->cfg.nent;
+
+ return &efi_systab_xen;
+}
--
1.7.10.4
efi_set_rtc_mmss() is never used to set RTC due to bugs found
on many EFI platforms. It is set directly by mach_set_rtc_mmss().
Signed-off-by: Daniel Kiper <[email protected]>
---
arch/x86/platform/efi/efi.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index deb4f05..bd3e080 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -244,6 +244,11 @@ static efi_status_t __init phys_efi_set_virtual_address_map(
return status;
}
+#if 0
+/*
+ * efi_set_rtc_mmss() is never used to set RTC due to bugs found
+ * on many EFI platforms. It is set directly by mach_set_rtc_mmss().
+ */
int efi_set_rtc_mmss(const struct timespec *now)
{
unsigned long nowtime = now->tv_sec;
@@ -279,6 +284,7 @@ int efi_set_rtc_mmss(const struct timespec *now)
}
return 0;
}
+#endif
void efi_get_time(struct timespec *now)
{
--
1.7.10.4
Remove redundant set_bit(EFI_SYSTEM_TABLES, &efi.flags) call.
It is executed earlier in efi_systab_init().
Signed-off-by: Daniel Kiper <[email protected]>
---
arch/x86/platform/efi/efi.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index e3d9d76..deb4f05 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -737,8 +737,6 @@ void __init efi_init(void)
if (efi_systab_init(efi_phys.systab))
return;
- set_bit(EFI_SYSTEM_TABLES, &efi.flags);
-
efi.config_table = (unsigned long)efi.systab->tables;
efi.fw_vendor = (unsigned long)efi.systab->fw_vendor;
efi.runtime = (unsigned long)efi.systab->runtime;
--
1.7.10.4
Introduce EFI_NO_DIRECT flag. If it is set then kernel runs
on EFI platform but it has not direct control on EFI stuff
like EFI runtime, tables, structures, etc. If not this means
that Linux Kernel has direct access to EFI infrastructure
and everything runs as usual.
This functionality is used in Xen dom0 because hypervisor
has full control on EFI stuff and all calls from dom0 to
EFI must be requested via special hypercall which in turn
executes relevant EFI code in behalf of dom0.
v5 - suggestions/fixes:
- rename EFI_DIRECT to EFI_NO_DIRECT
(suggested by David Vrabel),
- limit EFI_NO_DIRECT usage
(suggested by Jan Beulich and Matt Fleming),
- improve commit message
(suggested by David Vrabel).
Signed-off-by: Daniel Kiper <[email protected]>
---
arch/x86/platform/efi/efi.c | 27 +++++++++++++++++++++------
drivers/firmware/efi/efi.c | 22 +++++++++++++---------
include/linux/efi.h | 3 ++-
3 files changed, 36 insertions(+), 16 deletions(-)
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index dd1e351..e3d9d76 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -350,6 +350,9 @@ int __init efi_memblock_x86_reserve_range(void)
struct efi_info *e = &boot_params.efi_info;
unsigned long pmap;
+ if (efi_enabled(EFI_NO_DIRECT))
+ return 0;
+
#ifdef CONFIG_X86_32
/* Can't handle data above 4GB at this time */
if (e->efi_memmap_hi) {
@@ -617,13 +620,16 @@ static int __init efi_runtime_init(void)
* address of several of the EFI runtime functions, needed to
* set the firmware into virtual mode.
*/
- if (efi_enabled(EFI_64BIT))
- rv = efi_runtime_init64();
- else
- rv = efi_runtime_init32();
- if (rv)
- return rv;
+ if (!efi_enabled(EFI_NO_DIRECT)) {
+ if (efi_enabled(EFI_64BIT))
+ rv = efi_runtime_init64();
+ else
+ rv = efi_runtime_init32();
+
+ if (rv)
+ return rv;
+ }
set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
@@ -632,6 +638,9 @@ static int __init efi_runtime_init(void)
static int __init efi_memmap_init(void)
{
+ if (efi_enabled(EFI_NO_DIRECT))
+ return 0;
+
/* Map the EFI memory map */
memmap.map = early_memremap((unsigned long)memmap.phys_map,
memmap.nr_map * memmap.desc_size);
@@ -1188,6 +1197,9 @@ static void __init __efi_enter_virtual_mode(void)
void __init efi_enter_virtual_mode(void)
{
+ if (efi_enabled(EFI_NO_DIRECT))
+ return;
+
if (efi_setup)
kexec_enter_virtual_mode();
else
@@ -1220,6 +1232,9 @@ u64 efi_mem_attributes(unsigned long phys_addr)
efi_memory_desc_t *md;
void *p;
+ if (!efi_enabled(EFI_MEMMAP))
+ return 0;
+
for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
md = p;
if ((md->phys_addr <= phys_addr) &&
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 023937a..8bb1075 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -104,16 +104,20 @@ static struct attribute *efi_subsys_attrs[] = {
static umode_t efi_attr_is_visible(struct kobject *kobj,
struct attribute *attr, int n)
{
- umode_t mode = attr->mode;
-
- if (attr == &efi_attr_fw_vendor.attr)
- return (efi.fw_vendor == EFI_INVALID_TABLE_ADDR) ? 0 : mode;
- else if (attr == &efi_attr_runtime.attr)
- return (efi.runtime == EFI_INVALID_TABLE_ADDR) ? 0 : mode;
- else if (attr == &efi_attr_config_table.attr)
- return (efi.config_table == EFI_INVALID_TABLE_ADDR) ? 0 : mode;
+ if (attr == &efi_attr_fw_vendor.attr) {
+ if (efi_enabled(EFI_NO_DIRECT) ||
+ efi.fw_vendor == EFI_INVALID_TABLE_ADDR)
+ return 0;
+ } else if (attr == &efi_attr_runtime.attr) {
+ if (efi_enabled(EFI_NO_DIRECT) ||
+ efi.runtime == EFI_INVALID_TABLE_ADDR)
+ return 0;
+ } else if (attr == &efi_attr_config_table.attr) {
+ if (efi.config_table == EFI_INVALID_TABLE_ADDR)
+ return 0;
+ }
- return mode;
+ return attr->mode;
}
static struct attribute_group efi_subsys_attr_group = {
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 41bbf8b..e917c4a 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -916,7 +916,8 @@ extern int __init efi_setup_pcdp_console(char *);
#define EFI_RUNTIME_SERVICES 3 /* Can we use runtime services? */
#define EFI_MEMMAP 4 /* Can we use EFI memory map? */
#define EFI_64BIT 5 /* Is the firmware 64-bit? */
-#define EFI_ARCH_1 6 /* First arch-specific bit */
+#define EFI_NO_DIRECT 6 /* Can we access EFI directly? */
+#define EFI_ARCH_1 7 /* First arch-specific bit */
#ifdef CONFIG_EFI
/*
--
1.7.10.4
Use early_mem*() instead of early_io*() because all mapped EFI regions
are true RAM not I/O regions. Additionally, I/O family calls do not work
correctly under Xen in our case. AIUI, early_io*() maps/unmaps real machine
addresses. However, all artificial EFI structures created under Xen live
in dom0 memory and should be mapped/unmapped using early_mem*() family
calls which map domain memory.
Signed-off-by: Daniel Kiper <[email protected]>
---
arch/x86/platform/efi/efi.c | 42 +++++++++++++++++++++---------------------
drivers/firmware/efi/efi.c | 4 ++--
2 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 87fc96b..dd1e351 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -427,7 +427,7 @@ void __init efi_unmap_memmap(void)
{
clear_bit(EFI_MEMMAP, &efi.flags);
if (memmap.map) {
- early_iounmap(memmap.map, memmap.nr_map * memmap.desc_size);
+ early_memunmap(memmap.map, memmap.nr_map * memmap.desc_size);
memmap.map = NULL;
}
}
@@ -467,12 +467,12 @@ static int __init efi_systab_init(void *phys)
if (!data)
return -ENOMEM;
}
- systab64 = early_ioremap((unsigned long)phys,
- sizeof(*systab64));
+ systab64 = early_memremap((unsigned long)phys,
+ sizeof(*systab64));
if (systab64 == NULL) {
pr_err("Couldn't map the system table!\n");
if (data)
- early_iounmap(data, sizeof(*data));
+ early_memunmap(data, sizeof(*data));
return -ENOMEM;
}
@@ -504,9 +504,9 @@ static int __init efi_systab_init(void *phys)
systab64->tables;
tmp |= data ? data->tables : systab64->tables;
- early_iounmap(systab64, sizeof(*systab64));
+ early_memunmap(systab64, sizeof(*systab64));
if (data)
- early_iounmap(data, sizeof(*data));
+ early_memunmap(data, sizeof(*data));
#ifdef CONFIG_X86_32
if (tmp >> 32) {
pr_err("EFI data located above 4GB, disabling EFI.\n");
@@ -516,8 +516,8 @@ static int __init efi_systab_init(void *phys)
} else {
efi_system_table_32_t *systab32;
- systab32 = early_ioremap((unsigned long)phys,
- sizeof(*systab32));
+ systab32 = early_memremap((unsigned long)phys,
+ sizeof(*systab32));
if (systab32 == NULL) {
pr_err("Couldn't map the system table!\n");
return -ENOMEM;
@@ -537,7 +537,7 @@ static int __init efi_systab_init(void *phys)
efi_systab.nr_tables = systab32->nr_tables;
efi_systab.tables = systab32->tables;
- early_iounmap(systab32, sizeof(*systab32));
+ early_memunmap(systab32, sizeof(*systab32));
}
efi.systab = &efi_systab;
@@ -563,8 +563,8 @@ static int __init efi_runtime_init32(void)
{
efi_runtime_services_32_t *runtime;
- runtime = early_ioremap((unsigned long)efi.systab->runtime,
- sizeof(efi_runtime_services_32_t));
+ runtime = early_memremap((unsigned long)efi.systab->runtime,
+ sizeof(efi_runtime_services_32_t));
if (!runtime) {
pr_err("Could not map the runtime service table!\n");
return -ENOMEM;
@@ -578,7 +578,7 @@ static int __init efi_runtime_init32(void)
efi_phys.set_virtual_address_map =
(efi_set_virtual_address_map_t *)
(unsigned long)runtime->set_virtual_address_map;
- early_iounmap(runtime, sizeof(efi_runtime_services_32_t));
+ early_memunmap(runtime, sizeof(efi_runtime_services_32_t));
return 0;
}
@@ -587,8 +587,8 @@ static int __init efi_runtime_init64(void)
{
efi_runtime_services_64_t *runtime;
- runtime = early_ioremap((unsigned long)efi.systab->runtime,
- sizeof(efi_runtime_services_64_t));
+ runtime = early_memremap((unsigned long)efi.systab->runtime,
+ sizeof(efi_runtime_services_64_t));
if (!runtime) {
pr_err("Could not map the runtime service table!\n");
return -ENOMEM;
@@ -602,7 +602,7 @@ static int __init efi_runtime_init64(void)
efi_phys.set_virtual_address_map =
(efi_set_virtual_address_map_t *)
(unsigned long)runtime->set_virtual_address_map;
- early_iounmap(runtime, sizeof(efi_runtime_services_64_t));
+ early_memunmap(runtime, sizeof(efi_runtime_services_64_t));
return 0;
}
@@ -633,8 +633,8 @@ static int __init efi_runtime_init(void)
static int __init efi_memmap_init(void)
{
/* Map the EFI memory map */
- memmap.map = early_ioremap((unsigned long)memmap.phys_map,
- memmap.nr_map * memmap.desc_size);
+ memmap.map = early_memremap((unsigned long)memmap.phys_map,
+ memmap.nr_map * memmap.desc_size);
if (memmap.map == NULL) {
pr_err("Could not map the memory map!\n");
return -ENOMEM;
@@ -697,10 +697,10 @@ static int __init efi_reuse_config(u64 tables, int nr_tables)
((efi_config_table_64_t *)p)->table = data->smbios;
p += sz;
}
- early_iounmap(tablep, nr_tables * sz);
+ early_memunmap(tablep, nr_tables * sz);
out_memremap:
- early_iounmap(data, sizeof(*data));
+ early_memunmap(data, sizeof(*data));
out:
return ret;
}
@@ -737,14 +737,14 @@ void __init efi_init(void)
/*
* Show what we know for posterity
*/
- c16 = tmp = early_ioremap(efi.systab->fw_vendor, 2);
+ c16 = tmp = early_memremap(efi.systab->fw_vendor, 2);
if (c16) {
for (i = 0; i < sizeof(vendor) - 1 && *c16; ++i)
vendor[i] = *c16++;
vendor[i] = '\0';
} else
pr_err("Could not map the firmware vendor!\n");
- early_iounmap(tmp, 2);
+ early_memunmap(tmp, 2);
pr_info("EFI v%u.%.02u by %s\n",
efi.systab->hdr.revision >> 16,
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index cd36deb..023937a 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -298,7 +298,7 @@ int __init efi_config_init(efi_config_table_type_t *arch_tables)
if (table64 >> 32) {
pr_cont("\n");
pr_err("Table located above 4GB, disabling EFI.\n");
- early_iounmap(config_tables,
+ early_memunmap(config_tables,
efi.systab->nr_tables * sz);
return -EINVAL;
}
@@ -314,7 +314,7 @@ int __init efi_config_init(efi_config_table_type_t *arch_tables)
tablep += sz;
}
pr_cont("\n");
- early_iounmap(config_tables, efi.systab->nr_tables * sz);
+ early_memunmap(config_tables, efi.systab->nr_tables * sz);
set_bit(EFI_CONFIG_TABLES, &efi.flags);
--
1.7.10.4
On Fri, Jun 13, 2014 at 07:00:17PM +0200, Daniel Kiper wrote:
> Use early_mem*() instead of early_io*() because all mapped EFI regions
> are true RAM not I/O regions. Additionally, I/O family calls do not work
> correctly under Xen in our case. AIUI, early_io*() maps/unmaps real machine
> addresses. However, all artificial EFI structures created under Xen live
> in dom0 memory and should be mapped/unmapped using early_mem*() family
> calls which map domain memory.
Mapped EFI regions may include memory mapped flash, not merely true RAM.
--
Matthew Garrett | [email protected]
On 13/06/14 18:00, Daniel Kiper wrote:
> Introduce EFI_NO_DIRECT flag.
EFI_PARAVIRT would be a clearer name I think.
> +#define EFI_NO_DIRECT 6 /* Can we access EFI directly? */
#define EFI_PARAVIRT 6 /* Access is via a paravirt interface */
David
On 13/06/14 18:00, Daniel Kiper wrote:
> Define constants and structures which are needed to properly
> execute EFI related hypercall in Xen dom0.
>
> This patch is based on Jan Beulich and Tang Liang work.
>
> v5 - suggestions/fixes:
This version information should be after a '---' marker.
> + struct {
> + GUEST_HANDLE(void) name; /* UCS-2/UTF-16 string */
> + xen_ulong_t size;
> + GUEST_HANDLE(void) data;
> + struct xenpf_efi_guid {
> + uint32_t data1;
> + uint16_t data2;
> + uint16_t data3;
> + uint8_t data4[8];
> + } vendor_guid;
Indentation looks all messed up here and in many other places.
David
On Fri, 13 Jun 2014, Daniel Kiper wrote:
> efi_set_rtc_mmss() is never used to set RTC due to bugs found
> on many EFI platforms. It is set directly by mach_set_rtc_mmss().
>
> Signed-off-by: Daniel Kiper <[email protected]>
> ---
> arch/x86/platform/efi/efi.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index deb4f05..bd3e080 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -244,6 +244,11 @@ static efi_status_t __init phys_efi_set_virtual_address_map(
> return status;
> }
>
> +#if 0
> +/*
> + * efi_set_rtc_mmss() is never used to set RTC due to bugs found
> + * on many EFI platforms. It is set directly by mach_set_rtc_mmss().
> + */
> int efi_set_rtc_mmss(const struct timespec *now)
> {
> unsigned long nowtime = now->tv_sec;
> @@ -279,6 +284,7 @@ int efi_set_rtc_mmss(const struct timespec *now)
> }
> return 0;
> }
> +#endif
"Commenting out" code like that is not a good idea: it leaves
dead-rotting functions in the middle of your source file.
If the function is not used, just go ahead and remove it. We can use git
to get it back.
On 06/16/2014 01:43 PM, Stefano Stabellini wrote:
> On Fri, 13 Jun 2014, Daniel Kiper wrote:
>> efi_set_rtc_mmss() is never used to set RTC due to bugs found
>> on many EFI platforms. It is set directly by mach_set_rtc_mmss().
>>
>> Signed-off-by: Daniel Kiper <[email protected]>
>> ---
>> arch/x86/platform/efi/efi.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
>> index deb4f05..bd3e080 100644
>> --- a/arch/x86/platform/efi/efi.c
>> +++ b/arch/x86/platform/efi/efi.c
>> @@ -244,6 +244,11 @@ static efi_status_t __init phys_efi_set_virtual_address_map(
>> return status;
>> }
>>
>> +#if 0
>> +/*
>> + * efi_set_rtc_mmss() is never used to set RTC due to bugs found
>> + * on many EFI platforms. It is set directly by mach_set_rtc_mmss().
>> + */
>> int efi_set_rtc_mmss(const struct timespec *now)
>> {
>> unsigned long nowtime = now->tv_sec;
>> @@ -279,6 +284,7 @@ int efi_set_rtc_mmss(const struct timespec *now)
>> }
>> return 0;
>> }
>> +#endif
>
> "Commenting out" code like that is not a good idea: it leaves
> dead-rotting functions in the middle of your source file.
>
> If the function is not used, just go ahead and remove it. We can use git
> to get it back.
And shouldn't it be removed from include/linux/efi.h as well?
Juergen
On Fri, 13 Jun 2014, Daniel Kiper wrote:
> This patch enables EFI usage under Xen dom0. Standard EFI Linux
> Kernel infrastructure cannot be used because it requires direct
> access to EFI data and code. However, in dom0 case it is not possible
> because above mentioned EFI stuff is fully owned and controlled
> by Xen hypervisor. In this case all calls from dom0 to EFI must
> be requested via special hypercall which in turn executes relevant
> EFI code in behalf of dom0.
>
> When dom0 kernel boots it checks for EFI availability on a machine.
> If it is detected then artificial EFI system table is filled.
> Native EFI callas are replaced by functions which mimics them
> by calling relevant hypercall. Later pointer to EFI system table
> is passed to standard EFI machinery and it continues EFI subsystem
> initialization taking into account that there is no direct access
> to EFI boot services, runtime, tables, structures, etc. After that
> system runs as usual.
>
> This patch is based on Jan Beulich and Tang Liang work.
>
> v5 - suggestions/fixes:
> - improve macro usage readability
> (suggested by Andrew Cooper and David Vrabel),
> - conditions cleanup
> (suggested by David Vrabel),
> - use -fshort-wchar option
> (suggested by Jan Beulich),
> - Kconfig rule cleanup
> (suggested by Jan Beulich),
> - forward port fixes from SUSE kernel
> (suggested by Jan Beulich),
> - improve commit message
> (suggested by David Vrabel).
>
> v4 - suggestions/fixes:
> - "just populate an efi_system_table_t object"
> (suggested by Matt Fleming).
>
> Signed-off-by: Jan Beulich <[email protected]>
> Signed-off-by: Tang Liang <[email protected]>
> Signed-off-by: Daniel Kiper <[email protected]>
Sorry for commenting on your patches so late in the review cycle.
> arch/x86/xen/enlighten.c | 24 +++
> drivers/xen/Kconfig | 4 +
> drivers/xen/Makefile | 3 +
> drivers/xen/efi.c | 374 ++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 405 insertions(+)
> create mode 100644 drivers/xen/efi.c
>
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index f17b292..5dad11c 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -32,6 +32,7 @@
> #include <linux/gfp.h>
> #include <linux/memblock.h>
> #include <linux/edd.h>
> +#include <linux/efi.h>
>
> #include <xen/xen.h>
> #include <xen/events.h>
> @@ -150,6 +151,15 @@ struct shared_info *HYPERVISOR_shared_info = &xen_dummy_shared_info;
> */
> static int have_vcpu_info_placement = 1;
>
> +#ifdef CONFIG_XEN_EFI
> +extern efi_system_table_t __init *xen_efi_probe(void);
> +#else
> +static efi_system_table_t __init *xen_efi_probe(void)
> +{
> + return NULL;
> +}
> +#endif
> +
> struct tls_descs {
> struct desc_struct desc[3];
> };
> @@ -1520,6 +1530,7 @@ asmlinkage __visible void __init xen_start_kernel(void)
> {
> struct physdev_set_iopl set_iopl;
> int rc;
> + efi_system_table_t *efi_systab_xen;
>
> if (!xen_start_info)
> return;
> @@ -1715,6 +1726,19 @@ asmlinkage __visible void __init xen_start_kernel(void)
>
> xen_setup_runstate_info(0);
>
> + efi_systab_xen = xen_efi_probe();
> +
> + if (efi_systab_xen) {
> + strncpy((char *)&boot_params.efi_info.efi_loader_signature, "Xen",
> + sizeof(boot_params.efi_info.efi_loader_signature));
> + boot_params.efi_info.efi_systab = (__u32)__pa(efi_systab_xen);
> + boot_params.efi_info.efi_systab_hi = (__u32)(__pa(efi_systab_xen) >> 32);
> +
> + set_bit(EFI_BOOT, &efi.flags);
> + set_bit(EFI_NO_DIRECT, &efi.flags);
> + set_bit(EFI_64BIT, &efi.flags);
> + }
> +
> /* Start the world */
> #ifdef CONFIG_X86_32
> i386_start_kernel();
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index 38fb36e..8bc0183 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -240,4 +240,8 @@ config XEN_MCE_LOG
> config XEN_HAVE_PVMMU
> bool
>
> +config XEN_EFI
> + def_bool y
> + depends on X86_64 && EFI
> +
> endmenu
> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> index 45e00af..84044b5 100644
> --- a/drivers/xen/Makefile
> +++ b/drivers/xen/Makefile
> @@ -9,6 +9,8 @@ obj-y += xenbus/
> nostackp := $(call cc-option, -fno-stack-protector)
> CFLAGS_features.o := $(nostackp)
>
> +CFLAGS_efi.o += -fshort-wchar
> +
> dom0-$(CONFIG_PCI) += pci.o
> dom0-$(CONFIG_USB_SUPPORT) += dbgp.o
> dom0-$(CONFIG_ACPI) += acpi.o $(xen-pad-y)
> @@ -33,6 +35,7 @@ obj-$(CONFIG_XEN_STUB) += xen-stub.o
> obj-$(CONFIG_XEN_ACPI_HOTPLUG_MEMORY) += xen-acpi-memhotplug.o
> obj-$(CONFIG_XEN_ACPI_HOTPLUG_CPU) += xen-acpi-cpuhotplug.o
> obj-$(CONFIG_XEN_ACPI_PROCESSOR) += xen-acpi-processor.o
> +obj-$(CONFIG_XEN_EFI) += efi.o
> xen-evtchn-y := evtchn.o
> xen-gntdev-y := gntdev.o
> xen-gntalloc-y := gntalloc.o
> diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
> new file mode 100644
> index 0000000..e3d6618
> --- /dev/null
> +++ b/drivers/xen/efi.c
> @@ -0,0 +1,374 @@
> +/*
> + * EFI support for Xen.
> + *
> + * Copyright (C) 1999 VA Linux Systems
> + * Copyright (C) 1999 Walt Drummond <[email protected]>
> + * Copyright (C) 1999-2002 Hewlett-Packard Co.
> + * David Mosberger-Tang <[email protected]>
> + * Stephane Eranian <[email protected]>
> + * Copyright (C) 2005-2008 Intel Co.
> + * Fenghua Yu <[email protected]>
> + * Bibo Mao <[email protected]>
> + * Chandramouli Narayanan <[email protected]>
> + * Huang Ying <[email protected]>
> + * Copyright (C) 2011 Novell Co.
> + * Jan Beulich <[email protected]>
> + * Copyright (C) 2011-2012 Oracle Co.
> + * Liang Tang <[email protected]>
> + * Copyright (c) 2014 Oracle Corporation, Daniel Kiper
> + */
> +
> +#include <linux/bug.h>
> +#include <linux/efi.h>
> +#include <linux/init.h>
> +#include <linux/string.h>
> +
> +#include <xen/interface/xen.h>
> +#include <xen/interface/platform.h>
> +
> +#include <asm/xen/hypercall.h>
> +
> +#define INIT_EFI_OP(name) \
> + {.cmd = XENPF_efi_runtime_call, \
> + .u.efi_runtime_call.function = XEN_EFI_##name, \
> + .u.efi_runtime_call.misc = 0}
> +
> +#define efi_data(op) (op.u.efi_runtime_call)
> +
> +static efi_status_t xen_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
> +{
> + struct xen_platform_op op = INIT_EFI_OP(get_time);
> +
> + if (HYPERVISOR_dom0_op(&op) < 0)
> + return EFI_UNSUPPORTED;
> +
> + if (tm) {
> + BUILD_BUG_ON(sizeof(*tm) != sizeof(efi_data(op).u.get_time.time));
> + memcpy(tm, &efi_data(op).u.get_time.time, sizeof(*tm));
> + }
> +
> + if (tc) {
> + tc->resolution = efi_data(op).u.get_time.resolution;
> + tc->accuracy = efi_data(op).u.get_time.accuracy;
> + tc->sets_to_zero = !!(efi_data(op).misc &
> + XEN_EFI_GET_TIME_SET_CLEARS_NS);
> + }
> +
> + return efi_data(op).status;
> +}
> +
> +static efi_status_t xen_efi_set_time(efi_time_t *tm)
> +{
> + struct xen_platform_op op = INIT_EFI_OP(set_time);
> +
> + BUILD_BUG_ON(sizeof(*tm) != sizeof(efi_data(op).u.set_time));
> + memcpy(&efi_data(op).u.set_time, tm, sizeof(*tm));
> +
> + if (HYPERVISOR_dom0_op(&op) < 0)
> + return EFI_UNSUPPORTED;
> +
> + return efi_data(op).status;
> +}
> +
> +static efi_status_t xen_efi_get_wakeup_time(efi_bool_t *enabled,
> + efi_bool_t *pending,
> + efi_time_t *tm)
> +{
> + struct xen_platform_op op = INIT_EFI_OP(get_wakeup_time);
> +
> + if (HYPERVISOR_dom0_op(&op) < 0)
> + return EFI_UNSUPPORTED;
> +
> + if (tm) {
> + BUILD_BUG_ON(sizeof(*tm) != sizeof(efi_data(op).u.get_wakeup_time));
> + memcpy(tm, &efi_data(op).u.get_wakeup_time, sizeof(*tm));
> + }
> +
> + if (enabled)
> + *enabled = !!(efi_data(op).misc & XEN_EFI_GET_WAKEUP_TIME_ENABLED);
> +
> + if (pending)
> + *pending = !!(efi_data(op).misc & XEN_EFI_GET_WAKEUP_TIME_PENDING);
> +
> + return efi_data(op).status;
> +}
> +
> +static efi_status_t xen_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
> +{
> + struct xen_platform_op op = INIT_EFI_OP(set_wakeup_time);
> +
> + BUILD_BUG_ON(sizeof(*tm) != sizeof(efi_data(op).u.set_wakeup_time));
> + if (enabled)
> + efi_data(op).misc = XEN_EFI_SET_WAKEUP_TIME_ENABLE;
> + if (tm)
> + memcpy(&efi_data(op).u.set_wakeup_time, tm, sizeof(*tm));
> + else
> + efi_data(op).misc |= XEN_EFI_SET_WAKEUP_TIME_ENABLE_ONLY;
> +
> + if (HYPERVISOR_dom0_op(&op) < 0)
> + return EFI_UNSUPPORTED;
> +
> + return efi_data(op).status;
> +}
> +
> +static efi_status_t xen_efi_get_variable(efi_char16_t *name,
> + efi_guid_t *vendor,
> + u32 *attr,
> + unsigned long *data_size,
> + void *data)
> +{
> + struct xen_platform_op op = INIT_EFI_OP(get_variable);
> +
> + set_xen_guest_handle(efi_data(op).u.get_variable.name, name);
> + BUILD_BUG_ON(sizeof(*vendor) !=
> + sizeof(efi_data(op).u.get_variable.vendor_guid));
> + memcpy(&efi_data(op).u.get_variable.vendor_guid, vendor, sizeof(*vendor));
> + efi_data(op).u.get_variable.size = *data_size;
> + set_xen_guest_handle(efi_data(op).u.get_variable.data, data);
> +
> + if (HYPERVISOR_dom0_op(&op) < 0)
> + return EFI_UNSUPPORTED;
> +
> + *data_size = efi_data(op).u.get_variable.size;
> + if (attr)
> + *attr = efi_data(op).misc;
> +
> + return efi_data(op).status;
> +}
> +
> +static efi_status_t xen_efi_get_next_variable(unsigned long *name_size,
> + efi_char16_t *name,
> + efi_guid_t *vendor)
> +{
> + struct xen_platform_op op = INIT_EFI_OP(get_next_variable_name);
> +
> + efi_data(op).u.get_next_variable_name.size = *name_size;
> + set_xen_guest_handle(efi_data(op).u.get_next_variable_name.name, name);
> + BUILD_BUG_ON(sizeof(*vendor) !=
> + sizeof(efi_data(op).u.get_next_variable_name.vendor_guid));
> + memcpy(&efi_data(op).u.get_next_variable_name.vendor_guid, vendor,
> + sizeof(*vendor));
> +
> + if (HYPERVISOR_dom0_op(&op) < 0)
> + return EFI_UNSUPPORTED;
> +
> + *name_size = efi_data(op).u.get_next_variable_name.size;
> + memcpy(vendor, &efi_data(op).u.get_next_variable_name.vendor_guid,
> + sizeof(*vendor));
> +
> + return efi_data(op).status;
> +}
> +
> +static efi_status_t xen_efi_set_variable(efi_char16_t *name,
> + efi_guid_t *vendor,
> + u32 attr,
> + unsigned long data_size,
> + void *data)
> +{
> + struct xen_platform_op op = INIT_EFI_OP(set_variable);
> +
> + set_xen_guest_handle(efi_data(op).u.set_variable.name, name);
> + efi_data(op).misc = attr;
> + BUILD_BUG_ON(sizeof(*vendor) !=
> + sizeof(efi_data(op).u.set_variable.vendor_guid));
> + memcpy(&efi_data(op).u.set_variable.vendor_guid, vendor, sizeof(*vendor));
> + efi_data(op).u.set_variable.size = data_size;
> + set_xen_guest_handle(efi_data(op).u.set_variable.data, data);
> +
> + if (HYPERVISOR_dom0_op(&op) < 0)
> + return EFI_UNSUPPORTED;
> +
> + return efi_data(op).status;
> +}
> +
> +static efi_status_t xen_efi_query_variable_info(u32 attr,
> + u64 *storage_space,
> + u64 *remaining_space,
> + u64 *max_variable_size)
> +{
> + struct xen_platform_op op = INIT_EFI_OP(query_variable_info);
> +
> + if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
> + return EFI_UNSUPPORTED;
> +
> + efi_data(op).u.query_variable_info.attr = attr;
> +
> + if (HYPERVISOR_dom0_op(&op) < 0)
> + return EFI_UNSUPPORTED;
> +
> + *storage_space = efi_data(op).u.query_variable_info.max_store_size;
> + *remaining_space = efi_data(op).u.query_variable_info.remain_store_size;
> + *max_variable_size = efi_data(op).u.query_variable_info.max_size;
> +
> + return efi_data(op).status;
> +}
> +
> +static efi_status_t xen_efi_get_next_high_mono_count(u32 *count)
> +{
> + struct xen_platform_op op = INIT_EFI_OP(get_next_high_monotonic_count);
> +
> + if (HYPERVISOR_dom0_op(&op) < 0)
> + return EFI_UNSUPPORTED;
> +
> + *count = efi_data(op).misc;
> +
> + return efi_data(op).status;
> +}
> +
> +static efi_status_t xen_efi_update_capsule(efi_capsule_header_t **capsules,
> + unsigned long count,
> + unsigned long sg_list)
> +{
> + struct xen_platform_op op = INIT_EFI_OP(update_capsule);
> +
> + if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
> + return EFI_UNSUPPORTED;
> +
> + set_xen_guest_handle(efi_data(op).u.update_capsule.capsule_header_array,
> + capsules);
> + efi_data(op).u.update_capsule.capsule_count = count;
> + efi_data(op).u.update_capsule.sg_list = sg_list;
> +
> + if (HYPERVISOR_dom0_op(&op) < 0)
> + return EFI_UNSUPPORTED;
> +
> + return efi_data(op).status;
> +}
> +
> +static efi_status_t xen_efi_query_capsule_caps(efi_capsule_header_t **capsules,
> + unsigned long count,
> + u64 *max_size,
> + int *reset_type)
> +{
> + struct xen_platform_op op = INIT_EFI_OP(query_capsule_capabilities);
> +
> + if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
> + return EFI_UNSUPPORTED;
> +
> + set_xen_guest_handle(efi_data(op).u.query_capsule_capabilities.capsule_header_array,
> + capsules);
> + efi_data(op).u.query_capsule_capabilities.capsule_count = count;
> +
> + if (HYPERVISOR_dom0_op(&op) < 0)
> + return EFI_UNSUPPORTED;
> +
> + *max_size = efi_data(op).u.query_capsule_capabilities.max_capsule_size;
> + *reset_type = efi_data(op).u.query_capsule_capabilities.reset_type;
> +
> + return efi_data(op).status;
> +}
> +
> +static efi_char16_t vendor[100] __initdata;
> +
> +static efi_system_table_t efi_systab_xen __initdata = {
> + .hdr = {
> + .signature = EFI_SYSTEM_TABLE_SIGNATURE,
> + .revision = 0, /* Initialized later. */
> + .headersize = 0, /* Ignored by Linux Kernel. */
> + .crc32 = 0, /* Ignored by Linux Kernel. */
> + .reserved = 0
> + },
> + .fw_vendor = EFI_INVALID_TABLE_ADDR, /* Initialized later. */
> + .fw_revision = 0, /* Initialized later. */
> + .con_in_handle = EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
> + .con_in = EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
> + .con_out_handle = EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
> + .con_out = EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
> + .stderr_handle = EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
> + .stderr = EFI_INVALID_TABLE_ADDR, /* Not used under Xen. */
> + .runtime = (efi_runtime_services_t *)EFI_INVALID_TABLE_ADDR,
> + /* Not used under Xen. */
> + .boottime = (efi_boot_services_t *)EFI_INVALID_TABLE_ADDR,
> + /* Not used under Xen. */
> + .nr_tables = 0, /* Initialized later. */
> + .tables = EFI_INVALID_TABLE_ADDR /* Initialized later. */
> +};
> +
> +static const struct efi efi_xen __initconst = {
> + .systab = NULL, /* Initialized later. */
> + .runtime_version = 0, /* Initialized later. */
> + .mps = EFI_INVALID_TABLE_ADDR,
> + .acpi = EFI_INVALID_TABLE_ADDR,
> + .acpi20 = EFI_INVALID_TABLE_ADDR,
> + .smbios = EFI_INVALID_TABLE_ADDR,
> + .sal_systab = EFI_INVALID_TABLE_ADDR,
> + .boot_info = EFI_INVALID_TABLE_ADDR,
> + .hcdp = EFI_INVALID_TABLE_ADDR,
> + .uga = EFI_INVALID_TABLE_ADDR,
> + .uv_systab = EFI_INVALID_TABLE_ADDR,
> + .fw_vendor = EFI_INVALID_TABLE_ADDR,
> + .runtime = EFI_INVALID_TABLE_ADDR,
> + .config_table = EFI_INVALID_TABLE_ADDR,
> + .get_time = xen_efi_get_time,
> + .set_time = xen_efi_set_time,
> + .get_wakeup_time = xen_efi_get_wakeup_time,
> + .set_wakeup_time = xen_efi_set_wakeup_time,
> + .get_variable = xen_efi_get_variable,
> + .get_next_variable = xen_efi_get_next_variable,
> + .set_variable = xen_efi_set_variable,
> + .query_variable_info = xen_efi_query_variable_info,
> + .update_capsule = xen_efi_update_capsule,
> + .query_capsule_caps = xen_efi_query_capsule_caps,
> + .get_next_high_mono_count = xen_efi_get_next_high_mono_count,
> + .reset_system = NULL, /* Functionality provided by Xen. */
> + .set_virtual_address_map = NULL, /* Not used under Xen. */
> + .memmap = NULL, /* Not used under Xen. */
> + .flags = 0 /* Initialized later. */
> +};
> +
> +efi_system_table_t __init *xen_efi_probe(void)
> +{
> + struct xen_platform_op op = {
> + .cmd = XENPF_firmware_info,
> + .u.firmware_info = {
> + .type = XEN_FW_EFI_INFO,
> + .index = XEN_FW_EFI_CONFIG_TABLE
> + }
> + };
> + union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> +
> + if (!xen_initial_domain() || HYPERVISOR_dom0_op(&op) < 0)
> + return NULL;
> +
> + /* Here we know that Xen runs on EFI platform. */
> +
> + efi = efi_xen;
> +
> + op.cmd = XENPF_firmware_info;
> + op.u.firmware_info.type = XEN_FW_EFI_INFO;
> + op.u.firmware_info.index = XEN_FW_EFI_VENDOR;
> + info->vendor.bufsz = sizeof(vendor);
> + set_xen_guest_handle(info->vendor.name, vendor);
> +
> + if (HYPERVISOR_dom0_op(&op) == 0) {
> + efi_systab_xen.fw_vendor = __pa_symbol(vendor);
> + efi_systab_xen.fw_revision = info->vendor.revision;
> + } else
> + efi_systab_xen.fw_vendor = __pa_symbol(L"UNKNOWN");
> +
> + op.cmd = XENPF_firmware_info;
> + op.u.firmware_info.type = XEN_FW_EFI_INFO;
> + op.u.firmware_info.index = XEN_FW_EFI_VERSION;
> +
> + if (HYPERVISOR_dom0_op(&op) == 0)
> + efi_systab_xen.hdr.revision = info->version;
> +
> + op.cmd = XENPF_firmware_info;
> + op.u.firmware_info.type = XEN_FW_EFI_INFO;
> + op.u.firmware_info.index = XEN_FW_EFI_RT_VERSION;
> +
> + if (HYPERVISOR_dom0_op(&op) == 0)
> + efi.runtime_version = info->version;
> +
> + op.cmd = XENPF_firmware_info;
> + op.u.firmware_info.type = XEN_FW_EFI_INFO;
> + op.u.firmware_info.index = XEN_FW_EFI_CONFIG_TABLE;
> +
> + if (HYPERVISOR_dom0_op(&op) < 0)
> + BUG();
Is it really worth of a BUG()? Can't we just print a warning and return
NULL? We could still boot without EFI support.
On 13/06/14 18:00, Daniel Kiper wrote:
>
> v5 - suggestions/fixes:
Put after a --- marker.
> +static efi_char16_t vendor[100] __initdata;
Why 100?
David
On Mon, Jun 16, 2014 at 12:55:35PM +0100, Stefano Stabellini wrote:
> On Fri, 13 Jun 2014, Daniel Kiper wrote:
> > This patch enables EFI usage under Xen dom0. Standard EFI Linux
> > Kernel infrastructure cannot be used because it requires direct
> > access to EFI data and code. However, in dom0 case it is not possible
> > because above mentioned EFI stuff is fully owned and controlled
> > by Xen hypervisor. In this case all calls from dom0 to EFI must
> > be requested via special hypercall which in turn executes relevant
> > EFI code in behalf of dom0.
> >
> > When dom0 kernel boots it checks for EFI availability on a machine.
> > If it is detected then artificial EFI system table is filled.
> > Native EFI callas are replaced by functions which mimics them
> > by calling relevant hypercall. Later pointer to EFI system table
> > is passed to standard EFI machinery and it continues EFI subsystem
> > initialization taking into account that there is no direct access
> > to EFI boot services, runtime, tables, structures, etc. After that
> > system runs as usual.
> >
> > This patch is based on Jan Beulich and Tang Liang work.
> >
> > v5 - suggestions/fixes:
> > - improve macro usage readability
> > (suggested by Andrew Cooper and David Vrabel),
> > - conditions cleanup
> > (suggested by David Vrabel),
> > - use -fshort-wchar option
> > (suggested by Jan Beulich),
> > - Kconfig rule cleanup
> > (suggested by Jan Beulich),
> > - forward port fixes from SUSE kernel
> > (suggested by Jan Beulich),
> > - improve commit message
> > (suggested by David Vrabel).
> >
> > v4 - suggestions/fixes:
> > - "just populate an efi_system_table_t object"
> > (suggested by Matt Fleming).
> >
> > Signed-off-by: Jan Beulich <[email protected]>
> > Signed-off-by: Tang Liang <[email protected]>
> > Signed-off-by: Daniel Kiper <[email protected]>
>
> Sorry for commenting on your patches so late in the review cycle.
No problem.
[...]
> > +efi_system_table_t __init *xen_efi_probe(void)
> > +{
> > + struct xen_platform_op op = {
> > + .cmd = XENPF_firmware_info,
> > + .u.firmware_info = {
> > + .type = XEN_FW_EFI_INFO,
> > + .index = XEN_FW_EFI_CONFIG_TABLE
> > + }
> > + };
> > + union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> > +
> > + if (!xen_initial_domain() || HYPERVISOR_dom0_op(&op) < 0)
> > + return NULL;
> > +
> > + /* Here we know that Xen runs on EFI platform. */
> > +
> > + efi = efi_xen;
> > +
> > + op.cmd = XENPF_firmware_info;
> > + op.u.firmware_info.type = XEN_FW_EFI_INFO;
> > + op.u.firmware_info.index = XEN_FW_EFI_VENDOR;
> > + info->vendor.bufsz = sizeof(vendor);
> > + set_xen_guest_handle(info->vendor.name, vendor);
> > +
> > + if (HYPERVISOR_dom0_op(&op) == 0) {
> > + efi_systab_xen.fw_vendor = __pa_symbol(vendor);
> > + efi_systab_xen.fw_revision = info->vendor.revision;
> > + } else
> > + efi_systab_xen.fw_vendor = __pa_symbol(L"UNKNOWN");
> > +
> > + op.cmd = XENPF_firmware_info;
> > + op.u.firmware_info.type = XEN_FW_EFI_INFO;
> > + op.u.firmware_info.index = XEN_FW_EFI_VERSION;
> > +
> > + if (HYPERVISOR_dom0_op(&op) == 0)
> > + efi_systab_xen.hdr.revision = info->version;
> > +
> > + op.cmd = XENPF_firmware_info;
> > + op.u.firmware_info.type = XEN_FW_EFI_INFO;
> > + op.u.firmware_info.index = XEN_FW_EFI_RT_VERSION;
> > +
> > + if (HYPERVISOR_dom0_op(&op) == 0)
> > + efi.runtime_version = info->version;
> > +
> > + op.cmd = XENPF_firmware_info;
> > + op.u.firmware_info.type = XEN_FW_EFI_INFO;
> > + op.u.firmware_info.index = XEN_FW_EFI_CONFIG_TABLE;
> > +
> > + if (HYPERVISOR_dom0_op(&op) < 0)
> > + BUG();
>
> Is it really worth of a BUG()? Can't we just print a warning and return
> NULL? We could still boot without EFI support.
Earlier the same hypercall function succeeded so if here it failed
it means that something is really broken. However, I will try remove
this call and get all data from earlier one. This way we avoid this
BUG() call.
Daniel
On 06/16/2014 04:54 AM, Juergen Gross wrote:
>
> And shouldn't it be removed from include/linux/efi.h as well?
>
Indeed.
-hpa
On Mon, Jun 16, 2014 at 01:00:29PM +0100, David Vrabel wrote:
> On 13/06/14 18:00, Daniel Kiper wrote:
> >
> > v5 - suggestions/fixes:
>
> Put after a --- marker.
Why?
You mean:
---
v5 - suggestions/fixes:
...
> > +static efi_char16_t vendor[100] __initdata;
>
> Why 100?
Well... Quite arbitrary value. The same thing is used in
arch/x86/platform/efi/efi.c:efi_init(). Sadly UEFI sepec
says nothing about maximum vendor name length.
Daniel
(Pulling in Mark Salter for early_ioremap() knowledge)
On Fri, 13 Jun, at 07:00:17PM, Daniel Kiper wrote:
> Use early_mem*() instead of early_io*() because all mapped EFI regions
> are true RAM not I/O regions. Additionally, I/O family calls do not work
> correctly under Xen in our case. AIUI, early_io*() maps/unmaps real machine
> addresses. However, all artificial EFI structures created under Xen live
> in dom0 memory and should be mapped/unmapped using early_mem*() family
> calls which map domain memory.
>
> Signed-off-by: Daniel Kiper <[email protected]>
> ---
> arch/x86/platform/efi/efi.c | 42 +++++++++++++++++++++---------------------
> drivers/firmware/efi/efi.c | 4 ++--
> 2 files changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index 87fc96b..dd1e351 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -427,7 +427,7 @@ void __init efi_unmap_memmap(void)
> {
> clear_bit(EFI_MEMMAP, &efi.flags);
> if (memmap.map) {
> - early_iounmap(memmap.map, memmap.nr_map * memmap.desc_size);
> + early_memunmap(memmap.map, memmap.nr_map * memmap.desc_size);
> memmap.map = NULL;
> }
> }
> @@ -467,12 +467,12 @@ static int __init efi_systab_init(void *phys)
> if (!data)
> return -ENOMEM;
> }
> - systab64 = early_ioremap((unsigned long)phys,
> - sizeof(*systab64));
> + systab64 = early_memremap((unsigned long)phys,
> + sizeof(*systab64));
Please don't misalign the arguments :-( This makes the diff harder to
read when all you're doing is changing the function call. You're
indentation isn't an improvement.
As Matthew pointed out we may also need to access EFI mapped flash
devices.
Now, the only difference between early_memremap() and early_ioremap(),
at least on x86, is PAGE_KERNEL vs. PAGE_KERNEL_IO, where PAGE_KERNEL_IO
has the additional _PAGE_BIT_IOMAP bit set in the pte. But that's a
software bit for x86.
So, even though this change should be harmless, it's not clear to me why
this change is needed. You say "I/O family calls do not work correctly
under Xen in our case", but could you provide some more explanation as
to why they don't work correctly?
--
Matt Fleming, Intel Open Source Technology Center
On 18/06/14 13:17, Matt Fleming wrote:
> (Pulling in Mark Salter for early_ioremap() knowledge)
>
> On Fri, 13 Jun, at 07:00:17PM, Daniel Kiper wrote:
>> Use early_mem*() instead of early_io*() because all mapped EFI regions
>> are true RAM not I/O regions. Additionally, I/O family calls do not work
>> correctly under Xen in our case. AIUI, early_io*() maps/unmaps real machine
>> addresses. However, all artificial EFI structures created under Xen live
>> in dom0 memory and should be mapped/unmapped using early_mem*() family
>> calls which map domain memory.
>>
>> Signed-off-by: Daniel Kiper <[email protected]>
>> ---
>> arch/x86/platform/efi/efi.c | 42 +++++++++++++++++++++---------------------
>> drivers/firmware/efi/efi.c | 4 ++--
>> 2 files changed, 23 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
>> index 87fc96b..dd1e351 100644
>> --- a/arch/x86/platform/efi/efi.c
>> +++ b/arch/x86/platform/efi/efi.c
>> @@ -427,7 +427,7 @@ void __init efi_unmap_memmap(void)
>> {
>> clear_bit(EFI_MEMMAP, &efi.flags);
>> if (memmap.map) {
>> - early_iounmap(memmap.map, memmap.nr_map * memmap.desc_size);
>> + early_memunmap(memmap.map, memmap.nr_map * memmap.desc_size);
>> memmap.map = NULL;
>> }
>> }
>> @@ -467,12 +467,12 @@ static int __init efi_systab_init(void *phys)
>> if (!data)
>> return -ENOMEM;
>> }
>> - systab64 = early_ioremap((unsigned long)phys,
>> - sizeof(*systab64));
>> + systab64 = early_memremap((unsigned long)phys,
>> + sizeof(*systab64));
>
> Please don't misalign the arguments :-( This makes the diff harder to
> read when all you're doing is changing the function call. You're
> indentation isn't an improvement.
>
> As Matthew pointed out we may also need to access EFI mapped flash
> devices.
>
> Now, the only difference between early_memremap() and early_ioremap(),
> at least on x86, is PAGE_KERNEL vs. PAGE_KERNEL_IO, where PAGE_KERNEL_IO
> has the additional _PAGE_BIT_IOMAP bit set in the pte. But that's a
> software bit for x86.
_PAGE_BIT_IOMAP is supposed to be going away...
> So, even though this change should be harmless, it's not clear to me why
> this change is needed. You say "I/O family calls do not work correctly
> under Xen in our case", but could you provide some more explanation as
> to why they don't work correctly?
_PAGE_BIT_IOMAP causes a Xen PV guest to skip the PFN to MFN conversion
when building the PTE. Using it for RAM will attempt to map the wrong
machine frame.
David
On Wed, Jun 18, 2014 at 01:17:09PM +0100, Matt Fleming wrote:
> (Pulling in Mark Salter for early_ioremap() knowledge)
>
> On Fri, 13 Jun, at 07:00:17PM, Daniel Kiper wrote:
> > Use early_mem*() instead of early_io*() because all mapped EFI regions
> > are true RAM not I/O regions. Additionally, I/O family calls do not work
> > correctly under Xen in our case. AIUI, early_io*() maps/unmaps real machine
> > addresses. However, all artificial EFI structures created under Xen live
> > in dom0 memory and should be mapped/unmapped using early_mem*() family
> > calls which map domain memory.
> >
> > Signed-off-by: Daniel Kiper <[email protected]>
> > ---
> > arch/x86/platform/efi/efi.c | 42 +++++++++++++++++++++---------------------
> > drivers/firmware/efi/efi.c | 4 ++--
> > 2 files changed, 23 insertions(+), 23 deletions(-)
> >
> > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> > index 87fc96b..dd1e351 100644
> > --- a/arch/x86/platform/efi/efi.c
> > +++ b/arch/x86/platform/efi/efi.c
> > @@ -427,7 +427,7 @@ void __init efi_unmap_memmap(void)
> > {
> > clear_bit(EFI_MEMMAP, &efi.flags);
> > if (memmap.map) {
> > - early_iounmap(memmap.map, memmap.nr_map * memmap.desc_size);
> > + early_memunmap(memmap.map, memmap.nr_map * memmap.desc_size);
> > memmap.map = NULL;
> > }
> > }
> > @@ -467,12 +467,12 @@ static int __init efi_systab_init(void *phys)
> > if (!data)
> > return -ENOMEM;
> > }
> > - systab64 = early_ioremap((unsigned long)phys,
> > - sizeof(*systab64));
> > + systab64 = early_memremap((unsigned long)phys,
> > + sizeof(*systab64));
>
> Please don't misalign the arguments :-( This makes the diff harder to
> read when all you're doing is changing the function call. You're
> indentation isn't an improvement.
I think that it improves readability a bit but if you wish I will not
do that in the future.
> As Matthew pointed out we may also need to access EFI mapped flash
> devices.
Right, but I think it does not change a lot in that case. Flash
is simply slower type of memory used as permanent storage.
Am I missing something?
> Now, the only difference between early_memremap() and early_ioremap(),
> at least on x86, is PAGE_KERNEL vs. PAGE_KERNEL_IO, where PAGE_KERNEL_IO
> has the additional _PAGE_BIT_IOMAP bit set in the pte. But that's a
> software bit for x86.
>
> So, even though this change should be harmless, it's not clear to me why
> this change is needed. You say "I/O family calls do not work correctly
> under Xen in our case", but could you provide some more explanation as
> to why they don't work correctly?
As I saw David provided better explanation. If you wish I could include
it in commit message.
Daniel
On Fri, 13 Jun, at 07:00:18PM, Daniel Kiper wrote:
> Introduce EFI_NO_DIRECT flag. If it is set then kernel runs
> on EFI platform but it has not direct control on EFI stuff
> like EFI runtime, tables, structures, etc. If not this means
> that Linux Kernel has direct access to EFI infrastructure
> and everything runs as usual.
>
> This functionality is used in Xen dom0 because hypervisor
> has full control on EFI stuff and all calls from dom0 to
> EFI must be requested via special hypercall which in turn
> executes relevant EFI code in behalf of dom0.
>
> v5 - suggestions/fixes:
> - rename EFI_DIRECT to EFI_NO_DIRECT
> (suggested by David Vrabel),
> - limit EFI_NO_DIRECT usage
> (suggested by Jan Beulich and Matt Fleming),
> - improve commit message
> (suggested by David Vrabel).
>
> Signed-off-by: Daniel Kiper <[email protected]>
> ---
> arch/x86/platform/efi/efi.c | 27 +++++++++++++++++++++------
> drivers/firmware/efi/efi.c | 22 +++++++++++++---------
> include/linux/efi.h | 3 ++-
> 3 files changed, 36 insertions(+), 16 deletions(-)
[...]
> @@ -617,13 +620,16 @@ static int __init efi_runtime_init(void)
> * address of several of the EFI runtime functions, needed to
> * set the firmware into virtual mode.
> */
> - if (efi_enabled(EFI_64BIT))
> - rv = efi_runtime_init64();
> - else
> - rv = efi_runtime_init32();
>
> - if (rv)
> - return rv;
> + if (!efi_enabled(EFI_NO_DIRECT)) {
> + if (efi_enabled(EFI_64BIT))
> + rv = efi_runtime_init64();
> + else
> + rv = efi_runtime_init32();
> +
> + if (rv)
> + return rv;
> + }
>
> set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
>
This could do with some comments to explain why you want to set
EFI_RUNTIME_SERVICES even though you're skipping efi_runtime_init*(),
e.g. that for Xen things are already mapped.
I'm not likely to remember the rationale for this in 6 months time, and
anyone else hacking on this code that isn't part of this thread also may
not realise at first glance. Comments would go a long way to fixing
that.
> @@ -1220,6 +1232,9 @@ u64 efi_mem_attributes(unsigned long phys_addr)
> efi_memory_desc_t *md;
> void *p;
>
> + if (!efi_enabled(EFI_MEMMAP))
> + return 0;
> +
> for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
> md = p;
> if ((md->phys_addr <= phys_addr) &&
This should be a separate patch, please.
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 023937a..8bb1075 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -104,16 +104,20 @@ static struct attribute *efi_subsys_attrs[] = {
> static umode_t efi_attr_is_visible(struct kobject *kobj,
> struct attribute *attr, int n)
> {
> - umode_t mode = attr->mode;
> -
> - if (attr == &efi_attr_fw_vendor.attr)
> - return (efi.fw_vendor == EFI_INVALID_TABLE_ADDR) ? 0 : mode;
> - else if (attr == &efi_attr_runtime.attr)
> - return (efi.runtime == EFI_INVALID_TABLE_ADDR) ? 0 : mode;
> - else if (attr == &efi_attr_config_table.attr)
> - return (efi.config_table == EFI_INVALID_TABLE_ADDR) ? 0 : mode;
> + if (attr == &efi_attr_fw_vendor.attr) {
> + if (efi_enabled(EFI_NO_DIRECT) ||
> + efi.fw_vendor == EFI_INVALID_TABLE_ADDR)
> + return 0;
> + } else if (attr == &efi_attr_runtime.attr) {
> + if (efi_enabled(EFI_NO_DIRECT) ||
> + efi.runtime == EFI_INVALID_TABLE_ADDR)
> + return 0;
> + } else if (attr == &efi_attr_config_table.attr) {
> + if (efi.config_table == EFI_INVALID_TABLE_ADDR)
> + return 0;
> + }
>
> - return mode;
> + return attr->mode;
> }
Why don't you want to export efi.fw_vendor, etc? Rationale please.
> static struct attribute_group efi_subsys_attr_group = {
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 41bbf8b..e917c4a 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -916,7 +916,8 @@ extern int __init efi_setup_pcdp_console(char *);
> #define EFI_RUNTIME_SERVICES 3 /* Can we use runtime services? */
> #define EFI_MEMMAP 4 /* Can we use EFI memory map? */
> #define EFI_64BIT 5 /* Is the firmware 64-bit? */
> -#define EFI_ARCH_1 6 /* First arch-specific bit */
> +#define EFI_NO_DIRECT 6 /* Can we access EFI directly? */
> +#define EFI_ARCH_1 7 /* First arch-specific bit */
I like David's suggestion of using EFI_PARAVIRT.
Why the bit shuffling? Are you trying to keep the non-arch bits
together? That does make sense, and I can't help but feel that
EFI_ARCH_1 should probably be bit 31 so we can subtract 1 for each new
arch bit so we don't have to do this constant shuffling in future.
I'll need to think a bit harder about that.
EFI_PARAVIRT will be usable by architectures other than x86, correct? If
your intention is for it only ever to be used by x86, then it should
probably be EFI_ARCH_2.
--
Matt Fleming, Intel Open Source Technology Center
On Fri, 13 Jun, at 07:00:21PM, Daniel Kiper wrote:
> v5 - suggestions/fixes:
> - do not change indentation
> (suggested by Matt Fleming).
>
> Signed-off-by: Daniel Kiper <[email protected]>
> ---
> arch/x86/kernel/setup.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Looks good, but it's customary to have at least one line after the patch
title. Even something as trivial as,
"We've got constants, so let's use them instead of hard-coded values."
--
Matt Fleming, Intel Open Source Technology Center
On Fri, 13 Jun, at 07:00:22PM, Daniel Kiper wrote:
> Remove redundant set_bit(EFI_SYSTEM_TABLES, &efi.flags) call.
> It is executed earlier in efi_systab_init().
>
> Signed-off-by: Daniel Kiper <[email protected]>
> ---
> arch/x86/platform/efi/efi.c | 2 --
> 1 file changed, 2 deletions(-)
Looks good!
--
Matt Fleming, Intel Open Source Technology Center
>>> On 18.06.14 at 15:52, <[email protected]> wrote:
> EFI_PARAVIRT will be usable by architectures other than x86, correct? If
> your intention is for it only ever to be used by x86, then it should
> probably be EFI_ARCH_2.
I would expect ARM, once it gets UEFI support on the Xen side, to
be able to handle most of this identically to x86. Which raises the
question whether most of the new Xen-specific code (in one of the
other patches) wouldn't better live under drivers/xen/.
Jan
On Wed, Jun 18, 2014 at 02:56:37PM +0100, Matt Fleming wrote:
> On Fri, 13 Jun, at 07:00:22PM, Daniel Kiper wrote:
> > Remove redundant set_bit(EFI_SYSTEM_TABLES, &efi.flags) call.
> > It is executed earlier in efi_systab_init().
> >
> > Signed-off-by: Daniel Kiper <[email protected]>
> > ---
> > arch/x86/platform/efi/efi.c | 2 --
> > 1 file changed, 2 deletions(-)
>
> Looks good!
Is that an Acked-by or Reviewed-by ? :-)
>
> --
> Matt Fleming, Intel Open Source Technology Center
On Wed, 2014-06-18 at 13:17 +0100, Matt Fleming wrote:
> Now, the only difference between early_memremap() and early_ioremap(),
> at least on x86, is PAGE_KERNEL vs. PAGE_KERNEL_IO, where PAGE_KERNEL_IO
> has the additional _PAGE_BIT_IOMAP bit set in the pte. But that's a
> software bit for x86.
I can't comment on the x86 MMU details, but using early_memremap() will
get rid of some sparse warnings in the current code.
On Fri, 13 Jun, at 07:00:23PM, Daniel Kiper wrote:
> efi_set_rtc_mmss() is never used to set RTC due to bugs found
> on many EFI platforms. It is set directly by mach_set_rtc_mmss().
>
> Signed-off-by: Daniel Kiper <[email protected]>
> ---
> arch/x86/platform/efi/efi.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index deb4f05..bd3e080 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -244,6 +244,11 @@ static efi_status_t __init phys_efi_set_virtual_address_map(
> return status;
> }
>
> +#if 0
> +/*
> + * efi_set_rtc_mmss() is never used to set RTC due to bugs found
> + * on many EFI platforms. It is set directly by mach_set_rtc_mmss().
> + */
> int efi_set_rtc_mmss(const struct timespec *now)
> {
> unsigned long nowtime = now->tv_sec;
> @@ -279,6 +284,7 @@ int efi_set_rtc_mmss(const struct timespec *now)
> }
> return 0;
> }
> +#endif
>
> void efi_get_time(struct timespec *now)
> {
As others have said in this thread - just delete this sucker.
I did make an attempt to get rid of all the time functions recently, but
chickened out at the last minute because we were nearing the merge
window,
http://article.gmane.org/gmane.linux.kernel.janitors/30082
So yes, please go ahead and delete this function.
--
Matt Fleming, Intel Open Source Technology Center
On Wed, 18 Jun, at 10:01:01AM, Konrad Rzeszutek Wilk wrote:
> On Wed, Jun 18, 2014 at 02:56:37PM +0100, Matt Fleming wrote:
> > On Fri, 13 Jun, at 07:00:22PM, Daniel Kiper wrote:
> > > Remove redundant set_bit(EFI_SYSTEM_TABLES, &efi.flags) call.
> > > It is executed earlier in efi_systab_init().
> > >
> > > Signed-off-by: Daniel Kiper <[email protected]>
> > > ---
> > > arch/x86/platform/efi/efi.c | 2 --
> > > 1 file changed, 2 deletions(-)
> >
> > Looks good!
>
> Is that an Acked-by or Reviewed-by ? :-)
Since I assumed this series would be going through my tree, I didn't
think it mattered.
Reviewed-by: Matt Fleming <[email protected]>
--
Matt Fleming, Intel Open Source Technology Center
On Fri, 13 Jun, at 07:00:16PM, Daniel Kiper wrote:
> Hey,
>
> This patch series adds EFI support for Xen dom0 guests.
> It is based on Jan Beulich and Tang Liang work. I was
> trying to take into account all previous comments,
> however, if I missed something sorry for that.
>
> Daniel
>
> arch/x86/kernel/setup.c | 4 +-
> arch/x86/platform/efi/efi.c | 77 ++++++++++-------
> arch/x86/xen/enlighten.c | 24 ++++++
> drivers/firmware/efi/efi.c | 26 +++---
> drivers/xen/Kconfig | 4 +
> drivers/xen/Makefile | 3 +
> drivers/xen/efi.c | 374 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/efi.h | 3 +-
> include/xen/interface/platform.h | 123 +++++++++++++++++++++++++++
> 9 files changed, 595 insertions(+), 43 deletions(-)
FWIW I'm much happier with this version than v4. There's nothing that
jumps out at me as being obviously wrong. Just a few minor cleanups
needed.
Which tree is this intended to go through? I'm more than happy to take
it through the EFI tree, particularly since it touches
include/linux/efi.h and drivers/firmware/efi/efi.c which is the core EFI
interface for all platforms.
But if it's going through the Xen tree or something, I can supply my
Acked-by's (once the aforementioned minor cleanups are taken care of).
--
Matt Fleming, Intel Open Source Technology Center
On Wed, 18 Jun 2014, Jan Beulich wrote:
> >>> On 18.06.14 at 15:52, <[email protected]> wrote:
> > EFI_PARAVIRT will be usable by architectures other than x86, correct? If
> > your intention is for it only ever to be used by x86, then it should
> > probably be EFI_ARCH_2.
>
> I would expect ARM, once it gets UEFI support on the Xen side, to
> be able to handle most of this identically to x86. Which raises the
> question whether most of the new Xen-specific code (in one of the
> other patches) wouldn't better live under drivers/xen/.
I was thinking the same thing.
However this patch series doesn't add much code outside
drivers/xen/efi.c and include/xen/interface/platform.h.
I think it wouldn't be fair to ask Daniel to refactor the efi code
currently under arch/x86 to an arch-independent location.
Whoever comes in later and adds EFI Xen support for ARM can do that.
On Wed, Jun 18, 2014 at 03:15:41PM +0100, Matt Fleming wrote:
> On Fri, 13 Jun, at 07:00:16PM, Daniel Kiper wrote:
> > Hey,
> >
> > This patch series adds EFI support for Xen dom0 guests.
> > It is based on Jan Beulich and Tang Liang work. I was
> > trying to take into account all previous comments,
> > however, if I missed something sorry for that.
> >
> > Daniel
> >
> > arch/x86/kernel/setup.c | 4 +-
> > arch/x86/platform/efi/efi.c | 77 ++++++++++-------
> > arch/x86/xen/enlighten.c | 24 ++++++
> > drivers/firmware/efi/efi.c | 26 +++---
> > drivers/xen/Kconfig | 4 +
> > drivers/xen/Makefile | 3 +
> > drivers/xen/efi.c | 374 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/efi.h | 3 +-
> > include/xen/interface/platform.h | 123 +++++++++++++++++++++++++++
> > 9 files changed, 595 insertions(+), 43 deletions(-)
>
> FWIW I'm much happier with this version than v4. There's nothing that
> jumps out at me as being obviously wrong. Just a few minor cleanups
> needed.
Fantastic!
>
> Which tree is this intended to go through? I'm more than happy to take
> it through the EFI tree, particularly since it touches
> include/linux/efi.h and drivers/firmware/efi/efi.c which is the core EFI
> interface for all platforms.
Your tree is perfect. I presume you do some automatic regression testing
when you have accumulated a tons of patch - so if any of them cause havoc
we can catch them before the merge window and fix them.
>
> But if it's going through the Xen tree or something, I can supply my
> Acked-by's (once the aforementioned minor cleanups are taken care of).
Nah, lets do your tree. And either Boris, me or David will supply
the Acked-by on the drivers/xen* and arch/x86/xen/* so all the
right tags are in place.
Thanks again!
>
> --
> Matt Fleming, Intel Open Source Technology Center
On Wed, 18 Jun, at 10:31:43AM, Konrad Rzeszutek Wilk wrote:
>
> Your tree is perfect. I presume you do some automatic regression testing
> when you have accumulated a tons of patch - so if any of them cause havoc
> we can catch them before the merge window and fix them.
Yeah, I run regression tests to catch breakage to EFI platforms, usually
as soon as I apply them.
> Nah, lets do your tree. And either Boris, me or David will supply
> the Acked-by on the drivers/xen* and arch/x86/xen/* so all the
> right tags are in place.
Sounds like a plan.
> Thanks again!
My pleasure.
--
Matt Fleming, Intel Open Source Technology Center
On Wed, 18 Jun, at 02:59:57PM, Daniel Kiper wrote:
>
> I think that it improves readability a bit but if you wish I will not
> do that in the future.
That would be much appreciated.
> > As Matthew pointed out we may also need to access EFI mapped flash
> > devices.
>
> Right, but I think it does not change a lot in that case. Flash
> is simply slower type of memory used as permanent storage.
> Am I missing something?
No you're not missing anything, I hit send too quickly while typing my
reply so the above isn't a complete thought. I don't think this
distinction between mapped flash and RAM matters in this case.
> As I saw David provided better explanation. If you wish I could include
> it in commit message.
Yeah, I think that would be very helpful, thanks!
--
Matt Fleming, Intel Open Source Technology Center
On Wed, Jun 18, 2014 at 03:30:25PM +0100, Stefano Stabellini wrote:
> On Wed, 18 Jun 2014, Jan Beulich wrote:
> > >>> On 18.06.14 at 15:52, <[email protected]> wrote:
> > > EFI_PARAVIRT will be usable by architectures other than x86, correct? If
> > > your intention is for it only ever to be used by x86, then it should
> > > probably be EFI_ARCH_2.
> >
> > I would expect ARM, once it gets UEFI support on the Xen side, to
> > be able to handle most of this identically to x86. Which raises the
> > question whether most of the new Xen-specific code (in one of the
> > other patches) wouldn't better live under drivers/xen/.
>
> I was thinking the same thing.
>
> However this patch series doesn't add much code outside
> drivers/xen/efi.c and include/xen/interface/platform.h.
> I think it wouldn't be fair to ask Daniel to refactor the efi code
> currently under arch/x86 to an arch-independent location.
> Whoever comes in later and adds EFI Xen support for ARM can do that.
I was doing those EFI patches with ARM support in mind from the beginning.
That is why I moved everything, which was possible, to arch independent
place. There are only some simple init calls from arch/x86. Just a few
required lines. So I think that it will be quite easy to use this EFI
code on ARM platform.
Daniel
On Wed, 18 Jun, at 03:30:25PM, Stefano Stabellini wrote:
>
> I was thinking the same thing.
>
> However this patch series doesn't add much code outside
> drivers/xen/efi.c and include/xen/interface/platform.h.
> I think it wouldn't be fair to ask Daniel to refactor the efi code
> currently under arch/x86 to an arch-independent location.
> Whoever comes in later and adds EFI Xen support for ARM can do that.
I agree that we shouldn't necessarily build tons of arch-independent
infrastructure when there's only one user, but it is wise to be careful
about the design decisions being made if you know there's another user
coming around the corner.
What we don't want to happen, is for the designs that we have now to
become set in stone such that they can never be changed, to the
deteriment of code duplication and maintenance.
I don't think that's happening in this patch series, but it's definitely
smart discussing this stuff upfront.
--
Matt Fleming, Intel Open Source Technology Center
On Wed, Jun 18, 2014 at 02:52:29PM +0100, Matt Fleming wrote:
> On Fri, 13 Jun, at 07:00:18PM, Daniel Kiper wrote:
> > Introduce EFI_NO_DIRECT flag. If it is set then kernel runs
> > on EFI platform but it has not direct control on EFI stuff
> > like EFI runtime, tables, structures, etc. If not this means
> > that Linux Kernel has direct access to EFI infrastructure
> > and everything runs as usual.
> >
> > This functionality is used in Xen dom0 because hypervisor
> > has full control on EFI stuff and all calls from dom0 to
> > EFI must be requested via special hypercall which in turn
> > executes relevant EFI code in behalf of dom0.
> >
> > v5 - suggestions/fixes:
> > - rename EFI_DIRECT to EFI_NO_DIRECT
> > (suggested by David Vrabel),
> > - limit EFI_NO_DIRECT usage
> > (suggested by Jan Beulich and Matt Fleming),
> > - improve commit message
> > (suggested by David Vrabel).
> >
> > Signed-off-by: Daniel Kiper <[email protected]>
> > ---
> > arch/x86/platform/efi/efi.c | 27 +++++++++++++++++++++------
> > drivers/firmware/efi/efi.c | 22 +++++++++++++---------
> > include/linux/efi.h | 3 ++-
> > 3 files changed, 36 insertions(+), 16 deletions(-)
>
> [...]
>
> > @@ -617,13 +620,16 @@ static int __init efi_runtime_init(void)
> > * address of several of the EFI runtime functions, needed to
> > * set the firmware into virtual mode.
> > */
> > - if (efi_enabled(EFI_64BIT))
> > - rv = efi_runtime_init64();
> > - else
> > - rv = efi_runtime_init32();
> >
> > - if (rv)
> > - return rv;
> > + if (!efi_enabled(EFI_NO_DIRECT)) {
> > + if (efi_enabled(EFI_64BIT))
> > + rv = efi_runtime_init64();
> > + else
> > + rv = efi_runtime_init32();
> > +
> > + if (rv)
> > + return rv;
> > + }
> >
> > set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> >
>
> This could do with some comments to explain why you want to set
> EFI_RUNTIME_SERVICES even though you're skipping efi_runtime_init*(),
> e.g. that for Xen things are already mapped.
>
> I'm not likely to remember the rationale for this in 6 months time, and
> anyone else hacking on this code that isn't part of this thread also may
> not realise at first glance. Comments would go a long way to fixing
> that.
OK, I will add relevant comment here.
> > @@ -1220,6 +1232,9 @@ u64 efi_mem_attributes(unsigned long phys_addr)
> > efi_memory_desc_t *md;
> > void *p;
> >
> > + if (!efi_enabled(EFI_MEMMAP))
> > + return 0;
> > +
> > for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
> > md = p;
> > if ((md->phys_addr <= phys_addr) &&
>
> This should be a separate patch, please.
OK. I was not sure about that.
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 023937a..8bb1075 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -104,16 +104,20 @@ static struct attribute *efi_subsys_attrs[] = {
> > static umode_t efi_attr_is_visible(struct kobject *kobj,
> > struct attribute *attr, int n)
> > {
> > - umode_t mode = attr->mode;
> > -
> > - if (attr == &efi_attr_fw_vendor.attr)
> > - return (efi.fw_vendor == EFI_INVALID_TABLE_ADDR) ? 0 : mode;
> > - else if (attr == &efi_attr_runtime.attr)
> > - return (efi.runtime == EFI_INVALID_TABLE_ADDR) ? 0 : mode;
> > - else if (attr == &efi_attr_config_table.attr)
> > - return (efi.config_table == EFI_INVALID_TABLE_ADDR) ? 0 : mode;
> > + if (attr == &efi_attr_fw_vendor.attr) {
> > + if (efi_enabled(EFI_NO_DIRECT) ||
> > + efi.fw_vendor == EFI_INVALID_TABLE_ADDR)
> > + return 0;
> > + } else if (attr == &efi_attr_runtime.attr) {
> > + if (efi_enabled(EFI_NO_DIRECT) ||
> > + efi.runtime == EFI_INVALID_TABLE_ADDR)
> > + return 0;
> > + } else if (attr == &efi_attr_config_table.attr) {
> > + if (efi.config_table == EFI_INVALID_TABLE_ADDR)
> > + return 0;
> > + }
> >
> > - return mode;
> > + return attr->mode;
> > }
>
> Why don't you want to export efi.fw_vendor, etc? Rationale please.
I am exporting real addresses (machine addresses) of things which
I am able to get. Stuff which was created artificially and lives
in dom0 address space or does not exist are not exported.
> > static struct attribute_group efi_subsys_attr_group = {
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 41bbf8b..e917c4a 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -916,7 +916,8 @@ extern int __init efi_setup_pcdp_console(char *);
> > #define EFI_RUNTIME_SERVICES 3 /* Can we use runtime services? */
> > #define EFI_MEMMAP 4 /* Can we use EFI memory map? */
> > #define EFI_64BIT 5 /* Is the firmware 64-bit? */
> > -#define EFI_ARCH_1 6 /* First arch-specific bit */
> > +#define EFI_NO_DIRECT 6 /* Can we access EFI directly? */
> > +#define EFI_ARCH_1 7 /* First arch-specific bit */
>
> I like David's suggestion of using EFI_PARAVIRT.
>
> Why the bit shuffling? Are you trying to keep the non-arch bits
> together? That does make sense, and I can't help but feel that
Yep.
> EFI_ARCH_1 should probably be bit 31 so we can subtract 1 for each new
> arch bit so we don't have to do this constant shuffling in future.
>
> I'll need to think a bit harder about that.
Hmmm... I do not know what is wrong with this minimal shuffling. We are
playing here with internal stuff which is not visible outside of any
given kernel. Additionally, as I saw in a few places arch bits are
defined in following way:
#define ARCH_1 10
#define A_ARCH_CONST ARCH_1
#define B_ARCH_CONST (ARCH_1 + 1)
#define C_ARCH_CONST (ARCH_1 + 2)
...
So I think addition is more natural here than subtraction.
Daniel
On Wed, 18 Jun, at 06:48:35PM, Daniel Kiper wrote:
> >
> > Why don't you want to export efi.fw_vendor, etc? Rationale please.
>
> I am exporting real addresses (machine addresses) of things which
> I am able to get. Stuff which was created artificially and lives
> in dom0 address space or does not exist are not exported.
So, wouldn't it be easier to just leave those fields as
EFI_INVALID_TABLE_ADDR? If they're not usable why fill out an address?
> Hmmm... I do not know what is wrong with this minimal shuffling. We are
> playing here with internal stuff which is not visible outside of any
> given kernel. Additionally, as I saw in a few places arch bits are
> defined in following way:
>
> #define ARCH_1 10
>
> #define A_ARCH_CONST ARCH_1
> #define B_ARCH_CONST (ARCH_1 + 1)
> #define C_ARCH_CONST (ARCH_1 + 2)
> ...
>
> So I think addition is more natural here than subtraction.
No, because we hit the same problem if we need more non-arch bits after
bit 9, it moves the problem, it doesn't fix it. Though admittedly, using
this level of indirection (going through ARCH_1) instead of using
constants does reduce the problem.
Yes, these bits are internal, and yes the shuffling is minimal, but we
can do better.
I'm not suggesting you need to modify your patch. I'm really just
thinking out loud. I'll take care of fixing this up later.
--
Matt Fleming, Intel Open Source Technology Center
On Thu, Jun 19, 2014 at 03:41:12PM +0100, Matt Fleming wrote:
> On Wed, 18 Jun, at 06:48:35PM, Daniel Kiper wrote:
> > >
> > > Why don't you want to export efi.fw_vendor, etc? Rationale please.
> >
> > I am exporting real addresses (machine addresses) of things which
> > I am able to get. Stuff which was created artificially and lives
> > in dom0 address space or does not exist are not exported.
>
> So, wouldn't it be easier to just leave those fields as
> EFI_INVALID_TABLE_ADDR? If they're not usable why fill out an address?
It looks that EFI_PARAVIRT check is needed for efi.fw_vendor only.
Probably I prepared this chunk of code before filling
efi_systab_xen.runtime with EFI_INVALID_TABLE_ADDR. efi.fw_vendor
contains pointer to vendor name which lives in dom0 memory and it
is a copy of name living in EFI memory data. So, this pointer is useless
for potential user of /sys/firmware/efi/fw_vendor. efi.fw_vendor is
used in init code only. Hence, I was thinking once about wiping this
pointer with EFI_INVALID_TABLE_ADDR. However, it requires some changes
in arch/x86/platform/efi/efi.c (wipe it if EFI_PARAVIRT is set) and
it is a hack for me. I prefer to live with current solution.
> > Hmmm... I do not know what is wrong with this minimal shuffling. We are
> > playing here with internal stuff which is not visible outside of any
> > given kernel. Additionally, as I saw in a few places arch bits are
> > defined in following way:
> >
> > #define ARCH_1 10
> >
> > #define A_ARCH_CONST ARCH_1
> > #define B_ARCH_CONST (ARCH_1 + 1)
> > #define C_ARCH_CONST (ARCH_1 + 2)
> > ...
> >
> > So I think addition is more natural here than subtraction.
>
> No, because we hit the same problem if we need more non-arch bits after
> bit 9, it moves the problem, it doesn't fix it. Though admittedly, using
10 is just an example. It could be an arbitrary value. However, you
are right. This just moves the problem.
> this level of indirection (going through ARCH_1) instead of using
> constants does reduce the problem.
>
> Yes, these bits are internal, and yes the shuffling is minimal, but we
> can do better.
As always, it is a place for an improvement. However, I am not sure
it is worth fighting with that so hard.
> I'm not suggesting you need to modify your patch. I'm really just
> thinking out loud. I'll take care of fixing this up later.
OK. Thanks.
Daniel