Re: Is: Linux-EFI code to call Xen's EFI hypercalls [RFC PATCH comments] Was:Re: Xen 4.4 development update

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, Aug 15, 2013 at 04:50:47PM -0400, Konrad Rzeszutek Wilk wrote:
> On Thu, Aug 15, 2013 at 04:24:20PM -0400, Eric Shelton wrote:
> > On Thu, Aug 15, 2013 at 10:12 AM, Konrad Rzeszutek Wilk
> > <konrad.wilk@oracle.com> wrote:
> > > On Thu, Aug 15, 2013 at 01:32:12AM -0400, Eric Shelton wrote:
> > >
> > > First of, thank you for taking a stab at it and redoing it.
> > 
> > Sure.  The first releases of the patch were posted during a hectic
> > time in the release cycle.  Three months later, I wanted to make sure
> > this had not gotten lost in the noise, and instead got wrapped up
> > while it was still fairly fresh for me.
> > 
> > > Some general comments:
> > >  - the #ifdef CONFIG_XEN is generaly frowend upon. If you need them they should
> > >    be in header files. The exception is the CONFIG_X86 and
> > >    CONFIG_X86_64. Now said that there are other instances of code where
> > >    it is sprinkled with #ifdef CONFIG_ACPI .. #ifdef CONFIG_PCI, and so
> > >    on. It would have been nice if all of that got moved to a header file
> > >    but in reality that could make it just more confusing. Enough about
> > >    that - what I want to say is that doing #idef CONFIG_XEN is something
> > >    we have been trying the utmost to not put in generic code. The
> > >    reasoning is that instead of using the API (so for example if we have
> > >    an generic layer and then there are platform related drivers - we
> > >    want to implement the stuff in the platform drivers - not add
> > >    side-channels for a specific platform).
> > 
> > OK.  Hopefully the reorganization I suggest below will clear out most
> > or all of this.
> > 
> > >  - Is there any way to move the bulk of the hypercalls in the
> > >    efi_runtime services. Meaning alter the efi_runtime services
> > >    to do hypercalls instead of EFI calls? That way I would think most
> > >    of the EFI general logic would be untouched? This is going back to
> > >    the first comment - you would want to leave as much generic code
> > >    untouched as possible. And implement the bulk of the code in the
> > >    platform specific code.
> > 
> > This sounds similar to Matthew Garrett's suggestion "to do this by
> > replacing efi_call_*  I'm not quite sure how I would "alter the
> > efi_runtime services" to accomplish this - or at least not in some way
> > that is not worse than what is being done for things like
> > *_set_variable().  If you can more concretely show me how this might
> > be coded for one of the runtime service functions, I would be happy to
> > replicate that across the patch.
> 
> Ha! I am just hand-waving right now :-)
> 
> What I had in mind was that this:
> 
>          efi_phys.systab = (efi_system_table_t *)boot_params.efi_info.efi_systab;
> 
> Is done, and we could implement some stub functions in the efi_systab
> that would convert the Microsoft stack passing to normal Linux
> and then do the hypercalls.
> 
> It would be a bit of this:
> 
> virt_efi_get_time -> calls efi_call_virt2 (efi_stub_32|64.S) ->
> assembler code Linux to EFI stacks, and calls in
> 	efi.systab->runtime->get_time
> 
> The get_time would be our own little blob of code that alters the
> parameters from EFI stack to Linux and makes the hypercall (so probably
> in C). Then when the hypercall is done, it changes the stack back to EFI
> and returns.
> 
> In other words we would implement an EFI runtime code that actually
> would do hypercalls.
> 
> But from your analysis that does not solve the whole problem. Those
> efi_init* variants in some cases are unneccessary. Which brings another
> question - if we do barell throught them, what is the worst that will
> happen? Can the values returned be the same?

Right after I sent the email I thought about another option. Which is
the one I think Matthew was referring to. That is make efi_call*
function be more of a function pointer and the default one (compiled) is
the baremetal version. When Xen boots it over-writes it with its
specific. There is also some lookup to figure out which of the set_time,
get_time, etc calls are being called. This is all hand-waving and the
patch below has not even been compile tested.

>From 197339fe9e4c95abe5b8948cf2bc3119c0ec87b5 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Fri, 16 Aug 2013 10:06:52 -0400
Subject: [PATCH] efi/xen: Use a function pointer for baremetal and Xen
 specific

efi calls.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/include/asm/efi.h          | 43 +++++++++++++++++++++++++++++++------
 arch/x86/platform/efi/efi_64.c      | 11 ++++++++++
 arch/x86/platform/efi/efi_stub_64.S | 28 ++++++++++++------------
 arch/x86/xen/efi.c                  | 40 ++++++++++++++++++++++++++++++++++
 arch/x86/xen/setup.c                |  9 ++++++++
 arch/x86/xen/xen-ops.h              | 13 +++++++++++
 6 files changed, 123 insertions(+), 21 deletions(-)
 create mode 100644 arch/x86/xen/efi.c

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 0062a01..f234570 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -41,16 +41,45 @@ extern unsigned long asmlinkage efi_call_phys(void *, ...);
 
 #define EFI_LOADER_SIGNATURE	"EL64"
 
-extern u64 efi_call0(void *fp);
-extern u64 efi_call1(void *fp, u64 arg1);
-extern u64 efi_call2(void *fp, u64 arg1, u64 arg2);
-extern u64 efi_call3(void *fp, u64 arg1, u64 arg2, u64 arg3);
-extern u64 efi_call4(void *fp, u64 arg1, u64 arg2, u64 arg3, u64 arg4);
-extern u64 efi_call5(void *fp, u64 arg1, u64 arg2, u64 arg3,
+extern u64 native_efi_call0(void *fp);
+extern u64 native_efi_call1(void *fp, u64 arg1);
+extern u64 native_efi_call2(void *fp, u64 arg1, u64 arg2);
+extern u64 native_efi_call3(void *fp, u64 arg1, u64 arg2, u64 arg3);
+extern u64 native_efi_call4(void *fp, u64 arg1, u64 arg2, u64 arg3, u64 arg4);
+extern u64 native_efi_call5(void *fp, u64 arg1, u64 arg2, u64 arg3,
 		     u64 arg4, u64 arg5);
-extern u64 efi_call6(void *fp, u64 arg1, u64 arg2, u64 arg3,
+extern u64 native_efi_call6(void *fp, u64 arg1, u64 arg2, u64 arg3,
 		     u64 arg4, u64 arg5, u64 arg6);
 
+#ifdef CONFIG_PARAVIRT
+extern u64 (*platform_efi_call0)(void *fp);
+extern u64 (*platform_efi_call1)(void *fp, u64 arg1);
+extern u64 (*platform_efi_call2)(void *fp, u64 arg1, u64 arg2);
+extern u64 (*platform_efi_call3)(void *fp, u64 arg1, u64 arg2, u64 arg3);
+extern u64 (*platform_efi_call4)(void *fp, u64 arg1, u64 arg2, u64 arg3, u64 arg4);
+extern u64 (*platform_efi_call5)(void *fp, u64 arg1, u64 arg2, u64 arg3,
+		     u64 arg4, u64 arg5);
+extern u64 (*platform_efi_call6)(void *fp, u64 arg1, u64 arg2, u64 arg3,
+		     u64 arg4, u64 arg5, u64 arg6);
+
+#define efi_call0(f)	platform_efi_call0(f)
+#define efi_call1(f, a1)	platform_efi_call1(f, a1)
+#define efi_call2(f, a1, a2)	platform_efi_call2(f, a1, a2)
+#define efi_call3(f, a1, a2, a3)	platform_efi_call3(f, a1, a2, a3)
+#define efi_call4(f, a1, a2, a3, a4)	platform_efi_call4(f, a1, a2, a3, a4)
+#define efi_call5(f, a1, a2, a3, a4, a5)	platform_efi_call5(f, a1, a2, a3, a4, a5)
+#define efi_call6(f, a1, a2, a3, a4, a5, a6)	platform_efi_call6(f, a1, a2, a3, a4, a5, a6)
+#else
+#define efi_call0(f)	native_efi_call0(f)
+#define efi_call1(f, a1)	native_efi_call1(f, a1)
+#define efi_call2(f, a1, a2)	native_efi_call2(f, a1, a2)
+#define efi_call3(f, a1, a2, a3)	native_efi_call3(f, a1, a2, a3)
+#define efi_call4(f, a1, a2, a3, a4)	native_efi_call4(f, a1, a2, a3, a4)
+#define efi_call5(f, a1, a2, a3, a4, a5)	native_efi_call5(f, a1, a2, a3, a4, a5)
+#define efi_call6(f, a1, a2, a3, a4, a5, a6)	native_efi_call6(f, a1, a2, a3, a4, a5, a6)
+
+#endif
+
 #define efi_call_phys0(f)			\
 	efi_call0((f))
 #define efi_call_phys1(f, a1)			\
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 39a0e7f..afa4354 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -113,3 +113,14 @@ void __iomem *__init efi_ioremap(unsigned long phys_addr, unsigned long size,
 
 	return (void __iomem *)__va(phys_addr);
 }
+#ifdef CONFIG_PARAVIRT
+u64 (*platform_efi_call0)(void *fp) = native_efi_call0;
+u64 (*platform_efi_call1)(void *fp, u64 arg1) = native_efi_call1;
+u64 (*platform_efi_call2)(void *fp, u64 arg1, u64 arg2) = native_efi_call2;
+u64 (*platform_efi_call3)(void *fp, u64 arg1, u64 arg2, u64 arg3) = native_efi_call3;
+u64 (*platform_efi_call4)(void *fp, u64 arg1, u64 arg2, u64 arg3, u64 arg4) = native_efi_call4;
+u64 (*platform_efi_call5)(void *fp, u64 arg1, u64 arg2, u64 arg3,
+		     u64 arg4, u64 arg5) = native_efi_call5;
+u64 (*platform_efi_call6)(void *fp, u64 arg1, u64 arg2, u64 arg3,
+		     u64 arg4, u64 arg5, u64 arg6) = native_efi_call6;
+#endif
diff --git a/arch/x86/platform/efi/efi_stub_64.S b/arch/x86/platform/efi/efi_stub_64.S
index 4c07cca..60e993c 100644
--- a/arch/x86/platform/efi/efi_stub_64.S
+++ b/arch/x86/platform/efi/efi_stub_64.S
@@ -34,16 +34,16 @@
 	mov %rsi, %cr0;			\
 	mov (%rsp), %rsp
 
-ENTRY(efi_call0)
+ENTRY(native_efi_call0)
 	SAVE_XMM
 	subq $32, %rsp
 	call *%rdi
 	addq $32, %rsp
 	RESTORE_XMM
 	ret
-ENDPROC(efi_call0)
+ENDPROC(native_efi_call0)
 
-ENTRY(efi_call1)
+ENTRY(native_efi_call1)
 	SAVE_XMM
 	subq $32, %rsp
 	mov  %rsi, %rcx
@@ -51,9 +51,9 @@ ENTRY(efi_call1)
 	addq $32, %rsp
 	RESTORE_XMM
 	ret
-ENDPROC(efi_call1)
+ENDPROC(native_efi_call1)
 
-ENTRY(efi_call2)
+ENTRY(native_efi_call2)
 	SAVE_XMM
 	subq $32, %rsp
 	mov  %rsi, %rcx
@@ -61,9 +61,9 @@ ENTRY(efi_call2)
 	addq $32, %rsp
 	RESTORE_XMM
 	ret
-ENDPROC(efi_call2)
+ENDPROC(native_efi_call2)
 
-ENTRY(efi_call3)
+ENTRY(native_efi_call3)
 	SAVE_XMM
 	subq $32, %rsp
 	mov  %rcx, %r8
@@ -72,9 +72,9 @@ ENTRY(efi_call3)
 	addq $32, %rsp
 	RESTORE_XMM
 	ret
-ENDPROC(efi_call3)
+ENDPROC(native_efi_call3)
 
-ENTRY(efi_call4)
+ENTRY(native_efi_call4)
 	SAVE_XMM
 	subq $32, %rsp
 	mov %r8, %r9
@@ -84,9 +84,9 @@ ENTRY(efi_call4)
 	addq $32, %rsp
 	RESTORE_XMM
 	ret
-ENDPROC(efi_call4)
+ENDPROC(native_efi_call4)
 
-ENTRY(efi_call5)
+ENTRY(native_efi_call5)
 	SAVE_XMM
 	subq $48, %rsp
 	mov %r9, 32(%rsp)
@@ -97,9 +97,9 @@ ENTRY(efi_call5)
 	addq $48, %rsp
 	RESTORE_XMM
 	ret
-ENDPROC(efi_call5)
+ENDPROC(native_efi_call5)
 
-ENTRY(efi_call6)
+ENTRY(native_efi_call6)
 	SAVE_XMM
 	mov (%rsp), %rax
 	mov 8(%rax), %rax
@@ -113,4 +113,4 @@ ENTRY(efi_call6)
 	addq $48, %rsp
 	RESTORE_XMM
 	ret
-ENDPROC(efi_call6)
+ENDPROC(native_efi_call6)
diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c
new file mode 100644
index 0000000..f09f829
--- /dev/null
+++ b/arch/x86/xen/efi.c
@@ -0,0 +1,40 @@
+
+#include <asm/xen/hypercall.h>
+
+#include <xen/xen.h>
+#include <linux/efi.h>
+#include <asm/efi.h>
+#include <xen/interface/physdev.h>
+#include "xen-ops.h"
+
+efi_runtime_services_t xen_efi_fnc = {
+    .get_time = xen_get_time;
+    .set_time = xen_set_time;
+    /* and so on */
+};
+u64 xen_efi_call0(void *fp)
+{
+    char *func_idx;
+    u64 (*fnc)(void);
+
+    /*
+     * Look up which of the functions it is in the platform
+     * code, and find the same function in the Xen platform.
+     */
+    func_idx = &efi.systab->runtime - fp;
+    BUG_ON(func_idx == 0); /* Can't be the header! */
+    fnc = (char *)xen_efi_fnc + func_idx; /* This probably won't compile. */
+
+    BUG_ON(!fnc);
+    fnc();
+}
+/* and so on.
+u64 xen_efi_call1(void *fp, u64 arg1);
+u64 xen_efi_call2(void *fp, u64 arg1, u64 arg2);
+u64 xen_efi_call3(void *fp, u64 arg1, u64 arg2, u64 arg3);
+u64 xen_efi_call4(void *fp, u64 arg1, u64 arg2, u64 arg3, u64 arg4);
+u64 xen_efi_call5(void *fp, u64 arg1, u64 arg2, u64 arg3,
+      u64 arg4, u64 arg5);
+u64 xen_efi_call6(void *fp, u64 arg1, u64 arg2, u64 arg3,
+		     u64 arg4, u64 arg5, u64 arg6);
+*/
diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index 056d11f..cc820d2 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -563,4 +563,13 @@ void __init xen_arch_setup(void)
 #ifdef CONFIG_NUMA
 	numa_off = 1;
 #endif
+#ifdef CONFIG_EFI
+	platform_efi_call0 = xen_efi_call0;
+	platform_efi_call1 = xen_efi_call1;
+	platform_efi_call2 = xen_efi_call2;
+	platform_efi_call3 = xen_efi_call3;
+	platform_efi_call4 = xen_efi_call4;
+	platform_efi_call5 = xen_efi_call5;
+	platform_efi_call6 = xen_efi_call6;
+#endif
 }
diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
index 86782c5..d680558 100644
--- a/arch/x86/xen/xen-ops.h
+++ b/arch/x86/xen/xen-ops.h
@@ -123,4 +123,17 @@ void xen_adjust_exception_frame(void);
 
 extern int xen_panic_handler_init(void);
 
+#ifdef CONFIG_EFI
+extern u64 xen_efi_call0(void *fp);
+extern u64 xen_efi_call1(void *fp, u64 arg1);
+extern u64 xen_efi_call2(void *fp, u64 arg1, u64 arg2);
+extern u64 xen_efi_call3(void *fp, u64 arg1, u64 arg2, u64 arg3);
+extern u64 xen_efi_call4(void *fp, u64 arg1, u64 arg2, u64 arg3, u64 arg4);
+extern u64 xen_efi_call5(void *fp, u64 arg1, u64 arg2, u64 arg3,
+                    u64 arg4, u64 arg5);
+extern u64 xen_efi_call6(void *fp, u64 arg1, u64 arg2, u64 arg3,
+                    u64 arg4, u64 arg5, u64 arg6);
+
+
+#endif
 #endif /* XEN_OPS_H */
-- 
1.7.11.7