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 <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);
--
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))
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
