Re: [PATCH 2/2] .mod files: Strip annobin annotations and .eh_frame, and their relocations

[Peter Jones]

On Tue, Feb 20, 2018 at 03:51:59PM +0100, Daniel Kiper wrote:
> On Wed, Jan 31, 2018 at 11:27:00AM -0500, Peter Jones wrote:
> > This way debuginfo built from the .module will still include this
> > information, but the final result won't have the data we don't actually
> > need in the modules, either on-disk, loaded at runtime, or in prebuilt
> > images.
> >
> > Signed-off-by: Peter Jones 
> 
> Reviewed-by: Daniel Kiper 
> 
> I suppose that I can apply this patch without patch 1
> but I would like to be sure... So?

Yes, it should be completely safe.

-- 
  Peter

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/2] mkimage: avoid copying relocations for sections that won't be copied.

[Peter Jones]

On Tue, Feb 20, 2018 at 03:48:44PM +0100, Daniel Kiper wrote:
> On Wed, Jan 31, 2018 at 11:26:59AM -0500, Peter Jones wrote:
> > +static int
> > +SUFFIX (is_kept_section) (Elf_Shdr *s, const struct 
> > grub_install_image_target_desc *image_target);
> > +static int
> > +SUFFIX (is_kept_reloc_section) (Elf_Shdr *s, const struct 
> > grub_install_image_target_desc *image_target,
> > +   Elf_Shdr *sections, Elf_Half section_entsize, 
> > Elf_Half num_sections,
> > +   const char *strtab);
> 
> Ugh... Could not you create a struct and pass the pointer to it here?

Fair enough - I did it that way because the whole file seems averse to
that sort of thing, and once you start doing it, it gets a bit messy.
Nevertheless, I'll send you a patchset that includes that in just a few
minutes.

> PS Please CC me on the patches next time.

Will do.

-- 
  Peter

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v2 2/3] mkimage: avoid copying relocations for sections that won't be copied.

[Peter Jones]

Some versions of gcc include a plugin called "annobin", and in some
build systems this is enabled by default.  This plugin creates special
ELF note sections to track which ABI-breaking features are used by a
binary, as well as a series of relocations to annotate where.

If grub is compiled with this feature, then when grub-mkimage translates
the binary to another file format which does not strongly associate
relocation data with sections (i.e. when platform is *-efi), these
relocations appear to be against the .text section rather than the
original note section.  When the binary is loaded by the PE runtime
loader, hilarity ensues.

This issue is not necessarily limited to the annobin, but could arise
any time there are relocations in sections that are not represented in
grub-mkimage's output.

This patch seeks to avoid this issue by only including relocations that
refer to sections which will be included in the final binary.

As an aside, this should also obviate the need to avoid -funwind-tables,
-fasynchronous-unwind-tables, and any sections similar to .eh_frame in
the future.  I've tested it on x86-64-efi with the following gcc command
line options (as recorded by -grecord-gcc-flags), but I still need to
test the result on some other platforms that have been problematic in
the past (especially ARM Aarch64) before I feel comfortable making
changes to the configure.ac bits:

GNU C11 7.2.1 20180116 (Red Hat 7.2.1-7) -mno-mmx -mno-sse -mno-sse2 -mno-sse3 
-mno-3dnow -msoft-float -mno-stack-arg-probe -mcmodel=large -mno-red-zone -m64 
-mtune=generic -march=x86-64 -g3 -Os -freg-struct-return -fno-stack-protector 
-ffreestanding -funwind-tables -fasynchronous-unwind-tables 
-fno-strict-aliasing -fstack-clash-protection -fno-ident -fplugin=annobin

Signed-off-by: Peter Jones 
---
 util/grub-mkimagexx.c | 79 +--
 1 file changed, 71 insertions(+), 8 deletions(-)

diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
index 5c787ec56bf..c15079c96ba 100644
--- a/util/grub-mkimagexx.c
+++ b/util/grub-mkimagexx.c
@@ -716,6 +716,12 @@ arm_get_trampoline_size (Elf_Ehdr *e,
 }
 #endif
 
+static int
+SUFFIX (is_kept_section) (Elf_Shdr *s, const struct 
grub_install_image_target_desc *image_target);
+static int
+SUFFIX (is_kept_reloc_section) (Elf_Shdr *s, const struct 
grub_install_image_target_desc *image_target,
+   struct section_metadata *sdata);
+
 /* Deal with relocation information. This function relocates addresses
within the virtual address space starting from 0. So only relative
addresses can be fully resolved. Absolute addresses must be relocated
@@ -750,6 +756,14 @@ SUFFIX (relocate_addresses) (Elf_Ehdr *e, struct 
section_metadata *sdata,
Elf_Shdr *target_section;
Elf_Word j;
 
+   if (!SUFFIX (is_kept_section) (s, image_target) &&
+   !SUFFIX (is_kept_reloc_section) (s, image_target, sdata))
+ {
+   grub_util_info ("not translating relocations for omitted section 
%s",
+   sdata->strtab + grub_le_to_cpu32 (s->sh_name));
+   continue;
+ }
+
target_section_index = grub_target_to_host32 (s->sh_info);
target_section_addr = sdata->addresses[target_section_index];
target_section = (Elf_Shdr *) ((char *) sdata->sections
@@ -1654,6 +1668,13 @@ make_reloc_section (Elf_Ehdr *e, struct 
grub_mkimage_layout *layout,
Elf_Addr section_address;
Elf_Word j;
 
+   if (!SUFFIX (is_kept_reloc_section) (s, image_target, sdata))
+ {
+   grub_util_info ("not translating the skipped relocation section %s",
+   sdata->strtab + grub_le_to_cpu32 (s->sh_name));
+   continue;
+ }
+
grub_util_info ("translating the relocation section %s",
sdata->strtab + grub_le_to_cpu32 (s->sh_name));
 
@@ -1729,6 +1750,55 @@ SUFFIX (is_bss_section) (Elf_Shdr *s, const struct 
grub_install_image_target_des
  == SHF_ALLOC) && (grub_target_to_host32 (s->sh_type) == SHT_NOBITS);
 }
 
+/* Determine if a section is going to be in the final output */
+static int
+SUFFIX (is_kept_section) (Elf_Shdr *s, const struct 
grub_install_image_target_desc *image_target)
+{
+  /* We keep .text and .data */
+  if (SUFFIX (is_text_section) (s, image_target)
+  || SUFFIX (is_data_section) (s, image_target))
+return 1;
+
+  /* And we keep .bss if we're producing PE binaries  or the target doesn't
+   * have a relocating loader.  Platforms other than EFI and U-boot shouldn't
+   * have .bss in their binaries as we build with -Wl,-Ttext.
+   */
+  if (SUFFIX (is_bss_section) (s, image_target)
+  && (image_target->id == IMAGE_EFI || !is_relocatable (image_target)))
+return 1;
+
+  /* Otherwise this is not a section we're keeping in the final output. */
+  return 0;
+}
+
+static int
+SUFFIX (is_kept_reloc_section) (Elf_Shdr *s, const 

[PATCH v2 1/3] mkimage: refactor a bunch of section data into a struct.

[Peter Jones]

This basically moves a bunch of the section information we pass around a
lot into a struct, and passes a pointer to a single one of those
instead.

Because it's more convenient, it also puts the section_vaddresses
calculations in locate_section(), which no longer returns the allocation
for section_addresses directly either.

This shouldn't change the binary file output or the "grub-mkimage -v"
output in any way.

Signed-off-by: Peter Jones 
---
 util/grub-mkimagexx.c | 282 ++
 1 file changed, 123 insertions(+), 159 deletions(-)

diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
index a2bb05439f0..5c787ec56bf 100644
--- a/util/grub-mkimagexx.c
+++ b/util/grub-mkimagexx.c
@@ -84,6 +84,17 @@ struct fixup_block_list
 
 #define ALIGN_ADDR(x) (ALIGN_UP((x), image_target->voidp_sizeof))
 
+struct section_metadata
+{
+  Elf_Half num_sections;
+  Elf_Shdr *sections;
+  Elf_Addr *addresses;
+  Elf_Addr *vaddresses;
+  Elf_Half section_entsize;
+  Elf_Shdr *symtab;
+  const char *strtab;
+};
+
 static int
 is_relocatable (const struct grub_install_image_target_desc *image_target)
 {
@@ -499,9 +510,7 @@ SUFFIX (grub_mkimage_generate_elf) (const struct 
grub_install_image_target_desc
 /* Relocate symbols; note that this function overwrites the symbol table.
Return the address of a start symbol.  */
 static Elf_Addr
-SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
-  Elf_Shdr *symtab_section, Elf_Addr 
*section_addresses,
-  Elf_Half section_entsize, Elf_Half num_sections,
+SUFFIX (relocate_symbols) (Elf_Ehdr *e, struct section_metadata *sdata,
   void *jumpers, Elf_Addr jumpers_addr,
   Elf_Addr bss_start, Elf_Addr end,
   const struct grub_install_image_target_desc 
*image_target)
@@ -511,19 +520,18 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr 
*sections,
   Elf_Addr start_address = (Elf_Addr) -1;
   Elf_Sym *sym;
   Elf_Word i;
-  Elf_Shdr *strtab_section;
-  const char *strtab;
+  Elf_Shdr *symtab_section;
+  const char *symtab;
   grub_uint64_t *jptr = jumpers;
 
-  strtab_section
-= (Elf_Shdr *) ((char *) sections
- + (grub_target_to_host32 (symtab_section->sh_link)
-* section_entsize));
-  strtab = (char *) e + grub_target_to_host (strtab_section->sh_offset);
+  symtab_section = (Elf_Shdr *) ((char *) sdata->sections
++ grub_target_to_host32 
(sdata->symtab->sh_link)
+  * sdata->section_entsize);
+  symtab = (char *) e + grub_target_to_host (symtab_section->sh_offset);
 
-  symtab_size = grub_target_to_host (symtab_section->sh_size);
-  sym_size = grub_target_to_host (symtab_section->sh_entsize);
-  symtab_offset = grub_target_to_host (symtab_section->sh_offset);
+  symtab_size = grub_target_to_host (sdata->symtab->sh_size);
+  sym_size = grub_target_to_host (sdata->symtab->sh_entsize);
+  symtab_offset = grub_target_to_host (sdata->symtab->sh_offset);
   num_syms = symtab_size / sym_size;
 
   for (i = 0, sym = (Elf_Sym *) ((char *) e + symtab_offset);
@@ -533,7 +541,7 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
   Elf_Section cur_index;
   const char *name;
 
-  name = strtab + grub_target_to_host32 (sym->st_name);
+  name = symtab + grub_target_to_host32 (sym->st_name);
 
   cur_index = grub_target_to_host16 (sym->st_shndx);
   if (cur_index == STN_ABS)
@@ -551,12 +559,12 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr 
*sections,
  else
continue;
}
-  else if (cur_index >= num_sections)
+  else if (cur_index >= sdata->num_sections)
grub_util_error ("section %d does not exist", cur_index);
   else
{
  sym->st_value = (grub_target_to_host (sym->st_value)
-  + section_addresses[cur_index]);
+  + sdata->vaddresses[cur_index]);
}
 
   if (image_target->elf_target == EM_IA_64 && ELF_ST_TYPE (sym->st_info)
@@ -571,7 +579,7 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
   grub_util_info ("locating %s at 0x%"  GRUB_HOST_PRIxLONG_LONG
  " (0x%"  GRUB_HOST_PRIxLONG_LONG ")", name,
  (unsigned long long) sym->st_value,
- (unsigned long long) section_addresses[cur_index]);
+ (unsigned long long) sdata->vaddresses[cur_index]);
 
   if (start_address == (Elf_Addr)-1)
if (strcmp (name, "_start") == 0 || strcmp (name, "start") == 0)
@@ -713,12 +721,8 @@ arm_get_trampoline_size (Elf_Ehdr *e,
addresses can be fully resolved. Absolute addresses must be relocated
again by a PE32 relocator when loaded.  */
 static void
-SUFFIX (relocate_addresses) (Elf_Ehdr *e, Elf_Shdr *sections,
-Elf_Addr 

[PATCH v2 3/3] .mod files: Strip annobin annotations and .eh_frame, and their relocations

[Peter Jones]

This way debuginfo built from the .module will still include this
information, but the final result won't have the data we don't actually
need in the modules, either on-disk, loaded at runtime, or in prebuilt
images.

Signed-off-by: Peter Jones 
---
 grub-core/genmod.sh.in | 4 
 1 file changed, 4 insertions(+)

diff --git a/grub-core/genmod.sh.in b/grub-core/genmod.sh.in
index 3de06ee018f..1250589b3f5 100644
--- a/grub-core/genmod.sh.in
+++ b/grub-core/genmod.sh.in
@@ -58,6 +58,10 @@ if test x@TARGET_APPLE_LINKER@ != x1; then
-K grub_mod_init -K grub_mod_fini \
-K _grub_mod_init -K _grub_mod_fini \
-R .note.gnu.gold-version -R .note.GNU-stack \
+   -R .gnu.build.attributes \
+   -R .rel.gnu.build.attributes \
+   -R .rela.gnu.build.attributes \
+   -R .eh_frame -R .rela.eh_frame -R .rel.eh_frame \
-R .note -R .comment -R .ARM.exidx $tmpfile || exit 1
fi
if ! test -z "${TARGET_OBJ2ELF}"; then
-- 
2.15.0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] ieee1275: add nvme support within ofpath

[Daniel Kiper]

On Tue, Feb 20, 2018 at 09:57:14AM -0800, Eric Snowberg wrote:
> Add NVMe support within ofpath.
>
> The Open Firmware text representation for a NVMe device contains the
> Namespace ID. An invalid namespace ID is one whose value is zero or whose
> value is greater than the value reported by the Number of Namespaces (NN)
> field in the Identify Controller data structure.  At the moment  only a
> single Namespace is supported, therefore the value is currently hard coded
> to one.
>
> Signed-off-by: Eric Snowberg 

Just one nit pick... I will change this during commit.

Otherwise Reviewed-by: Daniel Kiper 

> ---
>  grub-core/osdep/linux/ofpath.c |   47 
> 
>  1 files changed, 47 insertions(+), 0 deletions(-)
>
> diff --git a/grub-core/osdep/linux/ofpath.c b/grub-core/osdep/linux/ofpath.c
> index 8d7d683..af068f9 100644
> --- a/grub-core/osdep/linux/ofpath.c
> +++ b/grub-core/osdep/linux/ofpath.c
> @@ -350,6 +350,50 @@ of_path_of_ide(const char *sys_devname 
> __attribute__((unused)), const char *devi
>return ret;
>  }
>
> +static char *
> +of_path_of_nvme(const char *sys_devname __attribute__((unused)),
> + const char *device,
> + const char *devnode __attribute__((unused)),
> + const char *devicenode)
> +{
> +  char *sysfs_path, *of_path, disk[MAX_DISK_CAT];
> +  const char *digit_string, *part_end;
> +
> +  digit_string = trailing_digits (device);
> +  part_end = devicenode + strlen (devicenode) - 1;
> +
> +  if ((digit_string != '\0') && (*part_end == 'p'))
> +{
> +  /* We have a partition number, strip it off. */
> +  int part;
> +  char *nvmedev, *end;
> +
> +  nvmedev = strdup (devicenode);
> +
> +  if (nvmedev == NULL)

"if (!nvmedev)" is the convention in this file.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Make pmtimer tsc calibration not take 51 seconds to fail.

[Daniel Kiper]

On Mon, Feb 19, 2018 at 05:37:56PM -0500, Peter Jones wrote:
> On my laptop running at 2.4GHz, if I run a VM where tsc calibration
> using pmtimer will fail presuming a broken pmtimer, it takes ~51 seconds
> to do so (as measured with the stopwatch on my phone), with a tsc delta
> of 0x1cd1c85300, or around 125 billion cycles.
>
> If instead of trying to wait for 5-200ms to show up on the pmtimer, we try
> to wait for 5-200us, it decides it's broken in ~0x2626aa0 TSCs, aka ~2.4
> million cycles, or more or less instantly.
>
> Additionally, this reading the pmtimer was returning 0x anyway,
> and that's obviously an invalid return.  I've added a check for that and
> 0 so we don't bother waiting for the test if what we're seeing is dead
> pins with no response at all.
>
> If "debug" is includes "pmtimer", you will see one of the following
> three outcomes.  If pmtimer gives all 0 or all 1 bits, you will see:
>
> kern/i386/tsc_pmtimer.c:77: pmtimer: 0xff bad_reads: 1
> kern/i386/tsc_pmtimer.c:77: pmtimer: 0xff bad_reads: 2
> kern/i386/tsc_pmtimer.c:77: pmtimer: 0xff bad_reads: 3
> kern/i386/tsc_pmtimer.c:77: pmtimer: 0xff bad_reads: 4
> kern/i386/tsc_pmtimer.c:77: pmtimer: 0xff bad_reads: 5
> kern/i386/tsc_pmtimer.c:77: pmtimer: 0xff bad_reads: 6
> kern/i386/tsc_pmtimer.c:77: pmtimer: 0xff bad_reads: 7
> kern/i386/tsc_pmtimer.c:77: pmtimer: 0xff bad_reads: 8
> kern/i386/tsc_pmtimer.c:77: pmtimer: 0xff bad_reads: 9
> kern/i386/tsc_pmtimer.c:77: pmtimer: 0xff bad_reads: 10
> kern/i386/tsc_pmtimer.c:78: timer is broken; giving up.

OK.

> This outcome was tested using qemu+kvm with UEFI (OVMF) firmware and
> these options: -machine pc-q35-2.10 -cpu Broadwell-noTSX
>
> If pmtimer gives any other bit patterns but is not actually marching
> forward fast enough to use for clock calibration, you will see:
>
> kern/i386/tsc_pmtimer.c:121: pmtimer delta is 0x0 (1904 iterations)
> kern/i386/tsc_pmtimer.c:124: tsc delta is implausible: 0x2626aa0

OK.

> This outcome was tested using grub compiled with GRUB_PMTIMER_IGNORE_BAD_READS
> defined (so as not to trip the bad read test) using qemu+kvm with UEFI
> (OVMF) firmware, and these options: -machine pc-q35-2.10 -cpu Broadwell-noTSX
>
> If pmtimer actually works, you'll see something like:
>
> kern/i386/tsc_pmtimer.c:121: pmtimer delta is 0x0 (1904 iterations)
> kern/i386/tsc_pmtimer.c:124: tsc delta is implausible: 0x2626aa0

Hmmm... Same as above?

> This outcome was tested using qemu+kvm with UEFI (OVMF) firmware, and
> these options: -machine pc-i440fx-2.4 -cpu Broadwell-noTSX
>
> I've also tested this outcome on a real Intel Xeon E3-1275v3 on an Intel
> Server Board S1200V3RPS using the SDV.RP.B8 "Release" build here:
> https://firmware.intel.com/sites/default/files/UEFIDevKit_S1200RP_vB8.zip
>
> Signed-off-by: Peter Jones 
> ---
>  grub-core/kern/i386/tsc_pmtimer.c | 109 
> +++---
>  1 file changed, 89 insertions(+), 20 deletions(-)
>
> diff --git a/grub-core/kern/i386/tsc_pmtimer.c 
> b/grub-core/kern/i386/tsc_pmtimer.c
> index c9c36169978..ca15c3aacd7 100644
> --- a/grub-core/kern/i386/tsc_pmtimer.c
> +++ b/grub-core/kern/i386/tsc_pmtimer.c
> @@ -28,40 +28,101 @@
>  #include 
>  #include 
>
> +/*
> + * Define GRUB_PMTIMER_IGNORE_BAD_READS if you're trying to test a timer 
> that's
> + * present but doesn't keep time well.
> + */
> +// #define GRUB_PMTIMER_IGNORE_BAD_READS
> +
>  grub_uint64_t
>  grub_pmtimer_wait_count_tsc (grub_port_t pmtimer,
>grub_uint16_t num_pm_ticks)
>  {
>grub_uint32_t start;
> -  grub_uint32_t last;
> -  grub_uint32_t cur, end;
> +  grub_uint64_t cur, end;
>grub_uint64_t start_tsc;
>grub_uint64_t end_tsc;
> -  int num_iter = 0;
> +  unsigned int num_iter = 0;
> +#ifndef GRUB_PMTIMER_IGNORE_BAD_READS
> +  int bad_reads = 0;
> +#endif
>
> -  start = grub_inl (pmtimer) & 0xff;
> -  last = start;
> +  /*
> +   * Some timers are 24-bit and some are 32-bit, but it doesn't make much
> +   * difference to us.  Caring which one we have isn't really worth it since
> +   * the low-order digits will give us enough data to calibrate TSC.  So just
> +   * mask the top-order byte off.
> +   */
> +  cur = start = grub_inl (pmtimer) & 0xffUL;

Just for the sake of readability I would do s/0xffUL/0x00ffUL/ here and 
below.

>end = start + num_pm_ticks;
>start_tsc = grub_get_tsc ();
>while (1)
>  {
> -  cur = grub_inl (pmtimer) & 0xff;
> -  if (cur < last)
> - cur |= 0x100;
> -  num_iter++;
> +  cur &= 0xff00ULL;

Could you put a comment before this line? It took me some time
to get it and required to take a look below. This is not obvious
at first sight.

> +  cur |= grub_inl (pmtimer) & 0xffUL;
> +
> +  end_tsc = grub_get_tsc();
> +
> +#ifndef GRUB_PMTIMER_IGNORE_BAD_READS
> +  /*
> +   * If we get 10 reads in a row that are 

[PATCH] ieee1275: add nvme support within ofpath

[Eric Snowberg]

Add NVMe support within ofpath.

The Open Firmware text representation for a NVMe device contains the
Namespace ID. An invalid namespace ID is one whose value is zero or whose
value is greater than the value reported by the Number of Namespaces (NN)
field in the Identify Controller data structure.  At the moment  only a
single Namespace is supported, therefore the value is currently hard coded
to one.

Signed-off-by: Eric Snowberg 
---
 grub-core/osdep/linux/ofpath.c |   47 
 1 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/grub-core/osdep/linux/ofpath.c b/grub-core/osdep/linux/ofpath.c
index 8d7d683..af068f9 100644
--- a/grub-core/osdep/linux/ofpath.c
+++ b/grub-core/osdep/linux/ofpath.c
@@ -350,6 +350,50 @@ of_path_of_ide(const char *sys_devname 
__attribute__((unused)), const char *devi
   return ret;
 }
 
+static char *
+of_path_of_nvme(const char *sys_devname __attribute__((unused)),
+   const char *device,
+   const char *devnode __attribute__((unused)),
+   const char *devicenode)
+{
+  char *sysfs_path, *of_path, disk[MAX_DISK_CAT];
+  const char *digit_string, *part_end;
+
+  digit_string = trailing_digits (device);
+  part_end = devicenode + strlen (devicenode) - 1;
+
+  if ((digit_string != '\0') && (*part_end == 'p'))
+{
+  /* We have a partition number, strip it off. */
+  int part;
+  char *nvmedev, *end;
+
+  nvmedev = strdup (devicenode);
+
+  if (nvmedev == NULL)
+return NULL;
+
+  end = nvmedev + strlen (nvmedev) - 1;
+  /* Remove the p. */
+  *end = '\0';
+  sscanf (digit_string, "%d", );
+  snprintf (disk, sizeof (disk), "/disk@1:%c", 'a' + (part - 1));
+  sysfs_path = block_device_get_sysfs_path_and_link (nvmedev);
+  free (nvmedev);
+}
+  else
+{
+  /* We do not have the parition. */
+  snprintf (disk, sizeof (disk), "/disk@1");
+  sysfs_path = block_device_get_sysfs_path_and_link (device);
+}
+
+  of_path = find_obppath (sysfs_path);
+  free (sysfs_path);
+  strcat (of_path, disk);
+  return of_path;
+}
+
 static int
 vendor_is_ATA(const char *path)
 {
@@ -681,6 +725,9 @@ grub_util_devname_to_ofpath (const char *sys_devname)
 /* All the models I've seen have a devalias "floppy".
New models have no floppy at all. */
 ofpath = xstrdup ("floppy");
+  else if (device[0] == 'n' && device[1] == 'v' && device[2] == 'm'
+   && device[3] == 'e')
+ofpath = of_path_of_nvme (name_buf, device, devnode, devicenode);
   else
 {
   grub_util_warn (_("unknown device type %s"), device);
-- 
1.7.1


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [GRUB PARTUUID PATCH V5 2/3] Add PARTUUID detection support to grub-probe

[Daniel Kiper]

On Sun, Feb 04, 2018 at 11:47:36AM -0800, Nicholas Vinson wrote:
> Add PARTUUID detection support grub-probe for MBR and GPT partition
> schemes.  The Linux kernel supports mounting the root filesystem by
> Linux device name or by the Partition [GU]UID.  GRUB's mkconfig,
> however, currently only supports specifing the rootfs in the kernel
> command-line by Linux device name unless an initramfs is also present.
> When an initramfs is present GRUB's mkconfig will set the kernel's root
> parameter value to either the Linux device name or to the filesystem
> [GU]UID.
>
> Therefore, the only way to protect a Linux system from failing to boot
> when its Linux storage device names change is to either manually edit
> grub.cfg or /etc/default/grub and append root=PARTUUID=xxx to the
> command-line or create an initramfs that understands how to mount
> devices by filesystem [G]UID and let grub-mkconfig pass the filesystem
> [GU]UID to the initramfs.
>
> The goal of this patch set is to enable root=PARTUUID=xxx support in
> grub-mkconfig, so that users don't have to manually edit
> /etc/default/grub or grub.cfg, or create an initramfs for the sole
> purpose of having a robust bootloader configuration for Linux.
> ---
>  util/grub-probe.c | 51 +++
>  1 file changed, 51 insertions(+)
>
> diff --git a/util/grub-probe.c b/util/grub-probe.c
> index 21cb80fbe..3656e32e8 100644
> --- a/util/grub-probe.c
> +++ b/util/grub-probe.c
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -62,6 +63,7 @@ enum {
>PRINT_DRIVE,
>PRINT_DEVICE,
>PRINT_PARTMAP,
> +  PRINT_PARTUUID,
>PRINT_ABSTRACTION,
>PRINT_CRYPTODISK_UUID,
>PRINT_HINT_STR,
> @@ -85,6 +87,7 @@ static const char *targets[] =
>  [PRINT_DRIVE]  = "drive",
>  [PRINT_DEVICE] = "device",
>  [PRINT_PARTMAP]= "partmap",
> +[PRINT_PARTUUID]   = "partuuid",
>  [PRINT_ABSTRACTION]= "abstraction",
>  [PRINT_CRYPTODISK_UUID]= "cryptodisk_uuid",
>  [PRINT_HINT_STR]   = "hints_string",
> @@ -181,6 +184,48 @@ probe_partmap (grub_disk_t disk, char delim)
>  }
>  }
>
> +static void
> +probe_partuuid (grub_disk_t disk, char delim)
> +{
> +  /*
> +   * Nested partitions not supported for now.
> +   * Non-nested partitions must have disk->partition->parent == NULL
> +   */
> +  if (disk->partition && disk->partition->parent == NULL)
> +{
> +  if (strcmp(disk->partition->partmap->name, "msdos") == 0)
> + {
> + /*
> +  * The partition GUID for MSDOS is the partition number (starting
> +  * with 1) prepended with the NT disk signature.
> +  */
> +grub_uint32_t nt_disk_sig;

Here you use spaces...

> + grub_partition_t p = disk->partition;

...and here tab and spaces. I prefer the latter.
Please fix this.

> + disk->partition = p->parent;
> +
> + if (grub_disk_read (disk, 0, GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC,
> + sizeof(nt_disk_sig), _disk_sig) == 0)
> +   {
> + grub_printf ("%08x-%02x",
> +  grub_le_to_cpu32(nt_disk_sig), 1 + p->number);
> +   }

These curly brackets are not needed. Please remove them and leave empty
line before next instruction.

> + disk->partition = p;
> + }
> +  else if (strcmp(disk->partition->partmap->name, "gpt") == 0)
> + {
> +   grub_partition_t p = disk->partition;
> +   struct grub_gpt_partentry gptdata;
> +
> +   disk->partition = p->parent;
> +
> +   if (grub_disk_read (disk, p->offset, p->index,
> +   sizeof(gptdata), ) == 0)
> + print_gpt_guid(gptdata.guid);

Please add empty line here.

> +   disk->partition = p;
> + }
> +}
> +}
> +

Otherwise patch LGMT.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [GRUB PARTUUID PATCH V5 3/3] Update grub script template files

[Daniel Kiper]

On Sun, Feb 04, 2018 at 11:47:37AM -0800, Nicholas Vinson wrote:
> Update grub-mkconfig.in and 10_linux.in to support grub-probe's new
> partuuid target.  Update grub.texi documenation.

As earlier lack of SOB. Otherwise LGTM.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [GRUB PARTUUID PATCH V5 1/3] Update grub_gpt_partentry; centralize guid prints

[Daniel Kiper]

On Sun, Feb 04, 2018 at 11:47:35AM -0800, Nicholas Vinson wrote:
> To help clean the code and simplify the code in util/grub-probe.c, this
> patch renames grub_gpt_part_type to grub_gpt_part_guid and updates
> grub_gpt_partentry to use this type for both the partition type GUID
> string and the partition GUID string entries.
>
> This patch also moves the GUID printing logic in util/grub-probe.c to a
> separate function.  This change allows the partuuid logic in the next
> commit to use the same printing logic without having to completely
> duplicate it.

One logical change per patch please...
And please add your SOB next time.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] libgcrypt: Import replacement CRC operations

[Daniel Kiper]

On Sun, Feb 11, 2018 at 12:18:12AM +, Colin Watson wrote:
> The CRC implementation imported from libgcrypt 1.5.3 is arguably
> non-free, due to being encumbered by the restrictive Internet Society
> licence on RFCs (see e.g. https://wiki.debian.org/NonFreeIETFDocuments).
> Fortunately, libgcrypt has since replaced it with a version that is both
> reportedly better-optimised and doesn't suffer from this encumbrance.
>
> The ideal solution would be to update to a new version of libgcrypt, and
> I spent some time trying to do that.  However, util/import_gcry.py
> requires complex modifications to cope with the new version, and I
> stalled part-way through; furthermore, GRUB's libgcrypt tree already
> contains some backports of upstream changes.  Rather than allowing the
> perfect to be the enemy of the good, I think it's best to backport this
> single change to at least sort out the licensing situation.  Doing so
> won't make things any harder for a future wholesale upgrade.
>
> This commit is mostly a straightforward backport of
> https://git.gnupg.org/cgi-bin/gitweb.cgi?p=libgcrypt.git;a=commitdiff;h=06e122baa3321483a47bbf82fd2a4540becfa0c9,
> but I also imported bufhelp.h from libgcrypt 1.7.0 (newer versions
> required further changes elsewhere).
>
> I've tested that "hashsum -h crc32" still produces correct output for a
> variety of files on both i386-pc and x86_64-emu targets.
>
> Signed-off-by: Colin Watson 

Reviewed-by: Daniel Kiper 

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] x86-64: Treat R_X86_64_PLT32 as R_X86_64_PC32

[Daniel Kiper]

On Sat, Feb 17, 2018 at 06:47:28AM -0800, H.J. Lu wrote:
> Starting from binutils commit bd7ab16b4537788ad53521c45469a1bdae84ad4a:
>
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=bd7ab16b4537788ad53521c45469a1bdae84ad4a
>
> x86-64 assembler generates R_X86_64_PLT32, instead of R_X86_64_PC32, for
> 32-bit PC-relative branches.  Grub2 should treat R_X86_64_PLT32 as
> R_X86_64_PC32.
>
> Signed-off-by: H.J. Lu 

Reviewed-by: Daniel Kiper 

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2] chainloader: Fix wrong break condition (must be AND not, OR)

[Daniel Kiper]

On Mon, Feb 19, 2018 at 03:26:35PM +0100, C. Masloch wrote:
> The definition of bpb's num_total_sectors_16 and num_total_sectors_32
> is that either the 16-bit field is non-zero and is used (in which case
> eg mkfs.fat sets the 32-bit field to zero), or it is zero and the
> 32-bit field is used. Therefore, a BPB is invalid only if *both*
> fields are zero; having one field as zero and the other as non-zero is
> the case to be expected. (Indeed, according to Microsoft's specification
> one of the fields *must* be zero, and the other non-zero.)
>
> This affects all users of grub_chainloader_patch_bpb which are in
> chainloader.c, freedos.c, and ntldr.c
>
> Some descriptions of the semantics of these two fields:
>
> https://www.win.tue.nl/~aeb/linux/fs/fat/fat-1.html
>
> > The old 2-byte fields "total number of sectors" and "number of
> > sectors per FAT" are now zero; this information is now found in
> > the new 4-byte fields.
>
> (Here given in the FAT32 EBPB section but the total sectors 16/32 bit
> fields semantic is true of FAT12 and FAT16 too.)
>
> https://wiki.osdev.org/FAT#BPB_.28BIOS_Parameter_Block.29
>
> > 19 | 2 | The total sectors in the logical volume. If this value is 0,
> > it means there are more than 65535 sectors in the volume, and the actual
> > count is stored in "Large Sectors (bytes 32-35).
>
> > 32 | 4 | Large amount of sector on media. This field is set if there
> > are more than 65535 sectors in the volume.
>
> (Doesn't specify what the "large" field is set to when unused, but as
> mentioned mkfs.fat sets it to zero then.)
>
> https://technet.microsoft.com/en-us/library/cc976796.aspx
>
> > 0x13 | WORD | 0x |
> > Small Sectors . The number of sectors on the volume represented in 16
> > bits (< 65,536). For volumes larger than 65,536 sectors, this field
> > has a value of zero and the Large Sectors field is used instead.
>
> > 0x20 | DWORD | 0x01F03E00 |
> > Large Sectors . If the value of the Small Sectors field is zero, this
> > field contains the total number of sectors in the FAT16 volume. If the
> > value of the Small Sectors field is not zero, the value of this field
> > is zero.
>
> https://staff.washington.edu/dittrich/misc/fatgen103.pdf page 10
>
> > BPB_TotSec16 | 19 | 2 |
> > This field is the old 16-bit total count of sectors on the volume.
> > This count includes the count of all sectors in all four regions of the
> > volume. This field can be 0; if it is 0, then BPB_TotSec32 must be
> > non-zero. For FAT32 volumes, this field must be 0. For FAT12 and
> > FAT16 volumes, this field contains the sector count, and
> > BPB_TotSec32 is 0 if the total sector count ???fits??? (is less than
> > 0x1).
>
> > BPB_TotSec32 | 32 | 4 |
> > This field is the new 32-bit total count of sectors on the volume.
> > This count includes the count of all sectors in all four regions of the
> > volume. This field can be 0; if it is 0, then BPB_TotSec16 must be
> > non-zero. For FAT32 volumes, this field must be non-zero. For
> > FAT12/FAT16 volumes, this field contains the sector count if
> > BPB_TotSec16 is 0 (count is greater than or equal to 0x1).
>
> (This specifies that an unused BPB_TotSec32 field is set to zero.)
>
> Tested with lDebug booted in qemu via grub2's
> FreeDOS direct loading support, refer to
> https://bitbucket.org/ecm/ldosboot + https://bitbucket.org/ecm/ldebug
>
> Signed-off-by: C. Masloch 

I will add to the commit message that by the way you are fixing comments in 
include/grub/fat.h.

Otherwise Reviewed-by: Daniel Kiper 

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: LiMux GRUB-Patches

[Max Harmathy]

Hi Adrian!

Thank you for the reply!

Am 13.02.2018 um 11:02 schrieb John Paul Adrian Glaubitz:
> Did you actually try setting a password without patch GRUB2? We
> just added the following to /etc/grub.d/40_custom:
> 
> #!/bin/sh
> exec tail -n +3 $0
> # This file provides an easy way to add custom menu entries.  Simply
> type the
> # menu entries you want to add after this comment.  Be careful not to
> change
> # the 'exec tail' line above.
> 
> #Password Protection
> set superusers="root"
> password_pbkdf2 root grub.pbkdf2.sha512.1.

This works like a charm!
Another upstream package we can use as is.
 Thank you!

Probably we would have been able to go without patching in the first place.


Max

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 2/2] .mod files: Strip annobin annotations and .eh_frame, and their relocations

[Daniel Kiper]

On Wed, Jan 31, 2018 at 11:27:00AM -0500, Peter Jones wrote:
> This way debuginfo built from the .module will still include this
> information, but the final result won't have the data we don't actually
> need in the modules, either on-disk, loaded at runtime, or in prebuilt
> images.
>
> Signed-off-by: Peter Jones 

Reviewed-by: Daniel Kiper 

I suppose that I can apply this patch without patch 1
but I would like to be sure... So?

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/2] mkimage: avoid copying relocations for sections that won't be copied.

[Daniel Kiper]

On Wed, Jan 31, 2018 at 11:26:59AM -0500, Peter Jones wrote:
> Some versions of gcc include a plugin called "annobin", and in some
> build systems this is enabled by default.  This plugin creates special
> ELF note sections to track which ABI-breaking features are used by a
> binary, as well as a series of relocations to annotate where.
>
> If grub is compiled with this feature, then when grub-mkimage translates
> the binary to another file format which does not strongly associate
> relocation data with sections (i.e. when platform is *-efi), these
> relocations appear to be against the .text section rather than the
> original note section.  When the binary is loaded by the PE runtime
> loader, hilarity ensues.
>
> This issue is not necessarily limited to the annobin, but could arise
> any time there are relocations in sections that are not represented in
> grub-mkimage's output.
>
> This patch seeks to avoid this issue by only including relocations that
> refer to sections which will be included in the final binary.
>
> As an aside, this should also obviate the need to avoid -funwind-tables,
> -fasynchronous-unwind-tables, and any sections similar to .eh_frame in
> the future.  I've tested it on x86-64-efi with the following gcc command
> line options (as recorded by -grecord-gcc-flags), but I still need to
> test the result on some other platforms that have been problematic in
> the past (especially ARM Aarch64) before I feel comfortable making
> changes to the configure.ac bits:
>
> GNU C11 7.2.1 20180116 (Red Hat 7.2.1-7) -mno-mmx -mno-sse -mno-sse2 
> -mno-sse3 -mno-3dnow -msoft-float -mno-stack-arg-probe -mcmodel=large 
> -mno-red-zone -m64 -mtune=generic -march=x86-64 -g3 -Os -freg-struct-return 
> -fno-stack-protector -ffreestanding -funwind-tables 
> -fasynchronous-unwind-tables -fno-strict-aliasing -fstack-clash-protection 
> -fno-ident -fplugin=annobin
>
> Signed-off-by: Peter Jones 

In general I am OK with the idea but...

> ---
>  util/grub-mkimagexx.c | 81 
> ++-
>  1 file changed, 73 insertions(+), 8 deletions(-)
>
> diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
> index a2bb05439f0..b016b061c8c 100644
> --- a/util/grub-mkimagexx.c
> +++ b/util/grub-mkimagexx.c
> @@ -708,6 +708,13 @@ arm_get_trampoline_size (Elf_Ehdr *e,
>  }
>  #endif
>
> +static int
> +SUFFIX (is_kept_section) (Elf_Shdr *s, const struct 
> grub_install_image_target_desc *image_target);
> +static int
> +SUFFIX (is_kept_reloc_section) (Elf_Shdr *s, const struct 
> grub_install_image_target_desc *image_target,
> + Elf_Shdr *sections, Elf_Half section_entsize, 
> Elf_Half num_sections,
> + const char *strtab);

Ugh... Could not you create a struct and pass the pointer to it here?

Daniel

PS Please CC me on the patches next time.

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel