Re: [PATCH v6 07/18] arm64: KVM/mm: Move SEA handling behind a single 'claim' interface
Hi Boris,
On 12/10/2018 11:02, Borislav Petkov wrote:
> On Fri, Sep 21, 2018 at 11:16:54PM +0100, James Morse wrote:
>> To split up APEIs in_nmi() path, we need the nmi-like callers to always
>> be in_nmi(). Add a helper to do the work and claim the notification.
>>
>> When KVM or the arch code takes an exception that might be a RAS
>> notification, it asks the APEI firmware-first code whether it wants
>> to claim the exception. We can then go on to see if (a future)
>> kernel-first mechanism wants to claim the notification, before
>> falling through to the existing default behaviour.
>>
>> The NOTIFY_SEA code was merged before we had multiple, possibly
>> interacting, NMI-like notifications and the need to consider kernel
>> first in the future. Make the 'claiming' behaviour explicit.
>>
>> As we're restructuring the APEI code to allow multiple NMI-like
>> notifications, any notification that might interrupt interrupts-masked
>> code must always be wrapped in nmi_enter()/nmi_exit(). This allows APEI
>> to use in_nmi() to use the right fixmap entries.
>>
>> We mask SError over this window to prevent an asynchronous RAS error
>> arriving and tripping 'nmi_enter()'s BUG_ON(in_nmi()).
>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
>> index ed46dc188b22..a9b8bba014b5 100644
>> --- a/arch/arm64/kernel/acpi.c
>> +++ b/arch/arm64/kernel/acpi.c
>> @@ -257,3 +259,30 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr)
>> return __pgprot(PROT_NORMAL_NC);
>> return __pgprot(PROT_DEVICE_nGnRnE);
>> }
>> +
>> +/*
>> + * Claim Synchronous External Aborts as a firmware first notification.
>> + *
>> + * Used by KVM and the arch do_sea handler.
>> + * @regs may be NULL when called from process context.
>> + */
>> +int apei_claim_sea(struct pt_regs *regs)
>> +{
>> +int err = -ENOENT;
>> +unsigned long current_flags = arch_local_save_flags();
>> +
>> +if (!IS_ENABLED(CONFIG_ACPI_APEI_SEA))
>> +return err;
>
> I don't know what side effects arch_local_save_flags() has on ARM but if
It reads the current 'masked' state for IRQs, debug exceptions and 'SError'.
> we return here, it looks to me like useless work.
Yes. I lazily assume the compiler will rip that out as the value is never used.
But in this case it can't, because its wrapped in asm-volatile, so it doesn't
know it has no side-effects.
I'll move it further down.
Thanks!
James
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v6 07/18] arm64: KVM/mm: Move SEA handling behind a single 'claim' interface
On Fri, Sep 21, 2018 at 11:16:54PM +0100, James Morse wrote:
> To split up APEIs in_nmi() path, we need the nmi-like callers to always
> be in_nmi(). Add a helper to do the work and claim the notification.
>
> When KVM or the arch code takes an exception that might be a RAS
> notification, it asks the APEI firmware-first code whether it wants
> to claim the exception. We can then go on to see if (a future)
> kernel-first mechanism wants to claim the notification, before
> falling through to the existing default behaviour.
>
> The NOTIFY_SEA code was merged before we had multiple, possibly
> interacting, NMI-like notifications and the need to consider kernel
> first in the future. Make the 'claiming' behaviour explicit.
>
> As we're restructuring the APEI code to allow multiple NMI-like
> notifications, any notification that might interrupt interrupts-masked
> code must always be wrapped in nmi_enter()/nmi_exit(). This allows APEI
> to use in_nmi() to use the right fixmap entries.
>
> We mask SError over this window to prevent an asynchronous RAS error
> arriving and tripping 'nmi_enter()'s BUG_ON(in_nmi()).
>
> Signed-off-by: James Morse
> Acked-by: Marc Zyngier
> Tested-by: Tyler Baicar
...
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index ed46dc188b22..a9b8bba014b5 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -28,8 +28,10 @@
> #include
> #include
>
> +#include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -257,3 +259,30 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr)
> return __pgprot(PROT_NORMAL_NC);
> return __pgprot(PROT_DEVICE_nGnRnE);
> }
> +
> +/*
> + * Claim Synchronous External Aborts as a firmware first notification.
> + *
> + * Used by KVM and the arch do_sea handler.
> + * @regs may be NULL when called from process context.
> + */
> +int apei_claim_sea(struct pt_regs *regs)
> +{
> + int err = -ENOENT;
> + unsigned long current_flags = arch_local_save_flags();
> +
> + if (!IS_ENABLED(CONFIG_ACPI_APEI_SEA))
> + return err;
I don't know what side effects arch_local_save_flags() has on ARM but if
we return here, it looks to me like useless work.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH v6 07/18] arm64: KVM/mm: Move SEA handling behind a single 'claim' interface
To split up APEIs in_nmi() path, we need the nmi-like callers to always
be in_nmi(). Add a helper to do the work and claim the notification.
When KVM or the arch code takes an exception that might be a RAS
notification, it asks the APEI firmware-first code whether it wants
to claim the exception. We can then go on to see if (a future)
kernel-first mechanism wants to claim the notification, before
falling through to the existing default behaviour.
The NOTIFY_SEA code was merged before we had multiple, possibly
interacting, NMI-like notifications and the need to consider kernel
first in the future. Make the 'claiming' behaviour explicit.
As we're restructuring the APEI code to allow multiple NMI-like
notifications, any notification that might interrupt interrupts-masked
code must always be wrapped in nmi_enter()/nmi_exit(). This allows APEI
to use in_nmi() to use the right fixmap entries.
We mask SError over this window to prevent an asynchronous RAS error
arriving and tripping 'nmi_enter()'s BUG_ON(in_nmi()).
Signed-off-by: James Morse
Acked-by: Marc Zyngier
Tested-by: Tyler Baicar
---
Why does apei_claim_sea() take a pt_regs? This gets used later to take
APEI by the hand through NMI->IRQ context, depending on what we
interrupted.
Changes since v4:
* Made irqs-unmasked comment a lockdep assert.
Changes since v3:
* Removed spurious whitespace change
* Updated comment in acpi.c to cover SError masking
Changes since v2:
* Added dummy definition for !ACPI and culled IS_ENABLED() checks.
squash: make 'call with irqs unmaksed' a lockdep assert, much better
---
arch/arm64/include/asm/acpi.h | 4
arch/arm64/include/asm/daifflags.h | 1 +
arch/arm64/include/asm/kvm_ras.h | 16 +++-
arch/arm64/kernel/acpi.c | 29 +
arch/arm64/mm/fault.c | 24 +---
5 files changed, 54 insertions(+), 20 deletions(-)
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 709208dfdc8b..f722d2d6bf2b 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -18,6 +18,7 @@
#include
#include
+#include
#include
#include
@@ -139,6 +140,9 @@ static inline pgprot_t
arch_apei_get_mem_attribute(phys_addr_t addr)
{
return __acpi_get_mem_attribute(addr);
}
+int apei_claim_sea(struct pt_regs *regs);
+#else
+static inline int apei_claim_sea(struct pt_regs *regs) { return -ENOENT; }
#endif /* CONFIG_ACPI_APEI */
#ifdef CONFIG_ACPI_NUMA
diff --git a/arch/arm64/include/asm/daifflags.h
b/arch/arm64/include/asm/daifflags.h
index 22e4c83de5a5..cbd753855bf3 100644
--- a/arch/arm64/include/asm/daifflags.h
+++ b/arch/arm64/include/asm/daifflags.h
@@ -20,6 +20,7 @@
#define DAIF_PROCCTX 0
#define DAIF_PROCCTX_NOIRQ PSR_I_BIT
+#define DAIF_ERRCTX(PSR_I_BIT | PSR_A_BIT)
/* mask/save/unmask/restore all exceptions, including interrupts. */
static inline void local_daif_mask(void)
diff --git a/arch/arm64/include/asm/kvm_ras.h b/arch/arm64/include/asm/kvm_ras.h
index 5f72b07b7912..5b56e7e297b1 100644
--- a/arch/arm64/include/asm/kvm_ras.h
+++ b/arch/arm64/include/asm/kvm_ras.h
@@ -4,8 +4,22 @@
#ifndef __ARM64_KVM_RAS_H__
#define __ARM64_KVM_RAS_H__
+#include
+#include
#include
-int kvm_handle_guest_sea(phys_addr_t addr, unsigned int esr);
+#include
+
+/*
+ * Was this synchronous external abort a RAS notification?
+ * Returns '0' for errors handled by some RAS subsystem, or -ENOENT.
+ */
+static inline int kvm_handle_guest_sea(phys_addr_t addr, unsigned int esr)
+{
+ /* apei_claim_sea(NULL) expects to mask interrupts itself */
+ lockdep_assert_irqs_enabled();
+
+ return apei_claim_sea(NULL);
+}
#endif /* __ARM64_KVM_RAS_H__ */
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index ed46dc188b22..a9b8bba014b5 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -28,8 +28,10 @@
#include
#include
+#include
#include
#include
+#include
#include
#include
@@ -257,3 +259,30 @@ pgprot_t __acpi_get_mem_attribute(phys_addr_t addr)
return __pgprot(PROT_NORMAL_NC);
return __pgprot(PROT_DEVICE_nGnRnE);
}
+
+/*
+ * Claim Synchronous External Aborts as a firmware first notification.
+ *
+ * Used by KVM and the arch do_sea handler.
+ * @regs may be NULL when called from process context.
+ */
+int apei_claim_sea(struct pt_regs *regs)
+{
+ int err = -ENOENT;
+ unsigned long current_flags = arch_local_save_flags();
+
+ if (!IS_ENABLED(CONFIG_ACPI_APEI_SEA))
+ return err;
+
+ /*
+* SEA can interrupt SError, mask it and describe this as an NMI so
+* that APEI defers the handling.
+*/
+ local_daif_restore(DAIF_ERRCTX);
+ nmi_enter();
+ err = ghes_notify_sea();
+ nmi_exit();
+ local_daif_restore(current_flags);
+
+ return err;
+}
diff --git a/arch/arm64/mm/fault.c
