Xen devel - Oct 2012 - ask a question about ERST

Hi maintainer,
I found below patch reverted part of erst header size check.
This lead to mismatch with kernel upstream code and erst disabled on
some machine like X4170 M3/X3-2.
According to the ACPI spec 4.0 and 5.0, the Serialization Header Length
should be the length of Serialization Header.
After revert below patch, xen succeed with erst table init.
So could this patch be reverted now to match acpi spec and kernel upstream?

[root@zhenzhong2 xen-unstable.hg]# hg export 23760
# HG changeset patch
# User Keir Fraser <keir@xen.org>
# Date 1312909603 -3600
# Node ID ae10d7804168c185166277bcef3b18ffc9227b66
# Parent aca07ff1f0a59cc7ebb5ef76875229b7e99ba3ff
ACPI ERST: Revert change to erst_check_table() to be more permissive.

Permits tables that apparently Xen cannot handle (causes boot failure
on many systems).

Signed-off-by: Keir Fraser <keir@xen.org>

diff -r aca07ff1f0a5 -r ae10d7804168 xen/drivers/acpi/apei/erst.c
--- a/xen/drivers/acpi/apei/erst.c Tue Aug 09 17:48:16 2011 +0100
+++ b/xen/drivers/acpi/apei/erst.c Tue Aug 09 18:06:43 2011 +0100
@@ -715,13 +715,7 @@

static int __init erst_check_table(struct acpi_table_erst *erst_tab)
{
- /*
- * Some old BIOSes include the ACPI standard header in the ERST header
- * length; new BIOSes do not. Our check allows for both methods.
- */
- if ((erst_tab->header_length !- (sizeof(struct acpi_table_erst) -
sizeof(erst_tab->header)))
- && (erst_tab->header_length != sizeof(struct acpi_table_erst)))
+ if (erst_tab->header_length != sizeof(struct acpi_table_erst))
return -EINVAL;
if (erst_tab->header.length < sizeof(struct acpi_table_erst))
return -EINVAL;

On Thu, 2012-10-18 at 09:31 +0100, Zhenzhong Duan wrote:
> Hi maintainer,
> I found below patch reverted part of erst header size check.
> This lead to mismatch with kernel upstream code and erst disabled on
> some machine like X4170 M3/X3-2.
> According to the ACPI spec 4.0 and 5.0, the Serialization Header Length
> should be the length of Serialization Header.
> After revert below patch, xen succeed with erst table init.
> So could this patch be reverted now to match acpi spec and kernel upstream?
Did you ask yourself *why* this patch was reverted and investigate what
has changed in order to resolve that situation?

As it happens this patch has been discussed on xen-devel just this week,
please check the archives.
> 
> [root@zhenzhong2 xen-unstable.hg]# hg export 23760
> # HG changeset patch
> # User Keir Fraser <keir@xen.org>
> # Date 1312909603 -3600
> # Node ID ae10d7804168c185166277bcef3b18ffc9227b66
> # Parent aca07ff1f0a59cc7ebb5ef76875229b7e99ba3ff
> ACPI ERST: Revert change to erst_check_table() to be more permissive.
> 
> Permits tables that apparently Xen cannot handle (causes boot failure
> on many systems).
> 
> Signed-off-by: Keir Fraser <keir@xen.org>
> 
> diff -r aca07ff1f0a5 -r ae10d7804168 xen/drivers/acpi/apei/erst.c
> --- a/xen/drivers/acpi/apei/erst.c Tue Aug 09 17:48:16 2011 +0100
> +++ b/xen/drivers/acpi/apei/erst.c Tue Aug 09 18:06:43 2011 +0100
> @@ -715,13 +715,7 @@
> 
> static int __init erst_check_table(struct acpi_table_erst *erst_tab)
> {
> - /*
> - * Some old BIOSes include the ACPI standard header in the ERST header
> - * length; new BIOSes do not. Our check allows for both methods.
> - */
> - if ((erst_tab->header_length !> - (sizeof(struct acpi_table_erst) -
sizeof(erst_tab->header)))
> - && (erst_tab->header_length != sizeof(struct
acpi_table_erst)))
> + if (erst_tab->header_length != sizeof(struct acpi_table_erst))
> return -EINVAL;
> if (erst_tab->header.length < sizeof(struct acpi_table_erst))
> return -EINVAL;
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On Thu, Oct 18, 2012 at 04:31:59PM +0800, Zhenzhong Duan
wrote:
> Hi maintainer,
> I found below patch reverted part of erst header size check.
> This lead to mismatch with kernel upstream code and erst disabled on
> some machine like X4170 M3/X3-2.
> According to the ACPI spec 4.0 and 5.0, the Serialization Header Length
> should be the length of Serialization Header.
> After revert below patch, xen succeed with erst table init.
> So could this patch be reverted now to match acpi spec and kernel upstream?
There was a discussion about this on xen-devel. Search for Jan and ERST
and I believe the idea was that an AMD machine needs to be checked
to figure out what to do.
> 
> [root@zhenzhong2 xen-unstable.hg]# hg export 23760
> # HG changeset patch
> # User Keir Fraser <keir@xen.org>
> # Date 1312909603 -3600
> # Node ID ae10d7804168c185166277bcef3b18ffc9227b66
> # Parent aca07ff1f0a59cc7ebb5ef76875229b7e99ba3ff
> ACPI ERST: Revert change to erst_check_table() to be more permissive.
> 
> Permits tables that apparently Xen cannot handle (causes boot failure
> on many systems).
> 
> Signed-off-by: Keir Fraser <keir@xen.org>
> 
> diff -r aca07ff1f0a5 -r ae10d7804168 xen/drivers/acpi/apei/erst.c
> --- a/xen/drivers/acpi/apei/erst.c Tue Aug 09 17:48:16 2011 +0100
> +++ b/xen/drivers/acpi/apei/erst.c Tue Aug 09 18:06:43 2011 +0100
> @@ -715,13 +715,7 @@
> 
> static int __init erst_check_table(struct acpi_table_erst *erst_tab)
> {
> - /*
> - * Some old BIOSes include the ACPI standard header in the ERST header
> - * length; new BIOSes do not. Our check allows for both methods.
> - */
> - if ((erst_tab->header_length !> - (sizeof(struct acpi_table_erst) -
sizeof(erst_tab->header)))
> - && (erst_tab->header_length != sizeof(struct
acpi_table_erst)))
> + if (erst_tab->header_length != sizeof(struct acpi_table_erst))
> return -EINVAL;
> if (erst_tab->header.length < sizeof(struct acpi_table_erst))
> return -EINVAL;
Gosh, your mailer mangled that patch :-)

On 2012-10-18 20:06, Konrad Rzeszutek Wilk wrote:
> On Thu, Oct 18, 2012 at 04:31:59PM +0800, Zhenzhong Duan wrote:
>> Hi maintainer,
>> I found below patch reverted part of erst header size check.
>> This lead to mismatch with kernel upstream code and erst disabled on
>> some machine like X4170 M3/X3-2.
>> According to the ACPI spec 4.0 and 5.0, the Serialization Header Length
>> should be the length of Serialization Header.
>> After revert below patch, xen succeed with erst table init.
>> So could this patch be reverted now to match acpi spec and kernel
upstream?
> There was a discussion about this on xen-devel. Search for Jan and ERST
> and I believe the idea was that an AMD machine needs to be checked
> to figure out what to do.
Well, I see. I''ll wait for the final fix.
thanks all