Re: [PATCH] efidisk: Respect block_io_protocol buffer alignment

[Leif Lindholm]

On Thu, Feb 18, 2016 at 04:27:58PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko 
wrote:
> On 18.02.2016 16:00, Leif Lindholm wrote:
> > --- a/grub-core/disk/efi/efidisk.c
> > +++ b/grub-core/disk/efi/efidisk.c
> > @@ -463,6 +463,7 @@ grub_efidisk_open (const char *name, struct grub_disk 
> > *disk)
> >    int num;
> >    struct grub_efidisk_data *d = 0;
> >    grub_efi_block_io_media_t *m;
> > +  unsigned long i = 0;
> >  
> >    grub_dprintf ("efidisk", "opening %s\n", name);
> >  
> > @@ -491,10 +492,21 @@ grub_efidisk_open (const char *name, struct grub_disk 
> > *disk)
> >  
> >    disk->id = ((num << GRUB_CHAR_BIT) | name[0]);
> >    m = d->block_io->media;
> > +  /* Ensure required buffer alignment is a power of two (or is zero). */
> > +  do
> > +    {
> > +      if ((m->io_align & (1UL << i)) && (m->io_align & ~(1UL << i)))
> > +   return grub_error (GRUB_ERR_IO, "invalid buffer alignment %d",
> > +                      m->io_align);
> > +    }
> > +  while (++i < (sizeof (grub_addr_t) * 8));
> > +
>
> if (m->io_align & (m->io_align - 1)) { ...error handling... }

Fair enough.

> > +  if ((grub_addr_t) buf & (io_align - 1))
> > +    {
> > +      aligned_buf = grub_memalign (io_align, num_bytes);
> > +      if (! aligned_buf)
> > +   return GRUB_EFI_OUT_OF_RESOURCES;
>
> Rather than constantly allocating and deallocating perhaps we should
> allocate maximum size and then always use the same buffer? See
> max_agglomerate. We already limit to at most 640KiB per read on EFI due
> to EFI bugs.

It'd start getting a bit fiddly, since you may have different
alignment requirements for different devices.
Certainly not impossible, but could we make the code functional first
and consider potential optimisations later?

/
    Leif