ERST problems

ERST problems

[Andrew Cooper]

Hello,

When debugging ERST boot time hangs, the following debugging proved
interesting

>From a Dell Poweredge 2950 with latest BIOS:

(XEN) Enabling APIC mode:  Flat.  Using 1 I/O APICs
(XEN) ACPI: HPET id: 0x8086a201 base: 0xfed00000
(XEN) erst_init()
(XEN) Found ERST
(XEN) ERST is good
(XEN) About to exec ctx_init()
(XEN) About to exec pre map
(XEN) Starting apei_exec_for_each_entry().  ctx->entries = 15
(XEN)  .. i = 0, entry = 0xffff82c3ffd7ab48, ins = 0x3
(XEN)  .. i = 1, entry = 0xffff82c3ffd7ab68, ins = 0x3
(XEN)  .. i = 2, entry = 0xffff82c3ffd7ab88, ins = 0x3
(XEN)  .. i = 3, entry = 0xffff82c3ffd7aba8, ins = 0x3
(XEN)  .. i = 4, entry = 0xffff82c3ffd7abc8, ins = 0x2
(XEN)  .. i = 5, entry = 0xffff82c3ffd7abe8, ins = 0x3
(XEN)  .. i = 6, entry = 0xffff82c3ffd7ac08, ins = 0x3
(XEN)  .. i = 7, entry = 0xffff82c3ffd7ac28, ins = 0x3
(XEN)  .. i = 8, entry = 0xffff82c3ffd7ac48, ins = 0x1
(XEN)  .. i = 9, entry = 0xffff82c3ffd7ac68, ins = 0x0
(XEN)  .. i = 10, entry = 0xffff82c3ffd7ac88, ins = 0x0
(XEN)  .. i = 11, entry = 0xffff82c3ffd7aca8, ins = 0x2
(XEN)  .. i = 12, entry = 0xffff82c3ffd7acc8, ins = 0x0
(XEN)  .. i = 13, entry = 0xffff82c3ffd7ace8, ins = 0x3
(XEN)  .. i = 14, entry = 0xffff82c3ffd7ad08, ins = 0x3
(XEN) About to get range
(XEN) in erst_get_erange()
(XEN)   range->base = 0xffff83007fb4f0a0
(XEN)   range->size = 0xffff83007fb4f0a0
(XEN)   range->attr = 0x7fb4f0a0
(XEN) About to pre map (0xffff83007fb4f0a0, 0xffff83007fb4f0a0)
(XEN) Error -12, about to unmap gars
(XEN) Starting apei_exec_for_each_entry().  ctx->entries = 15
(XEN)  .. i = 0, entry = 0xffff82c3ffd7ab48, ins = 0x3
(XEN)   apei_post_unmap_gar(0xffff82c3ffd7ab4c)... . aok
(XEN)  .. i = 1, entry = 0xffff82c3ffd7ab68, ins = 0x3
(XEN)   apei_post_unmap_gar(0xffff82c3ffd7ab6c)... . aok
(XEN)  .. i = 2, entry = 0xffff82c3ffd7ab88, ins = 0x3
(XEN)   apei_post_unmap_gar(0xffff82c3ffd7ab8c)... .

The hang at this point turns out to be trivial mis-locking issue, and I
have submitted a patch to fix it.

However, this debugging shows that erst_get_erange() is clearly
returning junk, causing apei_pre_map() to fail.

While in this case it is easy to identify the junk size parameter as it
is far greater than the system RAM (32G), I am not sure this is
necessarily the best approach to try and validate the information.  Is
there anything else we could sensibly do?

At the very least, erst_init() should not silently ignore its failures.

~Andrew

Re: ERST problems

[Andrew Cooper]

[-- Attachment #1: Type: text/plain, Size: 2756 bytes --]

On 21/03/13 23:22, Andrew Cooper wrote:
> Hello,
>
> When debugging ERST boot time hangs, the following debugging proved
> interesting
>
> From a Dell Poweredge 2950 with latest BIOS:
>
> (XEN) Enabling APIC mode:  Flat.  Using 1 I/O APICs
> (XEN) ACPI: HPET id: 0x8086a201 base: 0xfed00000
> (XEN) erst_init()
> (XEN) Found ERST
> (XEN) ERST is good
> (XEN) About to exec ctx_init()
> (XEN) About to exec pre map
> (XEN) Starting apei_exec_for_each_entry().  ctx->entries = 15
> (XEN)  .. i = 0, entry = 0xffff82c3ffd7ab48, ins = 0x3
> (XEN)  .. i = 1, entry = 0xffff82c3ffd7ab68, ins = 0x3
> (XEN)  .. i = 2, entry = 0xffff82c3ffd7ab88, ins = 0x3
> (XEN)  .. i = 3, entry = 0xffff82c3ffd7aba8, ins = 0x3
> (XEN)  .. i = 4, entry = 0xffff82c3ffd7abc8, ins = 0x2
> (XEN)  .. i = 5, entry = 0xffff82c3ffd7abe8, ins = 0x3
> (XEN)  .. i = 6, entry = 0xffff82c3ffd7ac08, ins = 0x3
> (XEN)  .. i = 7, entry = 0xffff82c3ffd7ac28, ins = 0x3
> (XEN)  .. i = 8, entry = 0xffff82c3ffd7ac48, ins = 0x1
> (XEN)  .. i = 9, entry = 0xffff82c3ffd7ac68, ins = 0x0
> (XEN)  .. i = 10, entry = 0xffff82c3ffd7ac88, ins = 0x0
> (XEN)  .. i = 11, entry = 0xffff82c3ffd7aca8, ins = 0x2
> (XEN)  .. i = 12, entry = 0xffff82c3ffd7acc8, ins = 0x0
> (XEN)  .. i = 13, entry = 0xffff82c3ffd7ace8, ins = 0x3
> (XEN)  .. i = 14, entry = 0xffff82c3ffd7ad08, ins = 0x3
> (XEN) About to get range
> (XEN) in erst_get_erange()
> (XEN)   range->base = 0xffff83007fb4f0a0
> (XEN)   range->size = 0xffff83007fb4f0a0
> (XEN)   range->attr = 0x7fb4f0a0
> (XEN) About to pre map (0xffff83007fb4f0a0, 0xffff83007fb4f0a0)
> (XEN) Error -12, about to unmap gars
> (XEN) Starting apei_exec_for_each_entry().  ctx->entries = 15
> (XEN)  .. i = 0, entry = 0xffff82c3ffd7ab48, ins = 0x3
> (XEN)   apei_post_unmap_gar(0xffff82c3ffd7ab4c)... . aok
> (XEN)  .. i = 1, entry = 0xffff82c3ffd7ab68, ins = 0x3
> (XEN)   apei_post_unmap_gar(0xffff82c3ffd7ab6c)... . aok
> (XEN)  .. i = 2, entry = 0xffff82c3ffd7ab88, ins = 0x3
> (XEN)   apei_post_unmap_gar(0xffff82c3ffd7ab8c)... .
>
> The hang at this point turns out to be trivial mis-locking issue, and I
> have submitted a patch to fix it.
>
> However, this debugging shows that erst_get_erange() is clearly
> returning junk, causing apei_pre_map() to fail.
>
> While in this case it is easy to identify the junk size parameter as it
> is far greater than the system RAM (32G), I am not sure this is
> necessarily the best approach to try and validate the information.  Is
> there anything else we could sensibly do?
>
> At the very least, erst_init() should not silently ignore its failures.
>
> ~Andrew

In case you are wondering, attached is the complete acpidump from the
server in question, and I have pre-extracted the ERST and EINJ tables.

~Andrew

[-- Attachment #2: dell-poweredge-2950.tar.gz --]
[-- Type: application/x-gzip, Size: 26193 bytes --]

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: ERST problems

[Jan Beulich]

>>> On 22.03.13 at 00:22, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> Hello,
> 
> When debugging ERST boot time hangs, the following debugging proved
> interesting
> 
> From a Dell Poweredge 2950 with latest BIOS:
> 
> (XEN) Enabling APIC mode:  Flat.  Using 1 I/O APICs
> (XEN) ACPI: HPET id: 0x8086a201 base: 0xfed00000
> (XEN) erst_init()
> (XEN) Found ERST
> (XEN) ERST is good
> (XEN) About to exec ctx_init()
> (XEN) About to exec pre map
> (XEN) Starting apei_exec_for_each_entry().  ctx->entries = 15
> (XEN)  .. i = 0, entry = 0xffff82c3ffd7ab48, ins = 0x3
> (XEN)  .. i = 1, entry = 0xffff82c3ffd7ab68, ins = 0x3
> (XEN)  .. i = 2, entry = 0xffff82c3ffd7ab88, ins = 0x3
> (XEN)  .. i = 3, entry = 0xffff82c3ffd7aba8, ins = 0x3
> (XEN)  .. i = 4, entry = 0xffff82c3ffd7abc8, ins = 0x2
> (XEN)  .. i = 5, entry = 0xffff82c3ffd7abe8, ins = 0x3
> (XEN)  .. i = 6, entry = 0xffff82c3ffd7ac08, ins = 0x3
> (XEN)  .. i = 7, entry = 0xffff82c3ffd7ac28, ins = 0x3
> (XEN)  .. i = 8, entry = 0xffff82c3ffd7ac48, ins = 0x1
> (XEN)  .. i = 9, entry = 0xffff82c3ffd7ac68, ins = 0x0
> (XEN)  .. i = 10, entry = 0xffff82c3ffd7ac88, ins = 0x0
> (XEN)  .. i = 11, entry = 0xffff82c3ffd7aca8, ins = 0x2
> (XEN)  .. i = 12, entry = 0xffff82c3ffd7acc8, ins = 0x0
> (XEN)  .. i = 13, entry = 0xffff82c3ffd7ace8, ins = 0x3
> (XEN)  .. i = 14, entry = 0xffff82c3ffd7ad08, ins = 0x3
> (XEN) About to get range
> (XEN) in erst_get_erange()
> (XEN)   range->base = 0xffff83007fb4f0a0
> (XEN)   range->size = 0xffff83007fb4f0a0
> (XEN)   range->attr = 0x7fb4f0a0
> (XEN) About to pre map (0xffff83007fb4f0a0, 0xffff83007fb4f0a0)
> (XEN) Error -12, about to unmap gars
> (XEN) Starting apei_exec_for_each_entry().  ctx->entries = 15
> (XEN)  .. i = 0, entry = 0xffff82c3ffd7ab48, ins = 0x3
> (XEN)   apei_post_unmap_gar(0xffff82c3ffd7ab4c)... . aok
> (XEN)  .. i = 1, entry = 0xffff82c3ffd7ab68, ins = 0x3
> (XEN)   apei_post_unmap_gar(0xffff82c3ffd7ab6c)... . aok
> (XEN)  .. i = 2, entry = 0xffff82c3ffd7ab88, ins = 0x3
> (XEN)   apei_post_unmap_gar(0xffff82c3ffd7ab8c)... .
> 
> The hang at this point turns out to be trivial mis-locking issue, and I
> have submitted a patch to fix it.
> 
> However, this debugging shows that erst_get_erange() is clearly
> returning junk, causing apei_pre_map() to fail.

If I'm getting this right, the three apei_exec_run()s there just do
nothing, leaving the output uninitialized. That's because the
ERST table doesn't contain instructions for the three actions
involved here.

All we appear to be missing in this case is Linux commit
eecf2f7124834dd1cad21807526a8ea031ba8217. I'll get that
ported over...

Jan

[PATCH] ACPI, APEI: Add apei_exec_run_optional

[Jan Beulich]

>>> On 22.03.13 at 10:07, "Jan Beulich" <JBeulich@suse.com> wrote:
> All we appear to be missing in this case is Linux commit
> eecf2f7124834dd1cad21807526a8ea031ba8217. I'll get that
> ported over...

ACPI, APEI: Add apei_exec_run_optional

Some actions in APEI ERST and EINJ tables are optional, for example,
ACPI_EINJ_BEGIN_OPERATION action is used to do some preparation for
error injection, and firmware may choose to do nothing here.  While
some other actions are mandatory, for example, firmware must provide
ACPI_EINJ_GET_ERROR_TYPE implementation.

Original implementation treats all actions as optional (that is, can
have no instructions), that may cause issue if firmware does not
provide some mandatory actions.  To fix this, this patch adds
apei_exec_run_optional, which should be used for optional actions.
The original apei_exec_run should be used for mandatory actions.

Signed-off-by: Huang Ying <ying.huang@intel.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/acpi/apei/apei-base.c
+++ b/xen/drivers/acpi/apei/apei-base.c
@@ -154,9 +154,10 @@ int apei_exec_noop(struct apei_exec_cont
  * Interpret the specified action. Go through whole action table,
  * execute all instructions belong to the action.
  */
-int apei_exec_run(struct apei_exec_context *ctx, u8 action)
+int __apei_exec_run(struct apei_exec_context *ctx, u8 action,
+		    bool_t optional)
 {
-	int rc;
+	int rc = -ENOENT;
 	u32 i, ip;
 	struct acpi_whea_header *entry;
 	apei_exec_ins_func_t run;
@@ -195,7 +196,7 @@ rewind:
 			goto rewind;
 	}
 
-	return 0;
+	return !optional && rc < 0 ? rc : 0;
 }
 
 typedef int (*apei_exec_entry_func_t)(struct apei_exec_context *ctx,
--- a/xen/drivers/acpi/apei/apei-internal.h
+++ b/xen/drivers/acpi/apei/apei-internal.h
@@ -48,7 +48,18 @@ static inline u64 apei_exec_ctx_get_outp
 	return ctx->value;
 }
 
-int apei_exec_run(struct apei_exec_context *ctx, u8 action);
+int __apei_exec_run(struct apei_exec_context *ctx, u8 action, bool_t optional);
+
+static inline int apei_exec_run(struct apei_exec_context *ctx, u8 action)
+{
+	return __apei_exec_run(ctx, action, 0);
+}
+
+/* It is optional whether the firmware provides the action */
+static inline int apei_exec_run_optional(struct apei_exec_context *ctx, u8 action)
+{
+	return __apei_exec_run(ctx, action, 1);
+}
 
 /* Common instruction implementation */

Re: [PATCH] ACPI, APEI: Add apei_exec_run_optional

[Andrew Cooper]

On 22/03/13 09:14, Jan Beulich wrote:
>>>> On 22.03.13 at 10:07, "Jan Beulich" <JBeulich@suse.com> wrote:
>> All we appear to be missing in this case is Linux commit
>> eecf2f7124834dd1cad21807526a8ea031ba8217. I'll get that
>> ported over...
> ACPI, APEI: Add apei_exec_run_optional
>
> Some actions in APEI ERST and EINJ tables are optional, for example,
> ACPI_EINJ_BEGIN_OPERATION action is used to do some preparation for
> error injection, and firmware may choose to do nothing here.  While
> some other actions are mandatory, for example, firmware must provide
> ACPI_EINJ_GET_ERROR_TYPE implementation.
>
> Original implementation treats all actions as optional (that is, can
> have no instructions), that may cause issue if firmware does not
> provide some mandatory actions.  To fix this, this patch adds
> apei_exec_run_optional, which should be used for optional actions.
> The original apei_exec_run should be used for mandatory actions.
>
> Signed-off-by: Huang Ying <ying.huang@intel.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Tested-by: Andrew Cooper <andrew.cooper3@citrix.com>

(Via backport to 4.2)

For what it is worth, with the spinlock fix, I disabled the "Intel only"
restriction, and so far I have been unable to find a problematic AMD box.

~Andrew

>
> --- a/xen/drivers/acpi/apei/apei-base.c
> +++ b/xen/drivers/acpi/apei/apei-base.c
> @@ -154,9 +154,10 @@ int apei_exec_noop(struct apei_exec_cont
>   * Interpret the specified action. Go through whole action table,
>   * execute all instructions belong to the action.
>   */
> -int apei_exec_run(struct apei_exec_context *ctx, u8 action)
> +int __apei_exec_run(struct apei_exec_context *ctx, u8 action,
> +		    bool_t optional)
>  {
> -	int rc;
> +	int rc = -ENOENT;
>  	u32 i, ip;
>  	struct acpi_whea_header *entry;
>  	apei_exec_ins_func_t run;
> @@ -195,7 +196,7 @@ rewind:
>  			goto rewind;
>  	}
>  
> -	return 0;
> +	return !optional && rc < 0 ? rc : 0;
>  }
>  
>  typedef int (*apei_exec_entry_func_t)(struct apei_exec_context *ctx,
> --- a/xen/drivers/acpi/apei/apei-internal.h
> +++ b/xen/drivers/acpi/apei/apei-internal.h
> @@ -48,7 +48,18 @@ static inline u64 apei_exec_ctx_get_outp
>  	return ctx->value;
>  }
>  
> -int apei_exec_run(struct apei_exec_context *ctx, u8 action);
> +int __apei_exec_run(struct apei_exec_context *ctx, u8 action, bool_t optional);
> +
> +static inline int apei_exec_run(struct apei_exec_context *ctx, u8 action)
> +{
> +	return __apei_exec_run(ctx, action, 0);
> +}
> +
> +/* It is optional whether the firmware provides the action */
> +static inline int apei_exec_run_optional(struct apei_exec_context *ctx, u8 action)
> +{
> +	return __apei_exec_run(ctx, action, 1);
> +}
>  
>  /* Common instruction implementation */
>  
>
>
>

Re: [PATCH] ACPI, APEI: Add apei_exec_run_optional

[Jan Beulich]

>>> On 22.03.13 at 12:27, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 22/03/13 09:14, Jan Beulich wrote:
>>>>> On 22.03.13 at 10:07, "Jan Beulich" <JBeulich@suse.com> wrote:
>>> All we appear to be missing in this case is Linux commit
>>> eecf2f7124834dd1cad21807526a8ea031ba8217. I'll get that
>>> ported over...
>> ACPI, APEI: Add apei_exec_run_optional
>>
>> Some actions in APEI ERST and EINJ tables are optional, for example,
>> ACPI_EINJ_BEGIN_OPERATION action is used to do some preparation for
>> error injection, and firmware may choose to do nothing here.  While
>> some other actions are mandatory, for example, firmware must provide
>> ACPI_EINJ_GET_ERROR_TYPE implementation.
>>
>> Original implementation treats all actions as optional (that is, can
>> have no instructions), that may cause issue if firmware does not
>> provide some mandatory actions.  To fix this, this patch adds
>> apei_exec_run_optional, which should be used for optional actions.
>> The original apei_exec_run should be used for mandatory actions.
>>
>> Signed-off-by: Huang Ying <ying.huang@intel.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Tested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> (Via backport to 4.2)
> 
> For what it is worth, with the spinlock fix, I disabled the "Intel only"
> restriction, and so far I have been unable to find a problematic AMD box.

So Ian, with those two fixes in, could you retry the revert of the
Intel-only enforcement in a full ad-hoc run? If successful, that
would then also give us reasonable assurance to backport all the
APEI fixes to the stable branches.

Thanks, Jan

Re: [PATCH] ACPI, APEI: Add apei_exec_run_optional

[Andrew Cooper]

On 22/03/13 11:45, Jan Beulich wrote:
>>>> On 22.03.13 at 12:27, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 22/03/13 09:14, Jan Beulich wrote:
>>>>>> On 22.03.13 at 10:07, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> All we appear to be missing in this case is Linux commit
>>>> eecf2f7124834dd1cad21807526a8ea031ba8217. I'll get that
>>>> ported over...
>>> ACPI, APEI: Add apei_exec_run_optional
>>>
>>> Some actions in APEI ERST and EINJ tables are optional, for example,
>>> ACPI_EINJ_BEGIN_OPERATION action is used to do some preparation for
>>> error injection, and firmware may choose to do nothing here.  While
>>> some other actions are mandatory, for example, firmware must provide
>>> ACPI_EINJ_GET_ERROR_TYPE implementation.
>>>
>>> Original implementation treats all actions as optional (that is, can
>>> have no instructions), that may cause issue if firmware does not
>>> provide some mandatory actions.  To fix this, this patch adds
>>> apei_exec_run_optional, which should be used for optional actions.
>>> The original apei_exec_run should be used for mandatory actions.
>>>
>>> Signed-off-by: Huang Ying <ying.huang@intel.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Tested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> (Via backport to 4.2)
>>
>> For what it is worth, with the spinlock fix, I disabled the "Intel only"
>> restriction, and so far I have been unable to find a problematic AMD box.
> So Ian, with those two fixes in, could you retry the revert of the
> Intel-only enforcement in a full ad-hoc run? If successful, that
> would then also give us reasonable assurance to backport all the
> APEI fixes to the stable branches.
>
> Thanks, Jan
>

>From the XenServer side of testing, I am running with the following
patches backported to 4.2:

25932 - ACPI: move tables.c fully into .init.*
26060 - ACPI: fix APEI related table size checking
26062 - ACPI/APEI: fix ERST MOVE_DATA instruction implementation
26736 - ACPI/APEI: Unlock apei_iomaps_lock on error path (Pre-emptivly
nabbed from staging)
And a backport of this patch

So far, no issues found.  I shall see about scheduling tests on some of
our older AMD boxes.

One minor suggestion I have is to use "ACPI:" prefixes in the error
messages.

~Andrew