On Tue, Nov 28, 2017 at 7:28 AM, Boris Ostrovsky
<[email protected]> wrote:
> Commit 1d3e53e8624a ("x86/entry/64: Refactor IRQ stacks and make
> them NMI-safe") added DEBUG_ENTRY_ASSERT_IRQS_OFF macro that acceses
> eflags using 'pushfq' instruction when testing for IF bit. On PV Xen
> guests looking at IF flag directly will always see it set, resulting
> in 'ud2'.
>
> Introduce SAVE_FLAGS() macro that will use appropriate save_fl pv op
> when running paravirt.
>
> Signed-off-by: Boris Ostrovsky <[email protected]>
> ---
> V2:
> * Preserve %rax in DEBUG_ENTRY_ASSERT_IRQS_OFF
> * Return (pop) %rax in SAVE_FLAGS for !CONFIG_PARAVIRT (irqflags.h)
>
> arch/x86/entry/entry_64.S | 7 ++++---
> arch/x86/include/asm/irqflags.h | 3 +++
> arch/x86/include/asm/paravirt.h | 9 +++++++++
> arch/x86/kernel/asm-offsets_64.c | 3 +++
> 4 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index f81d50d..c208dc1 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -466,12 +466,13 @@ END(irq_entries_start)
>
> .macro DEBUG_ENTRY_ASSERT_IRQS_OFF
> #ifdef CONFIG_DEBUG_ENTRY
> - pushfq
> - testl $X86_EFLAGS_IF, (%rsp)
> + pushq %rax
> + SAVE_FLAGS(CLBR_ANY)
> + testl $X86_EFLAGS_IF, %eax
Confused. You're both using CLBR_ANY and RAX. Did you perhaps mean CLBR_NONE?
> jz .Lokay_\@
> ud2
> .Lokay_\@:
> - addq $8, %rsp
> + popq %rax
> #endif
> .endm
>
> diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
> index c8ef23f..89f0895 100644
> --- a/arch/x86/include/asm/irqflags.h
> +++ b/arch/x86/include/asm/irqflags.h
> @@ -142,6 +142,9 @@ static inline notrace unsigned long arch_local_irq_save(void)
> swapgs; \
> sysretl
>
> +#ifdef CONFIG_DEBUG_ENTRY
> +#define SAVE_FLAGS(x) pushfq; popq %rax
> +#endif
> #else
> #define INTERRUPT_RETURN iret
> #define ENABLE_INTERRUPTS_SYSEXIT sti; sysexit
> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
> index 283efca..892df37 100644
> --- a/arch/x86/include/asm/paravirt.h
> +++ b/arch/x86/include/asm/paravirt.h
> @@ -927,6 +927,15 @@ static inline notrace unsigned long arch_local_irq_save(void)
> PARA_SITE(PARA_PATCH(pv_cpu_ops, PV_CPU_usergs_sysret64), \
> CLBR_NONE, \
> jmp PARA_INDIRECT(pv_cpu_ops+PV_CPU_usergs_sysret64))
> +
> +#ifdef CONFIG_DEBUG_ENTRY
> +#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);)
> +#endif
> +
> #endif /* CONFIG_X86_32 */
>
> #endif /* __ASSEMBLY__ */
> diff --git a/arch/x86/kernel/asm-offsets_64.c b/arch/x86/kernel/asm-offsets_64.c
> index 630212f..e3a5175 100644
> --- a/arch/x86/kernel/asm-offsets_64.c
> +++ b/arch/x86/kernel/asm-offsets_64.c
> @@ -23,6 +23,9 @@ int main(void)
> #ifdef CONFIG_PARAVIRT
> OFFSET(PV_CPU_usergs_sysret64, pv_cpu_ops, usergs_sysret64);
> OFFSET(PV_CPU_swapgs, pv_cpu_ops, swapgs);
> +#ifdef CONFIG_DEBUG_ENTRY
> + OFFSET(PV_IRQ_save_fl, pv_irq_ops, save_fl);
> +#endif
> BLANK();
> #endif
>
> --
> 1.7.1
>
On 12/01/2017 11:22 AM, Andy Lutomirski wrote:
> On Tue, Nov 28, 2017 at 7:28 AM, Boris Ostrovsky
> <[email protected]> wrote:
>> Commit 1d3e53e8624a ("x86/entry/64: Refactor IRQ stacks and make
>> them NMI-safe") added DEBUG_ENTRY_ASSERT_IRQS_OFF macro that acceses
>> eflags using 'pushfq' instruction when testing for IF bit. On PV Xen
>> guests looking at IF flag directly will always see it set, resulting
>> in 'ud2'.
>>
>> Introduce SAVE_FLAGS() macro that will use appropriate save_fl pv op
>> when running paravirt.
>>
>> Signed-off-by: Boris Ostrovsky <[email protected]>
>> ---
>> V2:
>> * Preserve %rax in DEBUG_ENTRY_ASSERT_IRQS_OFF
>> * Return (pop) %rax in SAVE_FLAGS for !CONFIG_PARAVIRT (irqflags.h)
>>
>> arch/x86/entry/entry_64.S | 7 ++++---
>> arch/x86/include/asm/irqflags.h | 3 +++
>> arch/x86/include/asm/paravirt.h | 9 +++++++++
>> arch/x86/kernel/asm-offsets_64.c | 3 +++
>> 4 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
>> index f81d50d..c208dc1 100644
>> --- a/arch/x86/entry/entry_64.S
>> +++ b/arch/x86/entry/entry_64.S
>> @@ -466,12 +466,13 @@ END(irq_entries_start)
>>
>> .macro DEBUG_ENTRY_ASSERT_IRQS_OFF
>> #ifdef CONFIG_DEBUG_ENTRY
>> - pushfq
>> - testl $X86_EFLAGS_IF, (%rsp)
>> + pushq %rax
>> + SAVE_FLAGS(CLBR_ANY)
>> + testl $X86_EFLAGS_IF, %eax
> Confused. You're both using CLBR_ANY and RAX. Did you perhaps mean CLBR_NONE?
CLBR_NONE will restore all registers, won't it? So it should be
CLBR_RAX, should it? Otherwise we'll lose return value.
-boris
>
>> jz .Lokay_\@
>> ud2
>> .Lokay_\@:
>> - addq $8, %rsp
>> + popq %rax
>> #endif
>> .endm
>>
>>
On Fri, Dec 1, 2017 at 8:57 AM, Boris Ostrovsky
<[email protected]> wrote:
> On 12/01/2017 11:22 AM, Andy Lutomirski wrote:
>> On Tue, Nov 28, 2017 at 7:28 AM, Boris Ostrovsky
>> <[email protected]> wrote:
>>> Commit 1d3e53e8624a ("x86/entry/64: Refactor IRQ stacks and make
>>> them NMI-safe") added DEBUG_ENTRY_ASSERT_IRQS_OFF macro that acceses
>>> eflags using 'pushfq' instruction when testing for IF bit. On PV Xen
>>> guests looking at IF flag directly will always see it set, resulting
>>> in 'ud2'.
>>>
>>> Introduce SAVE_FLAGS() macro that will use appropriate save_fl pv op
>>> when running paravirt.
>>>
>>> Signed-off-by: Boris Ostrovsky <[email protected]>
>>> ---
>>> V2:
>>> * Preserve %rax in DEBUG_ENTRY_ASSERT_IRQS_OFF
>>> * Return (pop) %rax in SAVE_FLAGS for !CONFIG_PARAVIRT (irqflags.h)
>>>
>>> arch/x86/entry/entry_64.S | 7 ++++---
>>> arch/x86/include/asm/irqflags.h | 3 +++
>>> arch/x86/include/asm/paravirt.h | 9 +++++++++
>>> arch/x86/kernel/asm-offsets_64.c | 3 +++
>>> 4 files changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
>>> index f81d50d..c208dc1 100644
>>> --- a/arch/x86/entry/entry_64.S
>>> +++ b/arch/x86/entry/entry_64.S
>>> @@ -466,12 +466,13 @@ END(irq_entries_start)
>>>
>>> .macro DEBUG_ENTRY_ASSERT_IRQS_OFF
>>> #ifdef CONFIG_DEBUG_ENTRY
>>> - pushfq
>>> - testl $X86_EFLAGS_IF, (%rsp)
>>> + pushq %rax
>>> + SAVE_FLAGS(CLBR_ANY)
>>> + testl $X86_EFLAGS_IF, %eax
>> Confused. You're both using CLBR_ANY and RAX. Did you perhaps mean CLBR_NONE?
>
> CLBR_NONE will restore all registers, won't it? So it should be
> CLBR_RAX, should it? Otherwise we'll lose return value.
Ugh, probably right. I've never grokked exactly what CLBR_ does.
>
> -boris
>
>>
>>> jz .Lokay_\@
>>> ud2
>>> .Lokay_\@:
>>> - addq $8, %rsp
>>> + popq %rax
>>> #endif
>>> .endm
>>>
>>>
>