[PATCH 0/2] Alternative efidisk alignment resolution

[PATCH 0/2] Alternative efidisk alignment resolution

[Leif Lindholm]

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

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

[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);
+}
+
 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

[PATCH 2/2] efidisk: respect block_io_protocol minimum buffer alignment

[Leif Lindholm]

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

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

[Andrei Borzenkov]

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;
>  
> 

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 <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

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

[Andrei Borzenkov]

[-- 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)

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

[Andrei Borzenkov]

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;
>  
> 

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

[Vladimir 'phcoder' Serbinenko]

[-- 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 --]

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

[Leif Lindholm]

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

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 <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?

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

[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.

/
    Leif

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

[Andrei Borzenkov]

[-- 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)

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

[Andrei Borzenkov]

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.

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

[Vladimir 'phcoder' Serbinenko]

[-- 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 --]

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

[Andrei Borzenkov]

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
>

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

[Vladimir 'phcoder' Serbinenko]

[-- 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 --]

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

[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.
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.

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

[Andrei Borzenkov]

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
> 

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

[Leif Lindholm]

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