On Tue, May 17, 2022 at 05:07:49PM +0200, Daniel Kiper wrote:
> 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?
Ugh... Sorry, I missed earlier thread. So, I am rewinding discussion a bit.
Anyway, please expand commit message and include as much information as
possible. Mostly it should come from the discussion around v1.
Daniel
> > >> 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 <ste...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
