Respect EFI block-io buffer alignment

Respect EFI block-io buffer alignment

[Leif Lindholm]

This resolves a complete failure to access devices connected to the
SATA port on the ARM ltd. Juno platform (apart from a violation of the
UEFI block io protocol).

The below is a bit of a hack, but I'd like some feedback on preferred
solution before over(or under)engineering something.

As far as I can tell, a struct_disk is only ever allocated in
kern/disk.c, using grub_zalloc(). So the only reason for the horrid
ifdefs is that there is no grub_memalign for EMU.

Do I:
- Keep the ifdefs?
- Implement grub_memalign() for EMU?
- Something else?

/
    Leif

From 2d8b7ae2dd4c639252517e9a1df783ed0564c112 Mon Sep 17 00:00:00 2001
From: Leif Lindholm <leif.lindholm@linaro.org>
Date: Wed, 17 Feb 2016 15:33:47 +0000
Subject: [PATCH] efidisk: Respect block_io_protocol buffer alignment

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.

Add an io_align field to struct grub_disk, and use it in kern/disk.c
when GRUB_MACHINE_EFI is defined.

Reported-by: Jeremy Linton <jeremy.linton@arm.com>
---
 grub-core/disk/efi/efidisk.c | 1 +
 grub-core/kern/disk.c        | 9 +++++++++
 include/grub/disk.h          | 3 +++
 3 files changed, 13 insertions(+)

diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
index 1c00e3e..424ff92 100644
--- a/grub-core/disk/efi/efidisk.c
+++ b/grub-core/disk/efi/efidisk.c
@@ -495,6 +495,7 @@ grub_efidisk_open (const char *name, struct grub_disk *disk)
      and total sectors should be replaced with total blocks.  */
   grub_dprintf ("efidisk", "m = %p, last block = %llx, block size = %x\n",
 		m, (unsigned long long) m->last_block, m->block_size);
+  disk->io_align = m->io_align;
   disk->total_sectors = m->last_block + 1;
   /* Don't increase this value due to bug in some EFI.  */
   disk->max_agglomerate = 0xa0000 >> (GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS);
diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c
index 789f8c0..27bef10 100644
--- a/grub-core/kern/disk.c
+++ b/grub-core/kern/disk.c
@@ -331,7 +331,12 @@ grub_disk_read_small_real (grub_disk_t disk, grub_disk_addr_t sector,
     }
 
   /* Allocate a temporary buffer.  */
+#ifdef GRUB_MACHINE_EFI
+  tmp_buf = grub_memalign (disk->io_align,
+			   GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS);
+#else
   tmp_buf = grub_malloc (GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS);
+#endif
   if (! tmp_buf)
     return grub_errno;
 
@@ -373,7 +378,11 @@ grub_disk_read_small_real (grub_disk_t disk, grub_disk_addr_t sector,
     num = ((size + offset + (1ULL << (disk->log_sector_size))
 	    - 1) >> (disk->log_sector_size));
 
+#ifdef GRUB_MACHINE_EFI
+    tmp_buf = grub_memalign (disk->io_align, num << disk->log_sector_size);
+#else
     tmp_buf = grub_malloc (num << disk->log_sector_size);
+#endif
     if (!tmp_buf)
       return grub_errno;
     
diff --git a/include/grub/disk.h b/include/grub/disk.h
index b385af8..b063bf6 100644
--- a/include/grub/disk.h
+++ b/include/grub/disk.h
@@ -126,6 +126,9 @@ struct grub_disk
   /* Logarithm of sector size.  */
   unsigned int log_sector_size;
 
+  /* Minimum read/write buffer alignment */
+  unsigned int io_align;
+
   /* Maximum number of sectors read divided by GRUB_DISK_CACHE_SIZE.  */
   unsigned int max_agglomerate;
 
-- 
2.1.4

Re: Respect EFI block-io buffer alignment

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 4271 bytes --]

On 17.02.2016 16:48, Leif Lindholm wrote:
> This resolves a complete failure to access devices connected to the
> SATA port on the ARM ltd. Juno platform (apart from a violation of the
> UEFI block io protocol).
> 
> The below is a bit of a hack, but I'd like some feedback on preferred
> solution before over(or under)engineering something.
> 
> As far as I can tell, a struct_disk is only ever allocated in
> kern/disk.c, using grub_zalloc(). So the only reason for the horrid
> ifdefs is that there is no grub_memalign for EMU.
> 
> Do I:
> - Keep the ifdefs?
> - Implement grub_memalign() for EMU?
You could insipire by grub_osdep_dl_memalign
> - Something else?
> 
The code as-is will not work. Buf is passed from external call to
grub_disk_read and grub_disk_read tries to read in-place whenever
possible. There are 2 cases in current codebase when we need a special
buffer: usbms and ahci. In both cases corresponding disk routines
allocate corresponding temporary buffer. While it would be nicer to
avoid such a buffer, it will result in messy code and I doubt there is
any speed benefit. So feel free to create temporary buffer or prove me
wrong about speed difference.
> /
>     Leif
> 
>>From 2d8b7ae2dd4c639252517e9a1df783ed0564c112 Mon Sep 17 00:00:00 2001
> From: Leif Lindholm <leif.lindholm@linaro.org>
> Date: Wed, 17 Feb 2016 15:33:47 +0000
> Subject: [PATCH] efidisk: Respect block_io_protocol buffer alignment
> 
> 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.
> 
> Add an io_align field to struct grub_disk, and use it in kern/disk.c
> when GRUB_MACHINE_EFI is defined.
> 
> Reported-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  grub-core/disk/efi/efidisk.c | 1 +
>  grub-core/kern/disk.c        | 9 +++++++++
>  include/grub/disk.h          | 3 +++
>  3 files changed, 13 insertions(+)
> 
> diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
> index 1c00e3e..424ff92 100644
> --- a/grub-core/disk/efi/efidisk.c
> +++ b/grub-core/disk/efi/efidisk.c
> @@ -495,6 +495,7 @@ grub_efidisk_open (const char *name, struct grub_disk *disk)
>       and total sectors should be replaced with total blocks.  */
>    grub_dprintf ("efidisk", "m = %p, last block = %llx, block size = %x\n",
>  		m, (unsigned long long) m->last_block, m->block_size);
> +  disk->io_align = m->io_align;
>    disk->total_sectors = m->last_block + 1;
>    /* Don't increase this value due to bug in some EFI.  */
>    disk->max_agglomerate = 0xa0000 >> (GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS);
> diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c
> index 789f8c0..27bef10 100644
> --- a/grub-core/kern/disk.c
> +++ b/grub-core/kern/disk.c
> @@ -331,7 +331,12 @@ grub_disk_read_small_real (grub_disk_t disk, grub_disk_addr_t sector,
>      }
>  
>    /* Allocate a temporary buffer.  */
> +#ifdef GRUB_MACHINE_EFI
> +  tmp_buf = grub_memalign (disk->io_align,
> +			   GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS);
> +#else
>    tmp_buf = grub_malloc (GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS);
> +#endif
>    if (! tmp_buf)
>      return grub_errno;
>  
> @@ -373,7 +378,11 @@ grub_disk_read_small_real (grub_disk_t disk, grub_disk_addr_t sector,
>      num = ((size + offset + (1ULL << (disk->log_sector_size))
>  	    - 1) >> (disk->log_sector_size));
>  
> +#ifdef GRUB_MACHINE_EFI
> +    tmp_buf = grub_memalign (disk->io_align, num << disk->log_sector_size);
> +#else
>      tmp_buf = grub_malloc (num << disk->log_sector_size);
> +#endif
>      if (!tmp_buf)
>        return grub_errno;
>      
> diff --git a/include/grub/disk.h b/include/grub/disk.h
> index b385af8..b063bf6 100644
> --- a/include/grub/disk.h
> +++ b/include/grub/disk.h
> @@ -126,6 +126,9 @@ struct grub_disk
>    /* Logarithm of sector size.  */
>    unsigned int log_sector_size;
>  
> +  /* Minimum read/write buffer alignment */
> +  unsigned int io_align;
> +
>    /* Maximum number of sectors read divided by GRUB_DISK_CACHE_SIZE.  */
>    unsigned int max_agglomerate;
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: Respect EFI block-io buffer alignment

[Andrei Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 1091 bytes --]

17.02.2016 19:23, Vladimir 'φ-coder/phcoder' Serbinenko пишет:
> On 17.02.2016 16:48, Leif Lindholm wrote:
>> This resolves a complete failure to access devices connected to the
>> SATA port on the ARM ltd. Juno platform (apart from a violation of the
>> UEFI block io protocol).
>>
>> The below is a bit of a hack, but I'd like some feedback on preferred
>> solution before over(or under)engineering something.
>>
>> As far as I can tell, a struct_disk is only ever allocated in
>> kern/disk.c, using grub_zalloc(). So the only reason for the horrid
>> ifdefs is that there is no grub_memalign for EMU.
>>
>> Do I:
>> - Keep the ifdefs?
>> - Implement grub_memalign() for EMU?
> You could insipire by grub_osdep_dl_memalign
>> - Something else?
>>
> The code as-is will not work. Buf is passed from external call to
> grub_disk_read and grub_disk_read tries to read in-place whenever
> possible. There are 2 cases in current codebase when we need a special

It can be changed to read into cache and copy in buf instead of read
into buf and copy in cache.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: Respect EFI block-io buffer alignment

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1532 bytes --]

On 17.02.2016 18:00, Andrei Borzenkov wrote:
> 17.02.2016 19:23, Vladimir 'φ-coder/phcoder' Serbinenko пишет:
>> On 17.02.2016 16:48, Leif Lindholm wrote:
>>> This resolves a complete failure to access devices connected to the
>>> SATA port on the ARM ltd. Juno platform (apart from a violation of the
>>> UEFI block io protocol).
>>>
>>> The below is a bit of a hack, but I'd like some feedback on preferred
>>> solution before over(or under)engineering something.
>>>
>>> As far as I can tell, a struct_disk is only ever allocated in
>>> kern/disk.c, using grub_zalloc(). So the only reason for the horrid
>>> ifdefs is that there is no grub_memalign for EMU.
>>>
>>> Do I:
>>> - Keep the ifdefs?
>>> - Implement grub_memalign() for EMU?
>> You could insipire by grub_osdep_dl_memalign
>>> - Something else?
>>>
>> The code as-is will not work. Buf is passed from external call to
>> grub_disk_read and grub_disk_read tries to read in-place whenever
>> possible. There are 2 cases in current codebase when we need a special
> 
> It can be changed to read into cache and copy in buf instead of read
> into buf and copy in cache.
> 
This is often not very good idea as you lose buffering as you split
transactions. This was a reason GRUB2 was slower than GRUB Legacy on
some systems until it was rewritten to current code.
> 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

[PATCH] efidisk: Respect block_io_protocol buffer alignment

[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.linton@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;
+  num_bytes = size << disk->log_sector_size;
+
+  if ((unsigned long) buf & (io_align - 1))
+    {
+      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))
+    {
+      if (!wr)
+	grub_memcpy (buf, aligned_buf, num_bytes);
+      grub_free (aligned_buf);
+    }
+
+  return status;
 }
 
 static grub_err_t
-- 
2.1.4

Re: [PATCH] efidisk: Respect block_io_protocol buffer alignment

[Andrei Borzenkov]

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.linton@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.

> +  num_bytes = size << disk->log_sector_size;
> +
> +  if ((unsigned long) buf & (io_align - 1))

grub_addr_t?

> +    {
> +      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))
> +    {
> +      if (!wr)
> +	grub_memcpy (buf, aligned_buf, num_bytes);
> +      grub_free (aligned_buf);
> +    }
> +
> +  return status;
>  }
>  
>  static grub_err_t
> 

Re: [PATCH] efidisk: Respect block_io_protocol buffer alignment

[Leif Lindholm]

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.linton@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?

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

Re: [PATCH] efidisk: Respect block_io_protocol buffer alignment

[Andrei Borzenkov]

On Thu, Feb 18, 2016 at 1:21 PM, Leif Lindholm <leif.lindholm@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.linton@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
>> >

Re: [PATCH] efidisk: Respect block_io_protocol buffer alignment

[Leif Lindholm]

On Thu, Feb 18, 2016 at 03:00:38PM +0300, Andrei Borzenkov wrote:
> On Thu, Feb 18, 2016 at 1:21 PM, Leif Lindholm <leif.lindholm@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.linton@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.

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