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

[Stefan Agner]

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
>>
>> 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
>> alignment is requested. Also make sure that IoAlign is still respected
>> as per UEFI specification if a higher alignment than block size is
>> requested.
>>
>> 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);

--
Stefan

> 
> Best regards
> 
> Heinrich
> 
>> +        io_align = bio->media->block_size;
>> +      else
>> +        io_align = bio->media->io_align;
>> +    }
>> +  else
>> +    io_align = 1;
>> +
>>     num_bytes = size << disk->log_sector_size;
>>
>>     if ((grub_addr_t) buf & (io_align - 1))