Re: [PATCH v2 04/15] ACPI: table: replace __attribute__((packed)) by __packed

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Mar 31, 2021 at 11:55:08PM +0800, Zhang Rui wrote:
> ...

> From e18c942855e2f51e814d057fff4dd951cd0d0907 Mon Sep 17 00:00:00 2001
> From: Zhang Rui <rui.zhang@intel.com>
> Date: Wed, 31 Mar 2021 20:34:13 +0800
> Subject: [PATCH] ACPI: tables: FPDT: Fix 64bit alignment issue
> 
> Some of the 64bit items in FPDT table may be 32bit aligned.
> Using __attribute__((packed)) is not needed in this case, fixing it by
> allowing 32bit alignment for these 64bit items.

1) Can you please add a spec reference for this?  I think it's ACPI
   v6.3, sec 5.2.23.5, or something close to that.

2) The exact layout in memory is prescribed by the spec.  I think
   that's basically what "packed" accomplishes.  I don't understand
   why using "aligned" would be preferable.  Using "aligned" means
   things can be at different offsets depending on the starting
   address of the structure.  We always want the identical layout, no
   matter what the starting address is.

> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/acpi/acpi_fpdt.c | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_fpdt.c b/drivers/acpi/acpi_fpdt.c
> index a89a806a7a2a..94e107b9a114 100644
> --- a/drivers/acpi/acpi_fpdt.c
> +++ b/drivers/acpi/acpi_fpdt.c
> @@ -23,12 +23,14 @@ enum fpdt_subtable_type {
>  	SUBTABLE_S3PT,
>  };
>  
> +typedef u64 __attribute__((aligned(4))) u64_align32;
> +
>  struct fpdt_subtable_entry {
>  	u16 type;		/* refer to enum fpdt_subtable_type */
>  	u8 length;
>  	u8 revision;
>  	u32 reserved;
> -	u64 address;		/* physical address of the S3PT/FBPT table */
> +	u64_align32 address;		/* physical address of the S3PT/FBPT table */
>  };
>  
>  struct fpdt_subtable_header {
> @@ -51,25 +53,25 @@ struct fpdt_record_header {
>  struct resume_performance_record {
>  	struct fpdt_record_header header;
>  	u32 resume_count;
> -	u64 resume_prev;
> -	u64 resume_avg;
> -} __attribute__((packed));
> +	u64_align32 resume_prev;
> +	u64_align32 resume_avg;
> +};
>  
>  struct boot_performance_record {
>  	struct fpdt_record_header header;
>  	u32 reserved;
> -	u64 firmware_start;
> -	u64 bootloader_load;
> -	u64 bootloader_launch;
> -	u64 exitbootservice_start;
> -	u64 exitbootservice_end;
> -} __attribute__((packed));
> +	u64_align32 firmware_start;
> +	u64_align32 bootloader_load;
> +	u64_align32 bootloader_launch;
> +	u64_align32 exitbootservice_start;
> +	u64_align32 exitbootservice_end;
> +};
>  
>  struct suspend_performance_record {
>  	struct fpdt_record_header header;
> -	u64 suspend_start;
> -	u64 suspend_end;
> -} __attribute__((packed));
> +	u64_align32 suspend_start;
> +	u64_align32 suspend_end;
> +};
>  
>  
>  static struct resume_performance_record *record_resume;
> -- 
> 2.17.1
> 
>