[PATCH v2 00/12] A set of trivial patches from the Fedora package

[PATCH v2 00/12] A set of trivial patches from the Fedora package

[Javier Martinez Canillas]

Hello,

This is a v2 a of patch-set consisting of mostly trivial patches that we
are carrying in the Fedora package for some time. This version addresses
some issues pointed out by Daniel Kiper.

Best regards,
Javier

Changes since v1:
- Split patches #8 and #9 since were doing more than one change.
- Add Reviewed-by tag from Daniel Kiper to reviewed patches.


Alexander Graf (1):
  efi/gop: Add support for BLT_ONLY adapters

Andrei Borzenkov (1):
  efi/uga: Use 64 bit for fb_base

Peter Jones (10):
  linux/getroot: Handle rssd storage device names
  efi: Print more debug info in our module loader
  Makefile: Make libgrub.pp depend on config-util.h
  kern: Add grub_debug_enabled()
  normal/completion: Fix possible NULL pointer dereference
  kern: Make grub_error() more verbose
  efi: Print error messages to grub_efi_allocate_pages_real()
  efi/uga: Use video instead of fb as debug condition
  efi/gop: Add debug output on GOP probing
  efi: Fix the type of grub_efi_status_t

 Makefile.am                        |  2 +-
 grub-core/kern/efi/efi.c           | 16 ++++++++++++---
 grub-core/kern/efi/mm.c            | 17 ++++++++++++---
 grub-core/kern/err.c               | 13 ++++++++++--
 grub-core/kern/misc.c              | 21 ++++++++++++++-----
 grub-core/normal/completion.c      |  3 ++-
 grub-core/osdep/linux/getroot.c    | 13 ++++++++++++
 grub-core/video/efi_gop.c          | 10 ++++++++-
 grub-core/video/efi_uga.c          | 33 +++++++++++++++---------------
 include/grub/efi/api.h             |  2 +-
 include/grub/efi/graphics_output.h |  3 ++-
 include/grub/err.h                 |  5 ++++-
 include/grub/misc.h                |  1 +
 13 files changed, 104 insertions(+), 35 deletions(-)

-- 
2.24.1

[PATCH v2 01/12] linux/getroot: Handle rssd storage device names

[Javier Martinez Canillas]

From: Peter Jones <pjones@redhat.com>

The Micron PCIe SSDs Linux driver (mtip32xx) exposes block devices
as /dev/rssd[a-z]+[0-9]*. Add support for these rssd device names.

Signed-off-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---

 grub-core/osdep/linux/getroot.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/grub-core/osdep/linux/getroot.c b/grub-core/osdep/linux/getroot.c
index 90d92d3ad5c..6d9f4e5faa2 100644
--- a/grub-core/osdep/linux/getroot.c
+++ b/grub-core/osdep/linux/getroot.c
@@ -921,6 +921,19 @@ grub_util_part_to_disk (const char *os_dev, struct stat *st,
 	  return path;
 	}
 
+      /* If this is an rssd device. */
+      if ((strncmp ("rssd", p, 4) == 0) && p[4] >= 'a' && p[4] <= 'z')
+	{
+	  char *pp = p + 4;
+	  while (*pp >= 'a' && *pp <= 'z')
+	    pp++;
+	  if (*pp)
+	    *is_part = 1;
+	  /* /dev/rssd[a-z]+[0-9]* */
+	  *pp = '\0';
+	  return path;
+	}
+
       /* If this is a loop device */
       if ((strncmp ("loop", p, 4) == 0) && p[4] >= '0' && p[4] <= '9')
 	{
-- 
2.24.1

[PATCH v2 02/12] efi: Print more debug info in our module loader

[Javier Martinez Canillas]

From: Peter Jones <pjones@redhat.com>

The function that searches the mods section base address does not have
any debug information. Add some debugging outputs that could be useful.

Signed-off-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---

 grub-core/kern/efi/efi.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index 6e1ceb90516..3a708ed7215 100644
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -308,13 +308,23 @@ grub_efi_modules_addr (void)
     }
 
   if (i == coff_header->num_sections)
-    return 0;
+    {
+      grub_dprintf("sections", "section %d is last section; invalid.\n", i);
+      return 0;
+    }
 
   info = (struct grub_module_info *) ((char *) image->image_base
 				      + section->virtual_address);
-  if (info->magic != GRUB_MODULE_MAGIC)
-    return 0;
+  if (section->name[0] != '.' && info->magic != GRUB_MODULE_MAGIC)
+    {
+      grub_dprintf("sections",
+		   "section %d has bad magic %08x, should be %08x\n",
+		   i, info->magic, GRUB_MODULE_MAGIC);
+      return 0;
+    }
 
+  grub_dprintf("sections", "returning section info for section %d: \"%s\"\n",
+	       i, section->name);
   return (grub_addr_t) info;
 }
 
-- 
2.24.1

[PATCH v2 03/12] Makefile: Make libgrub.pp depend on config-util.h

[Javier Martinez Canillas]

From: Peter Jones <pjones@redhat.com>

If you build with "make -j48" a lot, sometimes you see:

gcc -E -DHAVE_CONFIG_H -I. -I..  -Wall -W -DGRUB_UTIL=1 -D_FILE_OFFSET_BITS=64 -I./include -DGRUB_FILE=\"grub_script.tab.h\" -I. -I.. -I. -I.. -I../include -I./include -I../grub-core/lib/libgcrypt-grub/src/  -I../grub-core/lib/minilzo -I../grub-core/lib/xzembed -DMINILZO_HAVE_CONFIG_H -Wall -W -DGRUB_UTIL=1 -D_FILE_OFFSET_BITS=64 -I./include -DGRUB_FILE=\"grub_script.tab.h\" -I. -I.. -I. -I.. -I../include -I./include -I../grub-core/lib/libgcrypt-grub/src/  -I./grub-core/gnulib -I../grub-core/gnulib -I/builddir/build/BUILD/grub-2.02/grub-aarch64-efi-2.02 -D_FILE_OFFSET_BITS=64 \
  -D'GRUB_MOD_INIT(x)=@MARKER@x@' grub_script.tab.h grub_script.yy.h ../grub-core/commands/blocklist.c ../grub-core/commands/macbless.c ../grub-core/commands/xnu_uuid.c ../grub-core/commands/testload.c ../grub-core/commands/ls.c ../grub-core/disk/dmraid_nvidia.c ../grub-core/disk/loopback.c ../grub-core/disk/lvm.c ../grub-core/disk/mdraid_linux.c ../grub-core/disk/mdraid_linux_be.c ../grub-core/disk/mdraid1x_linux.c ../grub-core/disk/raid5_recover.c ../grub-core/disk/raid6_recover.c ../grub-core/font/font.c ../grub-core/gfxmenu/font.c ../grub-core/normal/charset.c ../grub-core/video/fb/fbblit.c ../grub-core/video/fb/fbutil.c ../grub-core/video/fb/fbfill.c ../grub-core/video/fb/video_fb.c ../grub-core/video/video.c ../grub-core/video/capture.c ../grub-core/video/colors.c ../grub-core/unidata.c ../grub-core/io/bufio.c ../grub-core/fs/affs.c ../grub-core/fs/afs.c ../grub-core/fs/bfs.c ../grub-core/fs/btrfs.c ../grub-core/fs/cbfs.c ../grub-core/fs/cpio.c ../grub-core/fs/cpio_be.c ../grub-core/fs/odc.c ../grub-core/fs/newc.c ../grub-core/fs/ext2.c ../grub-core/fs/fat.c ../grub-core/fs/exfat.c ../grub-core/fs/fshelp.c ../grub-core/fs/hfs.c ../grub-core/fs/hfsplus.c ../grub-core/fs/hfspluscomp.c ../grub-core/fs/iso9660.c ../grub-core/fs/jfs.c ../grub-core/fs/minix.c ../grub-core/fs/minix2.c ../grub-core/fs/minix3.c ../grub-core/fs/minix_be.c ../grub-core/fs/minix2_be.c ../grub-core/fs/minix3_be.c ../grub-core/fs/nilfs2.c ../grub-core/fs/ntfs.c ../grub-core/fs/ntfscomp.c ../grub-core/fs/reiserfs.c ../grub-core/fs/romfs.c ../grub-core/fs/sfs.c ../grub-core/fs/squash4.c ../grub-core/fs/tar.c ../grub-core/fs/udf.c ../grub-core/fs/ufs2.c ../grub-core/fs/ufs.c ../grub-core/fs/ufs_be.c ../grub-core/fs/xfs.c ../grub-core/fs/zfs/zfscrypt.c ../grub-core/fs/zfs/zfs.c ../grub-core/fs/zfs/zfsinfo.c ../grub-core/fs/zfs/zfs_lzjb.c ../grub-core/fs/zfs/zfs_lz4.c ../grub-core/fs/zfs/zfs_sha256.c ../grub-core/fs/zfs/zfs_fletcher.c ../grub-core/lib/envblk.c ../grub-core/lib/hexdump.c ../grub-core/lib/LzFind.c ../grub-core/lib/LzmaEnc.c ../grub-core/lib/crc.c ../grub-core/lib/adler32.c ../grub-core/lib/crc64.c ../grub-core/normal/datetime.c ../grub-core/normal/misc.c ../grub-core/partmap/acorn.c ../grub-core/partmap/amiga.c ../grub-core/partmap/apple.c ../grub-core/partmap/sun.c ../grub-core/partmap/plan.c ../grub-core/partmap/dvh.c ../grub-core/partmap/sunpc.c ../grub-core/partmap/bsdlabel.c ../grub-core/partmap/dfly.c ../grub-core/script/function.c ../grub-core/script/lexer.c ../grub-core/script/main.c ../grub-core/script/script.c ../grub-core/script/argv.c ../grub-core/io/gzio.c ../grub-core/io/xzio.c ../grub-core/io/lzopio.c ../grub-core/kern/ia64/dl_helper.c ../grub-core/kern/arm/dl_helper.c ../grub-core/kern/arm64/dl_helper.c ../grub-core/lib/minilzo/minilzo.c ../grub-core/lib/xzembed/xz_dec_bcj.c ../grub-core/lib/xzembed/xz_dec_lzma2.c ../grub-core/lib/xzembed/xz_dec_stream.c ../util/misc.c ../grub-core/kern/command.c ../grub-core/kern/device.c ../grub-core/kern/disk.c ../grub-core/lib/disk.c ../util/getroot.c ../grub-core/osdep/unix/getroot.c ../grub-core/osdep/getroot.c ../grub-core/osdep/devmapper/getroot.c ../grub-core/osdep/relpath.c ../grub-core/kern/emu/hostdisk.c ../grub-core/osdep/devmapper/hostdisk.c ../grub-core/osdep/hostdisk.c ../grub-core/osdep/unix/hostdisk.c ../grub-core/osdep/exec.c ../grub-core/osdep/sleep.c ../grub-core/osdep/password.c ../grub-core/kern/emu/misc.c ../grub-core/kern/emu/mm.c ../grub-core/kern/env.c ../grub-core/kern/err.c ../grub-core/kern/file.c ../grub-core/kern/fs.c ../grub-core/kern/list.c ../grub-core/kern/misc.c ../grub-core/kern/partition.c ../grub-core/lib/crypto.c ../grub-core/disk/luks.c ../grub-core/disk/geli.c ../grub-core/disk/cryptodisk.c ../grub-core/disk/AFSplitter.c ../grub-core/lib/pbkdf2.c ../grub-core/commands/extcmd.c ../grub-core/lib/arg.c ../grub-core/disk/ldm.c ../grub-core/disk/diskfilter.c ../grub-core/partmap/gpt.c ../grub-core/partmap/msdos.c ../grub-core/fs/proc.c ../grub-core/fs/archelp.c > libgrub.pp || (rm -f libgrub.pp; exit 1)
rm -f stamp-h1
touch ../config-util.h.in
cd . && /bin/sh ./config.status config-util.h
config.status: creating config-util.h
In file included from ../include/grub/mm.h:25:0,
                 from ../include/grub/disk.h:29,
                 from ../include/grub/file.h:26,
                 from ../grub-core/fs/btrfs.c:21:
./config.h:38:10: fatal error: ./config-util.h: No such file or directory
 #include <config-util.h>
          ^~~~~~~~~~~~~~~
compilation terminated.
make: *** [Makefile:13098: libgrub.pp] Error 1

This is because libgrub.pp is built with -DGRUB_UTIL=1, which means
it'll try to include config-util.h, but a parallel make is actually
building that file.  I think.

Signed-off-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---

 Makefile.am | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile.am b/Makefile.am
index 1f4bb9b8c5a..bf9c1ba64c9 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -37,7 +37,7 @@ grub_script.yy.c: grub_script.yy.h
 CLEANFILES += grub_script.yy.c grub_script.yy.h
 
 # For libgrub.a
-libgrub.pp: grub_script.tab.h grub_script.yy.h $(libgrubmods_a_SOURCES) $(libgrubkern_a_SOURCES)
+libgrub.pp: config-util.h grub_script.tab.h grub_script.yy.h $(libgrubmods_a_SOURCES) $(libgrubkern_a_SOURCES)
 	$(CPP) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(libgrubmods_a_CPPFLAGS) $(libgrubkern_a_CPPFLAGS) $(CPPFLAGS) \
 	  -D'GRUB_MOD_INIT(x)=@MARKER@x@' $^ > $@ || (rm -f $@; exit 1)
 CLEANFILES += libgrub.pp
-- 
2.24.1

[PATCH v2 04/12] kern: Add grub_debug_enabled()

[Javier Martinez Canillas]

From: Peter Jones <pjones@redhat.com>

Add a grub_debug_enabled() helper function instead of open coding it.

Signed-off-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---

 grub-core/kern/misc.c | 21 ++++++++++++++++-----
 include/grub/misc.h   |  1 +
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
index 76e7fb22872..d205fb1342e 100644
--- a/grub-core/kern/misc.c
+++ b/grub-core/kern/misc.c
@@ -158,17 +158,28 @@ int grub_err_printf (const char *fmt, ...)
 __attribute__ ((alias("grub_printf")));
 #endif
 
+int
+grub_debug_enabled (const char * condition)
+{
+  const char *debug;
+
+  debug = grub_env_get ("debug");
+  if (!debug)
+    return 0;
+
+  if (grub_strword (debug, "all") || grub_strword (debug, condition))
+    return 1;
+
+  return 0;
+}
+
 void
 grub_real_dprintf (const char *file, const int line, const char *condition,
 		   const char *fmt, ...)
 {
   va_list args;
-  const char *debug = grub_env_get ("debug");
-
-  if (! debug)
-    return;
 
-  if (grub_strword (debug, "all") || grub_strword (debug, condition))
+  if (grub_debug_enabled (condition))
     {
       grub_printf ("%s:%d: ", file, line);
       va_start (args, fmt);
diff --git a/include/grub/misc.h b/include/grub/misc.h
index ee48eb7a726..585c2a7644c 100644
--- a/include/grub/misc.h
+++ b/include/grub/misc.h
@@ -322,6 +322,7 @@ grub_puts (const char *s)
 }
 
 int EXPORT_FUNC(grub_puts_) (const char *s);
+int EXPORT_FUNC(grub_debug_enabled) (const char *condition);
 void EXPORT_FUNC(grub_real_dprintf) (const char *file,
                                      const int line,
                                      const char *condition,
-- 
2.24.1

[PATCH v2 05/12] normal/completion: Fix possible NULL pointer dereference

[Javier Martinez Canillas]

From: Peter Jones <pjones@redhat.com>

Coverity Scan reports that the grub_strrchr() function can return NULL if
the character is not found. Check if that's the case for dirfile pointer.

Signed-off-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---

 grub-core/normal/completion.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/grub-core/normal/completion.c b/grub-core/normal/completion.c
index 596102848c1..c07100a8de3 100644
--- a/grub-core/normal/completion.c
+++ b/grub-core/normal/completion.c
@@ -284,7 +284,8 @@ complete_file (void)
 
       /* Cut away the filename part.  */
       dirfile = grub_strrchr (dir, '/');
-      dirfile[1] = '\0';
+      if (dirfile)
+	dirfile[1] = '\0';
 
       /* Iterate the directory.  */
       (fs->fs_dir) (dev, dir, iterate_dir, NULL);
-- 
2.24.1

[PATCH v2 06/12] efi/gop: Add support for BLT_ONLY adapters

[Javier Martinez Canillas]

From: Alexander Graf <agraf@suse.de>

EFI GOP has support for multiple different bitness types of frame buffers
and for a special "BLT only" type which is always defined to be RGBx.

Because grub2 doesn't ever directly access the frame buffer but instead
only renders graphics via the BLT interface anyway, we can easily support
these adapters.

The reason this has come up now is the emerging support for virtio-gpu
in OVMF. That adapter does not have the notion of a memory mapped frame
buffer and thus is BLT only.

Signed-off-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---

 grub-core/video/efi_gop.c          | 2 ++
 include/grub/efi/graphics_output.h | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/grub-core/video/efi_gop.c b/grub-core/video/efi_gop.c
index 7f9d1c2dfa1..c9e40e8d4e9 100644
--- a/grub-core/video/efi_gop.c
+++ b/grub-core/video/efi_gop.c
@@ -121,6 +121,7 @@ grub_video_gop_get_bpp (struct grub_efi_gop_mode_info *in)
     {
     case GRUB_EFI_GOT_BGRA8:
     case GRUB_EFI_GOT_RGBA8:
+    case GRUB_EFI_GOT_BLT_ONLY:
       return 32;
 
     case GRUB_EFI_GOT_BITMASK:
@@ -187,6 +188,7 @@ grub_video_gop_fill_real_mode_info (unsigned mode,
   switch (in->pixel_format)
     {
     case GRUB_EFI_GOT_RGBA8:
+    case GRUB_EFI_GOT_BLT_ONLY:
       out->red_mask_size = 8;
       out->red_field_pos = 0;
       out->green_mask_size = 8;
diff --git a/include/grub/efi/graphics_output.h b/include/grub/efi/graphics_output.h
index 12977741192..e4388127c66 100644
--- a/include/grub/efi/graphics_output.h
+++ b/include/grub/efi/graphics_output.h
@@ -28,7 +28,8 @@ typedef enum
   {
     GRUB_EFI_GOT_RGBA8,
     GRUB_EFI_GOT_BGRA8,
-    GRUB_EFI_GOT_BITMASK
+    GRUB_EFI_GOT_BITMASK,
+    GRUB_EFI_GOT_BLT_ONLY,
   }
   grub_efi_gop_pixel_format_t;
 
-- 
2.24.1

[PATCH v2 07/12] efi/uga: Use 64 bit for fb_base

[Javier Martinez Canillas]

From: Andrei Borzenkov <arvidjaar@gmail.com>

We get 64 bit from PCI BAR but then truncate by assigning to 32 bit.
Make sure to check that pointer does not overflow on 32 bit platform.

Closes: 50931

Signed-off-by: Andrei Borzenkov <arvidjaar@gmail.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---

 grub-core/video/efi_uga.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/grub-core/video/efi_uga.c b/grub-core/video/efi_uga.c
index 044af1d20d3..97a607c01a5 100644
--- a/grub-core/video/efi_uga.c
+++ b/grub-core/video/efi_uga.c
@@ -34,7 +34,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
 
 static grub_efi_guid_t uga_draw_guid = GRUB_EFI_UGA_DRAW_GUID;
 static struct grub_efi_uga_draw_protocol *uga;
-static grub_uint32_t uga_fb;
+static grub_uint64_t uga_fb;
 static grub_uint32_t uga_pitch;
 
 static struct
@@ -52,7 +52,7 @@ static struct
 #define FBTEST_COUNT	8
 
 static int
-find_line_len (grub_uint32_t *fb_base, grub_uint32_t *line_len)
+find_line_len (grub_uint64_t *fb_base, grub_uint32_t *line_len)
 {
   grub_uint32_t *base = (grub_uint32_t *) (grub_addr_t) *fb_base;
   int i;
@@ -67,7 +67,7 @@ find_line_len (grub_uint32_t *fb_base, grub_uint32_t *line_len)
 	    {
 	      if ((base[j] & RGB_MASK) == RGB_MAGIC)
 		{
-		  *fb_base = (grub_uint32_t) (grub_addr_t) base;
+		  *fb_base = (grub_uint64_t) (grub_addr_t) base;
 		  *line_len = j << 2;
 
 		  return 1;
@@ -84,7 +84,7 @@ find_line_len (grub_uint32_t *fb_base, grub_uint32_t *line_len)
 /* Context for find_framebuf.  */
 struct find_framebuf_ctx
 {
-  grub_uint32_t *fb_base;
+  grub_uint64_t *fb_base;
   grub_uint32_t *line_len;
   int found;
 };
@@ -129,7 +129,9 @@ find_card (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)
 	      if (i == 5)
 		break;
 
-	      old_bar2 = grub_pci_read (addr + 4);
+	      i++;
+	      addr += 4;
+	      old_bar2 = grub_pci_read (addr);
 	    }
 	  else
 	    old_bar2 = 0;
@@ -138,10 +140,15 @@ find_card (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)
 	  base64 <<= 32;
 	  base64 |= (old_bar1 & GRUB_PCI_ADDR_MEM_MASK);
 
-	  grub_dprintf ("fb", "%s(%d): 0x%llx\n",
+	  grub_dprintf ("fb", "%s(%d): 0x%" PRIxGRUB_UINT64_T "\n",
 			((old_bar1 & GRUB_PCI_ADDR_MEM_PREFETCH) ?
-			"VMEM" : "MMIO"), i,
-		       (unsigned long long) base64);
+			"VMEM" : "MMIO"), type == GRUB_PCI_ADDR_MEM_TYPE_64 ? i - 1 : i,
+			base64);
+
+#if GRUB_CPU_SIZEOF_VOID_P == 4
+	  if (old_bar2)
+	    continue;
+#endif
 
 	  if ((old_bar1 & GRUB_PCI_ADDR_MEM_PREFETCH) && (! ctx->found))
 	    {
@@ -149,12 +156,6 @@ find_card (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)
 	      if (find_line_len (ctx->fb_base, ctx->line_len))
 		ctx->found++;
 	    }
-
-	  if (type == GRUB_PCI_ADDR_MEM_TYPE_64)
-	    {
-	      i++;
-	      addr += 4;
-	    }
 	}
     }
 
@@ -162,7 +163,7 @@ find_card (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)
 }
 
 static int
-find_framebuf (grub_uint32_t *fb_base, grub_uint32_t *line_len)
+find_framebuf (grub_uint64_t *fb_base, grub_uint32_t *line_len)
 {
   struct find_framebuf_ctx ctx = {
     .fb_base = fb_base,
-- 
2.24.1

[PATCH v2 08/12] kern: Make grub_error() more verbose

[Javier Martinez Canillas]

From: Peter Jones <pjones@redhat.com>

Add file and line to grub_error() output to make troubleshooting easier.

Signed-off-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 grub-core/kern/err.c | 13 +++++++++++--
 include/grub/err.h   |  5 ++++-
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/grub-core/kern/err.c b/grub-core/kern/err.c
index 53c734de70e..aebfe0cf839 100644
--- a/grub-core/kern/err.c
+++ b/grub-core/kern/err.c
@@ -33,15 +33,24 @@ static struct grub_error_saved grub_error_stack_items[GRUB_ERROR_STACK_SIZE];
 static int grub_error_stack_pos;
 static int grub_error_stack_assert;
 
+#ifdef grub_error
+#undef grub_error
+#endif
+
 grub_err_t
-grub_error (grub_err_t n, const char *fmt, ...)
+grub_error (grub_err_t n, const char *file, const int line, const char *fmt, ...)
 {
   va_list ap;
+  int m;
 
   grub_errno = n;
 
+  m = grub_snprintf (grub_errmsg, sizeof (grub_errmsg), "%s:%d:", file, line);
+  if (m < 0)
+    m = 0;
+
   va_start (ap, fmt);
-  grub_vsnprintf (grub_errmsg, sizeof (grub_errmsg), _(fmt), ap);
+  grub_vsnprintf (grub_errmsg + m, sizeof (grub_errmsg) - m, _(fmt), ap);
   va_end (ap);
 
   return n;
diff --git a/include/grub/err.h b/include/grub/err.h
index 24ba9f5f592..b68bbec3c72 100644
--- a/include/grub/err.h
+++ b/include/grub/err.h
@@ -85,7 +85,10 @@ struct grub_error_saved
 extern grub_err_t EXPORT_VAR(grub_errno);
 extern char EXPORT_VAR(grub_errmsg)[GRUB_MAX_ERRMSG];
 
-grub_err_t EXPORT_FUNC(grub_error) (grub_err_t n, const char *fmt, ...);
+grub_err_t EXPORT_FUNC(grub_error) (grub_err_t n, const char *file, const int line, const char *fmt, ...);
+
+#define grub_error(n, fmt, ...) grub_error (n, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
+
 void EXPORT_FUNC(grub_fatal) (const char *fmt, ...) __attribute__ ((noreturn));
 void EXPORT_FUNC(grub_error_push) (void);
 int EXPORT_FUNC(grub_error_pop) (void);
-- 
2.24.1

[PATCH v2 09/12] efi: Print error messages to grub_efi_allocate_pages_real()

[Javier Martinez Canillas]

From: Peter Jones <pjones@redhat.com>

No messages were printed in this function, add some to ease debugging.

Also, the function returns a void * pointer so return NULL instead of
0 to make the code more readable.

Signed-off-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 grub-core/kern/efi/mm.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
index b02fab1b102..457772d57f2 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -125,12 +125,20 @@ grub_efi_allocate_pages_real (grub_efi_physical_address_t address,
 
   /* Limit the memory access to less than 4GB for 32-bit platforms.  */
   if (address > GRUB_EFI_MAX_USABLE_ADDRESS)
-    return 0;
+    {
+      grub_error (GRUB_ERR_BAD_ARGUMENT,
+		  N_("invalid memory address (0x%llx > 0x%llx)"),
+		  address, GRUB_EFI_MAX_USABLE_ADDRESS);
+      return NULL;
+    }
 
   b = grub_efi_system_table->boot_services;
   status = efi_call_4 (b->allocate_pages, alloctype, memtype, pages, &address);
   if (status != GRUB_EFI_SUCCESS)
-    return 0;
+    {
+      grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
+      return NULL;
+    }
 
   if (address == 0)
     {
@@ -140,7 +148,10 @@ grub_efi_allocate_pages_real (grub_efi_physical_address_t address,
       status = efi_call_4 (b->allocate_pages, alloctype, memtype, pages, &address);
       grub_efi_free_pages (0, pages);
       if (status != GRUB_EFI_SUCCESS)
-	return 0;
+	{
+	  grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
+	  return NULL;
+	}
     }
 
   grub_efi_store_alloc (address, pages);
-- 
2.24.1

[PATCH v2 10/12] efi/uga: Use video instead of fb as debug condition

[Javier Martinez Canillas]

From: Peter Jones <pjones@redhat.com>

All other video drivers use "video" as the debug condition instead of "fb"
so change this in the efi/uga driver to make it consistent with the others.

Signed-off-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 grub-core/video/efi_uga.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/video/efi_uga.c b/grub-core/video/efi_uga.c
index 97a607c01a5..e74d6c23500 100644
--- a/grub-core/video/efi_uga.c
+++ b/grub-core/video/efi_uga.c
@@ -110,7 +110,7 @@ find_card (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)
     {
       int i;
 
-      grub_dprintf ("fb", "Display controller: %d:%d.%d\nDevice id: %x\n",
+      grub_dprintf ("video", "Display controller: %d:%d.%d\nDevice id: %x\n",
 		    grub_pci_get_bus (dev), grub_pci_get_device (dev),
 		    grub_pci_get_function (dev), pciid);
       addr += 8;
@@ -140,7 +140,7 @@ find_card (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)
 	  base64 <<= 32;
 	  base64 |= (old_bar1 & GRUB_PCI_ADDR_MEM_MASK);
 
-	  grub_dprintf ("fb", "%s(%d): 0x%" PRIxGRUB_UINT64_T "\n",
+	  grub_dprintf ("video", "%s(%d): 0x%" PRIxGRUB_UINT64_T "\n",
 			((old_bar1 & GRUB_PCI_ADDR_MEM_PREFETCH) ?
 			"VMEM" : "MMIO"), type == GRUB_PCI_ADDR_MEM_TYPE_64 ? i - 1 : i,
 			base64);
-- 
2.24.1

[PATCH v2 11/12] efi/gop: Add debug output on GOP probing

[Javier Martinez Canillas]

From: Peter Jones <pjones@redhat.com>

Add debug information to EFI GOP video driver probing function.

Signed-off-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 grub-core/video/efi_gop.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/grub-core/video/efi_gop.c b/grub-core/video/efi_gop.c
index c9e40e8d4e9..be446f8d291 100644
--- a/grub-core/video/efi_gop.c
+++ b/grub-core/video/efi_gop.c
@@ -71,7 +71,10 @@ check_protocol (void)
   handles = grub_efi_locate_handle (GRUB_EFI_BY_PROTOCOL,
 				    &graphics_output_guid, NULL, &num_handles);
   if (!handles || num_handles == 0)
-    return 0;
+    {
+      grub_dprintf ("video", "GOP: no handles\n");
+      return 0;
+    }
 
   for (i = 0; i < num_handles; i++)
     {
@@ -81,6 +84,7 @@ check_protocol (void)
       grub_video_gop_iterate (check_protocol_hook, &have_usable_mode);
       if (have_usable_mode)
 	{
+	  grub_dprintf ("video", "GOP: found usable mode\n");
 	  grub_free (handles);
 	  return 1;
 	}
@@ -89,6 +93,8 @@ check_protocol (void)
   gop = 0;
   gop_handle = 0;
 
+  grub_dprintf ("video", "GOP: no usable mode\n");
+
   return 0;
 }
 
-- 
2.24.1

[PATCH v2 12/12] efi: Fix the type of grub_efi_status_t

[Javier Martinez Canillas]

From: Peter Jones <pjones@redhat.com>

Currently, in some builds with some checkers, we see:

1. grub-core/disk/efi/efidisk.c:601: error[shiftTooManyBitsSigned]: Shifting signed 64-bit value by 63 bits is undefined behaviour

This is because grub_efi_status_t is defined as grub_efi_intn_t, which is
signed, and shifting into the sign bit is not defined behavior.  UEFI fixed
this in the spec in 2.3:

2.3 | Change the defined type of EFI_STATUS from INTN to UINTN | May 7, 2009

And the current EDK2 code has:
MdePkg/Include/Base.h-//
MdePkg/Include/Base.h-// Status codes common to all execution phases
MdePkg/Include/Base.h-//
MdePkg/Include/Base.h:typedef UINTN RETURN_STATUS;
MdePkg/Include/Base.h-
MdePkg/Include/Base.h-/**
MdePkg/Include/Base.h-  Produces a RETURN_STATUS code with the highest bit set.
MdePkg/Include/Base.h-
MdePkg/Include/Base.h-  @param  StatusCode    The status code value to convert into a warning code.
MdePkg/Include/Base.h-                        StatusCode must be in the range 0x00000000..0x7FFFFFFF.
MdePkg/Include/Base.h-
MdePkg/Include/Base.h-  @return The value specified by StatusCode with the highest bit set.
MdePkg/Include/Base.h-
MdePkg/Include/Base.h-**/
MdePkg/Include/Base.h-#define ENCODE_ERROR(StatusCode)     ((RETURN_STATUS)(MAX_BIT | (StatusCode)))
MdePkg/Include/Base.h-
MdePkg/Include/Base.h-/**
MdePkg/Include/Base.h-  Produces a RETURN_STATUS code with the highest bit clear.
MdePkg/Include/Base.h-
MdePkg/Include/Base.h-  @param  StatusCode    The status code value to convert into a warning code.
MdePkg/Include/Base.h-                        StatusCode must be in the range 0x00000000..0x7FFFFFFF.
MdePkg/Include/Base.h-
MdePkg/Include/Base.h-  @return The value specified by StatusCode with the highest bit clear.
MdePkg/Include/Base.h-
MdePkg/Include/Base.h-**/
MdePkg/Include/Base.h-#define ENCODE_WARNING(StatusCode)   ((RETURN_STATUS)(StatusCode))
MdePkg/Include/Base.h-
MdePkg/Include/Base.h-/**
MdePkg/Include/Base.h-  Returns TRUE if a specified RETURN_STATUS code is an error code.
MdePkg/Include/Base.h-
MdePkg/Include/Base.h-  This function returns TRUE if StatusCode has the high bit set.  Otherwise, FALSE is returned.
MdePkg/Include/Base.h-
MdePkg/Include/Base.h-  @param  StatusCode    The status code value to evaluate.
MdePkg/Include/Base.h-
MdePkg/Include/Base.h-  @retval TRUE          The high bit of StatusCode is set.
MdePkg/Include/Base.h-  @retval FALSE         The high bit of StatusCode is clear.
MdePkg/Include/Base.h-
MdePkg/Include/Base.h-**/
MdePkg/Include/Base.h-#define RETURN_ERROR(StatusCode)     (((INTN)(RETURN_STATUS)(StatusCode)) < 0)
...
Uefi/UefiBaseType.h:typedef RETURN_STATUS             EFI_STATUS;

This patch makes grub's implementation match the Edk2 declaration with regards
to the signedness of the type.

Signed-off-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

---

 include/grub/efi/api.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
index 57607438435..d6f4de17d63 100644
--- a/include/grub/efi/api.h
+++ b/include/grub/efi/api.h
@@ -531,7 +531,7 @@ typedef grub_uint64_t grub_efi_uint64_t;
 typedef grub_uint8_t grub_efi_char8_t;
 typedef grub_uint16_t grub_efi_char16_t;
 
-typedef grub_efi_intn_t grub_efi_status_t;
+typedef grub_efi_uintn_t grub_efi_status_t;
 
 #define GRUB_EFI_ERROR_CODE(value)	\
   ((((grub_efi_status_t) 1) << (sizeof (grub_efi_status_t) * 8 - 1)) | (value))
-- 
2.24.1

Re: [PATCH v2 08/12] kern: Make grub_error() more verbose

[Daniel Kiper]

On Wed, Mar 04, 2020 at 12:58:47PM +0100, Javier Martinez Canillas wrote:
> From: Peter Jones <pjones@redhat.com>
>
> Add file and line to grub_error() output to make troubleshooting easier.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH v2 09/12] efi: Print error messages to grub_efi_allocate_pages_real()

[Daniel Kiper]

On Wed, Mar 04, 2020 at 12:58:48PM +0100, Javier Martinez Canillas wrote:
> From: Peter Jones <pjones@redhat.com>
>
> No messages were printed in this function, add some to ease debugging.
>
> Also, the function returns a void * pointer so return NULL instead of
> 0 to make the code more readable.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH v2 10/12] efi/uga: Use video instead of fb as debug condition

[Daniel Kiper]

On Wed, Mar 04, 2020 at 12:58:49PM +0100, Javier Martinez Canillas wrote:
> From: Peter Jones <pjones@redhat.com>
>
> All other video drivers use "video" as the debug condition instead of "fb"
> so change this in the efi/uga driver to make it consistent with the others.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH v2 11/12] efi/gop: Add debug output on GOP probing

[Daniel Kiper]

On Wed, Mar 04, 2020 at 12:58:50PM +0100, Javier Martinez Canillas wrote:
> From: Peter Jones <pjones@redhat.com>
>
> Add debug information to EFI GOP video driver probing function.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH v2 08/12] kern: Make grub_error() more verbose

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 2543 bytes --]

Please evaluate size increase for this. In the past passing file and line
number to grub_dprintf was a huge source of increased Kern and core size

Le mer. 4 mars 2020 à 13:01, Javier Martinez Canillas <javierm@redhat.com>
a écrit :

> From: Peter Jones <pjones@redhat.com>
>
> Add file and line to grub_error() output to make troubleshooting easier.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
>  grub-core/kern/err.c | 13 +++++++++++--
>  include/grub/err.h   |  5 ++++-
>  2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/kern/err.c b/grub-core/kern/err.c
> index 53c734de70e..aebfe0cf839 100644
> --- a/grub-core/kern/err.c
> +++ b/grub-core/kern/err.c
> @@ -33,15 +33,24 @@ static struct grub_error_saved
> grub_error_stack_items[GRUB_ERROR_STACK_SIZE];
>  static int grub_error_stack_pos;
>  static int grub_error_stack_assert;
>
> +#ifdef grub_error
> +#undef grub_error
> +#endif
> +
>  grub_err_t
> -grub_error (grub_err_t n, const char *fmt, ...)
> +grub_error (grub_err_t n, const char *file, const int line, const char
> *fmt, ...)
>  {
>    va_list ap;
> +  int m;
>
>    grub_errno = n;
>
> +  m = grub_snprintf (grub_errmsg, sizeof (grub_errmsg), "%s:%d:", file,
> line);
> +  if (m < 0)
> +    m = 0;
> +
>    va_start (ap, fmt);
> -  grub_vsnprintf (grub_errmsg, sizeof (grub_errmsg), _(fmt), ap);
> +  grub_vsnprintf (grub_errmsg + m, sizeof (grub_errmsg) - m, _(fmt), ap);
>    va_end (ap);
>
>    return n;
> diff --git a/include/grub/err.h b/include/grub/err.h
> index 24ba9f5f592..b68bbec3c72 100644
> --- a/include/grub/err.h
> +++ b/include/grub/err.h
> @@ -85,7 +85,10 @@ struct grub_error_saved
>  extern grub_err_t EXPORT_VAR(grub_errno);
>  extern char EXPORT_VAR(grub_errmsg)[GRUB_MAX_ERRMSG];
>
> -grub_err_t EXPORT_FUNC(grub_error) (grub_err_t n, const char *fmt, ...);
> +grub_err_t EXPORT_FUNC(grub_error) (grub_err_t n, const char *file, const
> int line, const char *fmt, ...);
> +
> +#define grub_error(n, fmt, ...) grub_error (n, __FILE__, __LINE__, fmt,
> ##__VA_ARGS__)
> +
>  void EXPORT_FUNC(grub_fatal) (const char *fmt, ...) __attribute__
> ((noreturn));
>  void EXPORT_FUNC(grub_error_push) (void);
>  int EXPORT_FUNC(grub_error_pop) (void);
> --
> 2.24.1
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 3384 bytes --]

Re: [PATCH v2 08/12] kern: Make grub_error() more verbose

[Javier Martinez Canillas]

Hello Vladimir,

Thanks a lot for your feedback.

On 3/5/20 3:22 PM, Vladimir 'phcoder' Serbinenko wrote:
> Please evaluate size increase for this. In the past passing file and line
> number to grub_dprintf was a huge source of increased Kern and core size
> 
> Le mer. 4 mars 2020 à 13:01, Javier Martinez Canillas <javierm@redhat.com>
> a écrit :
> 

For a x86_64-pc build with platform=pc on master with and without the patch:

$ find -regex '.*\(mod\|img\|exec\)$' -exec du -c {} + | grep total$
2740    total

$ find -regex '.*\(mod\|img\|exec\)$' -exec du -c {} + | grep total$
2756    total

so in this case the increase is a 0.6% in size.

And doing the same in a x86_64 with platform=efi build:

$ find -regex '.*\(mod\|img\|exec\)$' -exec du -c {} + | grep total$
4296    total

$ find -regex '.*\(mod\|img\|exec\)$' -exec du -c {} + | grep total$
4356    total

The size increase it's a little bigger but just a 1.4%.

You are right that there's a size increase but I wouldn't call it huge and
I think that's a good trade off for the convenience of having that info.

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH v2 08/12] kern: Make grub_error() more verbose

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 990 bytes --]

Le ven. 6 mars 2020 à 12:38, Javier Martinez Canillas <javierm@redhat.com>
a écrit :

> Hello Vladimir,
>
> Thanks a lot for your feedback.
>
> On 3/5/20 3:22 PM, Vladimir 'phcoder' Serbinenko wrote:
> > Please evaluate size increase for this. In the past passing file and line
> > number to grub_dprintf was a huge source of increased Kern and core size
> >
> > Le mer. 4 mars 2020 à 13:01, Javier Martinez Canillas <
> javierm@redhat.com>
> > a écrit :
> >
>
> For a x86_64-pc build with platform=pc on master with and without the
> patch:
>
> $ find -regex '.*\(mod\|img\|exec\)$' -exec du -c {} + | grep total$
> 2740    total
>
> $ find -regex '.*\(mod\|img\|exec\)$' -exec du -c {} + | grep total$
> 2756    total
>
> so in this case the increase is a 0.6% in size.
>
This is not the comparison we care about. We care about kernel.img size and
core.img size for several common configs. And in bytes.
We have only 31K in mbr gap and every byte counts

[-- Attachment #2: Type: text/html, Size: 1467 bytes --]

Re: [PATCH v2 08/12] kern: Make grub_error() more verbose

[Daniel Kiper]

On Thu, Mar 05, 2020 at 03:22:31PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> Please evaluate size increase for this. In the past passing file and line
> number to grub_dprintf was a huge source of increased Kern and core size

I think it does not matter much if we stop pretending that we support
small MBR gaps right now [1]. Does it?

Daniel

[1] https://lists.gnu.org/archive/html/grub-devel/2020-03/msg00059.html

Re: [PATCH v2 08/12] kern: Make grub_error() more verbose

[Javier Martinez Canillas]

On 3/6/20 12:41 PM, Vladimir 'phcoder' Serbinenko wrote:
> Le ven. 6 mars 2020 à 12:38, Javier Martinez Canillas <javierm@redhat.com>
> a écrit :
> 
>> Hello Vladimir,
>>
>> Thanks a lot for your feedback.
>>
>> On 3/5/20 3:22 PM, Vladimir 'phcoder' Serbinenko wrote:
>>> Please evaluate size increase for this. In the past passing file and line
>>> number to grub_dprintf was a huge source of increased Kern and core size
>>>
>>> Le mer. 4 mars 2020 à 13:01, Javier Martinez Canillas <
>> javierm@redhat.com>
>>> a écrit :
>>>
>>
>> For a x86_64-pc build with platform=pc on master with and without the
>> patch:
>>
>> $ find -regex '.*\(mod\|img\|exec\)$' -exec du -c {} + | grep total$
>> 2740    total
>>
>> $ find -regex '.*\(mod\|img\|exec\)$' -exec du -c {} + | grep total$
>> 2756    total
>>
>> so in this case the increase is a 0.6% in size.
>>
> This is not the comparison we care about. We care about kernel.img size and
> core.img size for several common configs. And in bytes.
> We have only 31K in mbr gap and every byte counts
> 

Fair. Then I'll only do the comparison for pc platform since for EFI it
doesn't matter due the binary being in the ESP.

I built a core.img with the ext2, part_msdos and biosdisk modules which I
think is a pretty common configuration used. So the following command:

$ ./grub-mkimage --directory ./grub-core/ --prefix '(,msdos1)/grub2' --output core.img  --format 'i386-pc' --compression 'auto' 'ext2' 'part_msdos' 'biosdisk'

and I got the following sizes in bytes for the two cases:

without the patch: 25977

with the patch: 26350

So I think my comment still applies, it's just a 1.4% increase (373 bytes).

But also as Daniel said, in most cases the gap between the end of the MBR
and the start of the first partition will be at least 1 MiB due partitions
being aligned to a 1 MiB boundary. It shouldn't be an issue for that case.

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH v2 08/12] kern: Make grub_error() more verbose

[Daniel Kiper]

Re-adding grub-devel@gnu.org...

On Fri, Mar 06, 2020 at 12:44:05PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> Le ven. 6 mars 2020 à 12:42, Daniel Kiper <dkiper@net-space.pl> a écrit :
>
> > On Thu, Mar 05, 2020 at 03:22:31PM +0100, Vladimir 'phcoder' Serbinenko
> > wrote:
> > > Please evaluate size increase for this. In the past passing file and line
> > > number to grub_dprintf was a huge source of increased Kern and core size
> >
> > I think it does not matter much if we stop pretending that we support
> > small MBR gaps right now [1]. Does it?
> >
> There are still a lot of installations that used older tools and never
> reformatted. We still care about them

If we go that way then we have to care about them by the end of the
universe. And this means more and more issues like [1]. If somebody
wants to use new GRUB then he/she have to reinstall the machine or
something like that. IMO we should not care about users who do not want
upgrade their machines or whatnot. Or at least their choices cannot
impact GRUB development too much. And I think that this MBR constraint
is hindering the project too much at this point. So, as above...

Sorry for being blunt...

Daniel

[1] https://lists.gnu.org/archive/html/grub-devel/2019-11/msg00025.html

Re: [PATCH v2 08/12] kern: Make grub_error() more verbose

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1540 bytes --]

Le ven. 6 mars 2020 à 13:43, Daniel Kiper <dkiper@net-space.pl> a écrit :

> Re-adding grub-devel@gnu.org...
>
> On Fri, Mar 06, 2020 at 12:44:05PM +0100, Vladimir 'phcoder' Serbinenko
> wrote:
> > Le ven. 6 mars 2020 à 12:42, Daniel Kiper <dkiper@net-space.pl> a écrit
> :
> >
> > > On Thu, Mar 05, 2020 at 03:22:31PM +0100, Vladimir 'phcoder' Serbinenko
> > > wrote:
> > > > Please evaluate size increase for this. In the past passing file and
> line
> > > > number to grub_dprintf was a huge source of increased Kern and core
> size
> > >
> > > I think it does not matter much if we stop pretending that we support
> > > small MBR gaps right now [1]. Does it?
> > >
> > There are still a lot of installations that used older tools and never
> > reformatted. We still care about them
>
> If we go that way then we have to care about them by the end of the
> universe. And this means more and more issues like [1]. If somebody
> wants to use new GRUB then he/she have to reinstall the machine or
> something like that. IMO we should not care about users who do not want
> upgrade their machines or whatnot. Or at least their choices cannot
> impact GRUB development too much. And I think that this MBR constraint
> is hindering the project too much at this point. So, as above...
>
> Sorry for being blunt...
>
It does not. Core doesn't need to have everything and the kitchen sink.
It's small by design.

>
> Daniel
>
> [1] https://lists.gnu.org/archive/html/grub-devel/2019-11/msg00025.html
>

[-- Attachment #2: Type: text/html, Size: 2343 bytes --]

Re: [PATCH v2 08/12] kern: Make grub_error() more verbose

[Didier Spaier]

Le 06/03/2020 à 13:43, Daniel Kiper a écrit :
> If we go that way then we have to care about them by the end of the
> universe. And this means more and more issues like [1]. If somebody
> wants to use new GRUB then he/she have to reinstall the machine or
> something like that. IMO we should not care about users who do not want
> upgrade their machines or whatnot. Or at least their choices cannot
> impact GRUB development too much. And I think that this MBR constraint
> is hindering the project too much at this point. So, as above...
> 
> Sorry for being blunt...

Sorry to be off topic... In case of a GUID partition table, if I
understand the UEFI specification[1], as the first usable LBA should be
greater than or equal to 34 for a 512 bytes block size or 6 for a
4096 bytes logical block size, it could begin after a gap of 24K.

Then, if we assume that the first partition begins @ 1MiB, can't GRUB
use the space unused between the first usable LBA and 1MiB instead of
a Bios Boot partition, in case of "legacy" booting and a GPT? I ask as
then a Bios Boot partition wouldn't be necessary any more.

I just hope this question is not too silly...

Best regards,
Didier


[1]https://uefi.org/sites/default/files/resources/UEFI_Spec_2_8_A_Feb14.pdf
§ 5.3.1 GPT overview

Re: [PATCH v2 08/12] kern: Make grub_error() more verbose

[Vladimir 'phcoder' Serbinenko]

On Fri, Mar 6, 2020 at 3:02 PM Didier Spaier <didier@slint.fr> wrote:
>
> Le 06/03/2020 à 13:43, Daniel Kiper a écrit :
> > If we go that way then we have to care about them by the end of the
> > universe. And this means more and more issues like [1]. If somebody
> > wants to use new GRUB then he/she have to reinstall the machine or
> > something like that. IMO we should not care about users who do not want
> > upgrade their machines or whatnot. Or at least their choices cannot
> > impact GRUB development too much. And I think that this MBR constraint
> > is hindering the project too much at this point. So, as above...
> >
> > Sorry for being blunt...
>
> Sorry to be off topic... In case of a GUID partition table, if I
> understand the UEFI specification[1], as the first usable LBA should be
> greater than or equal to 34 for a 512 bytes block size or 6 for a
> 4096 bytes logical block size, it could begin after a gap of 24K.
>
> Then, if we assume that the first partition begins @ 1MiB, can't GRUB
> use the space unused between the first usable LBA and 1MiB instead of
> a Bios Boot partition, in case of "legacy" booting and a GPT? I ask as
> then a Bios Boot partition wouldn't be necessary any more.
What's the problem with having bios boot partition? You can put it
exactly in the spot you describe (in fact I do so) and it works as
well, just additionally it marks this space as occupied
>
> I just hope this question is not too silly...
>
> Best regards,
> Didier
>
>
> [1]https://uefi.org/sites/default/files/resources/UEFI_Spec_2_8_A_Feb14.pdf
> § 5.3.1 GPT overview
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel



-- 
Regards
Vladimir 'phcoder' Serbinenko

Re: [PATCH v2 08/12] kern: Make grub_error() more verbose

[Didier Spaier]

Le 06/03/2020 à 15:38, Vladimir 'phcoder' Serbinenko a écrit :
> What's the problem with having bios boot partition? You can put it
> exactly in the spot you describe (in fact I do so) and it works as
> well, just additionally it marks this space as occupied

That'd be just one less thing to care about for the Slint installer
in case of  an 'automatic' installation, nothing major.

Do you confirm that a Bios Boot partition starting at 1 KiB
and ending at 1 MiB won't overlap the GPT and is big enough?

Thanks,
Didier

Re: [PATCH v2 08/12] kern: Make grub_error() more verbose

[Lennart Sorensen]

On Fri, Mar 06, 2020 at 05:30:23PM +0100, Didier Spaier wrote:
> That'd be just one less thing to care about for the Slint installer
> in case of  an 'automatic' installation, nothing major.
> 
> Do you confirm that a Bios Boot partition starting at 1 KiB
> and ending at 1 MiB won't overlap the GPT and is big enough?

It would absolutely overlap GPT since GPT usually starts at 512 bytes
and goes up to 32KB or so.

-- 
Len Sorensen

Re: [PATCH v2 08/12] kern: Make grub_error() more verbose

[David Michael]

On Fri, Mar 6, 2020 at 9:02 AM Didier Spaier <didier@slint.fr> wrote:
> Le 06/03/2020 à 13:43, Daniel Kiper a écrit :
> > If we go that way then we have to care about them by the end of the
> > universe. And this means more and more issues like [1]. If somebody
> > wants to use new GRUB then he/she have to reinstall the machine or
> > something like that. IMO we should not care about users who do not want
> > upgrade their machines or whatnot. Or at least their choices cannot
> > impact GRUB development too much. And I think that this MBR constraint
> > is hindering the project too much at this point. So, as above...
> >
> > Sorry for being blunt...
>
> Sorry to be off topic... In case of a GUID partition table, if I
> understand the UEFI specification[1], as the first usable LBA should be
> greater than or equal to 34 for a 512 bytes block size or 6 for a
> 4096 bytes logical block size, it could begin after a gap of 24K.
>
> Then, if we assume that the first partition begins @ 1MiB, can't GRUB
> use the space unused between the first usable LBA and 1MiB instead of
> a Bios Boot partition, in case of "legacy" booting and a GPT? I ask as
> then a Bios Boot partition wouldn't be necessary any more.

It would be best to use a boot partition so the core.img space is
reserved by the GPT, but I once wanted to tack i386-pc GRUB onto an
existing UEFI GPT disk, so I wrote the commands that do what you're
describing here:

https://github.com/dm0-/installer/blob/master/examples/systems/fitpc.sh#L151-L178

I may be misremembering, but I think the core.img size was limited to
around half a megabyte, so it should be safe to write it to the unused
~1MiB before the first partition after the GPT.  But yes, using the
boot partition would probably be for the best if you are formatting a
new disk.

Thanks.

David

Re: [PATCH v2 08/12] kern: Make grub_error() more verbose

[Didier Spaier]

Le 06/03/2020 à 17:39, Lennart Sorensen a écrit :
> On Fri, Mar 06, 2020 at 05:30:23PM +0100, Didier Spaier wrote:
>> That'd be just one less thing to care about for the Slint installer
>> in case of  an 'automatic' installation, nothing major.
>>
>> Do you confirm that a Bios Boot partition starting at 1 KiB
>> and ending at 1 MiB won't overlap the GPT and is big enough?
> 
> It would absolutely overlap GPT since GPT usually starts at 512 bytes
> and goes up to 32KB or so.

My mistake, I should have written starting at 24 KiB as that is the
start address of the first usable LBA if I read correctly the UEFI
specification. Am I wrong?

Thanks for the heads-up.

Re: [PATCH v2 08/12] kern: Make grub_error() more verbose

[Bruce Dubbs]

On 3/6/20 11:03 AM, David Michael wrote:
> On Fri, Mar 6, 2020 at 9:02 AM Didier Spaier <didier@slint.fr> wrote:
>> Le 06/03/2020 à 13:43, Daniel Kiper a écrit :
>>> If we go that way then we have to care about them by the end of the
>>> universe. And this means more and more issues like [1]. If somebody
>>> wants to use new GRUB then he/she have to reinstall the machine or
>>> something like that. IMO we should not care about users who do not want
>>> upgrade their machines or whatnot. Or at least their choices cannot
>>> impact GRUB development too much. And I think that this MBR constraint
>>> is hindering the project too much at this point. So, as above...
>>>
>>> Sorry for being blunt...
>>
>> Sorry to be off topic... In case of a GUID partition table, if I
>> understand the UEFI specification[1], as the first usable LBA should be
>> greater than or equal to 34 for a 512 bytes block size or 6 for a
>> 4096 bytes logical block size, it could begin after a gap of 24K.
>>
>> Then, if we assume that the first partition begins @ 1MiB, can't GRUB
>> use the space unused between the first usable LBA and 1MiB instead of
>> a Bios Boot partition, in case of "legacy" booting and a GPT? I ask as
>> then a Bios Boot partition wouldn't be necessary any more.
> 
> It would be best to use a boot partition so the core.img space is
> reserved by the GPT, 

Just a terminology issue here.  Please don't call it a boot partition. 
It conflicts with those who mount /boot as a separate partition.  Please 
call it a grub partition.

   -- Bruce


but I once wanted to tack i386-pc GRUB onto an
> existing UEFI GPT disk, so I wrote the commands that do what you're
> describing here:
> 
> https://github.com/dm0-/installer/blob/master/examples/systems/fitpc.sh#L151-L178
> 
> I may be misremembering, but I think the core.img size was limited to
> around half a megabyte, so it should be safe to write it to the unused
> ~1MiB before the first partition after the GPT.  But yes, using the
> boot partition would probably be for the best if you are formatting a
> new disk.
> 
> Thanks.
> 
> David
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 

Re: [PATCH v2 08/12] kern: Make grub_error() more verbose

[Lennart Sorensen]

On Fri, Mar 06, 2020 at 11:12:14AM -0600, Bruce Dubbs wrote:
> Just a terminology issue here.  Please don't call it a boot partition. It
> conflicts with those who mount /boot as a separate partition.  Please call
> it a grub partition.

Well the official name is "BIOS boot partition", so I can see why people
tend to call it a boot partition.

-- 
Len Sorensen

Re: [PATCH v2 08/12] kern: Make grub_error() more verbose

[Lennart Sorensen]

On Fri, Mar 06, 2020 at 06:10:58PM +0100, Didier Spaier wrote:
> My mistake, I should have written starting at 24 KiB as that is the
> start address of the first usable LBA if I read correctly the UEFI
> specification. Am I wrong?
> 
> Thanks for the heads-up.

I was wrong about 32KB, it is 32 sectors of 512 when that is the block
size plus the MBR and header sectors, so 34 sectors at 512b, and yes
with 4K blocks you hit 24k.  Not sure if bigger sector sizes are likely
to happen at some point.  Maybe 32 or 64KB would be a safer starting
point and being a power of 2, slightly more likely to hit a disk aligned
block size.

-- 
Len Sorensen

Re: [PATCH v2 08/12] kern: Make grub_error() more verbose

[Didier Spaier]

Le 06/03/2020 à 18:03, David Michael a écrit :
> On Fri, Mar 6, 2020 at 9:02 AM Didier Spaier <didier@slint.fr> wrote:
>> Le 06/03/2020 à 13:43, Daniel Kiper a écrit :
>>> If we go that way then we have to care about them by the end of the
>>> universe. And this means more and more issues like [1]. If somebody
>>> wants to use new GRUB then he/she have to reinstall the machine or
>>> something like that. IMO we should not care about users who do not want
>>> upgrade their machines or whatnot. Or at least their choices cannot
>>> impact GRUB development too much. And I think that this MBR constraint
>>> is hindering the project too much at this point. So, as above...
>>>
>>> Sorry for being blunt...
>>
>> Sorry to be off topic... In case of a GUID partition table, if I
>> understand the UEFI specification[1], as the first usable LBA should be
>> greater than or equal to 34 for a 512 bytes block size or 6 for a
>> 4096 bytes logical block size, it could begin after a gap of 24K.
>>
>> Then, if we assume that the first partition begins @ 1MiB, can't GRUB
>> use the space unused between the first usable LBA and 1MiB instead of
>> a Bios Boot partition, in case of "legacy" booting and a GPT? I ask as
>> then a Bios Boot partition wouldn't be necessary any more.
> 
> It would be best to use a boot partition so the core.img space is
> reserved by the GPT, but I once wanted to tack i386-pc GRUB onto an
> existing UEFI GPT disk, so I wrote the commands that do what you're
> describing here:
> 
> https://github.com/dm0-/installer/blob/master/examples/systems/fitpc.sh#L151-L178
> 
> I may be misremembering, but I think the core.img size was limited to
> around half a megabyte, so it should be safe to write it to the unused
> ~1MiB before the first partition after the GPT.  But yes, using the
> boot partition would probably be for the best if you are formatting a
> new disk.

Thanks for sharing David. This can be useful to allow Legacy booting
off a drive initially partitioned for UEFI booting only.

Re: [PATCH v2 08/12] kern: Make grub_error() more verbose

[Vladimir 'phcoder' Serbinenko]

> It would be best to use a boot partition so the core.img space is
> reserved by the GPT, but I once wanted to tack i386-pc GRUB onto an
> existing UEFI GPT disk, so I wrote the commands that do what you're
> describing here:
>
> https://github.com/dm0-/installer/blob/master/examples/systems/fitpc.sh#L151-L178
>
Just use gdisk to add a partition into this 1MiB hole.  you only need
to ask it to reduce alignment.

Re: [PATCH v2 08/12] kern: Make grub_error() more verbose

[Daniel Kiper]

On Fri, Mar 06, 2020 at 02:37:43PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> Le ven. 6 mars 2020 à 13:43, Daniel Kiper <dkiper@net-space.pl> a écrit :
>
> > Re-adding grub-devel@gnu.org...
> >
> > On Fri, Mar 06, 2020 at 12:44:05PM +0100, Vladimir 'phcoder' Serbinenko
> > wrote:
> > > Le ven. 6 mars 2020 à 12:42, Daniel Kiper <dkiper@net-space.pl> a écrit
> > :
> > >
> > > > On Thu, Mar 05, 2020 at 03:22:31PM +0100, Vladimir 'phcoder' Serbinenko
> > > > wrote:
> > > > > Please evaluate size increase for this. In the past passing file and
> > line
> > > > > number to grub_dprintf was a huge source of increased Kern and core
> > size
> > > >
> > > > I think it does not matter much if we stop pretending that we support
> > > > small MBR gaps right now [1]. Does it?
> > > >
> > > There are still a lot of installations that used older tools and never
> > > reformatted. We still care about them
> >
> > If we go that way then we have to care about them by the end of the
> > universe. And this means more and more issues like [1]. If somebody
> > wants to use new GRUB then he/she have to reinstall the machine or
> > something like that. IMO we should not care about users who do not want
> > upgrade their machines or whatnot. Or at least their choices cannot
> > impact GRUB development too much. And I think that this MBR constraint
> > is hindering the project too much at this point. So, as above...
> >
> > Sorry for being blunt...
> >
> It does not. Core doesn't need to have everything and the kitchen sink.
> It's small by design.

I am not saying that. I am saying that limitations of one ancient platform
should not make development of other platforms more difficult.

Daniel