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

Message ID 20160222140255.GO1159@bivouac.eciton.net
State New
Headers show

Commit Message

Leif Lindholm Feb. 22, 2016, 2:02 p.m.
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 <leif.lindholm@linaro.org>
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(-)

Comments

Leif Lindholm Feb. 24, 2016, 11:59 a.m. | #1
On Mon, Feb 22, 2016 at 07:56:25PM +0300, Andrei Borzenkov wrote:
> 22.02.2016 17:02, 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.
> > 
> 
> Could you test attached patch with your alignment fixes on top. This
> implements my idea of using shared buffers. Seems to work in naive testing.

Testing this with a grub-mkstandalone image, I get:

kern/dl.c:556: flushing 0x10f1 bytes at 0x9ffb5ac20
kern/dl.c:649: module name: tar
kern/dl.c:650: init function: 0x9ffb5b220
kern/disk.c:217: Opening `memdisk'...
kern/fs.c:56: Detecting tarfs...

And then spectacular crash in UEFI due to an EL2 translation fault.

Regards,

Leif
Leif Lindholm Feb. 24, 2016, 1:57 p.m. | #2
On Wed, Feb 24, 2016 at 03:09:13PM +0300, Andrei Borzenkov wrote:
> >> Could you test attached patch with your alignment fixes on top. This

> >> implements my idea of using shared buffers. Seems to work in naive testing.

> >

> > Testing this with a grub-mkstandalone image, I get:

> >

> > kern/dl.c:556: flushing 0x10f1 bytes at 0x9ffb5ac20

> > kern/dl.c:649: module name: tar

> > kern/dl.c:650: init function: 0x9ffb5b220

> > kern/disk.c:217: Opening `memdisk'...

> > kern/fs.c:56: Detecting tarfs...

> >

> > And then spectacular crash in UEFI due to an EL2 translation fault.

> 

> To be sure - is it with my patch alone or with your patches? If some

> more patches are used - could you send exact diff to trunk to avoid

> misunderstanding?


Double checked with only your patch on top of
1b782e902e69516f82158203674d4951a40c82d4 (previously running with
_only_ my alignment fixup in efidisk.c). Same behaviour.

/
    Leif

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Leif Lindholm March 1, 2016, 5:11 p.m. | #3
On 24 February 2016 at 17:40, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
> 24.02.2016 16:57, Leif Lindholm пишет:
>> On Wed, Feb 24, 2016 at 03:09:13PM +0300, Andrei Borzenkov wrote:
>>>>> Could you test attached patch with your alignment fixes on top. This
>>>>> implements my idea of using shared buffers. Seems to work in naive testing.
>>>>
>>>> Testing this with a grub-mkstandalone image, I get:
>>>>
>>>> kern/dl.c:556: flushing 0x10f1 bytes at 0x9ffb5ac20
>>>> kern/dl.c:649: module name: tar
>>>> kern/dl.c:650: init function: 0x9ffb5b220
>>>> kern/disk.c:217: Opening `memdisk'...
>>>> kern/fs.c:56: Detecting tarfs...
>>>>
>>>> And then spectacular crash in UEFI due to an EL2 translation fault.
>>>
>>> To be sure - is it with my patch alone or with your patches? If some
>>> more patches are used - could you send exact diff to trunk to avoid
>>> misunderstanding?
>>
>> Double checked with only your patch on top of
>> 1b782e902e69516f82158203674d4951a40c82d4 (previously running with
>> _only_ my alignment fixup in efidisk.c). Same behaviour.
>
> I cannot reproduce it on x86_64 (also with mm-debug enabled) and I do
> not know how to load standalone image on ppc; is it possible to use QEMU
> to run ARM64 (I assume you have it)? If not what are options to test it?
>
> Anyway, there was one problem I fixed later (although I did not get any
> issue before as well), I attach updated version. Thank you for testing!

I can confirm that:
1) The first patch exhibits no bad behaviour on QEMU [1].
2) The second patch exhibits no bad behaviour on either QEMU nor HW.

This still generates fixups in efidisk for each call though.
Could I rework my disk->malloc patch on top of this for 2.02 release?

/
    Leif

[1] qemu-system-aarch64 -m 1024 -cpu cortex-a57 -M virt -bios
QEMU_EFI.fd -nographic -hda fat:fat/
With QEMU_EFI.fd from
http://releases.linaro.org/components/kernel/uefi-linaro/16.02/debug/qemu64/
And fat/ being a directory holding the image generated with grub-mkstandalone.
Leif Lindholm March 1, 2016, 8:19 p.m. | #4
On Tue, Mar 01, 2016 at 10:46:16PM +0300, Andrei Borzenkov wrote:
> >> I cannot reproduce it on x86_64 (also with mm-debug enabled) and I do

> >> not know how to load standalone image on ppc; is it possible to use QEMU

> >> to run ARM64 (I assume you have it)? If not what are options to test it?

> >>

> >> Anyway, there was one problem I fixed later (although I did not get any

> >> issue before as well), I attach updated version. Thank you for testing!

> > 

> > I can confirm that:

> > 1) The first patch exhibits no bad behaviour on QEMU [1].

> > 2) The second patch exhibits no bad behaviour on either QEMU nor HW.

> > 

> > This still generates fixups in efidisk for each call though.

> 

> Of course; it was intended to add framework for direct-to-cache IO, it

> did not change alignment.


Sure.

> > Could I rework my disk->malloc patch on top of this for 2.02 release?

> 

> Of course!


Splendid. Coming up.

> Not sure whether it is 2.02 material though, but we can keep

> it in next then. Hopefully 2.03 won't take so long :)


Well, it is a <=2x slowdown, and not just on platforms that previously
broke, but also on any platforms that specified alignment that was not
previously obeyed (but still worked).

> > /

> >     Leif

> > 

> > [1] qemu-system-aarch64 -m 1024 -cpu cortex-a57 -M virt -bios

> > QEMU_EFI.fd -nographic -hda fat:fat/

> > With QEMU_EFI.fd from

> > http://releases.linaro.org/components/kernel/uefi-linaro/16.02/debug/qemu64/

> > And fat/ being a directory holding the image generated with grub-mkstandalone.

> > 

> > _______________________________________________

> > Grub-devel mailing list

> > Grub-devel@gnu.org

> > https://lists.gnu.org/mailman/listinfo/grub-devel

> > 

> 

> 

> _______________________________________________

> Grub-devel mailing list

> Grub-devel@gnu.org

> https://lists.gnu.org/mailman/listinfo/grub-devel


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Patch hide | download patch | download mbox

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