[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] efidisk: Respect block_io_protocol buffer alignment
From: |
Leif Lindholm |
Subject: |
Re: [PATCH] efidisk: Respect block_io_protocol buffer alignment |
Date: |
Thu, 18 Feb 2016 12:22:36 +0000 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Thu, Feb 18, 2016 at 03:00:38PM +0300, Andrei Borzenkov wrote:
> On Thu, Feb 18, 2016 at 1:21 PM, Leif Lindholm <address@hidden> wrote:
> > On Thu, Feb 18, 2016 at 06:36:06AM +0300, Andrei Borzenkov wrote:
> >> 17.02.2016 22:44, Leif Lindholm пишет:
> >> > Returned from the OpenProtocol operation, the grub_efi_block_io_media
> >> > structure contains the io_align field, specifying the minimum alignment
> >> > required for buffers used in any data transfers with the device.
> >> >
> >> > Make grub_efidisk_readwrite() allocate a temporary buffer, aligned to
> >> > this boundary, if the buffer passed to it does not already meet the
> >> > requirements.
> >> >
> >> > Reported-by: Jeremy Linton <address@hidden>
> >> > ---
> >> >
> >> > This modified version is contained entirely within the efidisk driver.
> >> > There is some excessive copying going on, but it removes the risk of
> >> > the changes interfering with other disk drivers.
> >> >
> >> > grub-core/disk/efi/efidisk.c | 37 ++++++++++++++++++++++++++++++++-----
> >> > 1 file changed, 32 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
> >> > index 1c00e3e..901133f 100644
> >> > --- a/grub-core/disk/efi/efidisk.c
> >> > +++ b/grub-core/disk/efi/efidisk.c
> >> > @@ -524,15 +524,42 @@ grub_efidisk_readwrite (struct grub_disk *disk,
> >> > grub_disk_addr_t sector,
> >> > {
> >> > struct grub_efidisk_data *d;
> >> > grub_efi_block_io_t *bio;
> >> > + grub_efi_status_t status;
> >> > + grub_size_t io_align, num_bytes;
> >> > + char *aligned_buf;
> >> >
> >> > d = disk->data;
> >> > bio = d->block_io;
> >> >
> >> > - return efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks), bio,
> >> > - bio->media->media_id,
> >> > - (grub_efi_uint64_t) sector,
> >> > - (grub_efi_uintn_t) size << disk->log_sector_size,
> >> > - buf);
> >> > + /* Set alignment to 1 if 0 specified */
> >> > + io_align = bio->media->io_align ? bio->media->io_align : 1;
> >>
> >> We probably should sanity check that it is power of two in
> >> grub_efidisk_open.
> >
> > The UEFI specification says:
> > "IoAlign values of 0 and 1 mean that the buffer can be placed anywhere
> > in memory. Otherwise, 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."
> >
> > So we certainly could sanity check it, but what's the fallback if
> > firmware presents an invalid value? Scream loudly that things are
> > likely to break and then bail out, or silently round up to the
> > nearest power of two?
>
> grub_efidisk_open() fails if sector size is bad, I guess we should
> follow the suite. Please also add alignment print to grub_dprintf
> there.
Good call, that would be interesting to have.
I'll also add a grub_error() message, given it is a fatal system
error.
> It may be interesting to optionally keep stats how often we hit
> non-aligned buffer. I wonder what alignment requirement your system
> has - is it sector size?
In this instance it was (/happened to coincide with?) page size.
> > Of the two, the former seems more palatable.
> >
> >> > + num_bytes = size << disk->log_sector_size;
> >> > +
> >> > + if ((unsigned long) buf & (io_align - 1))
> >>
> >> grub_addr_t?
> >
> > Yeah, sorry, got lost in refactoring.
> >
> >> > + {
> >> > + aligned_buf = grub_memalign (io_align, num_bytes);
> >> > + if (! aligned_buf)
> >> > + return GRUB_EFI_OUT_OF_RESOURCES;
> >> > + if (wr)
> >> > + grub_memcpy (aligned_buf, buf, num_bytes);
> >> > + }
> >> > + else
> >> > + {
> >> > + aligned_buf = buf;
> >> > + }
> >> > +
> >> > + status = efi_call_5 ((wr ? bio->write_blocks : bio->read_blocks),
> >> > bio,
> >> > + bio->media->media_id, (grub_efi_uint64_t) sector,
> >> > + (grub_efi_uintn_t) num_bytes, aligned_buf);
> >> > +
> >> > + if ((unsigned long) buf & (io_align - 1))
>
> And here too of course.
Already in my tree :)
New version coming up.
/
Leif