[Xen-devel] [RFC] Linux save_fl PVOP for Xen

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

Having recently got some Broadwell hardware, our automatic test system
discovered that 32bit PV guests would reliably blow up while attempting
to boot.

It turns out that the save_fl PVOP is at fault.  The comment is false,
as setup_smap() uses it to check that the Alignment Check flag is clear.

As the Xen PVOP leaves everything other than %ah worth of eflags
uninitialised, the BUG_ON(eflags & X86_EFLAGS_AC) in setup_smap() is
unconditional undefined behaviour on all versions of Linux since SMAP
support was introduced.

I had developed a patch (see below) and was writing up the commit
message, but it would appear that this PVOP is also used by PVHVM
domains, which invalidates an assumption underlying the fix (insofar
that 'pushf' would unconditionally have IF set).

There are a few options available, but I would like to gather opinions,
as none of them are fantastic.

1) Extend the patch to work for PVHVM as well.  This is problem as it
will make a long hotpath even longer.

2) Change setup_smap() to use something like native_safe_fl().  Unlikely
to get traction upstream, and fragile to similar changes in the future.

3) Change PVHVM to use the native save_fl().  (I don't see why it
doesn't now), but this is a much more invasive change.

Suggestions/alternatives welcome.

~Andrew

diff --git a/arch/x86/xen/irq.c b/arch/x86/xen/irq.c
index a1207cb..fd4de44 100644
--- a/arch/x86/xen/irq.c
+++ b/arch/x86/xen/irq.c
@@ -26,18 +26,14 @@ void xen_force_evtchn_callback(void)
 asmlinkage __visible unsigned long xen_save_fl(void)
 {
        struct vcpu_info *vcpu;
-       unsigned long flags;
+       unsigned long flags = native_save_fl();
 
        vcpu = this_cpu_read(xen_vcpu);
 
-       /* flag has opposite sense of mask */
-       flags = !vcpu->evtchn_upcall_mask;
+       if (vcpu->evtchn_upcall_mask)
+               flags &= ~X86_EFLAGS_IF;
 
-       /* convert to IF type flag
-          -0 -> 0x00000000
-          -1 -> 0xffffffff
-       */
-       return (-flags) & X86_EFLAGS_IF;
+       return flags;
 }
 PV_CALLEE_SAVE_REGS_THUNK(xen_save_fl);
 
diff --git a/arch/x86/xen/xen-asm.S b/arch/x86/xen/xen-asm.S
index 3e45aa0..ba435ff 100644
--- a/arch/x86/xen/xen-asm.S
+++ b/arch/x86/xen/xen-asm.S
@@ -65,9 +65,18 @@ ENDPATCH(xen_irq_disable_direct)
  * x86 use opposite senses (mask vs enable).
  */
 ENTRY(xen_save_fl_direct)
-       testb $0xff, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_mask
-       setz %ah
-       addb %ah, %ah
+       pushf
+       testb   $0xff, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_mask
+       setnz   %al
+       shl     $1, %al
+       not     %al
+#ifdef CONFIG_X86_64
+       andb    %al, 1(%rsp)
+       pop     %rax
+#else
+       andb    %al, 1(%esp)
+       pop     %eax
+#endif
 ENDPATCH(xen_save_fl_direct)
        ret
        ENDPROC(xen_save_fl_direct)


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