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

[Daniel Kiper]

On Tue, May 10, 2022 at 10:40:15PM +0200, Stefan Agner wrote:
> On 2022-05-10 22:26, Heinrich Schuchardt wrote:
> > On 5/10/22 21:59, 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 some system interpret IoAlign > 1 to mean 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

I think this confusion comes from improper interpretation of this part
of UEFI spec: IoAlign Supplies the alignment requirement for any buffer
used in a data transfer. 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...

I think it would be nice to point that out in the commit message.

> >> However, some systems (e.g. U-Boot and some drivers in EDK II) do follow
> >> the UEFI specification.
> >>
> >> Work around by using an alignment of at least 512 bytes in case

When you look at the fix it is not obvious. So, I would update this
sentence. Hmmm... I would prefer to drop 512 number from here....

> >> alignment is requested. Also make sure that IoAlign is still respected
> >> as per UEFI specification if a higher alignment than block size is
> >> requested.

Could you explain how did you come up with this algorithm? Why did not
you use 512 number directly as lower limit as Heinrich suggested? And
your algorithm will not work for buggy UEFI implementations which has
IoAlign > 9. Do we care?

> >> 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 | 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 562543b35..dec4e2e8f 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)
> >
> > block_size is not required to be a power of two (though it typically
> > is). But io_align should be a power of two. So here some logic is
> > missing. E.g.
> >
> > if (bio->media->io_align < bio->media->block_size &&
> >     !(bio->media->block_size & (bio->media->block_size - 1))
> >
> > Or simply use a fixed value 512 as lower limit.
>
> This code in grub_efidisk_open should take care of that:
>
>   if (m->block_size & (m->block_size - 1) || !m->block_size)
>     return grub_error (GRUB_ERR_IO, "invalid sector size %d",
>                      m->block_size);

This should be mentioned in the commit message.

Daniel