Hi Marc,
On 26/03/18 18:49, Marc Zyngier wrote:
> On 22/03/18 18:14, James Morse wrote:
>> To ensure APEI always takes the same locks when processing a notification
>> we need the nmi-like callers to always call APEI 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 choose between the raw/regular spinlock routines.
>>
>> 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/include/asm/kvm_ras.h b/arch/arm64/include/asm/kvm_ras.h
>> index 5f72b07b7912..9d52bc333110 100644
>> --- a/arch/arm64/include/asm/kvm_ras.h
>> +++ b/arch/arm64/include/asm/kvm_ras.h
>> @@ -4,8 +4,26 @@
>> #ifndef __ARM64_KVM_RAS_H__
>> #define __ARM64_KVM_RAS_H__
>>
>> +#include <linux/acpi.h>
>> +#include <linux/errno.h>
>> #include <linux/types.h>
>>
>> -int kvm_handle_guest_sea(phys_addr_t addr, unsigned int esr);
>> +#include <asm/acpi.h>
>> +
>> +/*
>> + * Was this synchronous external abort a RAS notification?
>> + * Returns '0' for errors handled by some RAS subsystem, or -ENOENT.
>> + *
>> + * Call with irqs unmaksed.
Self-Nit: unmasked.
>> + */
>> +static inline int kvm_handle_guest_sea(phys_addr_t addr, unsigned int esr)
>> +{
>> + int ret = -ENOENT;
>> +
>> + if (IS_ENABLED(CONFIG_ACPI_APEI_SEA))
>> + ret = apei_claim_sea(NULL);
>
> Nit: it is a bit odd to see this "IS_ENABLED(CONFIG_ACPI_APEI_SEA)"
> check both in this function and in the only other function this calls
> (apei_claim_sea). Could this somehow be improved by having a dummy
> apei_claim_sea if CONFIG_ACPI_APEI doesn't exist?
Good point. Your suggestion also avoids more #ifdefs in the C file, which is
what I was trying to avoid.
>> +
>> + return ret;
>> +}
>>
>> #endif /* __ARM64_KVM_RAS_H__ */
> Otherwise:
>
> Acked-by: Marc Zyngier <marc.zyngier@xxxxxxx>
Thanks!
James
