Re: [RFC 1/2] ACPI, APEI, EINJ, Fix resource conflict on some machine

[Chen Gong <gong.chen@linux.intel.com>]

于 2011/8/30 14:28, Huang Ying 写道:
> Some APEI firmware implementaiton will access injected address
> specified in param1 to trigger the error when injecting memory error.
> This will cause resource conflict with RAM.  So remove it from trigger
> table resources to avoid conflict.
>
> Signed-off-by: Huang Ying<ying.huang@intel.com>
> Cc: Tony Luck<tony.luck@intel.com>
> ---
>   drivers/acpi/apei/apei-base.c     |   11 +++++++++++
>   drivers/acpi/apei/apei-internal.h |    3 +++
>   drivers/acpi/apei/einj.c          |   24 ++++++++++++++++++++++--
>   3 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
> index ac11aff..35a7a3a 100644
> --- a/drivers/acpi/apei/apei-base.c
> +++ b/drivers/acpi/apei/apei-base.c
> @@ -421,6 +421,17 @@ static int apei_resources_merge(struct apei_resources *resources1,
>   	return 0;
>   }
>
> +int apei_resources_add(struct apei_resources *resources,
> +		       unsigned long start, unsigned long size,
> +		       bool iomem)
> +{
> +	if (iomem)
> +		return apei_res_add(&resources->iomem, start, size);
> +	else
> +		return apei_res_add(&resources->ioport, start, size);
> +}
> +EXPORT_SYMBOL_GPL(apei_resources_add);
> +
>   /*
>    * EINJ has two groups of GARs (EINJ table entry and trigger table
>    * entry), so common resources are subtracted from the trigger table
> diff --git a/drivers/acpi/apei/apei-internal.h b/drivers/acpi/apei/apei-internal.h
> index f57050e..d778edd 100644
> --- a/drivers/acpi/apei/apei-internal.h
> +++ b/drivers/acpi/apei/apei-internal.h
> @@ -95,6 +95,9 @@ static inline void apei_resources_init(struct apei_resources *resources)
>   }
>
>   void apei_resources_fini(struct apei_resources *resources);
> +int apei_resources_add(struct apei_resources *resources,
> +		       unsigned long start, unsigned long size,
> +		       bool iomem);
>   int apei_resources_sub(struct apei_resources *resources1,
>   		       struct apei_resources *resources2);
>   int apei_resources_request(struct apei_resources *resources,
> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
> index 3178521..a126c67 100644
> --- a/drivers/acpi/apei/einj.c
> +++ b/drivers/acpi/apei/einj.c
> @@ -195,7 +195,8 @@ static int einj_check_trigger_header(struct acpi_einj_trigger *trigger_tab)
>   }
>
>   /* Execute instructions in trigger error action table */
> -static int __einj_error_trigger(u64 trigger_paddr)
> +static int __einj_error_trigger(u64 trigger_paddr, u32 type,
> +				u64 param1, u64 param2)
>   {
>   	struct acpi_einj_trigger *trigger_tab = NULL;
>   	struct apei_exec_context trigger_ctx;
> @@ -255,6 +256,25 @@ static int __einj_error_trigger(u64 trigger_paddr)
>   	rc = apei_resources_sub(&trigger_resources,&einj_resources);
>   	if (rc)
>   		goto out_fini;
> +	/*
> +	 * Some firmware will access target address specified in
> +	 * param1 to trigger the error when injecting memory error.
> +	 * This will cause resource conflict with regular memory.  So
> +	 * remove it from trigger table resources.
> +	 */
> +	if (param_extension&&  (type&  0x0038)&&  param2) {
> +		struct apei_resources addr_resources;
> +		apei_resources_init(&addr_resources);
> +		rc = apei_resources_add(&addr_resources,
> +					param1&  param2,
> +					~param2 + 1, true);

assuming following scenario:
param1: 0x827809000
param2: 0xffffffffffffffff
after above operation, only 1 byte will be added and be subtracted by
apei_resources_sub below, which means if 8 bytes are necessary to be
excluded from ioremap, finally only 1 byte is excluded, left 7 bytes
still be mapped via ioremap. The same thing will happen:

APEI: Can not request iomem region <00000000bf7b522a-00000000bf7b522c> 
for GARs

We can't control the value of param2 here because 1) it is read from
the user; 2) the param1 is already aligned, the value of param2 is not
important under this kind of situation.

We have hit above error in our tests.

> +		if (rc)
> +			goto out_fini;
> +		rc = apei_resources_sub(&trigger_resources,&addr_resources);
> +		apei_resources_fini(&addr_resources);
> +		if (rc)
> +			goto out_fini;
> +	}
>   	rc = apei_resources_request(&trigger_resources, "APEI EINJ Trigger");
>   	if (rc)
>   		goto out_fini;
> @@ -324,7 +344,7 @@ static int __einj_error_inject(u32 type, u64 param1, u64 param2)
>   	if (rc)
>   		return rc;
>   	trigger_paddr = apei_exec_ctx_get_output(&ctx);
> -	rc = __einj_error_trigger(trigger_paddr);
> +	rc = __einj_error_trigger(trigger_paddr, type, param1, param2);
>   	if (rc)
>   		return rc;
>   	rc = apei_exec_run_optional(&ctx, ACPI_EINJ_END_OPERATION);