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

[Stefan Agner]

Hi Heinrich,

On 2022-05-10 22:40, 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
>>>
>>> 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);

Do you agree with the patch as its stands?

I do have to return a test system which showed the issue. If more
changes are required I'd like to get them still tested as long as I have
access to it.

--
Stefan

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