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

[Stefan Agner]

On 2022-05-09 21:19, Heinrich Schuchardt wrote:
> On 5/5/22 15:54, 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 IoAlign > 1 means 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
>>
>> Pass 2 ^ IoAlign aligned buffers to make sure GRUB2 works properly on
>> all systems.
>>
>> 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 | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
>> index f077b5f55..0fc2f0826 100644
>> --- a/grub-core/disk/efi/efidisk.c
>> +++ b/grub-core/disk/efi/efidisk.c
>> @@ -553,8 +553,16 @@ 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 means alignment by 2^IoAlign
> 
> The UEFI specification unambiguously says:
> 
> "IoAlign must be a power of 2, and the requirement is that the start
> address of a buffer must be evenly divisible by IoAlign with no remainder."

I know.

What are your thoughts about the comment in the UEFI Driver Writer's
Guide?

> 
> For instance U-Boot will set this value to the sector size (typically
> 512). As
> 
>   0 == (int)(1 << 512)
> 
> with your code below you would set the alignment to 0 for U-Boot which
> is obviously wrong.

Ugh I see. I wonder if there are other system which have such high
alignment requirement. The systems I looked at had it set to 0 or 2.

> There may be UEFI implementations that are broken. To support these you
> might want to set the alignment value to the maximum of sector size and
> media->io_align.

Hm, how can we detect those implementations?

Maybe we could just add a work around for the number 2 (as this is what
the drivers writing guide was claiming to cause an alignment of 4, and
seems to be the actual problematic value in practice).

It seems that multiple Intel system are affected, but also Hyper-V based
virtual machines. This came up when we switched to GRUB2 with Home
Assistant OS during rc phase.
https://github.com/home-assistant/operating-system/issues/1830

--
Stefan

> 
> Best regards
> 
> Heinrich
> 
>> +   * Note: UEFI spec claims alignment by IoAlign. But there are systems
>> +   * with IoAlign=2 which return status 7 if 2 bytes aligned buffers are
>> +   * passed.
>> +   */
>> +  if (bio->media->io_align > 1)
>> +    io_align = 1 << bio->media->io_align;
>> +  else
>> +    io_align = 1;
>>     num_bytes = size << disk->log_sector_size;
>>
>>     if ((grub_addr_t) buf & (io_align - 1))