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

[Heinrich Schuchardt]

On 5/9/22 21:45, Stefan Agner wrote:
> 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?

UEFI firmware vendors try to pass the UEFI Self Certification Tests. But
only for the Ata Pass Thru Protocol I could find a check for IoAlign.

A test of IoAlign for the IO block protocol is missing. It should both
test that IoAlign is a power of two and that a buffer which has no
greater alignment then IoAlign is usable. You could file a bug report at
https://bugzilla.tianocore.org/ for product 'EDK2 Test' to have such a
test added.

A typical constraint for alignment is given by DMA transfer which often
requires 32byte or larger alignment.

Here are some alignment values used by EDK II:

OvmfPkg/VirtioScsiDxe/VirtioScsi.c:1172:
Dev->PassThruMode.IoAlign = 0;

NetworkPkg/IScsiDxe/IScsiMisc.c:1793:
Private->ExtScsiPassThruMode.IoAlign    = 4;

Platform/ARM/JunoPkg/Drivers/SataSiI3132Dxe/SataSiI3132.c:108:
AtaPassThruMode->IoAlign = 0x1000;

Silicon/Hisilicon/Drivers/SasV1Dxe/SasV1Dxe.c:920:
SasV1Info->ExtScsiPassThruMode.IoAlign  = 64; //cache line align

There is no easy way for GRUB to identify misbehaving UEFI
implementations. Therefore I would not use an alignment that is smaller
than the sector size when allocating buffers.

Best regards

Heinrich

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