grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Daniel Kiper
Subject: Re: [PATCH v2] efidisk: pass buffers with higher alignment
Date: Tue, 17 May 2022 17:21:04 +0200
User-agent: NeoMutt/20170113 (1.7.2)

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]