Re: [Xen-devel] [xen-unstable bisection] complete test-amd64-i386-qemuu-rhel6hvm-amd

["Liu, Jinsong" <jinsong.liu@xxxxxxxxx>]

Jan Beulich wrote:
>>>> On 17.10.12 at 11:02, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
>> On Wed, 2012-10-17 at 08:41 +0100, Jan Beulich wrote:
>>> So I just now realized that a similar change was already done
>>> by 23736:31683aa4bfb3 and then reverted by
>>> 23760:ae10d7804168. Nothing was done subsequently to
>>> address the actual problem(s). It is quite obvious that the more
>>> relaxed check uncovers other bugs in the ERST code, yet looking
>>> at the Linux history of the corresponding file doesn't reveal any
>>> fix the lack of which would explain an outright hang (rather than
>>> a crash, as I would expect to be the result of e.g. the missing
>>> ioremap()-s added by 0bbba38a61283a55f2061ab3e0910c572d19f462.
>>> 
>>> Most of the other changes are cosmetic or pstore related, so I
>>> wonder whether instead of reverting again we should try pulling
>>> in this one extra fix.
>> 
>> Worth a go. I assume this issue isn't related to the typo you found
>> this morning?
> 
> I didn't find any typo; the original author of the Linux side patch
> pointed out a typo in the commit message (which was copied
> verbatim from the Linux side).
> 
>>> If reverting is preferred (or turns out necessary if that second
>>> fix doesn't help), we should settle on the disposition of the whole
>>> APEI/ERST code, as my conclusion is that it is pretty much
>>> unmaintained since its original contribution over two years ago.
>> 
>> If we revert we should leave a big fat comment pointing to this
>> and/or the previous discussion to stop the next guy doing the same
>> thing. 
> 
> We should rather decide whether this should get fixed, or the
> code ripped out (decision mainly depending on the original
> contributer - Intel - being willing to fix the code).

I just checked the history.

1. It firstly checked in as c/s 22052 --> at that time it tested at an old 
intel platform (I forget type);
2. then it updated with c/s 23736 --> since when our QA tested at a newer intel 
platform, it cannot pass erst_check_table, after debug we found the newer intel 
platform bios has different header length definiation at its ERST table, so we 
allow both size table work;
3. then it reverted with c/s 23760 --> since when test at an AMD platform by 
Critix (see attached, complete test-amd64-i386-xl), it will hang, so Keir 
revert it.

c/s 22052 and 23736 were tested at 2 intel platforms, but we have no way to 
test at AMD platform. So the problem is, how to debug&fix it?

Thanks,
Jinsong

--- Begin Message ---

• To: Keir Fraser <keir@xxxxxxx>
• From: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
• Date: Tue, 9 Aug 2011 14:58:37 +0000
• Cc: Jan Beulich <JBeulich@xxxxxxxxxx>, Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx>, Jeremy Fitzhardinge <Jeremy.Fitzhardinge@xxxxxxxxxx>
• Thread-index: AcxWpNbpQT/v8LNISfqL/zQ9xkV3CA==
• Thread-topic: [Xen-devel] [xen-unstable bisection] complete test-amd64-i386-xl

Keir Fraser writes ("Re: [Xen-devel] [xen-unstable bisection] complete 
test-amd64-i386-xl"):
> Does reverting just the change to erst_check_table() fix the regression on
> the affected test boxes? What about the similar-looking boot failure that
> you see, Jeremy?

Indeed, reverting xen/drivers/acpi/apei/erst.c, ie the change to
erst_check_table, seems to fix it.  That is,

  23736:31683aa4bfb3 "acpi: Add support for old and new bios erst, ..."
   +
  23742:50ddc200a60c "fix regression from c/s 23735:537918f518ee"

fails.  That plus the diff below boots happily.

Ian.

diff --git a/xen/drivers/acpi/apei/erst.c b/xen/drivers/acpi/apei/erst.c
index e012cd3..eb666a6 100644
--- a/xen/drivers/acpi/apei/erst.c
+++ b/xen/drivers/acpi/apei/erst.c
@@ -715,13 +715,7 @@ int erst_clear(u64 record_id)
 
 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;

--- End Message ---

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