[BUGFIX] ACPI, APEI, EINJ Param support is disabled by default

[BUGFIX] ACPI, APEI, EINJ Param support is disabled by default

[Huang Ying]

EINJ parameter support is only usable for some specific BIOS.
Originally, it is expected to have no harm for BIOS does not support
it.  But now, we found it will cause issue (memory overwriting) for
some BIOS.  So param support is disabled by default and only enabled
when newly added module parameter named "param_extension" is
explicitly specified.

Signed-off-by: Huang Ying <ying.huang@intel.com>
Cc: Matthew Garrett <mjg@redhat.com>
Acked-by: Don Zickus <dzickus@redhat.com>
Acked-by: Tony Luck <tony.luck@intel.com>
---
 Documentation/acpi/apei/einj.txt |   11 +++++++++--
 drivers/acpi/apei/einj.c         |   39 ++++++++++++++++++++++++---------------
 2 files changed, 33 insertions(+), 17 deletions(-)

--- a/drivers/acpi/apei/einj.c
+++ b/drivers/acpi/apei/einj.c
@@ -46,7 +46,8 @@
  * Some BIOSes allow parameters to the SET_ERROR_TYPE entries in the
  * EINJ table through an unpublished extension. Use with caution as
  * most will ignore the parameter and make their own choice of address
- * for error injection.
+ * for error injection.  This extension is used only if
+ * param_extension module parameter is specified.
  */
 struct einj_parameter {
 	u64 type;
@@ -65,6 +66,9 @@ struct einj_parameter {
 	((struct acpi_whea_header *)((char *)(tab) +			\
 				    sizeof(struct acpi_table_einj)))
 
+static bool param_extension;
+module_param(param_extension, bool, 0);
+
 static struct acpi_table_einj *einj_tab;
 
 static struct apei_resources einj_resources;
@@ -489,14 +493,6 @@ static int __init einj_init(void)
 				     einj_debug_dir, NULL, &error_type_fops);
 	if (!fentry)
 		goto err_cleanup;
-	fentry = debugfs_create_x64("param1", S_IRUSR | S_IWUSR,
-				    einj_debug_dir, &error_param1);
-	if (!fentry)
-		goto err_cleanup;
-	fentry = debugfs_create_x64("param2", S_IRUSR | S_IWUSR,
-				    einj_debug_dir, &error_param2);
-	if (!fentry)
-		goto err_cleanup;
 	fentry = debugfs_create_file("error_inject", S_IWUSR,
 				     einj_debug_dir, NULL, &error_inject_fops);
 	if (!fentry)
@@ -513,12 +509,23 @@ static int __init einj_init(void)
 	rc = apei_exec_pre_map_gars(&ctx);
 	if (rc)
 		goto err_release;
-	param_paddr = einj_get_parameter_address();
-	if (param_paddr) {
-		einj_param = ioremap(param_paddr, sizeof(*einj_param));
-		rc = -ENOMEM;
-		if (!einj_param)
-			goto err_unmap;
+	if (param_extension) {
+		param_paddr = einj_get_parameter_address();
+		if (param_paddr) {
+			einj_param = ioremap(param_paddr, sizeof(*einj_param));
+			rc = -ENOMEM;
+			if (!einj_param)
+				goto err_unmap;
+			fentry = debugfs_create_x64("param1", S_IRUSR | S_IWUSR,
+						    einj_debug_dir, &error_param1);
+			if (!fentry)
+				goto err_unmap;
+			fentry = debugfs_create_x64("param2", S_IRUSR | S_IWUSR,
+						    einj_debug_dir, &error_param2);
+			if (!fentry)
+				goto err_unmap;
+		} else
+			pr_warn(EINJ_PFX "Parameter extension is not supported.\n");
 	}
 
 	pr_info(EINJ_PFX "Error INJection is initialized.\n");
@@ -526,6 +533,8 @@ static int __init einj_init(void)
 	return 0;
 
 err_unmap:
+	if (einj_param)
+		iounmap(einj_param);
 	apei_exec_post_unmap_gars(&ctx);
 err_release:
 	apei_resources_release(&einj_resources);
--- a/Documentation/acpi/apei/einj.txt
+++ b/Documentation/acpi/apei/einj.txt
@@ -48,12 +48,19 @@ directory apei/einj. The following files
 - param1
   This file is used to set the first error parameter value. Effect of
   parameter depends on error_type specified. For memory error, this is
-  physical memory address.
+  physical memory address.  Only available if param_extension module
+  parameter is specified.
 
 - param2
   This file is used to set the second error parameter value. Effect of
   parameter depends on error_type specified. For memory error, this is
-  physical memory address mask.
+  physical memory address mask.  Only available if param_extension
+  module parameter is specified.
+
+Injecting parameter support is a BIOS version specific extension, that
+is, it only works on some BIOS version.  If you want to use it, please
+make sure your BIOS version has the proper support and specify
+"param_extension=y" in module parameter.
 
 For more information about EINJ, please refer to ACPI specification
 version 4.0, section 17.5.

Re: [BUGFIX] ACPI, APEI, EINJ Param support is disabled by default

[Bjorn Helgaas]

On Wed, Jul 20, 2011 at 2:09 AM, Huang Ying <ying.huang@intel.com> wrote:
> EINJ parameter support is only usable for some specific BIOS.
> Originally, it is expected to have no harm for BIOS does not support
> it.  But now, we found it will cause issue (memory overwriting) for
> some BIOS.  So param support is disabled by default and only enabled
> when newly added module parameter named "param_extension" is
> explicitly specified.

Adding parameters always makes things harder for users.  Is there any
way this could be done with a whitelist or other automatic mechanism?

Per 6e320ec1d98 (which added EINJ parameter support), parameters are
an unpublished extension and are not part of the ACPI spec.  So if we
pick up an MMIO address from a SET_ERROR_TYPE WRITE_REGISTER
instruction and blindly fill in a structure (undefined by the spec)
presumed to be at that address, it doesn't seem surprising that things
will blow up on BIOSes that don't expect that behavior.  After all,
the spec says to write to a generic address structure (not an
einj_parameter structure) when we interpret the WRITE_REGISTER
instruction (not at the magic time between SET_ERROR_TYPE and
EXECUTE_OPERATION).

If EINJ parameter support is ever added to the ACPI spec, I would
expect a new EINJ flag bit or similar indication to be added at the
same time, so the OS would know when to use it.

Can you add a whitelist of BIOSes that do support this extension?
Maybe you could define a GUID that future platforms could supply so
you wouldn't have to update the whitelist for every new platform that
supports it?

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUGFIX] ACPI, APEI, EINJ Param support is disabled by default

[Huang Ying]

On 07/20/2011 11:22 PM, Bjorn Helgaas wrote:
> On Wed, Jul 20, 2011 at 2:09 AM, Huang Ying <ying.huang@intel.com> wrote:
>> EINJ parameter support is only usable for some specific BIOS.
>> Originally, it is expected to have no harm for BIOS does not support
>> it.  But now, we found it will cause issue (memory overwriting) for
>> some BIOS.  So param support is disabled by default and only enabled
>> when newly added module parameter named "param_extension" is
>> explicitly specified.
> 
> Adding parameters always makes things harder for users.  Is there any
> way this could be done with a whitelist or other automatic mechanism?

The only user of EINJ is RAS developer/tester.  So we adopt a simpler
solution.

> Per 6e320ec1d98 (which added EINJ parameter support), parameters are
> an unpublished extension and are not part of the ACPI spec.  So if we
> pick up an MMIO address from a SET_ERROR_TYPE WRITE_REGISTER
> instruction and blindly fill in a structure (undefined by the spec)
> presumed to be at that address, it doesn't seem surprising that things
> will blow up on BIOSes that don't expect that behavior.  After all,
> the spec says to write to a generic address structure (not an
> einj_parameter structure) when we interpret the WRITE_REGISTER
> instruction (not at the magic time between SET_ERROR_TYPE and
> EXECUTE_OPERATION).
> 
> If EINJ parameter support is ever added to the ACPI spec, I would
> expect a new EINJ flag bit or similar indication to be added at the
> same time, so the OS would know when to use it.

EINJ parameter support has not been added to ACPI spec, it may be added
in the future, and should has some way to indicate whether parameter is
supported.

> Can you add a whitelist of BIOSes that do support this extension?
> Maybe you could define a GUID that future platforms could supply so
> you wouldn't have to update the whitelist for every new platform that
> supports it?

IMHO, the EINJ will not be used by the ordinary users.  If so, can we
use a simpler solution such as that in this patch?

Best Regards,
Huang Ying

Re: [BUGFIX] ACPI, APEI, EINJ Param support is disabled by default

[Bjorn Helgaas]

On Wed, Jul 20, 2011 at 7:02 PM, Huang Ying <ying.huang@intel.com> wrote:
> On 07/20/2011 11:22 PM, Bjorn Helgaas wrote:
>> On Wed, Jul 20, 2011 at 2:09 AM, Huang Ying <ying.huang@intel.com> wrote:
>>> EINJ parameter support is only usable for some specific BIOS.
>>> Originally, it is expected to have no harm for BIOS does not support
>>> it.  But now, we found it will cause issue (memory overwriting) for
>>> some BIOS.  So param support is disabled by default and only enabled
>>> when newly added module parameter named "param_extension" is
>>> explicitly specified.
>>
>> Adding parameters always makes things harder for users.  Is there any
>> way this could be done with a whitelist or other automatic mechanism?
>
> The only user of EINJ is RAS developer/tester.  So we adopt a simpler
> solution.
>
>> Per 6e320ec1d98 (which added EINJ parameter support), parameters are
>> an unpublished extension and are not part of the ACPI spec.  So if we
>> pick up an MMIO address from a SET_ERROR_TYPE WRITE_REGISTER
>> instruction and blindly fill in a structure (undefined by the spec)
>> presumed to be at that address, it doesn't seem surprising that things
>> will blow up on BIOSes that don't expect that behavior.  After all,
>> the spec says to write to a generic address structure (not an
>> einj_parameter structure) when we interpret the WRITE_REGISTER
>> instruction (not at the magic time between SET_ERROR_TYPE and
>> EXECUTE_OPERATION).
>>
>> If EINJ parameter support is ever added to the ACPI spec, I would
>> expect a new EINJ flag bit or similar indication to be added at the
>> same time, so the OS would know when to use it.
>
> EINJ parameter support has not been added to ACPI spec, it may be added
> in the future, and should has some way to indicate whether parameter is
> supported.
>
>> Can you add a whitelist of BIOSes that do support this extension?
>> Maybe you could define a GUID that future platforms could supply so
>> you wouldn't have to update the whitelist for every new platform that
>> supports it?
>
> IMHO, the EINJ will not be used by the ordinary users.  If so, can we
> use a simpler solution such as that in this patch?

It's not very clear to the code reader when this extension might be
useful.  It's a bit of folklore that will have to be passed around via
word-of-mouth.  If I want to modify the parameter code, I have no
clues about what sort of machine I'd need to test it.

What happens if you load this module on a machine that supports the
extension, but you don't supply the module parameter?  I would expect
problems in that case, too, because the BIOS is expecting parameters
and we won't fill them in.

But maybe you're right that this is so specialized that we shouldn't
over-engineer it.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUGFIX] ACPI, APEI, EINJ Param support is disabled by default

[Huang Ying]

On 07/21/2011 12:46 PM, Bjorn Helgaas wrote:
> On Wed, Jul 20, 2011 at 7:02 PM, Huang Ying <ying.huang@intel.com> wrote:
>> On 07/20/2011 11:22 PM, Bjorn Helgaas wrote:
>>> On Wed, Jul 20, 2011 at 2:09 AM, Huang Ying <ying.huang@intel.com> wrote:
>>>> EINJ parameter support is only usable for some specific BIOS.
>>>> Originally, it is expected to have no harm for BIOS does not support
>>>> it.  But now, we found it will cause issue (memory overwriting) for
>>>> some BIOS.  So param support is disabled by default and only enabled
>>>> when newly added module parameter named "param_extension" is
>>>> explicitly specified.
>>>
>>> Adding parameters always makes things harder for users.  Is there any
>>> way this could be done with a whitelist or other automatic mechanism?
>>
>> The only user of EINJ is RAS developer/tester.  So we adopt a simpler
>> solution.
>>
>>> Per 6e320ec1d98 (which added EINJ parameter support), parameters are
>>> an unpublished extension and are not part of the ACPI spec.  So if we
>>> pick up an MMIO address from a SET_ERROR_TYPE WRITE_REGISTER
>>> instruction and blindly fill in a structure (undefined by the spec)
>>> presumed to be at that address, it doesn't seem surprising that things
>>> will blow up on BIOSes that don't expect that behavior.  After all,
>>> the spec says to write to a generic address structure (not an
>>> einj_parameter structure) when we interpret the WRITE_REGISTER
>>> instruction (not at the magic time between SET_ERROR_TYPE and
>>> EXECUTE_OPERATION).
>>>
>>> If EINJ parameter support is ever added to the ACPI spec, I would
>>> expect a new EINJ flag bit or similar indication to be added at the
>>> same time, so the OS would know when to use it.
>>
>> EINJ parameter support has not been added to ACPI spec, it may be added
>> in the future, and should has some way to indicate whether parameter is
>> supported.
>>
>>> Can you add a whitelist of BIOSes that do support this extension?
>>> Maybe you could define a GUID that future platforms could supply so
>>> you wouldn't have to update the whitelist for every new platform that
>>> supports it?
>>
>> IMHO, the EINJ will not be used by the ordinary users.  If so, can we
>> use a simpler solution such as that in this patch?
> 
> It's not very clear to the code reader when this extension might be
> useful.  It's a bit of folklore that will have to be passed around via
> word-of-mouth.  If I want to modify the parameter code, I have no
> clues about what sort of machine I'd need to test it.
> 
> What happens if you load this module on a machine that supports the
> extension, but you don't supply the module parameter?  I would expect
> problems in that case, too, because the BIOS is expecting parameters
> and we won't fill them in.

On my testing machine with parameter extension, if no injecting
parameter is specified, some BIOS specified default parameter will be used.

> But maybe you're right that this is so specialized that we shouldn't
> over-engineer it.

Best Regards,
Huang Ying

Re: [BUGFIX] ACPI, APEI, EINJ Param support is disabled by default

[Andi Kleen]

>  
> +static bool param_extension;
> +module_param(param_extension, bool, 0);

Make it 0644, then it can be changed at runtime. No need to require a reboot.

-Andi

Re: [BUGFIX] ACPI, APEI, EINJ Param support is disabled by default

[Huang Ying]

On 07/21/2011 10:42 AM, Andi Kleen wrote:
>>  
>> +static bool param_extension;
>> +module_param(param_extension, bool, 0);
> 
> Make it 0644, then it can be changed at runtime. No need to require a reboot.

Normally, EINJ is compiled as module, so we can change it at load time
without a reboot.

Do we need the real runtime switching capability?

Best Regards,
Huang Ying

Re: [BUGFIX] ACPI, APEI, EINJ Param support is disabled by default

[Andi Kleen]

On Thu, Jul 21, 2011 at 10:54:13AM +0800, Huang Ying wrote:
> On 07/21/2011 10:42 AM, Andi Kleen wrote:
> >>  
> >> +static bool param_extension;
> >> +module_param(param_extension, bool, 0);
> > 
> > Make it 0644, then it can be changed at runtime. No need to require a reboot.
> 
> Normally, EINJ is compiled as module, so we can change it at load time
> without a reboot.

Ok. 

> 
> Do we need the real runtime switching capability?

I usually do that with modparams when it's possible. After all it's very
simple and doesn't cost anything.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [BUGFIX] ACPI, APEI, EINJ Param support is disabled by default

[Huang Ying]

On 07/21/2011 12:16 PM, Andi Kleen wrote:
> On Thu, Jul 21, 2011 at 10:54:13AM +0800, Huang Ying wrote:
>> On 07/21/2011 10:42 AM, Andi Kleen wrote:
>>>>  
>>>> +static bool param_extension;
>>>> +module_param(param_extension, bool, 0);
>>>
>>> Make it 0644, then it can be changed at runtime. No need to require a reboot.
>>
>> Normally, EINJ is compiled as module, so we can change it at load time
>> without a reboot.
> 
> Ok. 
> 
>>
>> Do we need the real runtime switching capability?
> 
> I usually do that with modparams when it's possible. After all it's very
> simple and doesn't cost anything.

To avoid confuse user, the param1/param2 files are created only if
param_extension=y is specified.  So making modprams changeable in run
time may make situation a little complex.

Best Regards,
Huang Ying

Re: [BUGFIX] ACPI, APEI, EINJ Param support is disabled by default

[Andi Kleen]

> To avoid confuse user, the param1/param2 files are created only if
> param_extension=y is specified.  So making modprams changeable in run
> time may make situation a little complex.

Good point. Ok then not.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [BUGFIX] ACPI, APEI, EINJ Param support is disabled by default

[Len Brown]

applied as-is to the apei branch.

please minde the checkpatch 80-column thing.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [BUGFIX] ACPI, APEI, EINJ Param support is disabled by default

[Huang Ying]

On 08/03/2011 06:03 AM, Len Brown wrote:
> applied as-is to the apei branch.
> 
> please minde the checkpatch 80-column thing.

Sorry,  will pay more attention next time.

Best Regards,
Huang Ying