Re: [PATCH v3] efidisk: pass buffers with higher alignment

[Stefan Agner]

On 2022-05-18 10:59, Stefan Agner wrote:
> Some devices report a IoAlign value of 2, however seem to require a
> buffer with higher alignment.

After releasing Home Assistant OS 8.0 publicly, some systems still
refuse to boot even with this patch applied. See bug reports collected
in this issue:
https://github.com/home-assistant/operating-system/issues/1912

At least in one instance using block size alignment helped. I am going
to change to block size aligned unconditionally, and see how that
behaves in wider deployment.

--
Stefan

> 
> The UEFI specification is saying: "IoAlign values of 0 and 1 mean that
> the buffer can be placed anywhere in memory. Otherwise, IoAlign must
> be a power of 2, and the requirement is that the start address of a
> buffer must be evenly divisible by IoAlign with no remainder."
> 
> It seems that this got misinterpreted by some vendors assuming IoAlign
> is 2^IoAlign. There is also such a hint in an example in earlier
> versions of the Driver Writer's Guide:
> ScsiPassThruMode.IoAlign = 2; // Data must be alligned on 4-byte boundary
> 
> However, it is unsafe to just blindly align buffers by 2^IoAlign, as
> this would lead to an overflow for systems which use block size
> alignment (e.g. 512 bytes, for example U-Boot).
> 
> Work around by using an alignment of at least BlockSize (typically 512
> bytes) in case alignment is requested. Also make sure that IoAlign is
> still respected as per UEFI specification if a higher alignment than
> block size is requested.
> 
> Note: The problem has only noticed with compressed squashfs. It seems
> that ext4 (and presumably other file system drivers) pass buffers with
> a higher alignment already.
> 
> Acked-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  grub-core/disk/efi/efidisk.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
> index 9e20af70e..c4eb4f4e7 100644
> --- a/grub-core/disk/efi/efidisk.c
> +++ b/grub-core/disk/efi/efidisk.c
> @@ -553,8 +553,22 @@ grub_efidisk_readwrite (struct grub_disk *disk,
> grub_disk_addr_t sector,
>    d = disk->data;
>    bio = d->block_io;
>  
> -  /* Set alignment to 1 if 0 specified */
> -  io_align = bio->media->io_align ? bio->media->io_align : 1;
> +  /*
> +   * If IoAlign is > 1, it should represent the required alignment. However,
> +   * some UEFI implementation on Intel NUC systems seem to use IoAlign=2 but
> +   * require 2^IoAlign.
> +   * Make sure to align to at least block size if IO alignment is required.
> +   */
> +  if (bio->media->io_align > 1)
> +    {
> +      if (bio->media->io_align < bio->media->block_size)
> +        io_align = bio->media->block_size;
> +      else
> +        io_align = bio->media->io_align;
> +    }
> +  else
> +    io_align = 1;
> +
>    num_bytes = size << disk->log_sector_size;
>  
>    if ((grub_addr_t) buf & (io_align - 1))