Re: [PATCH v5 2/2] acpi: apei: Add SEI notification type support for ARMv8

[gengdongjiu]

Hi james,

On 2017/10/18 18:26, James Morse wrote:
> Hi Dongjiu Geng,
> 
> On 17/10/17 09:02, Dongjiu Geng wrote:
>> ARMv8.2 requires implementation of the RAS extension, in
>> this extension it adds SEI(SError Interrupt) notification
>> type, this patch adds new GHES error source SEI handling
>> functions.
> 
> This paragraph is merging two things that aren't related.
> The 'ARM v8.2 architecture extensions' have some RAS bits, which if your CPU
> implements v8.2 are required.
> 
> ACPIv6.1 added NOTIFY_SEI as a notification type for ARMv8 systems.
> 
> This patch adds a GHES function for NOTIFY_SEI. Please leave the CPU RAS
> extensions out of it.
Ok, thanks

> 
> 
>> Because this error source parsing and handling
>> methods are similar with the SEA. So share some SEA handling
>> functions with the SEI
>>
>> Expose one API ghes_notify_abort() to external users. External
>> modules can call this exposed API to parse and handle the
>> SEA or SEI.
> 
> This series doesn't add a caller/user for this new API, so why do we need to do
> this now?
 there is caller and user, it is in another series(RAS virtualization series), not included in this series

As shown:

+int handle_guest_sei(unsigned int esr)
+{
+	int ret = -ENOENT;
+
+	if (IS_ENABLED(CONFIG_ACPI_APEI_SEI))
+		ret = ghes_notify_abort(ACPI_HEST_NOTIFY_SEI);
+
+	return ret;
+}

> 
> (I still haven't had a usable answer for 'what does your firmware do when SError
> is masked', but I'll go beat that drum on the other thread).
sorry for my late response due to resent busy, I will answer your question in another thread.

May be tomorrow.


in short, regardless the physical SError is masked or unmasked, firmware will jump to
the corresponding SEA/SEI exception vector entry. there is only one PSTATE.DAIF which will be shared by different EL,
regardless EL1,EL2, EL3.

> 
> 
> More important for the APEI code is: How do SEA and SEI interact?
> 
> As far as I can see they can both interrupt each other, which isn't something
> the single in_nmi() path in APEI can handle. I thinks we should fix this first.
> (I'll try and polish my RFC that had a stab at that...)
if you have fix patch, you CC me. thanks.

> 
> 
> SEA gets away with a lot of things because its synchronous. SEI isn't. Xie XiuQi
> pointed to the memory_failure_queue() code. We can use this directly from SEA,
> but not SEI. (what happens if an SError arrives while we are queueing
> memory_failure work from an IRQ).
do you mean SError can interrupt memory_failure work from an IRQ?
memory_failure is in an process context, and in a work queue, not IRQ context.


> 
> The one that scares me is the trace-point reporting stuff. What happens if an
> SError arrives while we are enabling a trace point? (these are static-keys right?)
For the trace-point issue, may be we can consider it in the next step.
Now I am not consider the trace-point issue.


> 
> 
> I don't think we can just plumb SEI in like this and be done with it.
> (I'm looking at teasing out the estatus cache code from being x86:NMI only. This
> way we solve the same 'cant do this from NMI context' with the same code'.)
> 
> 
> Thanks,
> 
> James
> 
> 
> 
> boring nits below:
> 
>> Note: For the SEI(SError Interrupt), it is asynchronous external
>> abort, the error address recorded by firmware may be not accurate.
>> If not accurate, EL3 firmware needs to identify the address to a
>> invalid value.
> 
> This paragraph keeps cropping up. Who expects an address with an SError?
> We don't get one for IRQs, but that never needs stating.
> 
> 
>> Cc: Borislav Petkov <bp@xxxxxxx>
>> Cc: James Morse <james.morse@xxxxxxx>
>> Signed-off-by: Dongjiu Geng <gengdongjiu@xxxxxxxxxx>
>> Tested-by: Tyler Baicar <tbaicar@xxxxxxxxxxxxxx>
> 
>> Tested-by: Dongjiu Geng <gengdongjiu@xxxxxxxxxx>
> (It's expected you test your own code)

Ok

> 
> 
> 
>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>> index 2509e4f..c98c1b3 100644
>> --- a/arch/arm64/mm/fault.c
>> +++ b/arch/arm64/mm/fault.c
>> @@ -585,7 +585,7 @@ static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>>  		if (interrupts_enabled(regs))
>>  			nmi_enter();
>>  
>> -		ret = ghes_notify_sea();
>> +		ret = ghes_notify_abort(ACPI_HEST_NOTIFY_SEA);
>>  
>>  		if (interrupts_enabled(regs))
>>  			nmi_exit();
>> @@ -682,7 +682,7 @@ int handle_guest_sea(phys_addr_t addr, unsigned int esr)
>>  	int ret = -ENOENT;
>>  
>>  	if (IS_ENABLED(CONFIG_ACPI_APEI_SEA))
>> -		ret = ghes_notify_sea();
>> +		ret = ghes_notify_abort(ACPI_HEST_NOTIFY_SEA);
>>  
>>  	return ret;
>>  }
>> diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig
>> index de14d49..47fcb0c 100644
>> --- a/drivers/acpi/apei/Kconfig
>> +++ b/drivers/acpi/apei/Kconfig
>> @@ -54,6 +54,21 @@ config ACPI_APEI_SEA
>>  	  option allows the OS to look for such hardware error record, and
>>  	  take appropriate action.
>>  
>> +config ACPI_APEI_SEI
>> +	bool "APEI Asynchronous SError Interrupt logging/recovering support"
>> +	depends on ARM64 && ACPI_APEI_GHES
>> +	default y
>> +	help
>> +	  This option should be enabled if the system supports
>> +	  firmware first handling of SEI (asynchronous SError interrupt).
>> +
>> +	  SEI happens with asynchronous external abort for errors on device
>> +	  memory reads on ARMv8 systems. If a system supports firmware first
>> +	  handling of SEI, the platform analyzes and handles hardware error
>> +	  notifications from SEI, and it may then form a HW error record for
>> +	  the OS to parse and handle. This option allows the OS to look for
>> +	  such hardware error record, and take appropriate action.
>> +
>>  config ACPI_APEI_MEMORY_FAILURE
>>  	bool "APEI memory error recovering support"
>>  	depends on ACPI_APEI && MEMORY_FAILURE
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 3eee30a..24b4233 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -815,43 +815,67 @@ static struct notifier_block ghes_notifier_hed = {
>>  
>>  #ifdef CONFIG_ACPI_APEI_SEA
>>  static LIST_HEAD(ghes_sea);
>> +#endif
>> +
>> +#ifdef CONFIG_ACPI_APEI_SEI
>> +static LIST_HEAD(ghes_sei);
>> +#endif
>>  
>> +#if defined(CONFIG_ACPI_APEI_SEA) || defined(CONFIG_ACPI_APEI_SEI)
>>  /*
>> - * Return 0 only if one of the SEA error sources successfully reported an error
>> - * record sent from the firmware.
>> + * Return 0 only if one of the SEA or SEI error sources successfully
>> + * reported an error record sent from the firmware.
>>   */
>> -int ghes_notify_sea(void)
>> +int ghes_notify_abort(u8 type)
>>  {
>>  	struct ghes *ghes;
>> +	struct list_head *head = NULL;
>>  	int ret = -ENOENT;
>>  
>> -	rcu_read_lock();
>> -	list_for_each_entry_rcu(ghes, &ghes_sea, list) {
>> -		if (!ghes_proc(ghes))
>> -			ret = 0;
> 
>> +	if (type == ACPI_HEST_NOTIFY_SEA)
>> +		head = &ghes_sea;
>> +	else if (type == ACPI_HEST_NOTIFY_SEI)
>> +		head = &ghes_sei;
> 
> Surely if I only have one of CONFIG_ACPI_APEI_SE{A,I} this can't be compiled.
  No, it can be compiled, it is "||" not "&&"
> 
> 
>> +
>> +	if (head) {
>> +		rcu_read_lock();
>> +		list_for_each_entry_rcu(ghes, head, list) {
>> +			if (!ghes_proc(ghes))
>> +				ret = 0;
>> +		}
>> +		rcu_read_unlock();
>>  	}
>> -	rcu_read_unlock();
>>  	return ret;
>>  }
>>  
>> -static void ghes_sea_add(struct ghes *ghes)
>> +static void ghes_abort_add(struct ghes *ghes)
>>  {
>> -	mutex_lock(&ghes_list_mutex);
>> -	list_add_rcu(&ghes->list, &ghes_sea);
>> -	mutex_unlock(&ghes_list_mutex);
>> +	struct list_head *head = NULL;
>> +	u8 notify_type = ghes->generic->notify.type;
>> +
> 
>> +	if (notify_type == ACPI_HEST_NOTIFY_SEA)
>> +		head = &ghes_sea;
>> +	else if (notify_type == ACPI_HEST_NOTIFY_SEI)
>> +		head = &ghes_sei;
> 
> And here.
No, same above.

> 
> 
>> +
>> +	if (head) {
>> +		mutex_lock(&ghes_list_mutex);
>> +		list_add_rcu(&ghes->list, head);
>> +		mutex_unlock(&ghes_list_mutex);
>> +	}
>>  }
>>  
>> -static void ghes_sea_remove(struct ghes *ghes)
>> +static void ghes_abort_remove(struct ghes *ghes)
>>  {
>>  	mutex_lock(&ghes_list_mutex);
>>  	list_del_rcu(&ghes->list);
>>  	mutex_unlock(&ghes_list_mutex);
>>  	synchronize_rcu();
>>  }
>> -#else /* CONFIG_ACPI_APEI_SEA */
>> -static inline void ghes_sea_add(struct ghes *ghes) { }
>> -static inline void ghes_sea_remove(struct ghes *ghes) { }
>> -#endif /* CONFIG_ACPI_APEI_SEA */
>> +#else
>> +static inline void ghes_abort_add(struct ghes *ghes) { }
>> +static inline void ghes_abort_remove(struct ghes *ghes) { }
>> +#endif
>>  
>>  #ifdef CONFIG_HAVE_ACPI_APEI_NMI
>>  /*
>> @@ -1084,6 +1108,13 @@ static int ghes_probe(struct platform_device *ghes_dev)
>>  			goto err;
>>  		}
>>  		break;
>> +	case ACPI_HEST_NOTIFY_SEI:
>> +		if (!IS_ENABLED(CONFIG_ACPI_APEI_SEI)) {
>> +			pr_warn(GHES_PFX "Generic hardware error source: %d notified via SEI is not supported!\n",
>> +				generic->header.source_id);
>> +		goto err;
>> +	}
>> +	break;
>>  	case ACPI_HEST_NOTIFY_NMI:
>>  		if (!IS_ENABLED(CONFIG_HAVE_ACPI_APEI_NMI)) {
>>  			pr_warn(GHES_PFX "Generic hardware error source: %d notified via NMI interrupt is not supported!\n",
>> @@ -1153,7 +1184,8 @@ static int ghes_probe(struct platform_device *ghes_dev)
>>  		break;
>>  
>>  	case ACPI_HEST_NOTIFY_SEA:
>> -		ghes_sea_add(ghes);
>> +	case ACPI_HEST_NOTIFY_SEI:
>> +		ghes_abort_add(ghes);
>>  		break;
>>  	case ACPI_HEST_NOTIFY_NMI:
>>  		ghes_nmi_add(ghes);
>> @@ -1206,7 +1238,8 @@ static int ghes_remove(struct platform_device *ghes_dev)
>>  		break;
>>  
>>  	case ACPI_HEST_NOTIFY_SEA:
>> -		ghes_sea_remove(ghes);
>> +	case ACPI_HEST_NOTIFY_SEI:
>> +		ghes_abort_remove(ghes);
>>  		break;
>>  	case ACPI_HEST_NOTIFY_NMI:
>>  		ghes_nmi_remove(ghes);
>> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
>> index 9061c5c..ec6f4ba 100644
>> --- a/include/acpi/ghes.h
>> +++ b/include/acpi/ghes.h
>> @@ -118,6 +118,6 @@ static inline void *acpi_hest_get_next(struct acpi_hest_generic_data *gdata)
>>  	     (void *)section - (void *)(estatus + 1) < estatus->data_length; \
>>  	     section = acpi_hest_get_next(section))
>>  
>> -int ghes_notify_sea(void);
>> +int ghes_notify_abort(u8 type);
>>  
>>  #endif /* GHES_H */
>>
> 
> 
> .
>