grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Heinrich Schuchardt
Subject: Re: [PATCH v2] efidisk: pass buffers with higher alignment
Date: Tue, 10 May 2022 22:26:54 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.1

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.

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]