* [PATCH 0/2] Alternative efidisk alignment resolution @ 2016-02-19 16:18 Leif Lindholm 2016-02-19 16:18 ` [PATCH 1/2] disk: Add support for device-specific malloc function Leif Lindholm 2016-02-19 16:18 ` [PATCH 2/2] efidisk: respect block_io_protocol minimum buffer alignment Leif Lindholm 0 siblings, 2 replies; 19+ messages in thread From: Leif Lindholm @ 2016-02-19 16:18 UTC (permalink / raw) To: grub-devel The tmp buffer approach did add noticeable delay when booting with a full distro kernel and initrd. This alternative implementation starts by adding a hook for a device specific malloc function into grub_disk_t, and a wrapper in disk.c making use of it. Leif Lindholm (2): disk: Add support for device-specific malloc function efidisk: respect block_io_protocol minimum buffer alignment grub-core/disk/efi/efidisk.c | 19 +++++++++++++++++-- grub-core/kern/disk.c | 13 +++++++++++-- include/grub/disk.h | 5 +++++ 3 files changed, 33 insertions(+), 4 deletions(-) -- 2.1.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/2] disk: Add support for device-specific malloc function 2016-02-19 16:18 [PATCH 0/2] Alternative efidisk alignment resolution Leif Lindholm @ 2016-02-19 16:18 ` Leif Lindholm 2016-02-22 7:57 ` Andrei Borzenkov 2016-02-22 16:57 ` Andrei Borzenkov 2016-02-19 16:18 ` [PATCH 2/2] efidisk: respect block_io_protocol minimum buffer alignment Leif Lindholm 1 sibling, 2 replies; 19+ messages in thread From: Leif Lindholm @ 2016-02-19 16:18 UTC (permalink / raw) To: grub-devel 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. --- grub-core/kern/disk.c | 13 +++++++++++-- include/grub/disk.h | 5 +++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c index 789f8c0..e186586 100644 --- a/grub-core/kern/disk.c +++ b/grub-core/kern/disk.c @@ -184,6 +184,15 @@ find_part_sep (const char *name) return NULL; } +static void * +disk_malloc (struct grub_disk *disk, grub_size_t size) +{ + if (disk->malloc) + return disk->malloc (disk, size); + + return grub_malloc (size); +} + grub_disk_t grub_disk_open (const char *name) { @@ -331,7 +340,7 @@ grub_disk_read_small_real (grub_disk_t disk, grub_disk_addr_t sector, } /* Allocate a temporary buffer. */ - tmp_buf = grub_malloc (GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS); + tmp_buf = disk_malloc (disk, GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS); if (! tmp_buf) return grub_errno; @@ -373,7 +382,7 @@ 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)); - tmp_buf = grub_malloc (num << disk->log_sector_size); + tmp_buf = disk_malloc (disk, num << disk->log_sector_size); if (!tmp_buf) return grub_errno; diff --git a/include/grub/disk.h b/include/grub/disk.h index b385af8..0fdd779 100644 --- a/include/grub/disk.h +++ b/include/grub/disk.h @@ -111,6 +111,8 @@ typedef void (*grub_disk_read_hook_t) (grub_disk_addr_t sector, unsigned offset, unsigned length, void *data); +typedef void *(*grub_disk_malloc_t) (struct grub_disk *disk, grub_size_t size); + /* Disk. */ struct grub_disk { @@ -144,6 +146,9 @@ struct grub_disk /* Device-specific data. */ void *data; + + /* Device-specific malloc function. */ + grub_disk_malloc_t malloc; }; typedef struct grub_disk *grub_disk_t; -- 2.1.4 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] disk: Add support for device-specific malloc function 2016-02-19 16:18 ` [PATCH 1/2] disk: Add support for device-specific malloc function Leif Lindholm @ 2016-02-22 7:57 ` Andrei Borzenkov 2016-02-22 14:02 ` Leif Lindholm 2016-02-22 16:57 ` Andrei Borzenkov 1 sibling, 1 reply; 19+ messages in thread From: Andrei Borzenkov @ 2016-02-22 7:57 UTC (permalink / raw) To: grub-devel 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. > --- > grub-core/kern/disk.c | 13 +++++++++++-- > include/grub/disk.h | 5 +++++ > 2 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c > index 789f8c0..e186586 100644 > --- a/grub-core/kern/disk.c > +++ b/grub-core/kern/disk.c > @@ -184,6 +184,15 @@ find_part_sep (const char *name) > return NULL; > } > > +static void * > +disk_malloc (struct grub_disk *disk, grub_size_t size) > +{ > + if (disk->malloc) > + return disk->malloc (disk, size); > + > + return grub_malloc (size); > +} > + > grub_disk_t > grub_disk_open (const char *name) > { > @@ -331,7 +340,7 @@ grub_disk_read_small_real (grub_disk_t disk, grub_disk_addr_t sector, > } > > /* Allocate a temporary buffer. */ > - tmp_buf = grub_malloc (GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS); > + tmp_buf = disk_malloc (disk, GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS); > if (! tmp_buf) > return grub_errno; > > @@ -373,7 +382,7 @@ 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)); > > - tmp_buf = grub_malloc (num << disk->log_sector_size); > + tmp_buf = disk_malloc (disk, num << disk->log_sector_size); > if (!tmp_buf) > return grub_errno; > > diff --git a/include/grub/disk.h b/include/grub/disk.h > index b385af8..0fdd779 100644 > --- a/include/grub/disk.h > +++ b/include/grub/disk.h > @@ -111,6 +111,8 @@ typedef void (*grub_disk_read_hook_t) (grub_disk_addr_t sector, > unsigned offset, unsigned length, > void *data); > > +typedef void *(*grub_disk_malloc_t) (struct grub_disk *disk, grub_size_t size); > + > /* Disk. */ > struct grub_disk > { > @@ -144,6 +146,9 @@ struct grub_disk > > /* Device-specific data. */ > void *data; > + > + /* Device-specific malloc function. */ > + grub_disk_malloc_t malloc; > }; > typedef struct grub_disk *grub_disk_t; > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] disk: Add support for device-specific malloc function 2016-02-22 7:57 ` Andrei Borzenkov @ 2016-02-22 14:02 ` Leif Lindholm 2016-02-22 16:56 ` Andrei Borzenkov 0 siblings, 1 reply; 19+ messages in thread From: Leif Lindholm @ 2016-02-22 14:02 UTC (permalink / raw) To: The development of GNU GRUB 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(-) 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] disk: Add support for device-specific malloc function 2016-02-22 14:02 ` Leif Lindholm @ 2016-02-22 16:56 ` Andrei Borzenkov 2016-02-24 11:59 ` Leif Lindholm 0 siblings, 1 reply; 19+ messages in thread From: Andrei Borzenkov @ 2016-02-22 16:56 UTC (permalink / raw) To: grub-devel [-- Attachment #1: Type: text/plain, Size: 2021 bytes --] 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. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: disk-direct-cache-read.patch --] [-- Type: text/x-patch; name="disk-direct-cache-read.patch", Size: 6984 bytes --] From: Andrei Borzenkov <arvidjaar@gmail.com> Subject: [PATCH] disk: read into cache directly <patch description> --- grub-core/kern/disk.c | 94 ++++++++++++++++++++++++++++++--------------------- grub-core/lib/disk.c | 6 ++-- include/grub/disk.h | 11 +++++- 3 files changed, 69 insertions(+), 42 deletions(-) diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c index 789f8c0..945c146 100644 --- a/grub-core/kern/disk.c +++ b/grub-core/kern/disk.c @@ -56,6 +56,20 @@ grub_err_t (*grub_disk_write_weak) (grub_disk_t disk, #include "disk_common.c" void +grub_disk_cache_free (struct grub_cache_buffer *buf) +{ + if (!buf->count) + /* FIXME This means corruption; what can we do? */ + return; + + if (!--buf->count) + { + grub_free (buf->data); + grub_free (buf); + } +} + +void grub_disk_cache_invalidate_all (void) { unsigned i; @@ -64,10 +78,10 @@ grub_disk_cache_invalidate_all (void) { struct grub_disk_cache *cache = grub_disk_cache_table + i; - if (cache->data && ! cache->lock) + if (cache->buffer && ! cache->lock) { - grub_free (cache->data); - cache->data = 0; + grub_disk_cache_free (cache->buffer); + cache->buffer = 0; } } } @@ -82,14 +96,14 @@ grub_disk_cache_fetch (unsigned long dev_id, unsigned long disk_id, cache_index = grub_disk_cache_get_index (dev_id, disk_id, sector); cache = grub_disk_cache_table + cache_index; - if (cache->dev_id == dev_id && cache->disk_id == disk_id + if (cache->buffer && cache->dev_id == dev_id && cache->disk_id == disk_id && cache->sector == sector) { cache->lock = 1; #if DISK_CACHE_STATS grub_disk_cache_hits++; #endif - return cache->data; + return cache->buffer->data + cache->offset; } #if DISK_CACHE_STATS @@ -116,28 +130,35 @@ grub_disk_cache_unlock (unsigned long dev_id, unsigned long disk_id, static grub_err_t grub_disk_cache_store (unsigned long dev_id, unsigned long disk_id, - grub_disk_addr_t sector, const char *data) + grub_disk_addr_t sector, grub_disk_addr_t n, char *data) { - unsigned cache_index; - struct grub_disk_cache *cache; + struct grub_cache_buffer *buf; + grub_addr_t offset; - cache_index = grub_disk_cache_get_index (dev_id, disk_id, sector); - cache = grub_disk_cache_table + cache_index; + buf = grub_malloc (sizeof (*buf)); + if (! buf) + return grub_errno; + buf->data = data; + buf->count = 0; - cache->lock = 1; - grub_free (cache->data); - cache->data = 0; - cache->lock = 0; + for (offset = 0 ; n > 0; sector += GRUB_DISK_CACHE_SIZE, offset += (GRUB_DISK_CACHE_SIZE * GRUB_DISK_SECTOR_SIZE), n--) + { + unsigned cache_index; + struct grub_disk_cache *cache; - cache->data = grub_malloc (GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS); - if (! cache->data) - return grub_errno; + cache_index = grub_disk_cache_get_index (dev_id, disk_id, sector); + cache = grub_disk_cache_table + cache_index; - grub_memcpy (cache->data, data, - GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS); - cache->dev_id = dev_id; - cache->disk_id = disk_id; - cache->sector = sector; + cache->lock = 1; + grub_disk_cache_free (cache->buffer); + cache->buffer = buf; + cache->offset = offset; + buf->count++; + cache->lock = 0; + cache->dev_id = dev_id; + cache->disk_id = disk_id; + cache->sector = sector; + } return GRUB_ERR_NONE; } @@ -350,13 +371,11 @@ grub_disk_read_small_real (grub_disk_t disk, grub_disk_addr_t sector, /* Copy it and store it in the disk cache. */ grub_memcpy (buf, tmp_buf + offset, size); grub_disk_cache_store (disk->dev->id, disk->id, - sector, tmp_buf); - grub_free (tmp_buf); + sector, 1, tmp_buf); return GRUB_ERR_NONE; } } - grub_free (tmp_buf); grub_errno = GRUB_ERR_NONE; { @@ -373,10 +392,6 @@ 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)); - tmp_buf = grub_malloc (num << disk->log_sector_size); - if (!tmp_buf) - return grub_errno; - if ((disk->dev->read) (disk, transform_sector (disk, aligned_sector), num, tmp_buf)) { @@ -481,22 +496,25 @@ grub_disk_read (grub_disk_t disk, grub_disk_addr_t sector, if (agglomerate) { - grub_disk_addr_t i; + void *cache = grub_malloc (agglomerate << (GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS)); + if (!cache) + return grub_errno; err = (disk->dev->read) (disk, transform_sector (disk, sector), agglomerate << (GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS - disk->log_sector_size), - buf); + cache); if (err) - return err; + { + grub_free (cache); + return err; + } - for (i = 0; i < agglomerate; i ++) - grub_disk_cache_store (disk->dev->id, disk->id, - sector + (i << GRUB_DISK_CACHE_BITS), - (char *) buf - + (i << (GRUB_DISK_CACHE_BITS - + GRUB_DISK_SECTOR_BITS))); + grub_memcpy (buf, cache, + agglomerate << (GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS)); + grub_disk_cache_store (disk->dev->id, disk->id, + sector, agglomerate, cache); if (disk->read_hook) diff --git a/grub-core/lib/disk.c b/grub-core/lib/disk.c index 0f18688..07fb117 100644 --- a/grub-core/lib/disk.c +++ b/grub-core/lib/disk.c @@ -42,11 +42,11 @@ grub_disk_cache_invalidate (unsigned long dev_id, unsigned long disk_id, cache = grub_disk_cache_table + cache_index; if (cache->dev_id == dev_id && cache->disk_id == disk_id - && cache->sector == sector && cache->data) + && cache->sector == sector && cache->buffer) { cache->lock = 1; - grub_free (cache->data); - cache->data = 0; + grub_disk_cache_free (cache->buffer); + cache->buffer = 0; cache->lock = 0; } } diff --git a/include/grub/disk.h b/include/grub/disk.h index b385af8..9e60367 100644 --- a/include/grub/disk.h +++ b/include/grub/disk.h @@ -233,16 +233,25 @@ grub_stop_disk_firmware (void) } } +struct grub_cache_buffer + { + char *data; + unsigned count; + }; + /* Disk cache. */ struct grub_disk_cache { enum grub_disk_dev_id dev_id; unsigned long disk_id; grub_disk_addr_t sector; - char *data; + struct grub_cache_buffer *buffer; + grub_addr_t offset; int lock; }; +void EXPORT_FUNC(grub_disk_cache_free) (struct grub_cache_buffer *buf); + extern struct grub_disk_cache EXPORT_VAR(grub_disk_cache_table)[GRUB_DISK_CACHE_NUM]; #if defined (GRUB_UTIL) -- tg: (bc22096..) e/disk-cache-align (depends on: master) ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] disk: Add support for device-specific malloc function 2016-02-22 16:56 ` Andrei Borzenkov @ 2016-02-24 11:59 ` Leif Lindholm 2016-02-24 12:09 ` Andrei Borzenkov 0 siblings, 1 reply; 19+ messages in thread From: Leif Lindholm @ 2016-02-24 11:59 UTC (permalink / raw) To: The development of GNU GRUB 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] disk: Add support for device-specific malloc function 2016-02-24 11:59 ` Leif Lindholm @ 2016-02-24 12:09 ` Andrei Borzenkov 2016-02-24 13:57 ` Leif Lindholm 0 siblings, 1 reply; 19+ messages in thread From: Andrei Borzenkov @ 2016-02-24 12:09 UTC (permalink / raw) To: The development of GNU GRUB On Wed, Feb 24, 2016 at 2:59 PM, Leif Lindholm <leif.lindholm@linaro.org> 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? ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] disk: Add support for device-specific malloc function 2016-02-24 12:09 ` Andrei Borzenkov @ 2016-02-24 13:57 ` Leif Lindholm 2016-02-24 17:40 ` Andrei Borzenkov 0 siblings, 1 reply; 19+ messages in thread From: Leif Lindholm @ 2016-02-24 13:57 UTC (permalink / raw) To: The development of GNU GRUB 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] disk: Add support for device-specific malloc function 2016-02-24 13:57 ` Leif Lindholm @ 2016-02-24 17:40 ` Andrei Borzenkov 2016-02-24 19:04 ` Andrei Borzenkov 2016-03-01 17:11 ` Leif Lindholm 0 siblings, 2 replies; 19+ messages in thread From: Andrei Borzenkov @ 2016-02-24 17:40 UTC (permalink / raw) To: grub-devel [-- Attachment #1: Type: text/plain, Size: 1331 bytes --] 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! [-- Attachment #2: disk-direct-cache-read-v2.patch --] [-- Type: text/x-patch, Size: 6772 bytes --] From: Andrei Borzenkov <arvidjaar@gmail.com> Subject: [PATCH] disk: read into cache directly <patch description> --- grub-core/kern/disk.c | 95 ++++++++++++++++++++++++++++++--------------------- grub-core/lib/disk.c | 6 ++-- include/grub/disk.h | 11 +++++- 3 files changed, 70 insertions(+), 42 deletions(-) diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c index 789f8c0..04ab2ab 100644 --- a/grub-core/kern/disk.c +++ b/grub-core/kern/disk.c @@ -56,6 +56,20 @@ grub_err_t (*grub_disk_write_weak) (grub_disk_t disk, #include "disk_common.c" void +grub_disk_cache_free (struct grub_cache_buffer *buf) +{ + if (!buf->count) + /* FIXME This means corruption; what can we do? */ + return; + + if (!--buf->count) + { + grub_free (buf->data); + grub_free (buf); + } +} + +void grub_disk_cache_invalidate_all (void) { unsigned i; @@ -64,10 +78,10 @@ grub_disk_cache_invalidate_all (void) { struct grub_disk_cache *cache = grub_disk_cache_table + i; - if (cache->data && ! cache->lock) + if (cache->buffer && ! cache->lock) { - grub_free (cache->data); - cache->data = 0; + grub_disk_cache_free (cache->buffer); + cache->buffer = 0; } } } @@ -82,14 +96,14 @@ grub_disk_cache_fetch (unsigned long dev_id, unsigned long disk_id, cache_index = grub_disk_cache_get_index (dev_id, disk_id, sector); cache = grub_disk_cache_table + cache_index; - if (cache->dev_id == dev_id && cache->disk_id == disk_id + if (cache->buffer && cache->dev_id == dev_id && cache->disk_id == disk_id && cache->sector == sector) { cache->lock = 1; #if DISK_CACHE_STATS grub_disk_cache_hits++; #endif - return cache->data; + return cache->buffer->data + cache->offset; } #if DISK_CACHE_STATS @@ -116,28 +130,36 @@ grub_disk_cache_unlock (unsigned long dev_id, unsigned long disk_id, static grub_err_t grub_disk_cache_store (unsigned long dev_id, unsigned long disk_id, - grub_disk_addr_t sector, const char *data) + grub_disk_addr_t sector, grub_disk_addr_t n, char *data) { - unsigned cache_index; - struct grub_disk_cache *cache; + struct grub_cache_buffer *buf; + grub_addr_t offset; - cache_index = grub_disk_cache_get_index (dev_id, disk_id, sector); - cache = grub_disk_cache_table + cache_index; + buf = grub_malloc (sizeof (*buf)); + if (! buf) + return grub_errno; + buf->data = data; + buf->count = 0; - cache->lock = 1; - grub_free (cache->data); - cache->data = 0; - cache->lock = 0; + for (offset = 0 ; n > 0; sector += GRUB_DISK_CACHE_SIZE, offset += (GRUB_DISK_CACHE_SIZE * GRUB_DISK_SECTOR_SIZE), n--) + { + unsigned cache_index; + struct grub_disk_cache *cache; - cache->data = grub_malloc (GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS); - if (! cache->data) - return grub_errno; + cache_index = grub_disk_cache_get_index (dev_id, disk_id, sector); + cache = grub_disk_cache_table + cache_index; - grub_memcpy (cache->data, data, - GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS); - cache->dev_id = dev_id; - cache->disk_id = disk_id; - cache->sector = sector; + cache->lock = 1; + if (cache->buffer) + grub_disk_cache_free (cache->buffer); + cache->buffer = buf; + cache->offset = offset; + buf->count++; + cache->lock = 0; + cache->dev_id = dev_id; + cache->disk_id = disk_id; + cache->sector = sector; + } return GRUB_ERR_NONE; } @@ -350,13 +372,11 @@ grub_disk_read_small_real (grub_disk_t disk, grub_disk_addr_t sector, /* Copy it and store it in the disk cache. */ grub_memcpy (buf, tmp_buf + offset, size); grub_disk_cache_store (disk->dev->id, disk->id, - sector, tmp_buf); - grub_free (tmp_buf); + sector, 1, tmp_buf); return GRUB_ERR_NONE; } } - grub_free (tmp_buf); grub_errno = GRUB_ERR_NONE; { @@ -373,10 +393,6 @@ 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)); - tmp_buf = grub_malloc (num << disk->log_sector_size); - if (!tmp_buf) - return grub_errno; - if ((disk->dev->read) (disk, transform_sector (disk, aligned_sector), num, tmp_buf)) { @@ -481,22 +497,25 @@ grub_disk_read (grub_disk_t disk, grub_disk_addr_t sector, if (agglomerate) { - grub_disk_addr_t i; + void *cache = grub_malloc (agglomerate << (GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS)); + if (!cache) + return grub_errno; err = (disk->dev->read) (disk, transform_sector (disk, sector), agglomerate << (GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS - disk->log_sector_size), - buf); + cache); if (err) - return err; + { + grub_free (cache); + return err; + } - for (i = 0; i < agglomerate; i ++) - grub_disk_cache_store (disk->dev->id, disk->id, - sector + (i << GRUB_DISK_CACHE_BITS), - (char *) buf - + (i << (GRUB_DISK_CACHE_BITS - + GRUB_DISK_SECTOR_BITS))); + grub_memcpy (buf, cache, + agglomerate << (GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS)); + grub_disk_cache_store (disk->dev->id, disk->id, + sector, agglomerate, cache); if (disk->read_hook) diff --git a/grub-core/lib/disk.c b/grub-core/lib/disk.c index 0f18688..07fb117 100644 --- a/grub-core/lib/disk.c +++ b/grub-core/lib/disk.c @@ -42,11 +42,11 @@ grub_disk_cache_invalidate (unsigned long dev_id, unsigned long disk_id, cache = grub_disk_cache_table + cache_index; if (cache->dev_id == dev_id && cache->disk_id == disk_id - && cache->sector == sector && cache->data) + && cache->sector == sector && cache->buffer) { cache->lock = 1; - grub_free (cache->data); - cache->data = 0; + grub_disk_cache_free (cache->buffer); + cache->buffer = 0; cache->lock = 0; } } diff --git a/include/grub/disk.h b/include/grub/disk.h index b385af8..9e60367 100644 --- a/include/grub/disk.h +++ b/include/grub/disk.h @@ -233,16 +233,25 @@ grub_stop_disk_firmware (void) } } +struct grub_cache_buffer + { + char *data; + unsigned count; + }; + /* Disk cache. */ struct grub_disk_cache { enum grub_disk_dev_id dev_id; unsigned long disk_id; grub_disk_addr_t sector; - char *data; + struct grub_cache_buffer *buffer; + grub_addr_t offset; int lock; }; +void EXPORT_FUNC(grub_disk_cache_free) (struct grub_cache_buffer *buf); + extern struct grub_disk_cache EXPORT_VAR(grub_disk_cache_table)[GRUB_DISK_CACHE_NUM]; #if defined (GRUB_UTIL) -- tg: (bc22096..) e/disk-cache-align (depends on: master) ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] disk: Add support for device-specific malloc function 2016-02-24 17:40 ` Andrei Borzenkov @ 2016-02-24 19:04 ` Andrei Borzenkov 2016-02-26 13:27 ` Vladimir 'phcoder' Serbinenko 2016-03-01 17:11 ` Leif Lindholm 1 sibling, 1 reply; 19+ messages in thread From: Andrei Borzenkov @ 2016-02-24 19:04 UTC (permalink / raw) To: grub-devel 24.02.2016 20:40, Andrei Borzenkov пишет: > 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 still cannot reproduce it with either patch version using current GIT QEMU + binary QEMU_EFI.fd from http://snapshots.linaro.org/components/kernel/leg-virt-tianocore-edk2-upstream/554/QEMU-AARCH64/RELEASE_GCC49/QEMU_EFI.fd; I run it as aarch64-softmmu/qemu-system-aarch64 -m 1024 -cpu cortex-a57 -M virt -bios ~/vm/QEMU_EFI.fd -cdrom /tmp/grub.iso -serial stdio Where grub.iso is built pkgdatadir=$PWD ./grub-mkstandalone -d grub-core -O arm64-efi -o /tmp/grub.efi pkgdatadir=$PWD ./grub-mkrescue -d grub-core -o /tmp/grub.iso /grub.efi=/tmp/grub.efi and I do chainloader /grub.efi boot after that. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] disk: Add support for device-specific malloc function 2016-02-24 19:04 ` Andrei Borzenkov @ 2016-02-26 13:27 ` Vladimir 'phcoder' Serbinenko 2016-02-26 13:48 ` Andrei Borzenkov 0 siblings, 1 reply; 19+ messages in thread From: Vladimir 'phcoder' Serbinenko @ 2016-02-26 13:27 UTC (permalink / raw) To: The development of GNU GRUB [-- Attachment #1: Type: text/plain, Size: 2672 bytes --] Andrei, your patch is pretty complicated and would be a subject to putting into next branch because of its breakage potential. Issue at hand affects x86 as well. Can we split the solution from radical rework of disk.c with optimisations? Le mer. 24 févr. 2016 20:04, Andrei Borzenkov <arvidjaar@gmail.com> a écrit : > 24.02.2016 20:40, Andrei Borzenkov пишет: > > 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 still cannot reproduce it with either patch version using current GIT > QEMU + binary QEMU_EFI.fd from > > http://snapshots.linaro.org/components/kernel/leg-virt-tianocore-edk2-upstream/554/QEMU-AARCH64/RELEASE_GCC49/QEMU_EFI.fd > ; > I run it as > > aarch64-softmmu/qemu-system-aarch64 -m 1024 -cpu cortex-a57 -M virt > -bios ~/vm/QEMU_EFI.fd -cdrom /tmp/grub.iso -serial stdio > > Where grub.iso is built > > pkgdatadir=$PWD ./grub-mkstandalone -d grub-core -O arm64-efi -o > /tmp/grub.efi > pkgdatadir=$PWD ./grub-mkrescue -d grub-core -o /tmp/grub.iso > /grub.efi=/tmp/grub.efi > > and I do > > chainloader /grub.efi > boot > > after that. > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel > [-- Attachment #2: Type: text/html, Size: 3570 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] disk: Add support for device-specific malloc function 2016-02-26 13:27 ` Vladimir 'phcoder' Serbinenko @ 2016-02-26 13:48 ` Andrei Borzenkov 2016-02-26 13:52 ` Vladimir 'phcoder' Serbinenko 0 siblings, 1 reply; 19+ messages in thread From: Andrei Borzenkov @ 2016-02-26 13:48 UTC (permalink / raw) To: The development of GNU GRUB On Fri, Feb 26, 2016 at 4:27 PM, Vladimir 'phcoder' Serbinenko <phcoder@gmail.com> wrote: > Andrei, your patch is pretty complicated and would be a subject to putting > into next branch because of its breakage potential. Issue at hand affects > x86 as well. Can we split the solution from radical rework of disk.c with > optimisations? > Then we need to add additional small read to for unaligned beginning of user supplied buffer in main agglomeration loop. It is doable I think. > Le mer. 24 févr. 2016 20:04, Andrei Borzenkov <arvidjaar@gmail.com> a écrit > : >> >> 24.02.2016 20:40, Andrei Borzenkov пишет: >> > 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 still cannot reproduce it with either patch version using current GIT >> QEMU + binary QEMU_EFI.fd from >> >> http://snapshots.linaro.org/components/kernel/leg-virt-tianocore-edk2-upstream/554/QEMU-AARCH64/RELEASE_GCC49/QEMU_EFI.fd; >> I run it as >> >> aarch64-softmmu/qemu-system-aarch64 -m 1024 -cpu cortex-a57 -M virt >> -bios ~/vm/QEMU_EFI.fd -cdrom /tmp/grub.iso -serial stdio >> >> Where grub.iso is built >> >> pkgdatadir=$PWD ./grub-mkstandalone -d grub-core -O arm64-efi -o >> /tmp/grub.efi >> pkgdatadir=$PWD ./grub-mkrescue -d grub-core -o /tmp/grub.iso >> /grub.efi=/tmp/grub.efi >> >> and I do >> >> chainloader /grub.efi >> boot >> >> after that. >> >> _______________________________________________ >> 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 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] disk: Add support for device-specific malloc function 2016-02-26 13:48 ` Andrei Borzenkov @ 2016-02-26 13:52 ` Vladimir 'phcoder' Serbinenko 0 siblings, 0 replies; 19+ messages in thread From: Vladimir 'phcoder' Serbinenko @ 2016-02-26 13:52 UTC (permalink / raw) To: The development of GNU GRUB [-- Attachment #1: Type: text/plain, Size: 3954 bytes --] Le ven. 26 févr. 2016 14:48, Andrei Borzenkov <arvidjaar@gmail.com> a écrit : > On Fri, Feb 26, 2016 at 4:27 PM, Vladimir 'phcoder' Serbinenko > <phcoder@gmail.com> wrote: > > Andrei, your patch is pretty complicated and would be a subject to > putting > > into next branch because of its breakage potential. Issue at hand affects > > x86 as well. Can we split the solution from radical rework of disk.c with > > optimisations? > > > > Then we need to add additional small read to for unaligned beginning > of user supplied buffer in main agglomeration loop. It is doable I > think. > It's not. If you try, then the remaining read is not sector-aligned. I like Leif's approach: fix small reads and do copying if necessary. This fixes most of problems and lets us fix others whenever we feel to. It also allows us to easier use similar approach for DMA drivers. I'm thinking of having some code to reuse user-supplied buffer for DMA when possible > > > Le mer. 24 févr. 2016 20:04, Andrei Borzenkov <arvidjaar@gmail.com> a > écrit > > : > >> > >> 24.02.2016 20:40, Andrei Borzenkov пишет: > >> > 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 still cannot reproduce it with either patch version using current GIT > >> QEMU + binary QEMU_EFI.fd from > >> > >> > http://snapshots.linaro.org/components/kernel/leg-virt-tianocore-edk2-upstream/554/QEMU-AARCH64/RELEASE_GCC49/QEMU_EFI.fd > ; > >> I run it as > >> > >> aarch64-softmmu/qemu-system-aarch64 -m 1024 -cpu cortex-a57 -M virt > >> -bios ~/vm/QEMU_EFI.fd -cdrom /tmp/grub.iso -serial stdio > >> > >> Where grub.iso is built > >> > >> pkgdatadir=$PWD ./grub-mkstandalone -d grub-core -O arm64-efi -o > >> /tmp/grub.efi > >> pkgdatadir=$PWD ./grub-mkrescue -d grub-core -o /tmp/grub.iso > >> /grub.efi=/tmp/grub.efi > >> > >> and I do > >> > >> chainloader /grub.efi > >> boot > >> > >> after that. > >> > >> _______________________________________________ > >> 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 > [-- Attachment #2: Type: text/html, Size: 5913 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] disk: Add support for device-specific malloc function 2016-02-24 17:40 ` Andrei Borzenkov 2016-02-24 19:04 ` Andrei Borzenkov @ 2016-03-01 17:11 ` Leif Lindholm 2016-03-01 19:46 ` Andrei Borzenkov 1 sibling, 1 reply; 19+ messages in thread From: Leif Lindholm @ 2016-03-01 17:11 UTC (permalink / raw) To: The development of GNU GRUB 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] disk: Add support for device-specific malloc function 2016-03-01 17:11 ` Leif Lindholm @ 2016-03-01 19:46 ` Andrei Borzenkov 2016-03-01 20:19 ` Leif Lindholm 0 siblings, 1 reply; 19+ messages in thread From: Andrei Borzenkov @ 2016-03-01 19:46 UTC (permalink / raw) To: grub-devel 01.03.2016 20:11, Leif Lindholm пишет: > 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. Of course; it was intended to add framework for direct-to-cache IO, it did not change alignment. > Could I rework my disk->malloc patch on top of this for 2.02 release? > Of course! 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 :) > / > 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 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] disk: Add support for device-specific malloc function 2016-03-01 19:46 ` Andrei Borzenkov @ 2016-03-01 20:19 ` Leif Lindholm 0 siblings, 0 replies; 19+ messages in thread From: Leif Lindholm @ 2016-03-01 20:19 UTC (permalink / raw) To: The development of GNU GRUB 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] disk: Add support for device-specific malloc function 2016-02-19 16:18 ` [PATCH 1/2] disk: Add support for device-specific malloc function Leif Lindholm 2016-02-22 7:57 ` Andrei Borzenkov @ 2016-02-22 16:57 ` Andrei Borzenkov 2016-02-22 16:59 ` Vladimir 'phcoder' Serbinenko 1 sibling, 1 reply; 19+ messages in thread From: Andrei Borzenkov @ 2016-02-22 16:57 UTC (permalink / raw) To: grub-devel 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. > --- > grub-core/kern/disk.c | 13 +++++++++++-- > include/grub/disk.h | 5 +++++ > 2 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c > index 789f8c0..e186586 100644 > --- a/grub-core/kern/disk.c > +++ b/grub-core/kern/disk.c > @@ -184,6 +184,15 @@ find_part_sep (const char *name) > return NULL; > } > > +static void * > +disk_malloc (struct grub_disk *disk, grub_size_t size) > +{ > + if (disk->malloc) > + return disk->malloc (disk, size); > + > + return grub_malloc (size); > +} Just initialize disk->malloc to grub_alloc in all other drivers then, no need for repeated runtime check. > + > grub_disk_t > grub_disk_open (const char *name) > { > @@ -331,7 +340,7 @@ grub_disk_read_small_real (grub_disk_t disk, grub_disk_addr_t sector, > } > > /* Allocate a temporary buffer. */ > - tmp_buf = grub_malloc (GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS); > + tmp_buf = disk_malloc (disk, GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS); > if (! tmp_buf) > return grub_errno; > > @@ -373,7 +382,7 @@ 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)); > > - tmp_buf = grub_malloc (num << disk->log_sector_size); > + tmp_buf = disk_malloc (disk, num << disk->log_sector_size); > if (!tmp_buf) > return grub_errno; > > diff --git a/include/grub/disk.h b/include/grub/disk.h > index b385af8..0fdd779 100644 > --- a/include/grub/disk.h > +++ b/include/grub/disk.h > @@ -111,6 +111,8 @@ typedef void (*grub_disk_read_hook_t) (grub_disk_addr_t sector, > unsigned offset, unsigned length, > void *data); > > +typedef void *(*grub_disk_malloc_t) (struct grub_disk *disk, grub_size_t size); > + > /* Disk. */ > struct grub_disk > { > @@ -144,6 +146,9 @@ struct grub_disk > > /* Device-specific data. */ > void *data; > + > + /* Device-specific malloc function. */ > + grub_disk_malloc_t malloc; > }; > typedef struct grub_disk *grub_disk_t; > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] disk: Add support for device-specific malloc function 2016-02-22 16:57 ` Andrei Borzenkov @ 2016-02-22 16:59 ` Vladimir 'phcoder' Serbinenko 0 siblings, 0 replies; 19+ messages in thread From: Vladimir 'phcoder' Serbinenko @ 2016-02-22 16:59 UTC (permalink / raw) To: The development of GNU GRUB [-- Attachment #1: Type: text/plain, Size: 3062 bytes --] Rather than having extra code in all drivers, just put it in common init like we do with log sector size On Mon, 22 Feb 2016 17:59 Andrei Borzenkov <arvidjaar@gmail.com> 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. > > --- > > grub-core/kern/disk.c | 13 +++++++++++-- > > include/grub/disk.h | 5 +++++ > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c > > index 789f8c0..e186586 100644 > > --- a/grub-core/kern/disk.c > > +++ b/grub-core/kern/disk.c > > @@ -184,6 +184,15 @@ find_part_sep (const char *name) > > return NULL; > > } > > > > +static void * > > +disk_malloc (struct grub_disk *disk, grub_size_t size) > > +{ > > + if (disk->malloc) > > + return disk->malloc (disk, size); > > + > > + return grub_malloc (size); > > +} > > Just initialize disk->malloc to grub_alloc in all other drivers then, no > need for repeated runtime check. > > > + > > grub_disk_t > > grub_disk_open (const char *name) > > { > > @@ -331,7 +340,7 @@ grub_disk_read_small_real (grub_disk_t disk, > grub_disk_addr_t sector, > > } > > > > /* Allocate a temporary buffer. */ > > - tmp_buf = grub_malloc (GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS); > > + tmp_buf = disk_malloc (disk, GRUB_DISK_SECTOR_SIZE << > GRUB_DISK_CACHE_BITS); > > if (! tmp_buf) > > return grub_errno; > > > > @@ -373,7 +382,7 @@ 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)); > > > > - tmp_buf = grub_malloc (num << disk->log_sector_size); > > + tmp_buf = disk_malloc (disk, num << disk->log_sector_size); > > if (!tmp_buf) > > return grub_errno; > > > > diff --git a/include/grub/disk.h b/include/grub/disk.h > > index b385af8..0fdd779 100644 > > --- a/include/grub/disk.h > > +++ b/include/grub/disk.h > > @@ -111,6 +111,8 @@ typedef void (*grub_disk_read_hook_t) > (grub_disk_addr_t sector, > > unsigned offset, unsigned length, > > void *data); > > > > +typedef void *(*grub_disk_malloc_t) (struct grub_disk *disk, > grub_size_t size); > > + > > /* Disk. */ > > struct grub_disk > > { > > @@ -144,6 +146,9 @@ struct grub_disk > > > > /* Device-specific data. */ > > void *data; > > + > > + /* Device-specific malloc function. */ > > + grub_disk_malloc_t malloc; > > }; > > typedef struct grub_disk *grub_disk_t; > > > > > > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel > [-- Attachment #2: Type: text/html, Size: 3970 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/2] efidisk: respect block_io_protocol minimum buffer alignment 2016-02-19 16:18 [PATCH 0/2] Alternative efidisk alignment resolution Leif Lindholm 2016-02-19 16:18 ` [PATCH 1/2] disk: Add support for device-specific malloc function Leif Lindholm @ 2016-02-19 16:18 ` Leif Lindholm 1 sibling, 0 replies; 19+ messages in thread From: Leif Lindholm @ 2016-02-19 16:18 UTC (permalink / raw) To: grub-devel 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. Implement a driver-specific malloc function that allocates a buffer with the required alignment. Also verify on open that the alignment value is valid, or return failure. --- grub-core/disk/efi/efidisk.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c index 1c00e3e..9b42585 100644 --- a/grub-core/disk/efi/efidisk.c +++ b/grub-core/disk/efi/efidisk.c @@ -457,6 +457,13 @@ get_device (struct grub_efidisk_data *devices, int num) return 0; } +static void * +aligned_malloc (struct grub_disk *disk, grub_size_t size) +{ + struct grub_efidisk_data *d = disk->data; + return grub_memalign (d->block_io->media->io_align, size); +} + static grub_err_t grub_efidisk_open (const char *name, struct grub_disk *disk) { @@ -493,8 +500,15 @@ grub_efidisk_open (const char *name, struct grub_disk *disk) m = d->block_io->media; /* FIXME: Probably it is better to store the block size in the 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); + grub_dprintf ("efidisk", + "m = %p, last block = %llx, block size = %x, io align = %x\n", + m, (unsigned long long) m->last_block, m->block_size, + m->io_align); + + /* Ensure required buffer alignment is a power of two (or is zero). */ + if (m->io_align & (m->io_align - 1)) + return grub_error (GRUB_ERR_IO, "invalid buffer alignment %d", 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); @@ -505,6 +519,7 @@ grub_efidisk_open (const char *name, struct grub_disk *disk) (1U << disk->log_sector_size) < m->block_size; disk->log_sector_size++); disk->data = d; + disk->malloc = aligned_malloc; grub_dprintf ("efidisk", "opening %s succeeded\n", name); -- 2.1.4 ^ permalink raw reply related [flat|nested] 19+ messages in thread
end of thread, other threads:[~2016-03-01 20:19 UTC | newest] Thread overview: 19+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-02-19 16:18 [PATCH 0/2] Alternative efidisk alignment resolution Leif Lindholm 2016-02-19 16:18 ` [PATCH 1/2] disk: Add support for device-specific malloc function Leif Lindholm 2016-02-22 7:57 ` Andrei Borzenkov 2016-02-22 14:02 ` Leif Lindholm 2016-02-22 16:56 ` Andrei Borzenkov 2016-02-24 11:59 ` Leif Lindholm 2016-02-24 12:09 ` Andrei Borzenkov 2016-02-24 13:57 ` Leif Lindholm 2016-02-24 17:40 ` Andrei Borzenkov 2016-02-24 19:04 ` Andrei Borzenkov 2016-02-26 13:27 ` Vladimir 'phcoder' Serbinenko 2016-02-26 13:48 ` Andrei Borzenkov 2016-02-26 13:52 ` Vladimir 'phcoder' Serbinenko 2016-03-01 17:11 ` Leif Lindholm 2016-03-01 19:46 ` Andrei Borzenkov 2016-03-01 20:19 ` Leif Lindholm 2016-02-22 16:57 ` Andrei Borzenkov 2016-02-22 16:59 ` Vladimir 'phcoder' Serbinenko 2016-02-19 16:18 ` [PATCH 2/2] efidisk: respect block_io_protocol minimum buffer alignment Leif Lindholm
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.