Xen PV breakage after IRQ stack code refactoring

Xen PV breakage after IRQ stack code refactoring

[Boris Ostrovsky]

Andy,

(Can't find the original patch in my mailbox)

This hunk from 1d3e53e8624a ("x86/entry/64: Refactor IRQ stacks and make 
them NMI-safe")


diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index a9a8027..0d4483a 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -447,6 +447,59 @@ ENTRY(irq_entries_start)
      .endr
  END(irq_entries_start)

+.macro DEBUG_ENTRY_ASSERT_IRQS_OFF
+#ifdef CONFIG_DEBUG_ENTRY
+       pushfq
+       testl $X86_EFLAGS_IF, (%rsp)
+       jz .Lokay_\@
+       ud2
+.Lokay_\@:
+       addq $8, %rsp
+#endif
+.endm
+

makes Xen PV guests somewhat unhappy because IF flag will be set.

I was hoping to use ALTERNATIVE instruction but when we hit this for the 
first time we haven't rewritten instructions yet. Moving check_bugs() a 
bit higher helps but because this is common code I don't know how well 
it will work on other architectures (and, in fact, whether it is even 
safe on x86 in general, although that can be verified).

Another option is to also add a parameter to DEBUG_ENTRY_ASSERT_IRQS_OFF 
(or to ENTER_IRQ_STACK) from xen_do_hypervisor_callback (which is where 
the failure happens) but this looks pretty fragile in that it assumes 
that xen_do_hypervisor_callback is the only place where we use this 
codepath before alt instructions are set.

Any other suggestions?

-boris

Re: Xen PV breakage after IRQ stack code refactoring

[Andy Lutomirski]

On Sun, Nov 26, 2017 at 9:10 AM, Boris Ostrovsky
<boris.ostrovsky@oracle.com> wrote:
> Andy,
>
> (Can't find the original patch in my mailbox)
>
> This hunk from 1d3e53e8624a ("x86/entry/64: Refactor IRQ stacks and make
> them NMI-safe")
>
>
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index a9a8027..0d4483a 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -447,6 +447,59 @@ ENTRY(irq_entries_start)
>      .endr
>  END(irq_entries_start)
>
> +.macro DEBUG_ENTRY_ASSERT_IRQS_OFF
> +#ifdef CONFIG_DEBUG_ENTRY
> +       pushfq
> +       testl $X86_EFLAGS_IF, (%rsp)
> +       jz .Lokay_\@
> +       ud2
> +.Lokay_\@:
> +       addq $8, %rsp
> +#endif
> +.endm
> +
>
> makes Xen PV guests somewhat unhappy because IF flag will be set.
>
> I was hoping to use ALTERNATIVE instruction but when we hit this for the
> first time we haven't rewritten instructions yet. Moving check_bugs() a bit
> higher helps but because this is common code I don't know how well it will
> work on other architectures (and, in fact, whether it is even safe on x86 in
> general, although that can be verified).
>
> Another option is to also add a parameter to DEBUG_ENTRY_ASSERT_IRQS_OFF (or
> to ENTER_IRQ_STACK) from xen_do_hypervisor_callback (which is where the
> failure happens) but this looks pretty fragile in that it assumes that
> xen_do_hypervisor_callback is the only place where we use this codepath
> before alt instructions are set.
>
> Any other suggestions?

Do we have a convenient asm way to access the save_fl pvop?

>
> -boris
>

Re: Xen PV breakage after IRQ stack code refactoring

[Juergen Gross]

On 27/11/17 05:03, Andy Lutomirski wrote:
> On Sun, Nov 26, 2017 at 9:10 AM, Boris Ostrovsky
> <boris.ostrovsky@oracle.com> wrote:
>> Andy,
>>
>> (Can't find the original patch in my mailbox)
>>
>> This hunk from 1d3e53e8624a ("x86/entry/64: Refactor IRQ stacks and make
>> them NMI-safe")
>>
>>
>> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
>> index a9a8027..0d4483a 100644
>> --- a/arch/x86/entry/entry_64.S
>> +++ b/arch/x86/entry/entry_64.S
>> @@ -447,6 +447,59 @@ ENTRY(irq_entries_start)
>>      .endr
>>  END(irq_entries_start)
>>
>> +.macro DEBUG_ENTRY_ASSERT_IRQS_OFF
>> +#ifdef CONFIG_DEBUG_ENTRY
>> +       pushfq
>> +       testl $X86_EFLAGS_IF, (%rsp)
>> +       jz .Lokay_\@
>> +       ud2
>> +.Lokay_\@:
>> +       addq $8, %rsp
>> +#endif
>> +.endm
>> +
>>
>> makes Xen PV guests somewhat unhappy because IF flag will be set.
>>
>> I was hoping to use ALTERNATIVE instruction but when we hit this for the
>> first time we haven't rewritten instructions yet. Moving check_bugs() a bit
>> higher helps but because this is common code I don't know how well it will
>> work on other architectures (and, in fact, whether it is even safe on x86 in
>> general, although that can be verified).
>>
>> Another option is to also add a parameter to DEBUG_ENTRY_ASSERT_IRQS_OFF (or
>> to ENTER_IRQ_STACK) from xen_do_hypervisor_callback (which is where the
>> failure happens) but this looks pretty fragile in that it assumes that
>> xen_do_hypervisor_callback is the only place where we use this codepath
>> before alt instructions are set.
>>
>> Any other suggestions?
> 
> Do we have a convenient asm way to access the save_fl pvop?

No, but adding it would be pretty straight forward. Its something like:

#define SAVE_FLAGS(clobbers)                                        \
        PARA_SITE(PARA_PATCH(pv_irq_ops, PV_IRQ_save_fl), clobbers, \
                  PV_SAVE_REGS(clobbers | CLBR_CALLEE_SAVE);        \
                  call PARA_INDIRECT(pv_irq_ops+PV_IRQ_save_fl);    \
                  PV_RESTORE_REGS(clobbers | CLBR_CALLEE_SAVE);)

similar to DISABLE_INTERRUPTS(), requiring just the definition of
PV_IRQ_save_fl in asm-offsets.c and the non-pvops definition of
SAVE_FLAGS() in irqflags.h.


Juergen

Re: Xen PV breakage after IRQ stack code refactoring

[Boris Ostrovsky]

On 11/27/2017 12:34 AM, Juergen Gross wrote:
> On 27/11/17 05:03, Andy Lutomirski wrote:
>> On Sun, Nov 26, 2017 at 9:10 AM, Boris Ostrovsky
>> <boris.ostrovsky@oracle.com> wrote:
>>> Andy,
>>>
>>> (Can't find the original patch in my mailbox)
>>>
>>> This hunk from 1d3e53e8624a ("x86/entry/64: Refactor IRQ stacks and make
>>> them NMI-safe")
>>>
>>>
>>> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
>>> index a9a8027..0d4483a 100644
>>> --- a/arch/x86/entry/entry_64.S
>>> +++ b/arch/x86/entry/entry_64.S
>>> @@ -447,6 +447,59 @@ ENTRY(irq_entries_start)
>>>      .endr
>>>  END(irq_entries_start)
>>>
>>> +.macro DEBUG_ENTRY_ASSERT_IRQS_OFF
>>> +#ifdef CONFIG_DEBUG_ENTRY
>>> +       pushfq
>>> +       testl $X86_EFLAGS_IF, (%rsp)
>>> +       jz .Lokay_\@
>>> +       ud2
>>> +.Lokay_\@:
>>> +       addq $8, %rsp
>>> +#endif
>>> +.endm
>>> +
>>>
>>> makes Xen PV guests somewhat unhappy because IF flag will be set.
>>>
>>> I was hoping to use ALTERNATIVE instruction but when we hit this for the
>>> first time we haven't rewritten instructions yet. Moving check_bugs() a bit
>>> higher helps but because this is common code I don't know how well it will
>>> work on other architectures (and, in fact, whether it is even safe on x86 in
>>> general, although that can be verified).
>>>
>>> Another option is to also add a parameter to DEBUG_ENTRY_ASSERT_IRQS_OFF (or
>>> to ENTER_IRQ_STACK) from xen_do_hypervisor_callback (which is where the
>>> failure happens) but this looks pretty fragile in that it assumes that
>>> xen_do_hypervisor_callback is the only place where we use this codepath
>>> before alt instructions are set.
>>>
>>> Any other suggestions?
>> Do we have a convenient asm way to access the save_fl pvop?
> No, but adding it would be pretty straight forward. Its something like:
>
> #define SAVE_FLAGS(clobbers)                                        \
>         PARA_SITE(PARA_PATCH(pv_irq_ops, PV_IRQ_save_fl), clobbers, \
>                   PV_SAVE_REGS(clobbers | CLBR_CALLEE_SAVE);        \
>                   call PARA_INDIRECT(pv_irq_ops+PV_IRQ_save_fl);    \
>                   PV_RESTORE_REGS(clobbers | CLBR_CALLEE_SAVE);)
>
> similar to DISABLE_INTERRUPTS(), requiring just the definition of
> PV_IRQ_save_fl in asm-offsets.c and the non-pvops definition of
> SAVE_FLAGS() in irqflags.h.

Hmm.. Indeed, I haven't thought of this for whatever reasons. I'll send
the patch soon.

Thanks.
-boris