diff mbox series

[v9,01/26] arm64: Fix HCR.TGE status for NMI contexts

Message ID 1548084825-8803-2-git-send-email-julien.thierry@arm.com (mailing list archive)
State New, archived
Headers show
Series arm64: provide pseudo NMI with GICv3 | expand

Commit Message

Julien Thierry Jan. 21, 2019, 3:33 p.m. UTC
When using VHE, the host needs to clear HCR_EL2.TGE bit in order
to interract with guest TLBs, switching from EL2&0 translation regime
to EL1&0.

However, some non-maskable asynchronous event could happen while TGE is
cleared like SDEI. Because of this address translation operations
relying on EL2&0 translation regime could fail (tlb invalidation,
userspace access, ...).

Fix this by properly setting HCR_EL2.TGE when entering NMI context and
clear it if necessary when returning to the interrupted context.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
Suggested-by: Marc Zyngier <marc.zyngier@arm.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: linux-arch@vger.kernel.org
Cc: stable@vger.kernel.org
---
 arch/arm64/include/asm/hardirq.h | 28 ++++++++++++++++++++++++++++
 arch/arm64/kernel/irq.c          |  3 +++
 include/linux/hardirq.h          |  7 +++++++
 3 files changed, 38 insertions(+)

Comments

Sasha Levin Jan. 23, 2019, 10:57 p.m. UTC | #1
Hi,

[This is an automated email]

This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all

The bot has tested the following trees: v4.20.3, v4.19.16, v4.14.94, v4.9.151, v4.4.171, v3.18.132.

v4.20.3: Build OK!
v4.19.16: Build OK!
v4.14.94: Build OK!
v4.9.151: Failed to apply! Possible dependencies:
    096683724cb2 ("arm64: unwind: avoid percpu indirection for irq stack")
    34be98f4944f ("arm64: kernel: remove {THREAD,IRQ_STACK}_START_SP")
    a9ea0017ebe8 ("arm64: factor out current_stack_pointer")
    c02433dd6de3 ("arm64: split thread_info from task stack")
    c7365330753c ("arm64: unwind: disregard frame.sp when validating frame pointer")
    dbc9344a68e5 ("arm64: clean up THREAD_* definitions")
    f60ad4edcf07 ("arm64: clean up irq stack definitions")
    f60fe78f1332 ("arm64: use an irq stack pointer")

v4.4.171: Failed to apply! Possible dependencies:
    096683724cb2 ("arm64: unwind: avoid percpu indirection for irq stack")
    0a8ea52c3eb1 ("arm64: Add HAVE_REGS_AND_STACK_ACCESS_API feature")
    132cd887b5c5 ("arm64: Modify stack trace and dump for use with irq_stack")
    1ffe199b1c9b ("arm64: when walking onto the task stack, check sp & fp are in current->stack")
    20380bb390a4 ("arm64: ftrace: fix a stack tracer's output under function graph tracer")
    7596abf2e566 ("arm64: irq: fix walking from irq stack to task stack")
    8e23dacd12a4 ("arm64: Add do_softirq_own_stack() and enable irq_stacks")
    971c67ce37cf ("arm64: reduce stack use in irq_handler")
    a80a0eb70c35 ("arm64: make irq_stack_ptr more robust")
    c7365330753c ("arm64: unwind: disregard frame.sp when validating frame pointer")
    f60ad4edcf07 ("arm64: clean up irq stack definitions")
    f60fe78f1332 ("arm64: use an irq stack pointer")
    fe13f95b7200 ("arm64: pass a task parameter to unwind_frame()")

v3.18.132: Failed to apply! Possible dependencies:
    020295b4cb5b ("ACPI / processor: Make it possible to get CPU hardware ID via GICC")
    132cd887b5c5 ("arm64: Modify stack trace and dump for use with irq_stack")
    13ca62b243f6 ("ACPI: Fix minor syntax issues in processor_core.c")
    37655163ce1a ("ARM64 / ACPI: Get RSDP and ACPI boot-time tables")
    587064b610c7 ("arm64: Add framework for legacy instruction emulation")
    652261a7a86c ("ACPI: fix acpi_os_ioremap for arm64")
    828aef376d7a ("ACPI / processor: Introduce phys_cpuid_t for CPU hardware ID")
    96f0e00378d4 ("ARM: add basic support for on-demand backtrace of other CPUs")
    af2c632e234f ("arm64/include/asm: Fixed a warning about 'struct pt_regs'")
    af8f3f514d19 ("ACPI / processor: Convert apic_id to phys_id to make it arch agnostic")
    b4ff8389ed14 ("xen/events: Always allocate legacy interrupts on PV guests")
    d02dc27db0dc ("ACPI / processor: Rename acpi_(un)map_lsapic() to acpi_(un)map_cpu()")
    d60fc3892c4d ("irqchip: Add GICv2 specific ACPI boot support")
    ecf5636dcd59 ("ACPI: Add interfaces to parse IOAPIC ID for IOAPIC hotplug")
    f60ad4edcf07 ("arm64: clean up irq stack definitions")
    f60fe78f1332 ("arm64: use an irq stack pointer")


How should we proceed with this patch?

--
Thanks,
Sasha
James Morse Jan. 28, 2019, 11:48 a.m. UTC | #2
Hi Julien,

On 21/01/2019 15:33, Julien Thierry wrote:
> When using VHE, the host needs to clear HCR_EL2.TGE bit in order
> to interract with guest TLBs, switching from EL2&0 translation regime

(interact)


> to EL1&0.
> 
> However, some non-maskable asynchronous event could happen while TGE is
> cleared like SDEI. Because of this address translation operations
> relying on EL2&0 translation regime could fail (tlb invalidation,
> userspace access, ...).
> 
> Fix this by properly setting HCR_EL2.TGE when entering NMI context and
> clear it if necessary when returning to the interrupted context.

Yes please. This would not have been fun to debug!

Reviewed-by: James Morse <james.morse@arm.com>



I was looking for why we need core code to do this, instead of updating the
arch's call sites. Your 'irqdesc: Add domain handlers for NMIs' patch (pointed
to from the cover letter) is the reason: core-code calls nmi_enter()/nmi_exit()
itself.


Thanks,

James


> diff --git a/arch/arm64/include/asm/hardirq.h b/arch/arm64/include/asm/hardirq.h
> index 1473fc2..94b7481 100644
> --- a/arch/arm64/include/asm/hardirq.h
> +++ b/arch/arm64/include/asm/hardirq.h
> @@ -19,6 +19,7 @@
>  #include <linux/cache.h>
>  #include <linux/threads.h>
>  #include <asm/irq.h>
> +#include <asm/kvm_arm.h>

percpu.h?
sysreg.h?
barrier.h?


> @@ -37,6 +38,33 @@
>  
>  #define __ARCH_IRQ_EXIT_IRQS_DISABLED	1
>  
> +struct nmi_ctx {
> +	u64 hcr;
> +};
> +
> +DECLARE_PER_CPU(struct nmi_ctx, nmi_contexts);
> +
> +#define arch_nmi_enter()							\
> +	do {									\
> +		if (is_kernel_in_hyp_mode()) {					\
> +			struct nmi_ctx *nmi_ctx = this_cpu_ptr(&nmi_contexts);	\
> +			nmi_ctx->hcr = read_sysreg(hcr_el2);			\
> +			if (!(nmi_ctx->hcr & HCR_TGE)) {			\
> +				write_sysreg(nmi_ctx->hcr | HCR_TGE, hcr_el2);	\
> +				isb();						\
> +			}							\
> +		}								\
> +	} while (0)
> +
> +#define arch_nmi_exit()								\
> +	do {									\
> +		if (is_kernel_in_hyp_mode()) {					\
> +			struct nmi_ctx *nmi_ctx = this_cpu_ptr(&nmi_contexts);	\
> +			if (!(nmi_ctx->hcr & HCR_TGE))				\
> +				write_sysreg(nmi_ctx->hcr, hcr_el2);		\
> +		}								\
> +	} while (0)
> +
>  static inline void ack_bad_irq(unsigned int irq)
>  {
>  	extern unsigned long irq_err_count;



> diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h
> index 0fbbcdf..da0af63 100644
> --- a/include/linux/hardirq.h
> +++ b/include/linux/hardirq.h
> @@ -60,8 +60,14 @@ static inline void rcu_nmi_exit(void)
>   */
>  extern void irq_exit(void);
>  
> +#ifndef arch_nmi_enter
> +#define arch_nmi_enter()	do { } while (0)
> +#define arch_nmi_exit()		do { } while (0)
> +#endif
> +
>  #define nmi_enter()						\
>  	do {							\
> +		arch_nmi_enter();				\
>  		printk_nmi_enter();				\
>  		lockdep_off();					\
>  		ftrace_nmi_enter();				\
> @@ -80,6 +86,7 @@ static inline void rcu_nmi_exit(void)
>  		ftrace_nmi_exit();				\
>  		lockdep_on();					\
>  		printk_nmi_exit();				\
> +		arch_nmi_exit();				\
>  	} while (0)
>  
>  #endif /* LINUX_HARDIRQ_H */
>
Julien Thierry Jan. 28, 2019, 3:42 p.m. UTC | #3
Hi James,

On 28/01/2019 11:48, James Morse wrote:
> Hi Julien,
> 
> On 21/01/2019 15:33, Julien Thierry wrote:
>> When using VHE, the host needs to clear HCR_EL2.TGE bit in order
>> to interract with guest TLBs, switching from EL2&0 translation regime
> 
> (interact)
> 
> 
>> to EL1&0.
>>
>> However, some non-maskable asynchronous event could happen while TGE is
>> cleared like SDEI. Because of this address translation operations
>> relying on EL2&0 translation regime could fail (tlb invalidation,
>> userspace access, ...).
>>
>> Fix this by properly setting HCR_EL2.TGE when entering NMI context and
>> clear it if necessary when returning to the interrupted context.
> 
> Yes please. This would not have been fun to debug!
> 
> Reviewed-by: James Morse <james.morse@arm.com>
> 
> 

Thanks.

> 
> I was looking for why we need core code to do this, instead of updating the
> arch's call sites. Your 'irqdesc: Add domain handlers for NMIs' patch (pointed
> to from the cover letter) is the reason: core-code calls nmi_enter()/nmi_exit()
> itself.
> 

Yes, that's the main reason.

> 
>> diff --git a/arch/arm64/include/asm/hardirq.h b/arch/arm64/include/asm/hardirq.h
>> index 1473fc2..94b7481 100644
>> --- a/arch/arm64/include/asm/hardirq.h
>> +++ b/arch/arm64/include/asm/hardirq.h
>> @@ -19,6 +19,7 @@
>>  #include <linux/cache.h>
>>  #include <linux/threads.h>
>>  #include <asm/irq.h>
>> +#include <asm/kvm_arm.h>
> 
> percpu.h?
> sysreg.h?
> barrier.h?
> 

Good point, I'll add those.

Thanks,
Christoffer Dall Jan. 31, 2019, 8:19 a.m. UTC | #4
On Mon, Jan 28, 2019 at 03:42:42PM +0000, Julien Thierry wrote:
> Hi James,
> 
> On 28/01/2019 11:48, James Morse wrote:
> > Hi Julien,
> > 
> > On 21/01/2019 15:33, Julien Thierry wrote:
> >> When using VHE, the host needs to clear HCR_EL2.TGE bit in order
> >> to interract with guest TLBs, switching from EL2&0 translation regime
> > 
> > (interact)
> > 
> > 
> >> to EL1&0.
> >>
> >> However, some non-maskable asynchronous event could happen while TGE is
> >> cleared like SDEI. Because of this address translation operations
> >> relying on EL2&0 translation regime could fail (tlb invalidation,
> >> userspace access, ...).
> >>
> >> Fix this by properly setting HCR_EL2.TGE when entering NMI context and
> >> clear it if necessary when returning to the interrupted context.
> > 
> > Yes please. This would not have been fun to debug!
> > 
> > Reviewed-by: James Morse <james.morse@arm.com>
> > 
> > 
> 
> Thanks.
> 
> > 
> > I was looking for why we need core code to do this, instead of updating the
> > arch's call sites. Your 'irqdesc: Add domain handlers for NMIs' patch (pointed
> > to from the cover letter) is the reason: core-code calls nmi_enter()/nmi_exit()
> > itself.
> > 
> 
> Yes, that's the main reason.
> 
I wondered the same thing, but I don't understand the explanation :(

Why can't we do a local_daif_mask() around the (very small) calls that
clear TGE instead?


Thanks,

    Christoffer
Julien Thierry Jan. 31, 2019, 8:56 a.m. UTC | #5
On 31/01/2019 08:19, Christoffer Dall wrote:
> On Mon, Jan 28, 2019 at 03:42:42PM +0000, Julien Thierry wrote:
>> Hi James,
>>
>> On 28/01/2019 11:48, James Morse wrote:
>>> Hi Julien,
>>>
>>> On 21/01/2019 15:33, Julien Thierry wrote:
>>>> When using VHE, the host needs to clear HCR_EL2.TGE bit in order
>>>> to interract with guest TLBs, switching from EL2&0 translation regime
>>>
>>> (interact)
>>>
>>>
>>>> to EL1&0.
>>>>
>>>> However, some non-maskable asynchronous event could happen while TGE is
>>>> cleared like SDEI. Because of this address translation operations
>>>> relying on EL2&0 translation regime could fail (tlb invalidation,
>>>> userspace access, ...).
>>>>
>>>> Fix this by properly setting HCR_EL2.TGE when entering NMI context and
>>>> clear it if necessary when returning to the interrupted context.
>>>
>>> Yes please. This would not have been fun to debug!
>>>
>>> Reviewed-by: James Morse <james.morse@arm.com>
>>>
>>>
>>
>> Thanks.
>>
>>>
>>> I was looking for why we need core code to do this, instead of updating the
>>> arch's call sites. Your 'irqdesc: Add domain handlers for NMIs' patch (pointed
>>> to from the cover letter) is the reason: core-code calls nmi_enter()/nmi_exit()
>>> itself.
>>>
>>
>> Yes, that's the main reason.
>>
> I wondered the same thing, but I don't understand the explanation :(
> 
> Why can't we do a local_daif_mask() around the (very small) calls that
> clear TGE instead?
> 

That would protect against the pseudo-NMIs, but you can still get an
SDEI at that point even with all daif bits set. Or did I misunderstand
how SDEI works?

Thanks,
Christoffer Dall Jan. 31, 2019, 9:27 a.m. UTC | #6
On Thu, Jan 31, 2019 at 08:56:04AM +0000, Julien Thierry wrote:
> 
> 
> On 31/01/2019 08:19, Christoffer Dall wrote:
> > On Mon, Jan 28, 2019 at 03:42:42PM +0000, Julien Thierry wrote:
> >> Hi James,
> >>
> >> On 28/01/2019 11:48, James Morse wrote:
> >>> Hi Julien,
> >>>
> >>> On 21/01/2019 15:33, Julien Thierry wrote:
> >>>> When using VHE, the host needs to clear HCR_EL2.TGE bit in order
> >>>> to interract with guest TLBs, switching from EL2&0 translation regime
> >>>
> >>> (interact)
> >>>
> >>>
> >>>> to EL1&0.
> >>>>
> >>>> However, some non-maskable asynchronous event could happen while TGE is
> >>>> cleared like SDEI. Because of this address translation operations
> >>>> relying on EL2&0 translation regime could fail (tlb invalidation,
> >>>> userspace access, ...).
> >>>>
> >>>> Fix this by properly setting HCR_EL2.TGE when entering NMI context and
> >>>> clear it if necessary when returning to the interrupted context.
> >>>
> >>> Yes please. This would not have been fun to debug!
> >>>
> >>> Reviewed-by: James Morse <james.morse@arm.com>
> >>>
> >>>
> >>
> >> Thanks.
> >>
> >>>
> >>> I was looking for why we need core code to do this, instead of updating the
> >>> arch's call sites. Your 'irqdesc: Add domain handlers for NMIs' patch (pointed
> >>> to from the cover letter) is the reason: core-code calls nmi_enter()/nmi_exit()
> >>> itself.
> >>>
> >>
> >> Yes, that's the main reason.
> >>
> > I wondered the same thing, but I don't understand the explanation :(
> > 
> > Why can't we do a local_daif_mask() around the (very small) calls that
> > clear TGE instead?
> > 
> 
> That would protect against the pseudo-NMIs, but you can still get an
> SDEI at that point even with all daif bits set. Or did I misunderstand
> how SDEI works?
> 

I don't know the details of SDEI.  From looking at this patch, the
logical conclusion would be that SDEIs can then only be delivered once
we've called nmi_enter, but since we don't call this directly from the
code that clears TGE for doing guest TLB invalidation (or do we?) then
masking interrupts at the PSTATE level should be sufficient.

Surely I'm missing some part of the bigger picture here.

Thanks,

    Christoffer
Julien Thierry Jan. 31, 2019, 9:40 a.m. UTC | #7
On 31/01/2019 09:27, Christoffer Dall wrote:
> On Thu, Jan 31, 2019 at 08:56:04AM +0000, Julien Thierry wrote:
>>
>>
>> On 31/01/2019 08:19, Christoffer Dall wrote:
>>> On Mon, Jan 28, 2019 at 03:42:42PM +0000, Julien Thierry wrote:
>>>> Hi James,
>>>>
>>>> On 28/01/2019 11:48, James Morse wrote:
>>>>> Hi Julien,
>>>>>
>>>>> On 21/01/2019 15:33, Julien Thierry wrote:
>>>>>> When using VHE, the host needs to clear HCR_EL2.TGE bit in order
>>>>>> to interract with guest TLBs, switching from EL2&0 translation regime
>>>>>
>>>>> (interact)
>>>>>
>>>>>
>>>>>> to EL1&0.
>>>>>>
>>>>>> However, some non-maskable asynchronous event could happen while TGE is
>>>>>> cleared like SDEI. Because of this address translation operations
>>>>>> relying on EL2&0 translation regime could fail (tlb invalidation,
>>>>>> userspace access, ...).
>>>>>>
>>>>>> Fix this by properly setting HCR_EL2.TGE when entering NMI context and
>>>>>> clear it if necessary when returning to the interrupted context.
>>>>>
>>>>> Yes please. This would not have been fun to debug!
>>>>>
>>>>> Reviewed-by: James Morse <james.morse@arm.com>
>>>>>
>>>>>
>>>>
>>>> Thanks.
>>>>
>>>>>
>>>>> I was looking for why we need core code to do this, instead of updating the
>>>>> arch's call sites. Your 'irqdesc: Add domain handlers for NMIs' patch (pointed
>>>>> to from the cover letter) is the reason: core-code calls nmi_enter()/nmi_exit()
>>>>> itself.
>>>>>
>>>>
>>>> Yes, that's the main reason.
>>>>
>>> I wondered the same thing, but I don't understand the explanation :(
>>>
>>> Why can't we do a local_daif_mask() around the (very small) calls that
>>> clear TGE instead?
>>>
>>
>> That would protect against the pseudo-NMIs, but you can still get an
>> SDEI at that point even with all daif bits set. Or did I misunderstand
>> how SDEI works?
>>
> 
> I don't know the details of SDEI.  From looking at this patch, the
> logical conclusion would be that SDEIs can then only be delivered once
> we've called nmi_enter, but since we don't call this directly from the
> code that clears TGE for doing guest TLB invalidation (or do we?) then
> masking interrupts at the PSTATE level should be sufficient.
> 
> Surely I'm missing some part of the bigger picture here.
> 

I'm not sure I understand. SDEI uses the NMI context and AFAIU, it is an
interrupt that the firmware sends to the OS, and it is sent regardless
of the PSTATE at the OS EL.

So, the worrying part is:
- Hyp clears TGE
- Exception/interrupt taken to EL3
- Firmware decides it's a good time to send an SDEI to the OS
- SDEI handler (at EL2 for VHE) does nmi_enter()
- SDEI handler needs to do cache invalidation or something with the
EL2&0 translation regime but TGE is cleared

We don't expect the code that clears TGE to call nmi_enter().
Christoffer Dall Jan. 31, 2019, 9:48 a.m. UTC | #8
On Thu, Jan 31, 2019 at 09:40:02AM +0000, Julien Thierry wrote:
> 
> 
> On 31/01/2019 09:27, Christoffer Dall wrote:
> > On Thu, Jan 31, 2019 at 08:56:04AM +0000, Julien Thierry wrote:
> >>
> >>
> >> On 31/01/2019 08:19, Christoffer Dall wrote:
> >>> On Mon, Jan 28, 2019 at 03:42:42PM +0000, Julien Thierry wrote:
> >>>> Hi James,
> >>>>
> >>>> On 28/01/2019 11:48, James Morse wrote:
> >>>>> Hi Julien,
> >>>>>
> >>>>> On 21/01/2019 15:33, Julien Thierry wrote:
> >>>>>> When using VHE, the host needs to clear HCR_EL2.TGE bit in order
> >>>>>> to interract with guest TLBs, switching from EL2&0 translation regime
> >>>>>
> >>>>> (interact)
> >>>>>
> >>>>>
> >>>>>> to EL1&0.
> >>>>>>
> >>>>>> However, some non-maskable asynchronous event could happen while TGE is
> >>>>>> cleared like SDEI. Because of this address translation operations
> >>>>>> relying on EL2&0 translation regime could fail (tlb invalidation,
> >>>>>> userspace access, ...).
> >>>>>>
> >>>>>> Fix this by properly setting HCR_EL2.TGE when entering NMI context and
> >>>>>> clear it if necessary when returning to the interrupted context.
> >>>>>
> >>>>> Yes please. This would not have been fun to debug!
> >>>>>
> >>>>> Reviewed-by: James Morse <james.morse@arm.com>
> >>>>>
> >>>>>
> >>>>
> >>>> Thanks.
> >>>>
> >>>>>
> >>>>> I was looking for why we need core code to do this, instead of updating the
> >>>>> arch's call sites. Your 'irqdesc: Add domain handlers for NMIs' patch (pointed
> >>>>> to from the cover letter) is the reason: core-code calls nmi_enter()/nmi_exit()
> >>>>> itself.
> >>>>>
> >>>>
> >>>> Yes, that's the main reason.
> >>>>
> >>> I wondered the same thing, but I don't understand the explanation :(
> >>>
> >>> Why can't we do a local_daif_mask() around the (very small) calls that
> >>> clear TGE instead?
> >>>
> >>
> >> That would protect against the pseudo-NMIs, but you can still get an
> >> SDEI at that point even with all daif bits set. Or did I misunderstand
> >> how SDEI works?
> >>
> > 
> > I don't know the details of SDEI.  From looking at this patch, the
> > logical conclusion would be that SDEIs can then only be delivered once
> > we've called nmi_enter, but since we don't call this directly from the
> > code that clears TGE for doing guest TLB invalidation (or do we?) then
> > masking interrupts at the PSTATE level should be sufficient.
> > 
> > Surely I'm missing some part of the bigger picture here.
> > 
> 
> I'm not sure I understand. SDEI uses the NMI context and AFAIU, it is an
> interrupt that the firmware sends to the OS, and it is sent regardless
> of the PSTATE at the OS EL.
> 
> So, the worrying part is:
> - Hyp clears TGE
> - Exception/interrupt taken to EL3
> - Firmware decides it's a good time to send an SDEI to the OS
> - SDEI handler (at EL2 for VHE) does nmi_enter()
> - SDEI handler needs to do cache invalidation or something with the
> EL2&0 translation regime but TGE is cleared
> 
> We don't expect the code that clears TGE to call nmi_enter().
> 

You do understand :)

I didn't understand that the SDEI handler calls nmi_enter() -- and to be
fair the commit message didn't really provide that link -- but it
makes perfect sense now.  I naively thought that SDEI had respected the
pstate bits setting before, and that this was becoming a problem with
the introduction of pseudo-NMIs, but I clearly came at this from the
wrong direction.


Thanks for the explanation!

    Christoffer
Marc Zyngier Jan. 31, 2019, 9:53 a.m. UTC | #9
On 31/01/2019 09:40, Julien Thierry wrote:
> 
> 
> On 31/01/2019 09:27, Christoffer Dall wrote:
>> On Thu, Jan 31, 2019 at 08:56:04AM +0000, Julien Thierry wrote:
>>>
>>>
>>> On 31/01/2019 08:19, Christoffer Dall wrote:
>>>> On Mon, Jan 28, 2019 at 03:42:42PM +0000, Julien Thierry wrote:
>>>>> Hi James,
>>>>>
>>>>> On 28/01/2019 11:48, James Morse wrote:
>>>>>> Hi Julien,
>>>>>>
>>>>>> On 21/01/2019 15:33, Julien Thierry wrote:
>>>>>>> When using VHE, the host needs to clear HCR_EL2.TGE bit in order
>>>>>>> to interract with guest TLBs, switching from EL2&0 translation regime
>>>>>>
>>>>>> (interact)
>>>>>>
>>>>>>
>>>>>>> to EL1&0.
>>>>>>>
>>>>>>> However, some non-maskable asynchronous event could happen while TGE is
>>>>>>> cleared like SDEI. Because of this address translation operations
>>>>>>> relying on EL2&0 translation regime could fail (tlb invalidation,
>>>>>>> userspace access, ...).
>>>>>>>
>>>>>>> Fix this by properly setting HCR_EL2.TGE when entering NMI context and
>>>>>>> clear it if necessary when returning to the interrupted context.
>>>>>>
>>>>>> Yes please. This would not have been fun to debug!
>>>>>>
>>>>>> Reviewed-by: James Morse <james.morse@arm.com>
>>>>>>
>>>>>>
>>>>>
>>>>> Thanks.
>>>>>
>>>>>>
>>>>>> I was looking for why we need core code to do this, instead of updating the
>>>>>> arch's call sites. Your 'irqdesc: Add domain handlers for NMIs' patch (pointed
>>>>>> to from the cover letter) is the reason: core-code calls nmi_enter()/nmi_exit()
>>>>>> itself.
>>>>>>
>>>>>
>>>>> Yes, that's the main reason.
>>>>>
>>>> I wondered the same thing, but I don't understand the explanation :(
>>>>
>>>> Why can't we do a local_daif_mask() around the (very small) calls that
>>>> clear TGE instead?
>>>>
>>>
>>> That would protect against the pseudo-NMIs, but you can still get an
>>> SDEI at that point even with all daif bits set. Or did I misunderstand
>>> how SDEI works?
>>>
>>
>> I don't know the details of SDEI.  From looking at this patch, the
>> logical conclusion would be that SDEIs can then only be delivered once
>> we've called nmi_enter, but since we don't call this directly from the
>> code that clears TGE for doing guest TLB invalidation (or do we?) then
>> masking interrupts at the PSTATE level should be sufficient.
>>
>> Surely I'm missing some part of the bigger picture here.
>>
> 
> I'm not sure I understand. SDEI uses the NMI context and AFAIU, it is an
> interrupt that the firmware sends to the OS, and it is sent regardless
> of the PSTATE at the OS EL.

I don't think we can describe SDEI as an interrupt. It is not even an
exception. It is just EL3 ERET-ing to a pre-defined location. And yes,
it will completely ignore any form of mask bit.

> 
> So, the worrying part is:
> - Hyp clears TGE
> - Exception/interrupt taken to EL3
> - Firmware decides it's a good time to send an SDEI to the OS
> - SDEI handler (at EL2 for VHE) does nmi_enter()
> - SDEI handler needs to do cache invalidation or something with the
> EL2&0 translation regime but TGE is cleared
> 
> We don't expect the code that clears TGE to call nmi_enter().

Indeed. Without this patch, SDEI is already broken. Pseudo-NMIs only
make the bug easier to trigger.

Thanks,

	M.
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/hardirq.h b/arch/arm64/include/asm/hardirq.h
index 1473fc2..94b7481 100644
--- a/arch/arm64/include/asm/hardirq.h
+++ b/arch/arm64/include/asm/hardirq.h
@@ -19,6 +19,7 @@ 
 #include <linux/cache.h>
 #include <linux/threads.h>
 #include <asm/irq.h>
+#include <asm/kvm_arm.h>
 
 #define NR_IPI	7
 
@@ -37,6 +38,33 @@ 
 
 #define __ARCH_IRQ_EXIT_IRQS_DISABLED	1
 
+struct nmi_ctx {
+	u64 hcr;
+};
+
+DECLARE_PER_CPU(struct nmi_ctx, nmi_contexts);
+
+#define arch_nmi_enter()							\
+	do {									\
+		if (is_kernel_in_hyp_mode()) {					\
+			struct nmi_ctx *nmi_ctx = this_cpu_ptr(&nmi_contexts);	\
+			nmi_ctx->hcr = read_sysreg(hcr_el2);			\
+			if (!(nmi_ctx->hcr & HCR_TGE)) {			\
+				write_sysreg(nmi_ctx->hcr | HCR_TGE, hcr_el2);	\
+				isb();						\
+			}							\
+		}								\
+	} while (0)
+
+#define arch_nmi_exit()								\
+	do {									\
+		if (is_kernel_in_hyp_mode()) {					\
+			struct nmi_ctx *nmi_ctx = this_cpu_ptr(&nmi_contexts);	\
+			if (!(nmi_ctx->hcr & HCR_TGE))				\
+				write_sysreg(nmi_ctx->hcr, hcr_el2);		\
+		}								\
+	} while (0)
+
 static inline void ack_bad_irq(unsigned int irq)
 {
 	extern unsigned long irq_err_count;
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 780a12f..92fa817 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -33,6 +33,9 @@ 
 
 unsigned long irq_err_count;
 
+/* Only access this in an NMI enter/exit */
+DEFINE_PER_CPU(struct nmi_ctx, nmi_contexts);
+
 DEFINE_PER_CPU(unsigned long *, irq_stack_ptr);
 
 int arch_show_interrupts(struct seq_file *p, int prec)
diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h
index 0fbbcdf..da0af63 100644
--- a/include/linux/hardirq.h
+++ b/include/linux/hardirq.h
@@ -60,8 +60,14 @@  static inline void rcu_nmi_exit(void)
  */
 extern void irq_exit(void);
 
+#ifndef arch_nmi_enter
+#define arch_nmi_enter()	do { } while (0)
+#define arch_nmi_exit()		do { } while (0)
+#endif
+
 #define nmi_enter()						\
 	do {							\
+		arch_nmi_enter();				\
 		printk_nmi_enter();				\
 		lockdep_off();					\
 		ftrace_nmi_enter();				\
@@ -80,6 +86,7 @@  static inline void rcu_nmi_exit(void)
 		ftrace_nmi_exit();				\
 		lockdep_on();					\
 		printk_nmi_exit();				\
+		arch_nmi_exit();				\
 	} while (0)
 
 #endif /* LINUX_HARDIRQ_H */