On Fri, May 06, 2022 at 09:55:44PM +0100, Nikos Nikoleris wrote:
> All ACPI table definitions are provided with precise definitions of
> field sizes and offsets, make sure that no compiler optimization can
> interfere with the memory layout of the corresponding structs.
That seems like a reasonable thing to do. I'm wondering why Linux doesn't
appear to do it. I see u-boot does, but not for all tables, which also
makes me scratch my head... I see this patch packs every struct except
rsdp_descriptor. Is there a reason it was left out?
Another comment below
>
> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@xxxxxxx>
> ---
> lib/acpi.h | 11 ++++++++---
> x86/s3.c | 16 ++++------------
> 2 files changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/lib/acpi.h b/lib/acpi.h
> index 1e89840..42a2c16 100644
> --- a/lib/acpi.h
> +++ b/lib/acpi.h
> @@ -3,6 +3,11 @@
>
> #include "libcflat.h"
>
> +/*
> + * All tables and structures must be byte-packed to match the ACPI
> + * specification, since the tables are provided by the system BIOS
> + */
> +
> #define ACPI_SIGNATURE(c1, c2, c3, c4) \
> ((c1) | ((c2) << 8) | ((c3) << 16) | ((c4) << 24))
>
> @@ -44,12 +49,12 @@ struct rsdp_descriptor { /* Root System Descriptor Pointer */
> struct acpi_table {
> ACPI_TABLE_HEADER_DEF
> char data[0];
> -};
> +} __attribute__ ((packed));
>
> struct rsdt_descriptor_rev1 {
> ACPI_TABLE_HEADER_DEF
> u32 table_offset_entry[0];
> -};
> +} __attribute__ ((packed));
>
> struct fadt_descriptor_rev1
> {
> @@ -104,7 +109,7 @@ struct facs_descriptor_rev1
> u32 S4bios_f : 1; /* Indicates if S4BIOS support is present */
> u32 reserved1 : 31; /* Must be 0 */
> u8 reserved3 [40]; /* Reserved - must be zero */
> -};
> +} __attribute__ ((packed));
>
> void set_efi_rsdp(struct rsdp_descriptor *rsdp);
> void* find_acpi_table_addr(u32 sig);
> diff --git a/x86/s3.c b/x86/s3.c
The changes below in this file are unrelated, so they should be in a
separate patch. However, I'm also curious why they're needed. I see
that find_acpi_table_addr() can return NULL, so it doesn't seem like
we should be removing the check, but instead changing the check to
an assert.
> index 378d37a..89d69fc 100644
> --- a/x86/s3.c
> +++ b/x86/s3.c
> @@ -2,15 +2,6 @@
> #include "acpi.h"
> #include "asm/io.h"
>
> -static u32* find_resume_vector_addr(void)
> -{
> - struct facs_descriptor_rev1 *facs = find_acpi_table_addr(FACS_SIGNATURE);
> - if (!facs)
> - return 0;
> - printf("FACS is at %p\n", facs);
> - return &facs->firmware_waking_vector;
> -}
> -
> #define RTC_SECONDS_ALARM 1
> #define RTC_MINUTES_ALARM 3
> #define RTC_HOURS_ALARM 5
> @@ -40,12 +31,13 @@ extern char resume_start, resume_end;
> int main(int argc, char **argv)
> {
> struct fadt_descriptor_rev1 *fadt = find_acpi_table_addr(FACP_SIGNATURE);
> - volatile u32 *resume_vector_ptr = find_resume_vector_addr();
> + struct facs_descriptor_rev1 *facs = find_acpi_table_addr(FACS_SIGNATURE);
> char *addr, *resume_vec = (void*)0x1000;
>
> - *resume_vector_ptr = (u32)(ulong)resume_vec;
> + facs->firmware_waking_vector = (u32)(ulong)resume_vec;
>
> - printf("resume vector addr is %p\n", resume_vector_ptr);
> + printf("FACS is at %p\n", facs);
> + printf("resume vector addr is %p\n", &facs->firmware_waking_vector);
> for (addr = &resume_start; addr < &resume_end; addr++)
> *resume_vec++ = *addr;
> printf("copy resume code from %p\n", &resume_start);
> --
> 2.25.1
>
Thanks,
drew
