* [PATCH v2] x86/entry/64/paravirt: Use paravirt-safe macro to access eflags
@ 2017-11-28 15:28 Boris Ostrovsky
2017-11-29 7:25 ` Juergen Gross
2017-12-01 16:22 ` Andy Lutomirski
0 siblings, 2 replies; 5+ messages in thread
From: Boris Ostrovsky @ 2017-11-28 15:28 UTC (permalink / raw)
To: xen-devel, linux-kernel
Cc: tglx, mingo, hpa, x86, jgross, luto, Boris Ostrovsky
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 <boris.ostrovsky@oracle.com>
---
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
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v2] x86/entry/64/paravirt: Use paravirt-safe macro to access eflags
2017-11-28 15:28 [PATCH v2] x86/entry/64/paravirt: Use paravirt-safe macro to access eflags Boris Ostrovsky
@ 2017-11-29 7:25 ` Juergen Gross
2017-12-01 16:22 ` Andy Lutomirski
1 sibling, 0 replies; 5+ messages in thread
From: Juergen Gross @ 2017-11-29 7:25 UTC (permalink / raw)
To: Boris Ostrovsky, xen-devel, linux-kernel; +Cc: tglx, mingo, hpa, x86, luto
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 <boris.ostrovsky@oracle.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Juergen
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] x86/entry/64/paravirt: Use paravirt-safe macro to access eflags
2017-11-28 15:28 [PATCH v2] x86/entry/64/paravirt: Use paravirt-safe macro to access eflags Boris Ostrovsky
2017-11-29 7:25 ` Juergen Gross
@ 2017-12-01 16:22 ` Andy Lutomirski
2017-12-01 16:57 ` Boris Ostrovsky
1 sibling, 1 reply; 5+ messages in thread
From: Andy Lutomirski @ 2017-12-01 16:22 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: xen-devel, linux-kernel, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, X86 ML, Juergen Gross, Andrew Lutomirski
On Tue, Nov 28, 2017 at 7:28 AM, Boris Ostrovsky
<boris.ostrovsky@oracle.com> 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 <boris.ostrovsky@oracle.com>
> ---
> 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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] x86/entry/64/paravirt: Use paravirt-safe macro to access eflags
2017-12-01 16:22 ` Andy Lutomirski
@ 2017-12-01 16:57 ` Boris Ostrovsky
2017-12-01 17:44 ` Andy Lutomirski
0 siblings, 1 reply; 5+ messages in thread
From: Boris Ostrovsky @ 2017-12-01 16:57 UTC (permalink / raw)
To: Andy Lutomirski
Cc: xen-devel, linux-kernel, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, X86 ML, Juergen Gross
On 12/01/2017 11:22 AM, Andy Lutomirski wrote:
> On Tue, Nov 28, 2017 at 7:28 AM, Boris Ostrovsky
> <boris.ostrovsky@oracle.com> 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 <boris.ostrovsky@oracle.com>
>> ---
>> 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
>>
>>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] x86/entry/64/paravirt: Use paravirt-safe macro to access eflags
2017-12-01 16:57 ` Boris Ostrovsky
@ 2017-12-01 17:44 ` Andy Lutomirski
0 siblings, 0 replies; 5+ messages in thread
From: Andy Lutomirski @ 2017-12-01 17:44 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Andy Lutomirski, xen-devel, linux-kernel, Thomas Gleixner,
Ingo Molnar, H. Peter Anvin, X86 ML, Juergen Gross
On Fri, Dec 1, 2017 at 8:57 AM, Boris Ostrovsky
<boris.ostrovsky@oracle.com> wrote:
> On 12/01/2017 11:22 AM, Andy Lutomirski wrote:
>> On Tue, Nov 28, 2017 at 7:28 AM, Boris Ostrovsky
>> <boris.ostrovsky@oracle.com> 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 <boris.ostrovsky@oracle.com>
>>> ---
>>> 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
>>>
>>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-12-01 17:44 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-28 15:28 [PATCH v2] x86/entry/64/paravirt: Use paravirt-safe macro to access eflags Boris Ostrovsky
2017-11-29 7:25 ` Juergen Gross
2017-12-01 16:22 ` Andy Lutomirski
2017-12-01 16:57 ` Boris Ostrovsky
2017-12-01 17:44 ` Andy Lutomirski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).