Re: [kvm-unit-tests PATCH v2 02/23] lib: Ensure all struct definition for ACPI tables are packed

[Andrew Jones <drjones@redhat.com>]

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@arm.com>
> ---
>  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