Re: Respect EFI block-io buffer alignment

[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
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 <address@hidden>
> 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 <address@hidden>
> ---
>  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;
>  
>

signature.asc
Description: OpenPGP digital signature