| Message ID | 20160222140255.GO1159@bivouac.eciton.net |
|---|---|
| State | New |
| Headers | show |
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
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
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.
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
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