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