[PATCH 00/10] Another set of patches from Fedora

[PATCH 00/10] Another set of patches from Fedora

[Javier Martinez Canillas]

Hello,

This is another set of somewhat small patches that we have been carrying in the
Fedora package. I found that some of them have already been posted to the list,
so for those patches I tried to address the issues pointed out in that version.

I'm including all patches in the same set because I think that will make review
easier but I can split in smaller patch series if that is preferred.

Best regards,
Javier


Hans de Goede (5):
  efi/console: Do not set text-mode until we actually need it
  efi/console: Add grub_console_read_key_stroke() helper function
  efi/console: Implement getkeystatus() support
  kern/term: Make grub_getkeystatus helper funtion available everywhere
  kern/term: Accept ESC, F4 and holding SHIFT as user interrupt keys

Javier Martinez Canillas (1):
  efi/console: Move grub_console_set{colorstate,cursor} higher in the
    file

Peter Jones (3):
  i386: Make pmtimer tsc calibration not take 51 seconds to fail
  kern: Make it possible to subtract conditions from debug=
  module-verifier: make it possible to run checkers on
    grub-module-verifierxx.c

Steve Langasek (1):
  templates: Output a menu entry for firmware setup on UEFI FastBoot
    systems

 Makefile.util.def                 |   6 +
 grub-core/commands/keystatus.c    |  18 ---
 grub-core/commands/sleep.c        |   2 +-
 grub-core/kern/i386/tsc_pmtimer.c | 109 +++++++++++---
 grub-core/kern/misc.c             |  14 +-
 grub-core/kern/term.c             |  34 +++++
 grub-core/normal/menu.c           |   2 +-
 grub-core/term/efi/console.c      | 241 ++++++++++++++++++++----------
 include/grub/term.h               |   6 +-
 util/grub-module-verifier32.c     |   2 +
 util/grub-module-verifier64.c     |   2 +
 util/grub-module-verifierXX.c     |   9 ++
 util/grub.d/30_uefi-firmware.in   |  46 ++++++
 13 files changed, 371 insertions(+), 120 deletions(-)
 create mode 100644 util/grub.d/30_uefi-firmware.in

-- 
2.24.1

[PATCH 01/10] i386: Make pmtimer tsc calibration not take 51 seconds to fail

[Javier Martinez Canillas]

From: Peter Jones <pjones@redhat.com>

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 0xffffffff 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:

pmtimer: 0xffffff bad_reads: 1
pmtimer: 0xffffff bad_reads: 2
pmtimer: 0xffffff bad_reads: 3
pmtimer: 0xffffff bad_reads: 4
pmtimer: 0xffffff bad_reads: 5
pmtimer: 0xffffff bad_reads: 6
pmtimer: 0xffffff bad_reads: 7
pmtimer: 0xffffff bad_reads: 8
pmtimer: 0xffffff bad_reads: 9
pmtimer: 0xffffff bad_reads: 10
timer is broken; giving up.

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:

pmtimer delta is 0x0 (1904 iterations)
tsc delta is implausible: 0x2626aa0

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:

pmtimer delta is 0xdff
tsc delta is 0x278756

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 <pjones@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

---

 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..412f5fe3c83 100644
--- a/grub-core/kern/i386/tsc_pmtimer.c
+++ b/grub-core/kern/i386/tsc_pmtimer.c
@@ -28,40 +28,101 @@
 #include <grub/acpi.h>
 #include <grub/cpu/io.h>
 
+/*
+ * 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) & 0xffffff;
-  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) & 0x00ffffffUL;
   end = start + num_pm_ticks;
   start_tsc = grub_get_tsc ();
   while (1)
     {
-      cur = grub_inl (pmtimer) & 0xffffff;
-      if (cur < last)
-	cur |= 0x1000000;
-      num_iter++;
+      cur &= 0xffffffffff000000ULL;
+      cur |= grub_inl (pmtimer) & 0x00ffffffUL;
+
+      end_tsc = grub_get_tsc();
+
+#ifndef GRUB_PMTIMER_IGNORE_BAD_READS
+      /*
+       * If we get 10 reads in a row that are obviously dead pins, there's no
+       * reason to do this thousands of times.
+       */
+      if (cur == 0xffffffUL || cur == 0)
+	{
+	  bad_reads++;
+	  grub_dprintf ("pmtimer",
+			"pmtimer: 0x%"PRIxGRUB_UINT64_T" bad_reads: %d\n",
+			cur, bad_reads);
+	  grub_dprintf ("pmtimer", "timer is broken; giving up.\n");
+
+	  if (bad_reads == 10)
+	    return 0;
+	}
+#endif
+
+      if (cur < start)
+	cur += 0x1000000;
+
       if (cur >= end)
 	{
-	  end_tsc = grub_get_tsc ();
+	  grub_dprintf ("pmtimer", "pmtimer delta is 0x%"PRIxGRUB_UINT64_T"\n",
+			cur - start);
+	  grub_dprintf ("pmtimer", "tsc delta is 0x%"PRIxGRUB_UINT64_T"\n",
+			end_tsc - start_tsc);
 	  return end_tsc - start_tsc;
 	}
-      /* Check for broken PM timer.
-	 50000000 TSCs is between 5 ms (10GHz) and 200 ms (250 MHz)
-	 if after this time we still don't have 1 ms on pmtimer, then
-	 pmtimer is broken.
+
+      /*
+       * Check for broken PM timer.  1ms at 10GHz should be 1E+7 TSCs; at
+       * 250MHz it should be 2.5E5.  So if after 4E+7 TSCs on a 10GHz machine,
+       * we should have seen pmtimer show 4ms of change (i.e. cur =~
+       * start+14320); on a 250MHz machine that should be 160ms (start+572800).
+       * If after this a time we still don't have 1ms on pmtimer, then pmtimer
+       * is broken.
+       *
+       * Likewise, if our code is perfectly efficient and introduces no delays
+       * whatsoever, on a 10GHz system we should see a TSC delta of 3580 in
+       * ~3580 iterations.  On a 250MHz machine that should be ~900 iterations.
+       *
+       * With those factors in mind, there are two limits here.  There's a hard
+       * limit here at 8x our desired pm timer delta, picked as an arbitrarily
+       * large value that's still not a lot of time to humans, because if we
+       * get that far this is either an implausibly fast machine or the pmtimer
+       * is not running.  And there's another limit on 4x our 10GHz tsc delta
+       * without seeing cur converge on our target value.
        */
-      if ((num_iter & 0xffffff) == 0 && grub_get_tsc () - start_tsc > 5000000) {
-	return 0;
-      }
+      if ((++num_iter > (grub_uint32_t)num_pm_ticks << 3UL) ||
+	  end_tsc - start_tsc > 40000000)
+	{
+	  grub_dprintf ("pmtimer",
+			"pmtimer delta is 0x%"PRIxGRUB_UINT64_T" (%u iterations)\n",
+			cur - start, num_iter);
+	  grub_dprintf ("pmtimer",
+			"tsc delta is implausible: 0x%"PRIxGRUB_UINT64_T"\n",
+			end_tsc - start_tsc);
+	  return 0;
+	}
     }
 }
 
@@ -74,12 +135,20 @@ grub_tsc_calibrate_from_pmtimer (void)
 
   fadt = grub_acpi_find_fadt ();
   if (!fadt)
-    return 0;
+    {
+      grub_dprintf ("pmtimer", "No FADT found; not using pmtimer.\n");
+      return 0;
+    }
   pmtimer = fadt->pmtimer;
   if (!pmtimer)
-    return 0;
+    {
+      grub_dprintf ("pmtimer", "FADT does not specify pmtimer; skipping.\n");
+      return 0;
+    }
 
-  /* It's 3.579545 MHz clock. Wait 1 ms.  */
+  /*
+   * It's 3.579545 MHz clock. Wait 1 ms.
+   */
   tsc_diff = grub_pmtimer_wait_count_tsc (pmtimer, 3580);
   if (tsc_diff == 0)
     return 0;
-- 
2.24.1

[PATCH 02/10] efi/console: Move grub_console_set{colorstate, cursor} higher in the file

[Javier Martinez Canillas]

This is just a preparatory patch to move the functions higher in the file,
since these will be called by the grub_prepare_for_text_output() function
that will be introduced in a subsequent patch.

Moving the functions will avoid a forward declaration in that next patch.

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

---

 grub-core/term/efi/console.c | 82 ++++++++++++++++++------------------
 1 file changed, 41 insertions(+), 41 deletions(-)

diff --git a/grub-core/term/efi/console.c b/grub-core/term/efi/console.c
index 4840cc59d3f..59d9604472a 100644
--- a/grub-core/term/efi/console.c
+++ b/grub-core/term/efi/console.c
@@ -65,6 +65,47 @@ map_char (grub_uint32_t c)
   return c;
 }
 
+static void
+grub_console_setcolorstate (struct grub_term_output *term
+			    __attribute__ ((unused)),
+			    grub_term_color_state state)
+{
+  grub_efi_simple_text_output_interface_t *o;
+
+  if (grub_efi_is_finished)
+    return;
+
+  o = grub_efi_system_table->con_out;
+
+  switch (state) {
+    case GRUB_TERM_COLOR_STANDARD:
+      efi_call_2 (o->set_attributes, o, GRUB_TERM_DEFAULT_STANDARD_COLOR
+		  & 0x7f);
+      break;
+    case GRUB_TERM_COLOR_NORMAL:
+      efi_call_2 (o->set_attributes, o, grub_term_normal_color & 0x7f);
+      break;
+    case GRUB_TERM_COLOR_HIGHLIGHT:
+      efi_call_2 (o->set_attributes, o, grub_term_highlight_color & 0x7f);
+      break;
+    default:
+      break;
+  }
+}
+
+static void
+grub_console_setcursor (struct grub_term_output *term __attribute__ ((unused)),
+			int on)
+{
+  grub_efi_simple_text_output_interface_t *o;
+
+  if (grub_efi_is_finished)
+    return;
+
+  o = grub_efi_system_table->con_out;
+  efi_call_2 (o->enable_cursor, o, on);
+}
+
 static void
 grub_console_putchar (struct grub_term_output *term __attribute__ ((unused)),
 		      const struct grub_unicode_glyph *c)
@@ -281,47 +322,6 @@ grub_console_cls (struct grub_term_output *term __attribute__ ((unused)))
   efi_call_2 (o->set_attributes, o, orig_attr);
 }
 
-static void
-grub_console_setcolorstate (struct grub_term_output *term
-			    __attribute__ ((unused)),
-			    grub_term_color_state state)
-{
-  grub_efi_simple_text_output_interface_t *o;
-
-  if (grub_efi_is_finished)
-    return;
-
-  o = grub_efi_system_table->con_out;
-
-  switch (state) {
-    case GRUB_TERM_COLOR_STANDARD:
-      efi_call_2 (o->set_attributes, o, GRUB_TERM_DEFAULT_STANDARD_COLOR
-		  & 0x7f);
-      break;
-    case GRUB_TERM_COLOR_NORMAL:
-      efi_call_2 (o->set_attributes, o, grub_term_normal_color & 0x7f);
-      break;
-    case GRUB_TERM_COLOR_HIGHLIGHT:
-      efi_call_2 (o->set_attributes, o, grub_term_highlight_color & 0x7f);
-      break;
-    default:
-      break;
-  }
-}
-
-static void
-grub_console_setcursor (struct grub_term_output *term __attribute__ ((unused)),
-			int on)
-{
-  grub_efi_simple_text_output_interface_t *o;
-
-  if (grub_efi_is_finished)
-    return;
-
-  o = grub_efi_system_table->con_out;
-  efi_call_2 (o->enable_cursor, o, on);
-}
-
 static grub_err_t
 grub_efi_console_output_init (struct grub_term_output *term)
 {
-- 
2.24.1

[PATCH 03/10] efi/console: Do not set text-mode until we actually need it

[Javier Martinez Canillas]

From: Hans de Goede <hdegoede@redhat.com>

If we're running with a hidden menu we may never need text mode, so do not
change the video-mode to text until we actually need it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

---

 grub-core/term/efi/console.c | 74 +++++++++++++++++++++++-------------
 include/grub/term.h          |  4 +-
 2 files changed, 51 insertions(+), 27 deletions(-)

diff --git a/grub-core/term/efi/console.c b/grub-core/term/efi/console.c
index 59d9604472a..16ccd350ee0 100644
--- a/grub-core/term/efi/console.c
+++ b/grub-core/term/efi/console.c
@@ -24,6 +24,16 @@
 #include <grub/efi/api.h>
 #include <grub/efi/console.h>
 
+typedef enum {
+    GRUB_TEXT_MODE_UNDEFINED = -1,
+    GRUB_TEXT_MODE_UNAVAILABLE = 0,
+    GRUB_TEXT_MODE_AVAILABLE
+}
+grub_text_mode;
+
+static grub_text_mode text_mode = GRUB_TEXT_MODE_UNDEFINED;
+static grub_term_color_state text_colorstate = GRUB_TERM_COLOR_UNDEFINED;
+
 static grub_uint32_t
 map_char (grub_uint32_t c)
 {
@@ -106,15 +116,39 @@ grub_console_setcursor (struct grub_term_output *term __attribute__ ((unused)),
   efi_call_2 (o->enable_cursor, o, on);
 }
 
+static grub_err_t
+grub_prepare_for_text_output(struct grub_term_output *term)
+{
+  if (grub_efi_is_finished)
+    return GRUB_ERR_BAD_DEVICE;
+
+  if (text_mode != GRUB_TEXT_MODE_UNDEFINED)
+    return text_mode ? 0 : GRUB_ERR_BAD_DEVICE;
+
+  if (! grub_efi_set_text_mode (1))
+    {
+      /* This really should never happen */
+      grub_error (GRUB_ERR_BAD_DEVICE, "cannot set text mode");
+      text_mode = GRUB_TEXT_MODE_UNAVAILABLE;
+      return GRUB_ERR_BAD_DEVICE;
+    }
+
+  grub_console_setcursor (term, 1);
+  if (text_colorstate != GRUB_TERM_COLOR_UNDEFINED)
+    grub_console_setcolorstate (term, text_colorstate);
+  text_mode = GRUB_TEXT_MODE_AVAILABLE;
+  return 0;
+}
+
 static void
-grub_console_putchar (struct grub_term_output *term __attribute__ ((unused)),
+grub_console_putchar (struct grub_term_output *term,
 		      const struct grub_unicode_glyph *c)
 {
   grub_efi_char16_t str[2 + 30];
   grub_efi_simple_text_output_interface_t *o;
   unsigned i, j;
 
-  if (grub_efi_is_finished)
+  if (grub_prepare_for_text_output (term))
     return;
 
   o = grub_efi_system_table->con_out;
@@ -264,14 +298,15 @@ grub_console_getkey (struct grub_term_input *term)
 }
 
 static struct grub_term_coordinate
-grub_console_getwh (struct grub_term_output *term __attribute__ ((unused)))
+grub_console_getwh (struct grub_term_output *term)
 {
   grub_efi_simple_text_output_interface_t *o;
   grub_efi_uintn_t columns, rows;
 
   o = grub_efi_system_table->con_out;
-  if (grub_efi_is_finished || efi_call_4 (o->query_mode, o, o->mode->mode,
-					  &columns, &rows) != GRUB_EFI_SUCCESS)
+  if (grub_prepare_for_text_output (term) != GRUB_ERR_NONE ||
+      efi_call_4 (o->query_mode, o, o->mode->mode,
+		  &columns, &rows) != GRUB_EFI_SUCCESS)
     {
       /* Why does this fail?  */
       columns = 80;
@@ -286,7 +321,7 @@ grub_console_getxy (struct grub_term_output *term __attribute__ ((unused)))
 {
   grub_efi_simple_text_output_interface_t *o;
 
-  if (grub_efi_is_finished)
+  if (grub_efi_is_finished || text_mode != GRUB_TEXT_MODE_AVAILABLE)
     return (struct grub_term_coordinate) { 0, 0 };
 
   o = grub_efi_system_table->con_out;
@@ -294,12 +329,12 @@ grub_console_getxy (struct grub_term_output *term __attribute__ ((unused)))
 }
 
 static void
-grub_console_gotoxy (struct grub_term_output *term __attribute__ ((unused)),
+grub_console_gotoxy (struct grub_term_output *term,
 		     struct grub_term_coordinate pos)
 {
   grub_efi_simple_text_output_interface_t *o;
 
-  if (grub_efi_is_finished)
+  if (grub_prepare_for_text_output (term))
     return;
 
   o = grub_efi_system_table->con_out;
@@ -312,7 +347,7 @@ grub_console_cls (struct grub_term_output *term __attribute__ ((unused)))
   grub_efi_simple_text_output_interface_t *o;
   grub_efi_int32_t orig_attr;
 
-  if (grub_efi_is_finished)
+  if (grub_efi_is_finished || text_mode != GRUB_TEXT_MODE_AVAILABLE)
     return;
 
   o = grub_efi_system_table->con_out;
@@ -322,19 +357,15 @@ grub_console_cls (struct grub_term_output *term __attribute__ ((unused)))
   efi_call_2 (o->set_attributes, o, orig_attr);
 }
 
-static grub_err_t
-grub_efi_console_output_init (struct grub_term_output *term)
-{
-  grub_efi_set_text_mode (1);
-  grub_console_setcursor (term, 1);
-  return 0;
-}
-
 static grub_err_t
 grub_efi_console_output_fini (struct grub_term_output *term)
 {
+  if (text_mode != GRUB_TEXT_MODE_AVAILABLE)
+    return 0;
+
   grub_console_setcursor (term, 0);
   grub_efi_set_text_mode (0);
+  text_mode = GRUB_TEXT_MODE_UNDEFINED;
   return 0;
 }
 
@@ -348,7 +379,6 @@ static struct grub_term_input grub_console_term_input =
 static struct grub_term_output grub_console_term_output =
   {
     .name = "console",
-    .init = grub_efi_console_output_init,
     .fini = grub_efi_console_output_fini,
     .putchar = grub_console_putchar,
     .getwh = grub_console_getwh,
@@ -364,14 +394,6 @@ static struct grub_term_output grub_console_term_output =
 void
 grub_console_init (void)
 {
-  /* FIXME: it is necessary to consider the case where no console control
-     is present but the default is already in text mode.  */
-  if (! grub_efi_set_text_mode (1))
-    {
-      grub_error (GRUB_ERR_BAD_DEVICE, "cannot set text mode");
-      return;
-    }
-
   grub_term_register_output ("console", &grub_console_term_output);
   grub_term_register_input ("console", &grub_console_term_input);
 }
diff --git a/include/grub/term.h b/include/grub/term.h
index 8117e2a24da..486b30e7125 100644
--- a/include/grub/term.h
+++ b/include/grub/term.h
@@ -75,9 +75,11 @@
 /* These are used to represent the various color states we use.  */
 typedef enum
   {
+    /* Used for uninitialized grub_term_color_state variables */
+    GRUB_TERM_COLOR_UNDEFINED = -1,
     /* The color used to display all text that does not use the
        user defined colors below.  */
-    GRUB_TERM_COLOR_STANDARD,
+    GRUB_TERM_COLOR_STANDARD = 0,
     /* The user defined colors for normal text.  */
     GRUB_TERM_COLOR_NORMAL,
     /* The user defined colors for highlighted text.  */
-- 
2.24.1

Re: [PATCH 01/10] i386: Make pmtimer tsc calibration not take 51 seconds to fail

[Daniel Kiper]

On Fri, Mar 13, 2020 at 08:06:33PM +0100, Javier Martinez Canillas wrote:
> From: Peter Jones <pjones@redhat.com>
>
> 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 0xffffffff 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:
>
> pmtimer: 0xffffff bad_reads: 1
> pmtimer: 0xffffff bad_reads: 2
> pmtimer: 0xffffff bad_reads: 3
> pmtimer: 0xffffff bad_reads: 4
> pmtimer: 0xffffff bad_reads: 5
> pmtimer: 0xffffff bad_reads: 6
> pmtimer: 0xffffff bad_reads: 7
> pmtimer: 0xffffff bad_reads: 8
> pmtimer: 0xffffff bad_reads: 9
> pmtimer: 0xffffff bad_reads: 10
> timer is broken; giving up.
>
> 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:
>
> pmtimer delta is 0x0 (1904 iterations)
> tsc delta is implausible: 0x2626aa0
>
> 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:
>
> pmtimer delta is 0xdff
> tsc delta is 0x278756
>
> 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 <pjones@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

It seems to me that this is very similar or even the same patch to
  https://lists.gnu.org/archive/html/grub-devel/2018-02/msg00067.html

If yes I had some comments which were never resolved:
  https://lists.gnu.org/archive/html/grub-devel/2018-02/msg00078.html

If you take my comments into account I am happy to get it into 2.06.

Daniel

> ---
>
>  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..412f5fe3c83 100644
> --- a/grub-core/kern/i386/tsc_pmtimer.c
> +++ b/grub-core/kern/i386/tsc_pmtimer.c
> @@ -28,40 +28,101 @@
>  #include <grub/acpi.h>
>  #include <grub/cpu/io.h>
>
> +/*
> + * 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) & 0xffffff;
> -  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) & 0x00ffffffUL;
>    end = start + num_pm_ticks;
>    start_tsc = grub_get_tsc ();
>    while (1)
>      {
> -      cur = grub_inl (pmtimer) & 0xffffff;
> -      if (cur < last)
> -	cur |= 0x1000000;
> -      num_iter++;
> +      cur &= 0xffffffffff000000ULL;
> +      cur |= grub_inl (pmtimer) & 0x00ffffffUL;
> +
> +      end_tsc = grub_get_tsc();
> +
> +#ifndef GRUB_PMTIMER_IGNORE_BAD_READS
> +      /*
> +       * If we get 10 reads in a row that are obviously dead pins, there's no
> +       * reason to do this thousands of times.
> +       */
> +      if (cur == 0xffffffUL || cur == 0)
> +	{
> +	  bad_reads++;
> +	  grub_dprintf ("pmtimer",
> +			"pmtimer: 0x%"PRIxGRUB_UINT64_T" bad_reads: %d\n",
> +			cur, bad_reads);
> +	  grub_dprintf ("pmtimer", "timer is broken; giving up.\n");
> +
> +	  if (bad_reads == 10)
> +	    return 0;
> +	}
> +#endif
> +
> +      if (cur < start)
> +	cur += 0x1000000;
> +
>        if (cur >= end)
>  	{
> -	  end_tsc = grub_get_tsc ();
> +	  grub_dprintf ("pmtimer", "pmtimer delta is 0x%"PRIxGRUB_UINT64_T"\n",
> +			cur - start);
> +	  grub_dprintf ("pmtimer", "tsc delta is 0x%"PRIxGRUB_UINT64_T"\n",
> +			end_tsc - start_tsc);
>  	  return end_tsc - start_tsc;
>  	}
> -      /* Check for broken PM timer.
> -	 50000000 TSCs is between 5 ms (10GHz) and 200 ms (250 MHz)
> -	 if after this time we still don't have 1 ms on pmtimer, then
> -	 pmtimer is broken.
> +
> +      /*
> +       * Check for broken PM timer.  1ms at 10GHz should be 1E+7 TSCs; at
> +       * 250MHz it should be 2.5E5.  So if after 4E+7 TSCs on a 10GHz machine,
> +       * we should have seen pmtimer show 4ms of change (i.e. cur =~
> +       * start+14320); on a 250MHz machine that should be 160ms (start+572800).
> +       * If after this a time we still don't have 1ms on pmtimer, then pmtimer
> +       * is broken.
> +       *
> +       * Likewise, if our code is perfectly efficient and introduces no delays
> +       * whatsoever, on a 10GHz system we should see a TSC delta of 3580 in
> +       * ~3580 iterations.  On a 250MHz machine that should be ~900 iterations.
> +       *
> +       * With those factors in mind, there are two limits here.  There's a hard
> +       * limit here at 8x our desired pm timer delta, picked as an arbitrarily
> +       * large value that's still not a lot of time to humans, because if we
> +       * get that far this is either an implausibly fast machine or the pmtimer
> +       * is not running.  And there's another limit on 4x our 10GHz tsc delta
> +       * without seeing cur converge on our target value.
>         */
> -      if ((num_iter & 0xffffff) == 0 && grub_get_tsc () - start_tsc > 5000000) {
> -	return 0;
> -      }
> +      if ((++num_iter > (grub_uint32_t)num_pm_ticks << 3UL) ||
> +	  end_tsc - start_tsc > 40000000)
> +	{
> +	  grub_dprintf ("pmtimer",
> +			"pmtimer delta is 0x%"PRIxGRUB_UINT64_T" (%u iterations)\n",
> +			cur - start, num_iter);
> +	  grub_dprintf ("pmtimer",
> +			"tsc delta is implausible: 0x%"PRIxGRUB_UINT64_T"\n",
> +			end_tsc - start_tsc);
> +	  return 0;
> +	}
>      }
>  }
>
> @@ -74,12 +135,20 @@ grub_tsc_calibrate_from_pmtimer (void)
>
>    fadt = grub_acpi_find_fadt ();
>    if (!fadt)
> -    return 0;
> +    {
> +      grub_dprintf ("pmtimer", "No FADT found; not using pmtimer.\n");
> +      return 0;
> +    }
>    pmtimer = fadt->pmtimer;
>    if (!pmtimer)
> -    return 0;
> +    {
> +      grub_dprintf ("pmtimer", "FADT does not specify pmtimer; skipping.\n");
> +      return 0;
> +    }
>
> -  /* It's 3.579545 MHz clock. Wait 1 ms.  */
> +  /*
> +   * It's 3.579545 MHz clock. Wait 1 ms.
> +   */
>    tsc_diff = grub_pmtimer_wait_count_tsc (pmtimer, 3580);
>    if (tsc_diff == 0)
>      return 0;
> --
> 2.24.1

Re: [PATCH 00/10] Another set of patches from Fedora

[Daniel Kiper]

On Fri, Mar 13, 2020 at 08:06:32PM +0100, Javier Martinez Canillas wrote:
> Hello,
>
> This is another set of somewhat small patches that we have been carrying in the
> Fedora package. I found that some of them have already been posted to the list,
> so for those patches I tried to address the issues pointed out in that version.
>
> I'm including all patches in the same set because I think that will make review
> easier but I can split in smaller patch series if that is preferred.
>
> Best regards,
> Javier
>
>
> Hans de Goede (5):
>   efi/console: Do not set text-mode until we actually need it
>   efi/console: Add grub_console_read_key_stroke() helper function
>   efi/console: Implement getkeystatus() support
>   kern/term: Make grub_getkeystatus helper funtion available everywhere
>   kern/term: Accept ESC, F4 and holding SHIFT as user interrupt keys
>
> Javier Martinez Canillas (1):
>   efi/console: Move grub_console_set{colorstate,cursor} higher in the
>     file

I think these patches should form one patchset.

> Peter Jones (3):
>   i386: Make pmtimer tsc calibration not take 51 seconds to fail
>   kern: Make it possible to subtract conditions from debug=
>   module-verifier: make it possible to run checkers on
>     grub-module-verifierxx.c
>
> Steve Langasek (1):
>   templates: Output a menu entry for firmware setup on UEFI FastBoot
>     systems

These should be posted separately, not in patchset...

Please take into account my comments and repost the patches. I am still
considering all of them as 2.06 material.

Daniel

Re: [PATCH 00/10] Another set of patches from Fedora

[Daniel Kiper]

On Fri, Apr 03, 2020 at 09:34:34PM +0200, Daniel Kiper wrote:
> On Fri, Mar 13, 2020 at 08:06:32PM +0100, Javier Martinez Canillas wrote:
> > Hello,
> >
> > This is another set of somewhat small patches that we have been carrying in the
> > Fedora package. I found that some of them have already been posted to the list,
> > so for those patches I tried to address the issues pointed out in that version.
> >
> > I'm including all patches in the same set because I think that will make review
> > easier but I can split in smaller patch series if that is preferred.
> >
> > Best regards,
> > Javier
> >
> >
> > Hans de Goede (5):
> >   efi/console: Do not set text-mode until we actually need it
> >   efi/console: Add grub_console_read_key_stroke() helper function
> >   efi/console: Implement getkeystatus() support
> >   kern/term: Make grub_getkeystatus helper funtion available everywhere
> >   kern/term: Accept ESC, F4 and holding SHIFT as user interrupt keys
> >
> > Javier Martinez Canillas (1):
> >   efi/console: Move grub_console_set{colorstate,cursor} higher in the
> >     file
>
> I think these patches should form one patchset.

...and functionality introduced by this ptachset begs for update in the
documentation...

> > Peter Jones (3):
> >   i386: Make pmtimer tsc calibration not take 51 seconds to fail
> >   kern: Make it possible to subtract conditions from debug=

...ditto...

Daniel

Re: [PATCH 01/10] i386: Make pmtimer tsc calibration not take 51 seconds to fail

[Javier Martinez Canillas]

Hello Daniel,

On 4/3/20 6:40 PM, Daniel Kiper wrote:

[snip]

>>
>> 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 <pjones@redhat.com>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> It seems to me that this is very similar or even the same patch to
>   https://lists.gnu.org/archive/html/grub-devel/2018-02/msg00067.html
>

Yes. Since we have been carrying our patches for a long time, before
posting any patch I search in the mailing list if it was previously
proposed and try to address any issues that were pointed out there.

In the future I'll explicit mention this and refer to the old thread
if that's the case.
 
> If yes I had some comments which were never resolved:
>   https://lists.gnu.org/archive/html/grub-devel/2018-02/msg00078.html
>
> If you take my comments into account I am happy to get it into 2.06.
>

I think that addressed most of them. I noticed now that I forgot to add
some comments that you asked to explain why some values were chosen. I
don't know the answer so I'll ask Peter about it and post a new version.
 
> Daniel
> 
Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH 00/10] Another set of patches from Fedora

[Javier Martinez Canillas]

On 4/3/20 9:34 PM, Daniel Kiper wrote:
> On Fri, Mar 13, 2020 at 08:06:32PM +0100, Javier Martinez Canillas wrote:
>> Hello,
>>
>> This is another set of somewhat small patches that we have been carrying in the
>> Fedora package. I found that some of them have already been posted to the list,
>> so for those patches I tried to address the issues pointed out in that version.
>>
>> I'm including all patches in the same set because I think that will make review
>> easier but I can split in smaller patch series if that is preferred.
>>
>> Best regards,
>> Javier
>>
>>
>> Hans de Goede (5):
>>   efi/console: Do not set text-mode until we actually need it
>>   efi/console: Add grub_console_read_key_stroke() helper function
>>   efi/console: Implement getkeystatus() support
>>   kern/term: Make grub_getkeystatus helper funtion available everywhere
>>   kern/term: Accept ESC, F4 and holding SHIFT as user interrupt keys
>>
>> Javier Martinez Canillas (1):
>>   efi/console: Move grub_console_set{colorstate,cursor} higher in the
>>     file
> 
> I think these patches should form one patchset.
>

Thanks a lot for your feedback and comments. I'll split this and also make sure
to update the docs besides addressing the issues you pointed out.
 
>> Peter Jones (3):
>>   i386: Make pmtimer tsc calibration not take 51 seconds to fail
>>   kern: Make it possible to subtract conditions from debug=
>>   module-verifier: make it possible to run checkers on
>>     grub-module-verifierxx.c
>>
>> Steve Langasek (1):
>>   templates: Output a menu entry for firmware setup on UEFI FastBoot
>>     systems
> 
> These should be posted separately, not in patchset...
>

Ok.
 
> Please take into account my comments and repost the patches. I am still
> considering all of them as 2.06 material.
>

Great, thanks again.
 
> Daniel
> 

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