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

[Heinrich Schuchardt]

On 5/16/22 14:22, Stefan Agner wrote:
> 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.

Acked-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> --
> 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))