Re: [PATCH v2] x86/entry/64/paravirt: Use paravirt-safe macro to access eflags

[Andy Lutomirski]

On Fri, Dec 1, 2017 at 8:57 AM, Boris Ostrovsky
 wrote:
> On 12/01/2017 11:22 AM, Andy Lutomirski wrote:
>> On Tue, Nov 28, 2017 at 7:28 AM, Boris Ostrovsky
>>  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 
>>> ---
>>> 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
>>>
>>>
>

Re: [PATCH v2] x86/entry/64/paravirt: Use paravirt-safe macro to access eflags

[Andy Lutomirski]

On Fri, Dec 1, 2017 at 8:57 AM, Boris Ostrovsky
 wrote:
> On 12/01/2017 11:22 AM, Andy Lutomirski wrote:
>> On Tue, Nov 28, 2017 at 7:28 AM, Boris Ostrovsky
>>  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 
>>> ---
>>> 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
>>>
>>>
>

Re: [PATCH v2] x86/entry/64/paravirt: Use paravirt-safe macro to access eflags

[Boris Ostrovsky]

On 12/01/2017 11:22 AM, Andy Lutomirski wrote:
> On Tue, Nov 28, 2017 at 7:28 AM, Boris Ostrovsky
>  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 
>> ---
>> 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
>>
>>

Re: [PATCH v2] x86/entry/64/paravirt: Use paravirt-safe macro to access eflags

[Boris Ostrovsky]

On 12/01/2017 11:22 AM, Andy Lutomirski wrote:
> On Tue, Nov 28, 2017 at 7:28 AM, Boris Ostrovsky
>  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 
>> ---
>> 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
>>
>>

Re: [PATCH v2] x86/entry/64/paravirt: Use paravirt-safe macro to access eflags

[Andy Lutomirski]

On Tue, Nov 28, 2017 at 7:28 AM, Boris Ostrovsky
 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 
> ---
> 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
>

Re: [PATCH v2] x86/entry/64/paravirt: Use paravirt-safe macro to access eflags

[Andy Lutomirski]

On Tue, Nov 28, 2017 at 7:28 AM, Boris Ostrovsky
 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 
> ---
> 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
>

Re: [PATCH v2] x86/entry/64/paravirt: Use paravirt-safe macro to access eflags

[Juergen Gross]

On 28/11/17 16:28, Boris Ostrovsky 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 

Reviewed-by: Juergen Gross 


Juergen

Re: [PATCH v2] x86/entry/64/paravirt: Use paravirt-safe macro to access eflags

[Juergen Gross]

On 28/11/17 16:28, Boris Ostrovsky 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 

Reviewed-by: Juergen Gross 


Juergen