Re: [PATCH 1/2] disk: Add support for device-specific malloc function

[Leif Lindholm]

On Mon, Feb 22, 2016 at 10:57:24AM +0300, Andrei Borzenkov wrote:
> 19.02.2016 19:18, Leif Lindholm пишет:
> > Some disk types have allocation requirements beyond normal grub_malloc.
> > Add a function pointer to grub_disk_t and a wrapper function in
> > kern/disk.c making use of that function if available, to enable these
> > disk drivers to implement their own malloc.
> 
> The problem is not (only) grub_disk_read_small(), but this part in
> grub_disk_read:
> 
>       if (agglomerate)
>         {
>           grub_disk_addr_t i;
> 
>           err = (disk->dev->read) (disk, transform_sector (disk, sector),
>                                    agglomerate << (GRUB_DISK_CACHE_BITS
>                                                    + GRUB_DISK_SECTOR_BITS
>                                                    - disk->log_sector_size),
>                                    buf);
> 
> which reads directly into user supplied buffer. May be we can allocate
> contiguous cache block here but put pointers to multiple chunks inside
> it. Possible implementation is to have second layer of reference counted
> memory blocks with cache entries containing pointer + offset into them.

Whoops!

Understood.

So how about merging the two concepts?
Including a patch to go with (after) the previous two to catch any
remaining unaligned accesses in grub_efidisk_readwrite().
With this applied, I get no fixups from a normal Linux boot (linux +
initrd), but see them when exploring filesystems from the command
line.

Whilst a bit clunky, this seems much short-term preferable to going
back and redesigning the disk subsystem to understand that alignment
matters. Although given the number of exceptions we seem to be
amassing, that does not sound like a bad idea for future.

And hopefully we can get rid of things like these:
https://github.com/tianocore/edk2/blob/master/OvmfPkg/XenPvBlkDxe/BlockIo.c#L117

Regards,

Leif

>From 41bea6c3fdcbf97d05ce8f90e1300f7a347b8a34 Mon Sep 17 00:00:00 2001
From: Leif Lindholm <address@hidden>
Date: Mon, 22 Feb 2016 13:44:46 +0000
Subject: [PATCH] efidisk: handle unaligned buffers

With a dedicated buffer allocator, the vast majority of efidisk
accesses are now performed compliant to block_io_protocol.
Implement temporary buffers to fix up any remaining unaligned
buffers, such as refernces directly into the disk cache.
---
 grub-core/disk/efi/efidisk.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
index 9b42585..4c5caf2 100644
--- a/grub-core/disk/efi/efidisk.c
+++ b/grub-core/disk/efi/efidisk.c
@@ -539,15 +539,41 @@ 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;
+  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))
+    {
+      grub_dprintf ("efidisk", "using temporary buffer to handle alignment\n");
+      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,
+                      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 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);
+  return status;
 }
 
 static grub_err_t
-- 
2.1.4