Linux save_fl PVOP for Xen

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

index a1207cb..fd4de44 100644
@@ -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);
 

index 3e45aa0..ba435ff 100644
@@ -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)

I'm somewhat surprised this ever worked...

PVHVM uses native save/restore flags.

This one liner seems preferred to me. Can you try it and see if the x86 maintainers agree?

David