diff mbox

[for-2.6] acpi: fix bios linker loadder COMMAND_ALLOCATE on bigendian host

Message ID 1461933880-214779-1-git-send-email-imammedo@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Igor Mammedov April 29, 2016, 12:44 p.m. UTC
'make check' fails with:

ERROR:tests/bios-tables-test.c:493:load_expected_aml:
   assertion failed: (g_file_test(aml_file, G_FILE_TEST_EXISTS))

since commit:
caf50c7166a6ed96c462ab5db4b495e1234e4cc6
tests: pc: acpi: drop not needed 'expected SSDT' blobs

Assert happens because qemu-system-x86_64 generates
SSDT table and test looks for a corresponding expected
table to compare with.

However there is no expected SSDT blob anymore, since
QEMU souldn't generate one. As it happens BIOS is not
able to read ACPI tables from QEMU and fallbacks to
embeded legacy ACPI codepath, which generates SSDT.
That happens due to wrongly sized endiannes conversion
which makes
 uint8_t BiosLinkerLoaderEntry.alloc.zone
end up with 0 due to truncation of 32 bit integer
which on host is 1 or 2.

Fix it by dropping invalid cpu_to_le32() as uint8_t
doesn't require any conversion.

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1330174

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/bios-linker-loader.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Laurent Vivier April 29, 2016, 1:16 p.m. UTC | #1
On 29/04/2016 14:44, Igor Mammedov wrote:
> 'make check' fails with:
> 
> ERROR:tests/bios-tables-test.c:493:load_expected_aml:
>    assertion failed: (g_file_test(aml_file, G_FILE_TEST_EXISTS))
> 
> since commit:
> caf50c7166a6ed96c462ab5db4b495e1234e4cc6
> tests: pc: acpi: drop not needed 'expected SSDT' blobs
> 
> Assert happens because qemu-system-x86_64 generates
> SSDT table and test looks for a corresponding expected
> table to compare with.
> 
> However there is no expected SSDT blob anymore, since
> QEMU souldn't generate one. As it happens BIOS is not
> able to read ACPI tables from QEMU and fallbacks to
> embeded legacy ACPI codepath, which generates SSDT.
> That happens due to wrongly sized endiannes conversion
> which makes
>  uint8_t BiosLinkerLoaderEntry.alloc.zone
> end up with 0 due to truncation of 32 bit integer
> which on host is 1 or 2.
> 
> Fix it by dropping invalid cpu_to_le32() as uint8_t
> doesn't require any conversion.
> 
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1330174
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Tested-by: Laurent Vivier <lvivier@redhat.com>

Fix the problem.

We have always some warnings but they were already here in the previous
releases.

Laurent
Igor Mammedov April 29, 2016, 1:45 p.m. UTC | #2
On Fri, 29 Apr 2016 15:16:07 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> On 29/04/2016 14:44, Igor Mammedov wrote:
> > 'make check' fails with:
> > 
> > ERROR:tests/bios-tables-test.c:493:load_expected_aml:
> >    assertion failed: (g_file_test(aml_file, G_FILE_TEST_EXISTS))
> > 
> > since commit:
> > caf50c7166a6ed96c462ab5db4b495e1234e4cc6
> > tests: pc: acpi: drop not needed 'expected SSDT' blobs
> > 
> > Assert happens because qemu-system-x86_64 generates
> > SSDT table and test looks for a corresponding expected
> > table to compare with.
> > 
> > However there is no expected SSDT blob anymore, since
> > QEMU souldn't generate one. As it happens BIOS is not
> > able to read ACPI tables from QEMU and fallbacks to
> > embeded legacy ACPI codepath, which generates SSDT.
> > That happens due to wrongly sized endiannes conversion
> > which makes
> >  uint8_t BiosLinkerLoaderEntry.alloc.zone
> > end up with 0 due to truncation of 32 bit integer
> > which on host is 1 or 2.
> > 
> > Fix it by dropping invalid cpu_to_le32() as uint8_t
> > doesn't require any conversion.
> > 
> > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1330174
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> 
> Tested-by: Laurent Vivier <lvivier@redhat.com>
> 
> Fix the problem.
> 
> We have always some warnings but they were already here in the previous
> releases.
That warnings are due to broken endianess handling in iasl,
it looks at table header size and says it's too big
because it doesn't take into account that all integers in ACPI
are little-endian.


> 
> Laurent
>
Marcel Apfelbaum May 1, 2016, 8:14 a.m. UTC | #3
On 04/29/2016 03:44 PM, Igor Mammedov wrote:
> 'make check' fails with:
>
> ERROR:tests/bios-tables-test.c:493:load_expected_aml:
>     assertion failed: (g_file_test(aml_file, G_FILE_TEST_EXISTS))
>
> since commit:
> caf50c7166a6ed96c462ab5db4b495e1234e4cc6
> tests: pc: acpi: drop not needed 'expected SSDT' blobs
>
> Assert happens because qemu-system-x86_64 generates
> SSDT table and test looks for a corresponding expected
> table to compare with.
>
> However there is no expected SSDT blob anymore, since
> QEMU souldn't generate one. As it happens BIOS is not
> able to read ACPI tables from QEMU and fallbacks to
> embeded legacy ACPI codepath, which generates SSDT.
> That happens due to wrongly sized endiannes conversion
> which makes
>   uint8_t BiosLinkerLoaderEntry.alloc.zone
> end up with 0 due to truncation of 32 bit integer
> which on host is 1 or 2.
>
> Fix it by dropping invalid cpu_to_le32() as uint8_t
> doesn't require any conversion.
>
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1330174
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   hw/acpi/bios-linker-loader.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
> index ace9abb..5153ab1 100644
> --- a/hw/acpi/bios-linker-loader.c
> +++ b/hw/acpi/bios-linker-loader.c
> @@ -135,9 +135,8 @@ void bios_linker_loader_alloc(GArray *linker,
>       strncpy(entry.alloc.file, file, sizeof entry.alloc.file - 1);
>       entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_ALLOCATE);
>       entry.alloc.align = cpu_to_le32(alloc_align);
> -    entry.alloc.zone = cpu_to_le32(alloc_fseg ?
> -                                    BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG :
> -                                    BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH);
> +    entry.alloc.zone = alloc_fseg ? BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG :
> +                                    BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH;
>
>       /* Alloc entries must come first, so prepend them */
>       g_array_prepend_vals(linker, &entry, sizeof entry);
>

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel
diff mbox

Patch

diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
index ace9abb..5153ab1 100644
--- a/hw/acpi/bios-linker-loader.c
+++ b/hw/acpi/bios-linker-loader.c
@@ -135,9 +135,8 @@  void bios_linker_loader_alloc(GArray *linker,
     strncpy(entry.alloc.file, file, sizeof entry.alloc.file - 1);
     entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_ALLOCATE);
     entry.alloc.align = cpu_to_le32(alloc_align);
-    entry.alloc.zone = cpu_to_le32(alloc_fseg ?
-                                    BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG :
-                                    BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH);
+    entry.alloc.zone = alloc_fseg ? BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG :
+                                    BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH;
 
     /* Alloc entries must come first, so prepend them */
     g_array_prepend_vals(linker, &entry, sizeof entry);