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: Stefan Agner
Subject: Re: [PATCH v2] efidisk: pass buffers with higher alignment
Date: Tue, 17 May 2022 17:23:50 +0200
User-agent: Roundcube Webmail/1.4.9

On 2022-05-17 17:21, Daniel Kiper wrote:
> 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.
>>

Agreed, this is likely due to misinterpretation of that section. Also,
the section stayed the same since the first revision I was able to
access (1.10).

I'll add that to 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.

I was just drafting an email pointing out that it should work since I
don't do the bit shifting stuff from v1 anymore :)

Will send a revision with updated commit message.

--
Stefan

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