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

[Andrei Borzenkov]

On Wed, Feb 24, 2016 at 2:59 PM, Leif Lindholm <address@hidden> wrote:
> 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.
>

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?