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

[Heinrich Schuchardt]

On 5/5/22 15:54, Stefan Agner wrote:
> Despite the UEFI specification saying "the requirement is that the
> start address of a buffer must be evenly divisible by IoAlign with
> no remainder.", it seems that a higher alignment requirement is
> neecssary on some system (e.g. a Intel NUC system with NVMe SSD).
> That particular system has IoAlign set to 2, and sometimes returns
> status 7 when buffers with alignment of 2 are passed. Things seem
> to work fine with buffers aligned to 4 bytes.
>
> It seems that IoAlign > 1 means 2 ^ IoAlign. There is also such a hint
> in an example printed in the Driver Writer's Guide:
> ScsiPassThruMode.IoAlign = 2; // Data must be alligned on 4-byte boundary
>
> Pass 2 ^ IoAlign aligned buffers to make sure GRUB2 works properly on
> all systems.
>
> 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.
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>   grub-core/disk/efi/efidisk.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
> index f077b5f55..0fc2f0826 100644
> --- a/grub-core/disk/efi/efidisk.c
> +++ b/grub-core/disk/efi/efidisk.c
> @@ -553,8 +553,16 @@ 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 means alignment by 2^IoAlign

The UEFI specification unambiguously says:

"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."

For instance U-Boot will set this value to the sector size (typically
512). As

  0 == (int)(1 << 512)

with your code below you would set the alignment to 0 for U-Boot which
is obviously wrong.

There may be UEFI implementations that are broken. To support these you
might want to set the alignment value to the maximum of sector size and
media->io_align.

Best regards

Heinrich

> +   * Note: UEFI spec claims alignment by IoAlign. But there are systems
> +   * with IoAlign=2 which return status 7 if 2 bytes aligned buffers are
> +   * passed.
> +   */
> +  if (bio->media->io_align > 1)
> +    io_align = 1 << bio->media->io_align;
> +  else
> +    io_align = 1;
>     num_bytes = size << disk->log_sector_size;
>
>     if ((grub_addr_t) buf & (io_align - 1))