[PATCH v2] x86/EFI: meet further spec requirements for runtime calls

[PATCH v2] x86/EFI: meet further spec requirements for runtime calls

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 12527 bytes --]

So far we didn't guarantee 16-byte alignment of the stack: While (so
far) we don't tell the compiler to use smaller alignment, we also don't
guarantee 16-byte alignment when establishing stack pointers for new
vCPU-s. Runtime service functions using SSE instructions may end with
#GP(0) without that.

Note that making use of -mpreferred-stack-boundary=3, as mentioned in
the comment, wouldn't help to reduce the needed alignment: The compiler
would then be free to align the stack of the function with the aligned
object, but would be permitted to place an odd number of 8-byte objects
there, resulting in the callee to still run on an unaligned stack.

(The only working alternative to the approach chosen here would be to
use -mincoming-stack-boundary=3, but that would affect all functions in
runtime.c, not just the ones actually making runtime services calls.
And it would still require the manual alignment logic here to be used
with gcc 5.2 and earlier - not permitting that command line option -,
just that then the alignment amount would become conditional.)

Hence enforce the needed alignment by making efi_rs_enter() return a
suitably aligned structure, which the caller then necessarily has to
store in a suitably aligned local variable, the address of which then
gets passed to efi_rs_leave(). Also (to limit exposure) move the
function declarations to where they belong: They're local to runtime.c,
and shared only with compat.c (by the latter including the former).

Furthermore we should avoid #MF to be raised on the FLDCW we do.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Almost complete re-work.

--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -8,6 +8,25 @@
 
 DEFINE_XEN_GUEST_HANDLE(CHAR16);
 
+struct efi_rs_state {
+#ifdef CONFIG_X86
+ /*
+  * The way stacks get set up leads to them always being on an 8-byte
+  * boundary not evenly divisible by 16 (see asm-x86/current.h). The EFI ABI,
+  * just like the CPU one, however requires stacks to be 16-byte aligned
+  * before every function call. Since the compiler assumes this (unless
+  * passing it -mpreferred-stack-boundary=3), it wouldn't generate code to
+  * align the stack to 16 bytes even if putting a 16-byte aligned object
+  * there. Hence we need to force larger than 16-byte alignment, even if we
+  * don't strictly need that.
+  */
+ unsigned long __aligned(32) cr3;
+#endif
+};
+
+struct efi_rs_state efi_rs_enter(void);
+void efi_rs_leave(struct efi_rs_state *);
+
 #ifndef COMPAT
 
 /*
@@ -54,17 +73,19 @@ struct efi __read_mostly efi = {
 const struct efi_pci_rom *__read_mostly efi_pci_roms;
 
 #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
-unsigned long efi_rs_enter(void)
+
+struct efi_rs_state efi_rs_enter(void)
 {
     static const u16 fcw = FCW_DEFAULT;
     static const u32 mxcsr = MXCSR_DEFAULT;
-    unsigned long cr3 = read_cr3();
+    struct efi_rs_state state = { .cr3 = 0 };
 
     if ( !efi_l4_pgtable )
-        return 0;
+        return state;
 
+    state.cr3 = read_cr3();
     save_fpu_enable();
-    asm volatile ( "fldcw %0" :: "m" (fcw) );
+    asm volatile ( "fnclex; fldcw %0" :: "m" (fcw) );
     asm volatile ( "ldmxcsr %0" :: "m" (mxcsr) );
 
     spin_lock(&efi_rs_lock);
@@ -87,14 +108,14 @@ unsigned long efi_rs_enter(void)
 
     write_cr3(virt_to_maddr(efi_l4_pgtable));
 
-    return cr3;
+    return state;
 }
 
-void efi_rs_leave(unsigned long cr3)
+void efi_rs_leave(struct efi_rs_state *state)
 {
-    if ( !cr3 )
+    if ( !state->cr3 )
         return;
-    write_cr3(cr3);
+    write_cr3(state->cr3);
     if ( is_pv_vcpu(current) && !is_idle_vcpu(current) )
     {
         struct desc_ptr gdt_desc = {
@@ -121,14 +142,15 @@ unsigned long efi_get_time(void)
 {
     EFI_TIME time;
     EFI_STATUS status;
-    unsigned long cr3 = efi_rs_enter(), flags;
+    struct efi_rs_state state = efi_rs_enter();
+    unsigned long flags;
 
-    if ( !cr3 )
+    if ( !state.cr3 )
         return 0;
     spin_lock_irqsave(&rtc_lock, flags);
     status = efi_rs->GetTime(&time, NULL);
     spin_unlock_irqrestore(&rtc_lock, flags);
-    efi_rs_leave(cr3);
+    efi_rs_leave(&state);
 
     if ( EFI_ERROR(status) )
         return 0;
@@ -140,12 +162,12 @@ unsigned long efi_get_time(void)
 void efi_halt_system(void)
 {
     EFI_STATUS status;
-    unsigned long cr3 = efi_rs_enter();
+    struct efi_rs_state state = efi_rs_enter();
 
-    if ( !cr3 )
+    if ( !state.cr3 )
         return;
     status = efi_rs->ResetSystem(EfiResetShutdown, EFI_SUCCESS, 0, NULL);
-    efi_rs_leave(cr3);
+    efi_rs_leave(&state);
 
     printk(XENLOG_WARNING "EFI: could not halt system (%#lx)\n", status);
 }
@@ -153,13 +175,13 @@ void efi_halt_system(void)
 void efi_reset_system(bool_t warm)
 {
     EFI_STATUS status;
-    unsigned long cr3 = efi_rs_enter();
+    struct efi_rs_state state = efi_rs_enter();
 
-    if ( !cr3 )
+    if ( !state.cr3 )
         return;
     status = efi_rs->ResetSystem(warm ? EfiResetWarm : EfiResetCold,
                                  EFI_SUCCESS, 0, NULL);
-    efi_rs_leave(cr3);
+    efi_rs_leave(&state);
 
     printk(XENLOG_WARNING "EFI: could not reset system (%#lx)\n", status);
 }
@@ -179,12 +201,12 @@ int efi_get_info(uint32_t idx, union xen
         break;
     case XEN_FW_EFI_RT_VERSION:
     {
-        unsigned long cr3 = efi_rs_enter();
+        struct efi_rs_state state = efi_rs_enter();
 
-        if ( !cr3 )
+        if ( !state.cr3 )
             return -EOPNOTSUPP;
         info->version = efi_rs->Hdr.Revision;
-        efi_rs_leave(cr3);
+        efi_rs_leave(&state);
         break;
     }
     case XEN_FW_EFI_CONFIG_TABLE:
@@ -302,7 +324,8 @@ static inline EFI_GUID *cast_guid(struct
 
 int efi_runtime_call(struct xenpf_efi_runtime_call *op)
 {
-    unsigned long cr3, flags;
+    struct efi_rs_state state;
+    unsigned long flags;
     EFI_STATUS status = EFI_NOT_STARTED;
     int rc = 0;
 
@@ -315,13 +338,13 @@ int efi_runtime_call(struct xenpf_efi_ru
         if ( op->misc )
             return -EINVAL;
 
-        cr3 = efi_rs_enter();
-        if ( !cr3 )
+        state = efi_rs_enter();
+        if ( !state.cr3 )
             return -EOPNOTSUPP;
         spin_lock_irqsave(&rtc_lock, flags);
         status = efi_rs->GetTime(cast_time(&op->u.get_time.time), &caps);
         spin_unlock_irqrestore(&rtc_lock, flags);
-        efi_rs_leave(cr3);
+        efi_rs_leave(&state);
 
         if ( !EFI_ERROR(status) )
         {
@@ -337,13 +360,13 @@ int efi_runtime_call(struct xenpf_efi_ru
         if ( op->misc )
             return -EINVAL;
 
-        cr3 = efi_rs_enter();
-        if ( !cr3 )
+        state = efi_rs_enter();
+        if ( !state.cr3 )
             return -EOPNOTSUPP;
         spin_lock_irqsave(&rtc_lock, flags);
         status = efi_rs->SetTime(cast_time(&op->u.set_time));
         spin_unlock_irqrestore(&rtc_lock, flags);
-        efi_rs_leave(cr3);
+        efi_rs_leave(&state);
         break;
 
     case XEN_EFI_get_wakeup_time:
@@ -353,14 +376,14 @@ int efi_runtime_call(struct xenpf_efi_ru
         if ( op->misc )
             return -EINVAL;
 
-        cr3 = efi_rs_enter();
-        if ( !cr3 )
+        state = efi_rs_enter();
+        if ( !state.cr3 )
             return -EOPNOTSUPP;
         spin_lock_irqsave(&rtc_lock, flags);
         status = efi_rs->GetWakeupTime(&enabled, &pending,
                                        cast_time(&op->u.get_wakeup_time));
         spin_unlock_irqrestore(&rtc_lock, flags);
-        efi_rs_leave(cr3);
+        efi_rs_leave(&state);
 
         if ( !EFI_ERROR(status) )
         {
@@ -377,8 +400,8 @@ int efi_runtime_call(struct xenpf_efi_ru
                           XEN_EFI_SET_WAKEUP_TIME_ENABLE_ONLY) )
             return -EINVAL;
 
-        cr3 = efi_rs_enter();
-        if ( !cr3 )
+        state = efi_rs_enter();
+        if ( !state.cr3 )
             return -EOPNOTSUPP;
         spin_lock_irqsave(&rtc_lock, flags);
         status = efi_rs->SetWakeupTime(!!(op->misc &
@@ -388,7 +411,7 @@ int efi_runtime_call(struct xenpf_efi_ru
                                        NULL :
                                        cast_time(&op->u.set_wakeup_time));
         spin_unlock_irqrestore(&rtc_lock, flags);
-        efi_rs_leave(cr3);
+        efi_rs_leave(&state);
 
         op->misc = 0;
         break;
@@ -397,12 +420,12 @@ int efi_runtime_call(struct xenpf_efi_ru
         if ( op->misc )
             return -EINVAL;
 
-        cr3 = efi_rs_enter();
-        if ( cr3 )
+        state = efi_rs_enter();
+        if ( state.cr3 )
             status = efi_rs->GetNextHighMonotonicCount(&op->misc);
         else
             rc = -EOPNOTSUPP;
-        efi_rs_leave(cr3);
+        efi_rs_leave(&state);
         break;
 
     case XEN_EFI_get_variable:
@@ -436,13 +459,13 @@ int efi_runtime_call(struct xenpf_efi_ru
         else
             data = NULL;
 
-        cr3 = efi_rs_enter();
-        if ( cr3 )
+        state = efi_rs_enter();
+        if ( state.cr3 )
         {
             status = efi_rs->GetVariable(
                 name, cast_guid(&op->u.get_variable.vendor_guid),
                 &op->misc, &size, data);
-            efi_rs_leave(cr3);
+            efi_rs_leave(&state);
 
             if ( !EFI_ERROR(status) &&
                  copy_to_guest(op->u.get_variable.data, data, size) )
@@ -479,14 +502,14 @@ int efi_runtime_call(struct xenpf_efi_ru
             rc = -EFAULT;
         else
         {
-            cr3 = efi_rs_enter();
-            if ( cr3 )
+            state = efi_rs_enter();
+            if ( state.cr3 )
                 status = efi_rs->SetVariable(
                     name, cast_guid(&op->u.set_variable.vendor_guid),
                     op->misc, op->u.set_variable.size, data);
             else
                 rc = -EOPNOTSUPP;
-            efi_rs_leave(cr3);
+            efi_rs_leave(&state);
         }
 
         xfree(data);
@@ -516,13 +539,13 @@ int efi_runtime_call(struct xenpf_efi_ru
             return -EFAULT;
         }
 
-        cr3 = efi_rs_enter();
-        if ( cr3 )
+        state = efi_rs_enter();
+        if ( state.cr3 )
         {
             status = efi_rs->GetNextVariableName(
                 &size, name.str,
                 cast_guid(&op->u.get_next_variable_name.vendor_guid));
-            efi_rs_leave(cr3);
+            efi_rs_leave(&state);
 
             /*
              * Copy the variable name if necessary. The caller provided size
@@ -571,10 +594,10 @@ int efi_runtime_call(struct xenpf_efi_ru
             break;
         }
 
-        cr3 = efi_rs_enter();
-        if ( !cr3 || (efi_rs->Hdr.Revision >> 16) < 2 )
+        state = efi_rs_enter();
+        if ( !state.cr3 || (efi_rs->Hdr.Revision >> 16) < 2 )
         {
-            efi_rs_leave(cr3);
+            efi_rs_leave(&state);
             return -EOPNOTSUPP;
         }
         status = efi_rs->QueryVariableInfo(
@@ -582,7 +605,7 @@ int efi_runtime_call(struct xenpf_efi_ru
             &op->u.query_variable_info.max_store_size,
             &op->u.query_variable_info.remain_store_size,
             &op->u.query_variable_info.max_size);
-        efi_rs_leave(cr3);
+        efi_rs_leave(&state);
         break;
 
     case XEN_EFI_query_capsule_capabilities:
@@ -590,13 +613,13 @@ int efi_runtime_call(struct xenpf_efi_ru
         if ( op->misc )
             return -EINVAL;
 
-        cr3 = efi_rs_enter();
-        if ( !cr3 || (efi_rs->Hdr.Revision >> 16) < 2 )
+        state = efi_rs_enter();
+        if ( !state.cr3 || (efi_rs->Hdr.Revision >> 16) < 2 )
         {
-            efi_rs_leave(cr3);
+            efi_rs_leave(&state);
             return -EOPNOTSUPP;
         }
-        efi_rs_leave(cr3);
+        efi_rs_leave(&state);
         /* XXX fall through for now */
     default:
         return -ENOSYS;
--- a/xen/common/efi/efi.h
+++ b/xen/common/efi/efi.h
@@ -36,6 +36,3 @@ extern const struct efi_pci_rom *efi_pci
 
 extern UINT64 efi_boot_max_var_store_size, efi_boot_remain_var_store_size,
               efi_boot_max_var_size;
-
-unsigned long efi_rs_enter(void);
-void efi_rs_leave(unsigned long);



[-- Attachment #2: x86-EFI-rs-state.patch --]
[-- Type: text/plain, Size: 12584 bytes --]

x86/EFI: meet further spec requirements for runtime calls

So far we didn't guarantee 16-byte alignment of the stack: While (so
far) we don't tell the compiler to use smaller alignment, we also don't
guarantee 16-byte alignment when establishing stack pointers for new
vCPU-s. Runtime service functions using SSE instructions may end with
#GP(0) without that.

Note that making use of -mpreferred-stack-boundary=3, as mentioned in
the comment, wouldn't help to reduce the needed alignment: The compiler
would then be free to align the stack of the function with the aligned
object, but would be permitted to place an odd number of 8-byte objects
there, resulting in the callee to still run on an unaligned stack.

(The only working alternative to the approach chosen here would be to
use -mincoming-stack-boundary=3, but that would affect all functions in
runtime.c, not just the ones actually making runtime services calls.
And it would still require the manual alignment logic here to be used
with gcc 5.2 and earlier - not permitting that command line option -,
just that then the alignment amount would become conditional.)

Hence enforce the needed alignment by making efi_rs_enter() return a
suitably aligned structure, which the caller then necessarily has to
store in a suitably aligned local variable, the address of which then
gets passed to efi_rs_leave(). Also (to limit exposure) move the
function declarations to where they belong: They're local to runtime.c,
and shared only with compat.c (by the latter including the former).

Furthermore we should avoid #MF to be raised on the FLDCW we do.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Almost complete re-work.

--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -8,6 +8,25 @@
 
 DEFINE_XEN_GUEST_HANDLE(CHAR16);
 
+struct efi_rs_state {
+#ifdef CONFIG_X86
+ /*
+  * The way stacks get set up leads to them always being on an 8-byte
+  * boundary not evenly divisible by 16 (see asm-x86/current.h). The EFI ABI,
+  * just like the CPU one, however requires stacks to be 16-byte aligned
+  * before every function call. Since the compiler assumes this (unless
+  * passing it -mpreferred-stack-boundary=3), it wouldn't generate code to
+  * align the stack to 16 bytes even if putting a 16-byte aligned object
+  * there. Hence we need to force larger than 16-byte alignment, even if we
+  * don't strictly need that.
+  */
+ unsigned long __aligned(32) cr3;
+#endif
+};
+
+struct efi_rs_state efi_rs_enter(void);
+void efi_rs_leave(struct efi_rs_state *);
+
 #ifndef COMPAT
 
 /*
@@ -54,17 +73,19 @@ struct efi __read_mostly efi = {
 const struct efi_pci_rom *__read_mostly efi_pci_roms;
 
 #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
-unsigned long efi_rs_enter(void)
+
+struct efi_rs_state efi_rs_enter(void)
 {
     static const u16 fcw = FCW_DEFAULT;
     static const u32 mxcsr = MXCSR_DEFAULT;
-    unsigned long cr3 = read_cr3();
+    struct efi_rs_state state = { .cr3 = 0 };
 
     if ( !efi_l4_pgtable )
-        return 0;
+        return state;
 
+    state.cr3 = read_cr3();
     save_fpu_enable();
-    asm volatile ( "fldcw %0" :: "m" (fcw) );
+    asm volatile ( "fnclex; fldcw %0" :: "m" (fcw) );
     asm volatile ( "ldmxcsr %0" :: "m" (mxcsr) );
 
     spin_lock(&efi_rs_lock);
@@ -87,14 +108,14 @@ unsigned long efi_rs_enter(void)
 
     write_cr3(virt_to_maddr(efi_l4_pgtable));
 
-    return cr3;
+    return state;
 }
 
-void efi_rs_leave(unsigned long cr3)
+void efi_rs_leave(struct efi_rs_state *state)
 {
-    if ( !cr3 )
+    if ( !state->cr3 )
         return;
-    write_cr3(cr3);
+    write_cr3(state->cr3);
     if ( is_pv_vcpu(current) && !is_idle_vcpu(current) )
     {
         struct desc_ptr gdt_desc = {
@@ -121,14 +142,15 @@ unsigned long efi_get_time(void)
 {
     EFI_TIME time;
     EFI_STATUS status;
-    unsigned long cr3 = efi_rs_enter(), flags;
+    struct efi_rs_state state = efi_rs_enter();
+    unsigned long flags;
 
-    if ( !cr3 )
+    if ( !state.cr3 )
         return 0;
     spin_lock_irqsave(&rtc_lock, flags);
     status = efi_rs->GetTime(&time, NULL);
     spin_unlock_irqrestore(&rtc_lock, flags);
-    efi_rs_leave(cr3);
+    efi_rs_leave(&state);
 
     if ( EFI_ERROR(status) )
         return 0;
@@ -140,12 +162,12 @@ unsigned long efi_get_time(void)
 void efi_halt_system(void)
 {
     EFI_STATUS status;
-    unsigned long cr3 = efi_rs_enter();
+    struct efi_rs_state state = efi_rs_enter();
 
-    if ( !cr3 )
+    if ( !state.cr3 )
         return;
     status = efi_rs->ResetSystem(EfiResetShutdown, EFI_SUCCESS, 0, NULL);
-    efi_rs_leave(cr3);
+    efi_rs_leave(&state);
 
     printk(XENLOG_WARNING "EFI: could not halt system (%#lx)\n", status);
 }
@@ -153,13 +175,13 @@ void efi_halt_system(void)
 void efi_reset_system(bool_t warm)
 {
     EFI_STATUS status;
-    unsigned long cr3 = efi_rs_enter();
+    struct efi_rs_state state = efi_rs_enter();
 
-    if ( !cr3 )
+    if ( !state.cr3 )
         return;
     status = efi_rs->ResetSystem(warm ? EfiResetWarm : EfiResetCold,
                                  EFI_SUCCESS, 0, NULL);
-    efi_rs_leave(cr3);
+    efi_rs_leave(&state);
 
     printk(XENLOG_WARNING "EFI: could not reset system (%#lx)\n", status);
 }
@@ -179,12 +201,12 @@ int efi_get_info(uint32_t idx, union xen
         break;
     case XEN_FW_EFI_RT_VERSION:
     {
-        unsigned long cr3 = efi_rs_enter();
+        struct efi_rs_state state = efi_rs_enter();
 
-        if ( !cr3 )
+        if ( !state.cr3 )
             return -EOPNOTSUPP;
         info->version = efi_rs->Hdr.Revision;
-        efi_rs_leave(cr3);
+        efi_rs_leave(&state);
         break;
     }
     case XEN_FW_EFI_CONFIG_TABLE:
@@ -302,7 +324,8 @@ static inline EFI_GUID *cast_guid(struct
 
 int efi_runtime_call(struct xenpf_efi_runtime_call *op)
 {
-    unsigned long cr3, flags;
+    struct efi_rs_state state;
+    unsigned long flags;
     EFI_STATUS status = EFI_NOT_STARTED;
     int rc = 0;
 
@@ -315,13 +338,13 @@ int efi_runtime_call(struct xenpf_efi_ru
         if ( op->misc )
             return -EINVAL;
 
-        cr3 = efi_rs_enter();
-        if ( !cr3 )
+        state = efi_rs_enter();
+        if ( !state.cr3 )
             return -EOPNOTSUPP;
         spin_lock_irqsave(&rtc_lock, flags);
         status = efi_rs->GetTime(cast_time(&op->u.get_time.time), &caps);
         spin_unlock_irqrestore(&rtc_lock, flags);
-        efi_rs_leave(cr3);
+        efi_rs_leave(&state);
 
         if ( !EFI_ERROR(status) )
         {
@@ -337,13 +360,13 @@ int efi_runtime_call(struct xenpf_efi_ru
         if ( op->misc )
             return -EINVAL;
 
-        cr3 = efi_rs_enter();
-        if ( !cr3 )
+        state = efi_rs_enter();
+        if ( !state.cr3 )
             return -EOPNOTSUPP;
         spin_lock_irqsave(&rtc_lock, flags);
         status = efi_rs->SetTime(cast_time(&op->u.set_time));
         spin_unlock_irqrestore(&rtc_lock, flags);
-        efi_rs_leave(cr3);
+        efi_rs_leave(&state);
         break;
 
     case XEN_EFI_get_wakeup_time:
@@ -353,14 +376,14 @@ int efi_runtime_call(struct xenpf_efi_ru
         if ( op->misc )
             return -EINVAL;
 
-        cr3 = efi_rs_enter();
-        if ( !cr3 )
+        state = efi_rs_enter();
+        if ( !state.cr3 )
             return -EOPNOTSUPP;
         spin_lock_irqsave(&rtc_lock, flags);
         status = efi_rs->GetWakeupTime(&enabled, &pending,
                                        cast_time(&op->u.get_wakeup_time));
         spin_unlock_irqrestore(&rtc_lock, flags);
-        efi_rs_leave(cr3);
+        efi_rs_leave(&state);
 
         if ( !EFI_ERROR(status) )
         {
@@ -377,8 +400,8 @@ int efi_runtime_call(struct xenpf_efi_ru
                           XEN_EFI_SET_WAKEUP_TIME_ENABLE_ONLY) )
             return -EINVAL;
 
-        cr3 = efi_rs_enter();
-        if ( !cr3 )
+        state = efi_rs_enter();
+        if ( !state.cr3 )
             return -EOPNOTSUPP;
         spin_lock_irqsave(&rtc_lock, flags);
         status = efi_rs->SetWakeupTime(!!(op->misc &
@@ -388,7 +411,7 @@ int efi_runtime_call(struct xenpf_efi_ru
                                        NULL :
                                        cast_time(&op->u.set_wakeup_time));
         spin_unlock_irqrestore(&rtc_lock, flags);
-        efi_rs_leave(cr3);
+        efi_rs_leave(&state);
 
         op->misc = 0;
         break;
@@ -397,12 +420,12 @@ int efi_runtime_call(struct xenpf_efi_ru
         if ( op->misc )
             return -EINVAL;
 
-        cr3 = efi_rs_enter();
-        if ( cr3 )
+        state = efi_rs_enter();
+        if ( state.cr3 )
             status = efi_rs->GetNextHighMonotonicCount(&op->misc);
         else
             rc = -EOPNOTSUPP;
-        efi_rs_leave(cr3);
+        efi_rs_leave(&state);
         break;
 
     case XEN_EFI_get_variable:
@@ -436,13 +459,13 @@ int efi_runtime_call(struct xenpf_efi_ru
         else
             data = NULL;
 
-        cr3 = efi_rs_enter();
-        if ( cr3 )
+        state = efi_rs_enter();
+        if ( state.cr3 )
         {
             status = efi_rs->GetVariable(
                 name, cast_guid(&op->u.get_variable.vendor_guid),
                 &op->misc, &size, data);
-            efi_rs_leave(cr3);
+            efi_rs_leave(&state);
 
             if ( !EFI_ERROR(status) &&
                  copy_to_guest(op->u.get_variable.data, data, size) )
@@ -479,14 +502,14 @@ int efi_runtime_call(struct xenpf_efi_ru
             rc = -EFAULT;
         else
         {
-            cr3 = efi_rs_enter();
-            if ( cr3 )
+            state = efi_rs_enter();
+            if ( state.cr3 )
                 status = efi_rs->SetVariable(
                     name, cast_guid(&op->u.set_variable.vendor_guid),
                     op->misc, op->u.set_variable.size, data);
             else
                 rc = -EOPNOTSUPP;
-            efi_rs_leave(cr3);
+            efi_rs_leave(&state);
         }
 
         xfree(data);
@@ -516,13 +539,13 @@ int efi_runtime_call(struct xenpf_efi_ru
             return -EFAULT;
         }
 
-        cr3 = efi_rs_enter();
-        if ( cr3 )
+        state = efi_rs_enter();
+        if ( state.cr3 )
         {
             status = efi_rs->GetNextVariableName(
                 &size, name.str,
                 cast_guid(&op->u.get_next_variable_name.vendor_guid));
-            efi_rs_leave(cr3);
+            efi_rs_leave(&state);
 
             /*
              * Copy the variable name if necessary. The caller provided size
@@ -571,10 +594,10 @@ int efi_runtime_call(struct xenpf_efi_ru
             break;
         }
 
-        cr3 = efi_rs_enter();
-        if ( !cr3 || (efi_rs->Hdr.Revision >> 16) < 2 )
+        state = efi_rs_enter();
+        if ( !state.cr3 || (efi_rs->Hdr.Revision >> 16) < 2 )
         {
-            efi_rs_leave(cr3);
+            efi_rs_leave(&state);
             return -EOPNOTSUPP;
         }
         status = efi_rs->QueryVariableInfo(
@@ -582,7 +605,7 @@ int efi_runtime_call(struct xenpf_efi_ru
             &op->u.query_variable_info.max_store_size,
             &op->u.query_variable_info.remain_store_size,
             &op->u.query_variable_info.max_size);
-        efi_rs_leave(cr3);
+        efi_rs_leave(&state);
         break;
 
     case XEN_EFI_query_capsule_capabilities:
@@ -590,13 +613,13 @@ int efi_runtime_call(struct xenpf_efi_ru
         if ( op->misc )
             return -EINVAL;
 
-        cr3 = efi_rs_enter();
-        if ( !cr3 || (efi_rs->Hdr.Revision >> 16) < 2 )
+        state = efi_rs_enter();
+        if ( !state.cr3 || (efi_rs->Hdr.Revision >> 16) < 2 )
         {
-            efi_rs_leave(cr3);
+            efi_rs_leave(&state);
             return -EOPNOTSUPP;
         }
-        efi_rs_leave(cr3);
+        efi_rs_leave(&state);
         /* XXX fall through for now */
     default:
         return -ENOSYS;
--- a/xen/common/efi/efi.h
+++ b/xen/common/efi/efi.h
@@ -36,6 +36,3 @@ extern const struct efi_pci_rom *efi_pci
 
 extern UINT64 efi_boot_max_var_store_size, efi_boot_remain_var_store_size,
               efi_boot_max_var_size;
-
-unsigned long efi_rs_enter(void);
-void efi_rs_leave(unsigned long);

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2] x86/EFI: meet further spec requirements for runtime calls

[Andrew Cooper]

On 14/11/16 10:32, Jan Beulich wrote:
> So far we didn't guarantee 16-byte alignment of the stack: While (so
> far) we don't tell the compiler to use smaller alignment, we also don't
> guarantee 16-byte alignment when establishing stack pointers for new
> vCPU-s. Runtime service functions using SSE instructions may end with
> #GP(0) without that.
>
> Note that making use of -mpreferred-stack-boundary=3, as mentioned in
> the comment, wouldn't help to reduce the needed alignment: The compiler
> would then be free to align the stack of the function with the aligned
> object, but would be permitted to place an odd number of 8-byte objects
> there, resulting in the callee to still run on an unaligned stack.
>
> (The only working alternative to the approach chosen here would be to
> use -mincoming-stack-boundary=3, but that would affect all functions in
> runtime.c, not just the ones actually making runtime services calls.
> And it would still require the manual alignment logic here to be used
> with gcc 5.2 and earlier - not permitting that command line option -,
> just that then the alignment amount would become conditional.)
>
> Hence enforce the needed alignment by making efi_rs_enter() return a
> suitably aligned structure, which the caller then necessarily has to
> store in a suitably aligned local variable, the address of which then
> gets passed to efi_rs_leave(). Also (to limit exposure) move the
> function declarations to where they belong: They're local to runtime.c,
> and shared only with compat.c (by the latter including the former).

Why does this guarantee alignment?  What prevents the compiler from
reordering the items in its stack layout?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2] x86/EFI: meet further spec requirements for runtime calls

[Jan Beulich]

>>> On 15.11.16 at 16:47, <andrew.cooper3@citrix.com> wrote:
> On 14/11/16 10:32, Jan Beulich wrote:
>> So far we didn't guarantee 16-byte alignment of the stack: While (so
>> far) we don't tell the compiler to use smaller alignment, we also don't
>> guarantee 16-byte alignment when establishing stack pointers for new
>> vCPU-s. Runtime service functions using SSE instructions may end with
>> #GP(0) without that.
>>
>> Note that making use of -mpreferred-stack-boundary=3, as mentioned in
>> the comment, wouldn't help to reduce the needed alignment: The compiler
>> would then be free to align the stack of the function with the aligned
>> object, but would be permitted to place an odd number of 8-byte objects
>> there, resulting in the callee to still run on an unaligned stack.
>>
>> (The only working alternative to the approach chosen here would be to
>> use -mincoming-stack-boundary=3, but that would affect all functions in
>> runtime.c, not just the ones actually making runtime services calls.
>> And it would still require the manual alignment logic here to be used
>> with gcc 5.2 and earlier - not permitting that command line option -,
>> just that then the alignment amount would become conditional.)
>>
>> Hence enforce the needed alignment by making efi_rs_enter() return a
>> suitably aligned structure, which the caller then necessarily has to
>> store in a suitably aligned local variable, the address of which then
>> gets passed to efi_rs_leave(). Also (to limit exposure) move the
>> function declarations to where they belong: They're local to runtime.c,
>> and shared only with compat.c (by the latter including the former).
> 
> Why does this guarantee alignment?  What prevents the compiler from
> reordering the items in its stack layout?

The compiler will always allocate stack variables such that called
functions will see an ABI-compliant stack. Without variables of
bigger alignment, it does this by implying that the current function
also has an aligned stack. Since we start out with a stack frame on
an 8 mod 16 boundary, said compiler behavior propagates
this through all call hierarchies. With variables of bigger alignment
the compiler arranges for the current frame to be suitably
expanded, and it will of course continue to guarantee that all
callees get to see a 16-byte aligned stack. IOW all we need to do
is break this 8 mod 16 thing once.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Ping: [PATCH v2] x86/EFI: meet further spec requirements for runtime calls

[Jan Beulich]

>>> On 15.11.16 at 17:06, <JBeulich@suse.com> wrote:
>>>> On 15.11.16 at 16:47, <andrew.cooper3@citrix.com> wrote:
>> On 14/11/16 10:32, Jan Beulich wrote:
>>> So far we didn't guarantee 16-byte alignment of the stack: While (so
>>> far) we don't tell the compiler to use smaller alignment, we also don't
>>> guarantee 16-byte alignment when establishing stack pointers for new
>>> vCPU-s. Runtime service functions using SSE instructions may end with
>>> #GP(0) without that.
>>>
>>> Note that making use of -mpreferred-stack-boundary=3, as mentioned in
>>> the comment, wouldn't help to reduce the needed alignment: The compiler
>>> would then be free to align the stack of the function with the aligned
>>> object, but would be permitted to place an odd number of 8-byte objects
>>> there, resulting in the callee to still run on an unaligned stack.
>>>
>>> (The only working alternative to the approach chosen here would be to
>>> use -mincoming-stack-boundary=3, but that would affect all functions in
>>> runtime.c, not just the ones actually making runtime services calls.
>>> And it would still require the manual alignment logic here to be used
>>> with gcc 5.2 and earlier - not permitting that command line option -,
>>> just that then the alignment amount would become conditional.)
>>>
>>> Hence enforce the needed alignment by making efi_rs_enter() return a
>>> suitably aligned structure, which the caller then necessarily has to
>>> store in a suitably aligned local variable, the address of which then
>>> gets passed to efi_rs_leave(). Also (to limit exposure) move the
>>> function declarations to where they belong: They're local to runtime.c,
>>> and shared only with compat.c (by the latter including the former).
>> 
>> Why does this guarantee alignment?  What prevents the compiler from
>> reordering the items in its stack layout?
> 
> The compiler will always allocate stack variables such that called
> functions will see an ABI-compliant stack. Without variables of
> bigger alignment, it does this by implying that the current function
> also has an aligned stack. Since we start out with a stack frame on
> an 8 mod 16 boundary, said compiler behavior propagates
> this through all call hierarchies. With variables of bigger alignment
> the compiler arranges for the current frame to be suitably
> expanded, and it will of course continue to guarantee that all
> callees get to see a 16-byte aligned stack. IOW all we need to do
> is break this 8 mod 16 thing once.

Ping? We have a problem to solve here, so I think I can expect that
either the proposed solution (even if not covering all theoretical
cases of possibly compiler behavior) is accepted, or an alternative
proposal is put up. I'd really like to avoid seeing 4.8 go out with the
problem un-addressed. (From a strictly formal perspective, me being
the only maintainer of EFI code, I could put the patch in without any
ack [other than Wei's release one], but I'd like to avoid that if at all
possible.)

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Ping: [PATCH v2] x86/EFI: meet further spec requirements for runtime calls

[Wei Liu]

On Tue, Nov 22, 2016 at 03:09:37AM -0700, Jan Beulich wrote:
> >>> On 15.11.16 at 17:06, <JBeulich@suse.com> wrote:
> >>>> On 15.11.16 at 16:47, <andrew.cooper3@citrix.com> wrote:
> >> On 14/11/16 10:32, Jan Beulich wrote:
> >>> So far we didn't guarantee 16-byte alignment of the stack: While (so
> >>> far) we don't tell the compiler to use smaller alignment, we also don't
> >>> guarantee 16-byte alignment when establishing stack pointers for new
> >>> vCPU-s. Runtime service functions using SSE instructions may end with
> >>> #GP(0) without that.
> >>>
> >>> Note that making use of -mpreferred-stack-boundary=3, as mentioned in
> >>> the comment, wouldn't help to reduce the needed alignment: The compiler
> >>> would then be free to align the stack of the function with the aligned
> >>> object, but would be permitted to place an odd number of 8-byte objects
> >>> there, resulting in the callee to still run on an unaligned stack.
> >>>
> >>> (The only working alternative to the approach chosen here would be to
> >>> use -mincoming-stack-boundary=3, but that would affect all functions in
> >>> runtime.c, not just the ones actually making runtime services calls.
> >>> And it would still require the manual alignment logic here to be used
> >>> with gcc 5.2 and earlier - not permitting that command line option -,
> >>> just that then the alignment amount would become conditional.)
> >>>
> >>> Hence enforce the needed alignment by making efi_rs_enter() return a
> >>> suitably aligned structure, which the caller then necessarily has to
> >>> store in a suitably aligned local variable, the address of which then
> >>> gets passed to efi_rs_leave(). Also (to limit exposure) move the
> >>> function declarations to where they belong: They're local to runtime.c,
> >>> and shared only with compat.c (by the latter including the former).
> >> 
> >> Why does this guarantee alignment?  What prevents the compiler from
> >> reordering the items in its stack layout?
> > 
> > The compiler will always allocate stack variables such that called
> > functions will see an ABI-compliant stack. Without variables of
> > bigger alignment, it does this by implying that the current function
> > also has an aligned stack. Since we start out with a stack frame on
> > an 8 mod 16 boundary, said compiler behavior propagates
> > this through all call hierarchies. With variables of bigger alignment
> > the compiler arranges for the current frame to be suitably
> > expanded, and it will of course continue to guarantee that all
> > callees get to see a 16-byte aligned stack. IOW all we need to do
> > is break this 8 mod 16 thing once.
> 
> Ping? We have a problem to solve here, so I think I can expect that
> either the proposed solution (even if not covering all theoretical
> cases of possibly compiler behavior) is accepted, or an alternative
> proposal is put up. I'd really like to avoid seeing 4.8 go out with the
> problem un-addressed. (From a strictly formal perspective, me being
> the only maintainer of EFI code, I could put the patch in without any
> ack [other than Wei's release one], but I'd like to avoid that if at all
> possible.)
> 

I can't judge the technical correctness of this patch. I think it would
be good to fix this, so:

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Ping: [PATCH v2] x86/EFI: meet further spec requirements for runtime calls

[Andrew Cooper]

On 22/11/16 10:09, Jan Beulich wrote:
>>>> On 15.11.16 at 17:06, <JBeulich@suse.com> wrote:
>>>>> On 15.11.16 at 16:47, <andrew.cooper3@citrix.com> wrote:
>>> On 14/11/16 10:32, Jan Beulich wrote:
>>>> So far we didn't guarantee 16-byte alignment of the stack: While (so
>>>> far) we don't tell the compiler to use smaller alignment, we also don't
>>>> guarantee 16-byte alignment when establishing stack pointers for new
>>>> vCPU-s. Runtime service functions using SSE instructions may end with
>>>> #GP(0) without that.
>>>>
>>>> Note that making use of -mpreferred-stack-boundary=3, as mentioned in
>>>> the comment, wouldn't help to reduce the needed alignment: The compiler
>>>> would then be free to align the stack of the function with the aligned
>>>> object, but would be permitted to place an odd number of 8-byte objects
>>>> there, resulting in the callee to still run on an unaligned stack.
>>>>
>>>> (The only working alternative to the approach chosen here would be to
>>>> use -mincoming-stack-boundary=3, but that would affect all functions in
>>>> runtime.c, not just the ones actually making runtime services calls.
>>>> And it would still require the manual alignment logic here to be used
>>>> with gcc 5.2 and earlier - not permitting that command line option -,
>>>> just that then the alignment amount would become conditional.)
>>>>
>>>> Hence enforce the needed alignment by making efi_rs_enter() return a
>>>> suitably aligned structure, which the caller then necessarily has to
>>>> store in a suitably aligned local variable, the address of which then
>>>> gets passed to efi_rs_leave(). Also (to limit exposure) move the
>>>> function declarations to where they belong: They're local to runtime.c,
>>>> and shared only with compat.c (by the latter including the former).
>>> Why does this guarantee alignment?  What prevents the compiler from
>>> reordering the items in its stack layout?
>> The compiler will always allocate stack variables such that called
>> functions will see an ABI-compliant stack. Without variables of
>> bigger alignment, it does this by implying that the current function
>> also has an aligned stack. Since we start out with a stack frame on
>> an 8 mod 16 boundary, said compiler behavior propagates
>> this through all call hierarchies. With variables of bigger alignment
>> the compiler arranges for the current frame to be suitably
>> expanded, and it will of course continue to guarantee that all
>> callees get to see a 16-byte aligned stack. IOW all we need to do
>> is break this 8 mod 16 thing once.
> Ping? We have a problem to solve here, so I think I can expect that
> either the proposed solution (even if not covering all theoretical
> cases of possibly compiler behavior) is accepted, or an alternative
> proposal is put up.

Right, so the proposed patch is half a solution, in that it will resolve
the issue when a compiler make one of two choices.  From this point of
view, it is an improvement.

> I'd really like to avoid seeing 4.8 go out with the
> problem un-addressed.

However, calling the issue addressed after this patch is false.  A
compiler which chooses to reorder stack objects differently will still
fail to maintain the alignment you are trying to impose.

> (From a strictly formal perspective, me being
> the only maintainer of EFI code, I could put the patch in without any
> ack [other than Wei's release one], but I'd like to avoid that if at all
> possible.)

If you want an ack, then fine.  Acked-by: Andrew Cooper
<andrew.cooper3@citrix.com> but I don't expect this fully resolve the issue.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Ping: [PATCH v2] x86/EFI: meet further spec requirements for runtime calls

[Jan Beulich]

>>> On 22.11.16 at 11:41, <andrew.cooper3@citrix.com> wrote:
> On 22/11/16 10:09, Jan Beulich wrote:
>>>>> On 15.11.16 at 17:06, <JBeulich@suse.com> wrote:
>>>>>> On 15.11.16 at 16:47, <andrew.cooper3@citrix.com> wrote:
>>>> On 14/11/16 10:32, Jan Beulich wrote:
>>>>> So far we didn't guarantee 16-byte alignment of the stack: While (so
>>>>> far) we don't tell the compiler to use smaller alignment, we also don't
>>>>> guarantee 16-byte alignment when establishing stack pointers for new
>>>>> vCPU-s. Runtime service functions using SSE instructions may end with
>>>>> #GP(0) without that.
>>>>>
>>>>> Note that making use of -mpreferred-stack-boundary=3, as mentioned in
>>>>> the comment, wouldn't help to reduce the needed alignment: The compiler
>>>>> would then be free to align the stack of the function with the aligned
>>>>> object, but would be permitted to place an odd number of 8-byte objects
>>>>> there, resulting in the callee to still run on an unaligned stack.
>>>>>
>>>>> (The only working alternative to the approach chosen here would be to
>>>>> use -mincoming-stack-boundary=3, but that would affect all functions in
>>>>> runtime.c, not just the ones actually making runtime services calls.
>>>>> And it would still require the manual alignment logic here to be used
>>>>> with gcc 5.2 and earlier - not permitting that command line option -,
>>>>> just that then the alignment amount would become conditional.)
>>>>>
>>>>> Hence enforce the needed alignment by making efi_rs_enter() return a
>>>>> suitably aligned structure, which the caller then necessarily has to
>>>>> store in a suitably aligned local variable, the address of which then
>>>>> gets passed to efi_rs_leave(). Also (to limit exposure) move the
>>>>> function declarations to where they belong: They're local to runtime.c,
>>>>> and shared only with compat.c (by the latter including the former).
>>>> Why does this guarantee alignment?  What prevents the compiler from
>>>> reordering the items in its stack layout?
>>> The compiler will always allocate stack variables such that called
>>> functions will see an ABI-compliant stack. Without variables of
>>> bigger alignment, it does this by implying that the current function
>>> also has an aligned stack. Since we start out with a stack frame on
>>> an 8 mod 16 boundary, said compiler behavior propagates
>>> this through all call hierarchies. With variables of bigger alignment
>>> the compiler arranges for the current frame to be suitably
>>> expanded, and it will of course continue to guarantee that all
>>> callees get to see a 16-byte aligned stack. IOW all we need to do
>>> is break this 8 mod 16 thing once.
>> Ping? We have a problem to solve here, so I think I can expect that
>> either the proposed solution (even if not covering all theoretical
>> cases of possibly compiler behavior) is accepted, or an alternative
>> proposal is put up.
> 
> Right, so the proposed patch is half a solution, in that it will resolve
> the issue when a compiler make one of two choices.  From this point of
> view, it is an improvement.
> 
>> I'd really like to avoid seeing 4.8 go out with the
>> problem un-addressed.
> 
> However, calling the issue addressed after this patch is false.  A
> compiler which chooses to reorder stack objects differently will still
> fail to maintain the alignment you are trying to impose.

Considering that the alternatives are (from what I can tell) more
expensive, I can't see why a compiler would do that. Or maybe
I'm still not getting the exact scenario you're thinking about ...

>> (From a strictly formal perspective, me being
>> the only maintainer of EFI code, I could put the patch in without any
>> ack [other than Wei's release one], but I'd like to avoid that if at all
>> possible.)
> 
> If you want an ack, then fine.  Acked-by: Andrew Cooper
> <andrew.cooper3@citrix.com> but I don't expect this fully resolve the issue.

Thanks. And yes, we likely will want to make our stack handling
ABI compliant, at which point this patch could be reverted.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel