On Thu, Feb 18, 2016 at 1:21 PM, Leif Lindholm <leif.lindh...@linaro.org> 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 <jeremy.lin...@arm.com>
>> > ---
>> >
>> > 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.
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?
> 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.
>> > + {
>> > + if (!wr)
>> > + grub_memcpy (buf, aligned_buf, num_bytes);
>> > + grub_free (aligned_buf);
>> > + }
>> > +
>> > + return status;
>> > }
>> >
>> > static grub_err_t
>> >
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
