Re: [PATCH] arm64/apei: Filter the apei GAS address from the ECAM space

Re: [PATCH] arm64/apei: Filter the apei GAS address from the ECAM space

[Lorenzo Pieralisi]

[+linux-acpi, Rafael, James, Tony, Borislav]

On Wed, Sep 08, 2021 at 03:02:53PM +0800, Xuesong Chen wrote:
> ECAM configuration memory space is reserved during the initializaion of the
> kernel, but sometimes the apei needs to access the GAS address within the
> ECAM space, thus the request_mem_region(...) will be failed in this case,
> the error message looks like:
> ...
> APEI: Can not request [mem 0x50100000-0x50100003] for APEI EINJ Trigger registers
> ...
> 
> This patch provides an arm64 specific filter function to remove the GAS address
> range from the reserved ECAM resource regions, which will make the apei's GAS
> address can pass the check while not affecting the original reserved ECAM area.
> 
> Signed-off-by: Xuesong Chen <xuesong.chen@linux.alibaba.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Steve Capper <steve.capper@arm.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
>  arch/arm64/kernel/pci.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 1006ed2..6096165 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -19,6 +19,13 @@
>  #include <linux/slab.h>
>  
>  #ifdef CONFIG_ACPI
> +struct pci_mcfg_res_region {
> +	struct list_head list;
> +	struct resource res;
> +};
> +
> +static LIST_HEAD(pci_mcfg_res_list);
> +static DEFINE_MUTEX(pci_mcfg_lock);
>  /*
>   * Try to assign the IRQ number when probing a new device
>   */
> @@ -107,6 +114,58 @@ static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
>  	return status;
>  }
>  
> +#ifdef CONFIG_ACPI_APEI
> +extern int (*arch_apei_filter_addr)(int (*func)(__u64 start, __u64 size,
> +		void *data), void *data);
> +
> +static int pci_mcfg_for_each_region(int (*func)(__u64 start, __u64 size,
> +		void *data), void *data)
> +{
> +	struct pci_mcfg_res_region *e;
> +	int rc = 0;
> +
> +	if (list_empty(&pci_mcfg_res_list))
> +		return 0;
> +
> +	list_for_each_entry(e, &pci_mcfg_res_list, list) {
> +		rc = func(e->res.start, resource_size(&e->res), data);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	return 0;
> +}
> +
> +#define set_apei_filter() \
> +	do { \
> +		if (!arch_apei_filter_addr) \
> +			arch_apei_filter_addr = pci_mcfg_for_each_region; \
> +	} while (0)
> +#else
> +#define set_apei_filter()
> +#endif
> +
> +static int pci_mcfg_res_add(struct resource *res)
> +{
> +	struct pci_mcfg_res_region *new;
> +
> +	if (!res)
> +		return -EINVAL;
> +
> +	new = kzalloc(sizeof(*new), GFP_KERNEL);
> +	if (!new)
> +		return -ENOMEM;
> +
> +	new->res.start = res->start;
> +	new->res.end = res->end;
> +
> +	mutex_lock(&pci_mcfg_lock);
> +	list_add(&new->list, &pci_mcfg_res_list);
> +	mutex_unlock(&pci_mcfg_lock);
> +
> +	return 0;
> +}
> +
>  /*
>   * Lookup the bus range for the domain in MCFG, and set up config space
>   * mapping.
> @@ -144,6 +203,11 @@ static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
>  		return NULL;
>  	}
>  
> +	/* insert the mcfg resource region of current segment into the list */
> +	ret = pci_mcfg_res_add(&cfgres);
> +	if (ret)
> +		dev_warn(dev, "add %pR into the mcfg res list failed\n", &cfgres);
> +
>  	return cfg;
>  }
>  
> @@ -204,6 +268,8 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>  	list_for_each_entry(child, &bus->children, node)
>  		pcie_bus_configure_settings(child);
>  
> +	set_apei_filter();
> +
>  	return bus;
>  }

This is an x86 code copy and paste. IIUC, the "arch" resources filtering
(that isn't arch specific at all) was introduced in:

commit d91525eb8ee6 ("ACPI, EINJ: Enhance error injection tolerance
level")

I don't believe though that there is anything arch specific in the
apei_get_arch_resources() implementation, after all what x86 does
is filtering out MCFG regions, with the arch_apei_filter_add(*) that
is not arch specific anyway (ie pci_mmcfg_for_each_region() on x86).

Should we take this opportunity to remove the arch hooks and just
filter out MCFG resources directly in APEI code (because that's what
the arch specific hook does _anyway_) ?

I see no reason to duplicate this for arm64, please let me know
if I am missing something.

Thanks,
Lorenzo

Re: [PATCH] arm64/apei: Filter the apei GAS address from the ECAM space

[Xuesong Chen]

Hello Lorenzo,

On 29/09/2021 20:59, Lorenzo Pieralisi wrote:
> [+linux-acpi, Rafael, James, Tony, Borislav]
> 
> On Wed, Sep 08, 2021 at 03:02:53PM +0800, Xuesong Chen wrote:
>> ECAM configuration memory space is reserved during the initializaion of the
>> kernel, but sometimes the apei needs to access the GAS address within the
>> ECAM space, thus the request_mem_region(...) will be failed in this case,
>> the error message looks like:
>> ...
>> APEI: Can not request [mem 0x50100000-0x50100003] for APEI EINJ Trigger registers
>> ...
>>
>> This patch provides an arm64 specific filter function to remove the GAS address
>> range from the reserved ECAM resource regions, which will make the apei's GAS
>> address can pass the check while not affecting the original reserved ECAM area.
>>
>> Signed-off-by: Xuesong Chen <xuesong.chen@linux.alibaba.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Steve Capper <steve.capper@arm.com>
>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> ---
>>   arch/arm64/kernel/pci.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 66 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> index 1006ed2..6096165 100644
>> --- a/arch/arm64/kernel/pci.c
>> +++ b/arch/arm64/kernel/pci.c
>> @@ -19,6 +19,13 @@
>>   #include <linux/slab.h>
>>   
>>   #ifdef CONFIG_ACPI
>> +struct pci_mcfg_res_region {
>> +	struct list_head list;
>> +	struct resource res;
>> +};
>> +
>> +static LIST_HEAD(pci_mcfg_res_list);
>> +static DEFINE_MUTEX(pci_mcfg_lock);
>>   /*
>>    * Try to assign the IRQ number when probing a new device
>>    */
>> @@ -107,6 +114,58 @@ static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
>>   	return status;
>>   }
>>   
>> +#ifdef CONFIG_ACPI_APEI
>> +extern int (*arch_apei_filter_addr)(int (*func)(__u64 start, __u64 size,
>> +		void *data), void *data);
>> +
>> +static int pci_mcfg_for_each_region(int (*func)(__u64 start, __u64 size,
>> +		void *data), void *data)
>> +{
>> +	struct pci_mcfg_res_region *e;
>> +	int rc = 0;
>> +
>> +	if (list_empty(&pci_mcfg_res_list))
>> +		return 0;
>> +
>> +	list_for_each_entry(e, &pci_mcfg_res_list, list) {
>> +		rc = func(e->res.start, resource_size(&e->res), data);
>> +		if (rc)
>> +			return rc;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +#define set_apei_filter() \
>> +	do { \
>> +		if (!arch_apei_filter_addr) \
>> +			arch_apei_filter_addr = pci_mcfg_for_each_region; \
>> +	} while (0)
>> +#else
>> +#define set_apei_filter()
>> +#endif
>> +
>> +static int pci_mcfg_res_add(struct resource *res)
>> +{
>> +	struct pci_mcfg_res_region *new;
>> +
>> +	if (!res)
>> +		return -EINVAL;
>> +
>> +	new = kzalloc(sizeof(*new), GFP_KERNEL);
>> +	if (!new)
>> +		return -ENOMEM;
>> +
>> +	new->res.start = res->start;
>> +	new->res.end = res->end;
>> +
>> +	mutex_lock(&pci_mcfg_lock);
>> +	list_add(&new->list, &pci_mcfg_res_list);
>> +	mutex_unlock(&pci_mcfg_lock);
>> +
>> +	return 0;
>> +}
>> +
>>   /*
>>    * Lookup the bus range for the domain in MCFG, and set up config space
>>    * mapping.
>> @@ -144,6 +203,11 @@ static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
>>   		return NULL;
>>   	}
>>   
>> +	/* insert the mcfg resource region of current segment into the list */
>> +	ret = pci_mcfg_res_add(&cfgres);
>> +	if (ret)
>> +		dev_warn(dev, "add %pR into the mcfg res list failed\n", &cfgres);
>> +
>>   	return cfg;
>>   }
>>   
>> @@ -204,6 +268,8 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>>   	list_for_each_entry(child, &bus->children, node)
>>   		pcie_bus_configure_settings(child);
>>   
>> +	set_apei_filter();
>> +
>>   	return bus;
>>   }
> 
> This is an x86 code copy and paste. IIUC, the "arch" resources filtering
> (that isn't arch specific at all) was introduced in:
> 
> commit d91525eb8ee6 ("ACPI, EINJ: Enhance error injection tolerance
> level")
> 
> I don't believe though that there is anything arch specific in the
> apei_get_arch_resources() implementation, after all what x86 does
> is filtering out MCFG regions, with the arch_apei_filter_add(*) that
> is not arch specific anyway (ie pci_mmcfg_for_each_region() on x86).
> 
I totally agree with you that there's no arch specific code in filtering
out the MCFG regions, IMO the root cause is that the x86 and arm64 have
similar but separate MCFG code path: x86 uses 'pci_mmcfg_list' list to
link mmcfg region instance, while the standard acpi code uses the 
'pci_mcfg_list' to link the mcfg entry.

> Should we take this opportunity to remove the arch hooks and just
> filter out MCFG resources directly in APEI code (because that's what
> the arch specific hook does _anyway_) ?
> 
> I see no reason to duplicate this for arm64, please let me know
> if I am missing something.

I think we can make it gradually, the first step is to consolidate the 
'pci_mcfg_list' for all the arches, then the arch specific hook can be 
removed after that. Let me try to figure it out and send out another 
patch soon...


Thanks,
Xuesong
> 
> Thanks,
> Lorenzo
>