[PATCH v2] i386: Make pmtimer tsc calibration not take 51 seconds to fail

2020-05-29 Thread Javier Martinez Canillas 
From: Peter Jones 

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:

pmtimer: 0xff bad_reads: 1
pmtimer: 0xff bad_reads: 2
pmtimer: 0xff bad_reads: 3
pmtimer: 0xff bad_reads: 4
pmtimer: 0xff bad_reads: 5
pmtimer: 0xff bad_reads: 6
pmtimer: 0xff bad_reads: 7
pmtimer: 0xff bad_reads: 8
pmtimer: 0xff bad_reads: 9
pmtimer: 0xff 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 
Signed-off-by: Javier Martinez Canillas 
---

Hello Daniel,

I think that addressed all the issues you pointed out to Peter in
https://lists.gnu.org/archive/html/grub-devel/2018-02/msg00078.html

Please let me know if I missed anything.

Best regards,
Javier

Changes in v2:
- Address issues pointed out by Daniel Kiper on previously posted version.

 grub-core/kern/i386/tsc_pmtimer.c | 112 --
 1 file changed, 92 insertions(+), 20 deletions(-)

diff --git a/grub-core/kern/i386/tsc_pmtimer.c 
b/grub-core/kern/i386/tsc_pmtimer.c
index c9c36169978..d9b3765b018 100644
--- a/grub-core/kern/i386/tsc_pmtimer.c
+++ b/grub-core/kern/i386/tsc_pmtimer.c
@@ -28,40 +28,104 @@
 #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) & 0x00ffUL;
   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;
+  /*
+   * Only take the low-order 24-bit for the reason explained above.
+   */
+  cur |= grub_inl (pmtimer) & 0x00ffUL;
+
+  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 == 0xffUL || 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 += 0x100;
+
   if (cur >= end)
{
- end_tsc

[BOOTLOADER SPECIFICATION RFC] The bootloader log format for TrenchBoot and others

2020-05-29 Thread Daniel Kiper 
Hey,

Below you can find my rough idea of the bootloader log format which is
generic thing but initially will be used for TrenchBoot work. I discussed
this proposal with Ross and Daniel S. So, the idea went through initial
sanitization. Now I would like to take feedback from other folks too.
So, please take a look and complain...

In general we want to pass the messages produced by the bootloader to the OS
kernel and finally to the userspace for further processing and analysis. Below
is the description of the structures which will be used for this thing.

  struct bootloader_log_msgs
  {
grub_uint32_t level;
grub_uint32_t facility;
char type[];
char msg[];
  }

  struct bootloader_log
  {
grub_uint32_t version;
grub_uint32_t producer;
grub_uint32_t size;
grub_uint32_t next_off;
bootloader_log_msgs msgs[];
  }

The members of struct bootloader_log:
  - version: the bootloader log format version number, 1 for now,
  - producer: the producer/bootloader type; we can steal some values from
linux/Documentation/x86/boot.rst:type_of_loader,
  - size: total size of the log buffer including the bootloader_log struct,
  - next_off: offset in bytes, from start of the bootloader_log struct,
of the next byte after the last log message in the msgs[];
i.e. the offset of the next available log message slot,
  - msgs: the array of log messages.

The members of struct bootloader_log_msgs:
  - level: similar to syslog meaning; can be used to differentiate
normal messages from debug messages; exact interpretation depends
on the current producer/bootloader type specified in the
bootloader_log.producer,
  - facility: similar to syslog meaning; can be used to differentiate
the sources of the messages, e.g. message produced by networking
module; exact interpretation depends on the current producer/bootloader
type specified in the bootloader_log.producer,
  - type: similar to the facility member but NUL terminated string instead of 
integer;
this will be used by GRUB for messages printed using grub_dprintf(),
  - msg: the bootloader log message, NUL terminated string.

Note: The bootloaders are free to use/ignore any given set of level,
  facility and/or type members. Though the usage of these members
  has to be clearly defined. Ignored integer members should be set
  to 0. Ignored type member should contain an empty NUL terminated
  string. msg member is mandatory but can be an empty NUL terminated
  string.

Taking into account [1] and [2] I want to make this functionality generic
as much as possible. So, this bootloader log can be used with any bootloader
and OS kernel. However, initially the functionality will be implemented for
the Linux kernel and its boot protocol.

In case of Linux kernel the pointer to the bootloader_log struct should
be passed from the bootloader to the kernel through the boot_params and
the bootloader_log struct contents should be exposed via sysfs. E.g.
somewhere at /sys/kernel/debug or /sys/kernel/tracing or maybe we should
create new /sys/bootloader/log node.

If everybody is OK with this rough proposal then I will start working
on making it a part of Multiboot2 specification (the text above is just
raw description of the idea; it is not final text which land into the
spec). If you see better place for this thing just drop me a line.

Daniel

[1] https://lists.gnu.org/archive/html/grub-devel/2019-10/msg00107.html
[2] https://lists.gnu.org/archive/html/grub-devel/2019-11/msg00079.html

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

Re: [PATCH REBASED] verify: search keyid in hashed signature subpackets

2020-05-29 Thread Daniel Kiper 
On Fri, May 29, 2020 at 02:10:46PM +1000, Daniel Axtens wrote:
> Charles Duffy  writes:
>
> > Amended the test repo to apply this patch; it applies and works-as-intended
> > on both 2.04 and current master.
> >
> > As for the DCO assertions, my portion of the contribution was implemented
> > strictly on personal time/equipment, so I'm able to to make the relevant
> > assertions in my individual capacity; amended below thusly.
>
> Awesome, me too.

Oh, nice to see that work revived...

> >> (Add further description per thread at
> >> https://lists.gnu.org/archive/html/grub-devel/2016-11/msg00073.html)
>
> I will leave doing further revisions to you - looking through the thread
> from 2016 it looks like the commit message needs more details and maybe
> some variable names and constants need to be cleaned up etc. Now that we
> have all the relevant Signed-off-bys, that should all be just a matter of
> programming. My understanding is that you should maintain all three

You mean that I have to wait for next version of it...

> S-O-Bs in the commit message for future spins, but I've never been clear
> on what order they should be in if you make further revisions.

Well, it seems to me that it depends on the project and maintainers
preference. I prefer the oldest SOB at the top. So, in this case:

  Signed-off-by: Ignat Korchagin 
  Signed-off-by: Charles Duffy 
  Signed-off-by: Daniel Axtens 

Daniel

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

Re: [PATCH 2/6] net/http: Return an error on HTTP error responses

2020-05-29 Thread Daniel Kiper 
On Thu, May 28, 2020 at 09:10:59AM +0200, Javier Martinez Canillas wrote:
> On 5/25/20 9:02 PM, Daniel Kiper wrote:
> > From: Olaf Hering 
> >
> > A http transfer will hang if an error is returned. The error branch
> > returns the value GRUB_ERR_NONE which is not expected by the caller.
> >
> > Signed-off-by: Olaf Hering 
> > Signed-off-by: Daniel Kiper 
> > ---
> >  grub-core/net/http.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/grub-core/net/http.c b/grub-core/net/http.c
> > index b616cf40b..0d3da7501 100644
> > --- a/grub-core/net/http.c
> > +++ b/grub-core/net/http.c
> > @@ -118,14 +118,14 @@ parse_line (grub_file_t file, http_data_t data, char 
> > *ptr, grub_size_t len)
> > case 404:
> >   data->err = GRUB_ERR_FILE_NOT_FOUND;
> >   data->errmsg = grub_xasprintf (_("file `%s' not found"), 
> > data->filename);
> > - return GRUB_ERR_NONE;
> > + return GRUB_ERR_FILE_NOT_FOUND;
> > default:
> >   data->err = GRUB_ERR_NET_UNKNOWN_ERROR;
> >   /* TRANSLATORS: GRUB HTTP code is pretty young. So even perfectly
> >  valid answers like 403 will trigger this very generic message.  */
> >   data->errmsg = grub_xasprintf (_("unsupported HTTP error %d: %s"),
> >  code, ptr);
> > - return GRUB_ERR_NONE;
> > + return GRUB_ERR_FILE_READ_ERROR;
> > }
> >data->first_line_recv = 1;
> >return GRUB_ERR_NONE;
> >
>
> There is one case that is still returning GRUB_ERR_NONE even when is taken as
> an error and that is when the HTTP version != 1.1. Maybe fixing that as well?

OK, I will take a look at it.

Daniel

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

Re: [PATCH 4/6] tpm: Drop unneeded code

2020-05-29 Thread Daniel Kiper 
On Thu, May 28, 2020 at 09:22:34AM +0200, Javier Martinez Canillas wrote:
> On 5/25/20 9:02 PM, Daniel Kiper wrote:
> > Drop unused grub_tpm*_execute() and declaration of nonexistent 
> > grub_tpm_init().
> >
> > Signed-off-by: Daniel Kiper 
> > ---
>
> [snip]
>
> > -static grub_err_t
> > -grub_tpm2_execute (grub_efi_handle_t tpm_handle,
> > -   PassThroughToTPM_InputParamBlock *inbuf,
> > -   PassThroughToTPM_OutputParamBlock *outbuf)
> > -{
> > -  grub_efi_status_t status;
> > -  grub_efi_tpm2_protocol_t *tpm;
> > -  grub_uint32_t inhdrsize = sizeof (*inbuf) - sizeof (inbuf->TPMOperandIn);
> > -  grub_uint32_t outhdrsize =
> > -sizeof (*outbuf) - sizeof (outbuf->TPMOperandOut);
> > -
> > -  tpm = grub_efi_open_protocol (tpm_handle, &tpm2_guid,
> > -   GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> > -
> > -  if (!grub_tpm2_present (tpm))
> > -return 0;
> > -
> > -  /* UEFI TPM protocol takes the raw operand block, no param block header. 
> > */
> > -  status = efi_call_5 (tpm->submit_command, tpm,
> > -  inbuf->IPBLength - inhdrsize, inbuf->TPMOperandIn,
> > -  outbuf->OPBLength - outhdrsize, outbuf->TPMOperandOut);
> > -
>
> I think this would be useful if we ever add support for sending TPM commands.
> But I agree that should be removed since is unused and can always be brought
> back from the git history if needed.

Yeah, I was thinking about mentioning something about this but later
forgot about it. I will add following clarification to the commit
message before pushing the patch:
  If somebody needs the functionality implemented in the dropped code then
  he/she can readd it later. Now it only increases the GRUB code/image size.

> Reviewed-by: Javier Martinez Canillas 

Thanks a lot for review!

Daniel

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

Re: [PATCH] json: Remove invalid typedef redefinition

2020-05-29 Thread Daniel Kiper 
On Wed, May 27, 2020 at 11:18:24AM +0200, Patrick Steinhardt wrote:
> The C standard does not allow for typedef redefinitions, even if they
> map to the same underlying type. In order to avoid including the
> "jsmn.h" in "json.h" and thus exposing jsmn's internals, we have exactly
> such a forward-declaring typedef in "json.h". If enforcing the GNU99 C
> standard, clang may generate a warning about this non-standard
> construct.
>
> Fix the issue by using a simple `struct jsmntok` forward declaration
> instead of using a typedef.
>
> Signed-off-by: Patrick Steinhardt 

Reviewed-by: Daniel Kiper 

Daniel A., could you check this patch?

> ---
>
> Sorry for the late response, didn't notice at first that this directly
> impacts code I wrote. I don't have Clang available on my computer and
> GCC seems to lack the equivalent option for this, so I wasn't able to
> reproduce the warning. Below patch should fix the issue, though, as we
> simply avoid using a typedef for the forward declaration.

Not a problem. Thanks a lot for fixing this issue...

Daniel

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

Re: GRUB 2.06 release

2020-05-29 Thread Daniel Kiper 
On Wed, Apr 22, 2020 at 12:24:40PM +0200, Daniel Kiper wrote:
> On Mon, Mar 16, 2020 at 05:41:29PM +0100, Daniel Kiper wrote:
> > On Wed, Mar 11, 2020 at 11:47:35AM +0100, Daniel Kiper wrote:
> > > On Tue, Mar 03, 2020 at 06:26:03PM +0100, Daniel Kiper wrote:
> > > > On Wed, Feb 19, 2020 at 04:01:38PM +0100, Daniel Kiper wrote:
> > > > > Hi all,
> > > > >
> > > > > As I told during my FOSDEM 2020 presentation we are preparing for
> > > > > GRUB 2.06 release. Tentative schedule is below:
> > > > >   - code freeze: 15th of March, 23:59:59 UTC; everything posted after
> > > > > that date will not be considered as a release material,
> > > >
> > > > Just a kind reminder... Less than two weeks left...
> > >
> > > Less than a week left... This is final call...
> >
> > Freeze in force! No new features are accepted at this point!
> >
> > I will be reviewing all patches posted before the end of Sunday. All of
> > them will be considered for inclusion in the release. Additionally, all
> > features under active development and discussed on grub-devel still can
> > be considered for inclusion. However, if there is a substantial danger
> > of destabilizing the code or delaying the release I may reject any patch
> > at any point.
> >
> > I am especially interested in fixes and cleanups at this point...
> >
> > I am going to cut an RC in 2-4 weeks.
>
> Bad news is that we have delays... :-( Good news is that everything is going
> forward... :-) I merged next portion of patches yesterday. I am working
> on some fixes for issues which I found. I am also trying to work out
> the best solution for other bugs which are lingering here and there. So,
> please be patient... However, if you have any questions or suggestions
> just drop me a line...

Unfortunately there are still delays due to some unexpected things
happening... Though I am still ploughing through backlog. I have
recently posted a few fixes and cleanups required for rc1. Sadly there
are still a few issues which have to be fixed. I am working on them. So,
I hope that in the following weeks we will be able to cut rc1...

If you are waiting for my review please bear with me. I will get back to
you as soon as I can. Though I am mostly looking at important fixes in
first order right now.

Daniel

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

Re: [PATCH v2] Warn if MBR gap is small and user uses advanced modules

2020-05-29 Thread Vladimir 'phcoder' Serbinenko 
On Tue, May 12, 2020 at 1:48 PM Daniel Kiper  wrote:
>
> On Mon, Apr 27, 2020 at 05:51:34PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> > We don't want to support small MBR gap in pair with anything but
> > the simplest config of biosdisk+part_msdos+simple filesystem. In this
> > path "simple filesystems" are all current filesystems except zfs and
> > btrfs.
>
> The patch LGTM. If there are no objections I will take it in a week or so.
>
> Additionally, I hope that Vladimir will be able to prepare a doc patch
> describing the issue and why we introduced these warnings.
>
> Daniel
>
> > ---
> >  grub-core/partmap/gpt.c |  9 -
> >  grub-core/partmap/msdos.c   |  7 ++-
> >  include/grub/partition.h|  4 +++-
> >  include/grub/util/install.h |  7 +--
> >  util/grub-install-common.c  | 24 
> >  util/grub-install.c | 10 +++---
> >  util/grub-setup.c   |  2 +-
> >  util/setup.c|  5 +++--
> >  8 files changed, 57 insertions(+), 11 deletions(-)
> >
> > diff --git a/grub-core/partmap/gpt.c b/grub-core/partmap/gpt.c
> > index 103f6796f..6b979f7c3 100644
> > --- a/grub-core/partmap/gpt.c
> > +++ b/grub-core/partmap/gpt.c
> > @@ -25,6 +25,9 @@
> >  #include 
> >  #include 
> >  #include 
> > +#ifdef GRUB_UTIL
> > +#include 
> > +#endif
> >
> >  GRUB_MOD_LICENSE ("GPLv3+");
> >
> > @@ -169,7 +172,8 @@ static grub_err_t
> >  gpt_partition_map_embed (struct grub_disk *disk, unsigned int *nsectors,
> >   unsigned int max_nsectors,
> >   grub_embed_type_t embed_type,
> > - grub_disk_addr_t **sectors)
> > + grub_disk_addr_t **sectors,
> > + int warn_short)
> >  {
> >struct gpt_partition_map_embed_ctx ctx = {
> >  .start = 0,
> > @@ -191,6 +195,9 @@ gpt_partition_map_embed (struct grub_disk *disk,
> > unsigned int *nsectors,
> >  N_("this GPT partition label contains no BIOS Boot Partition;"
> > " embedding won't be possible"));
> >
> > +  if (ctx.len < GRUB_MIN_RECOMMENDED_MBRGAP) {
> > +grub_util_warn("Your BIOS Boot Partition is under 1 MiB, please
> > increase its size.");
> > +  }
> >if (ctx.len < *nsectors)
> >  return grub_error (GRUB_ERR_OUT_OF_RANGE,
> >  N_("your BIOS Boot Partition is too small;"
> > diff --git a/grub-core/partmap/msdos.c b/grub-core/partmap/msdos.c
> > index 7b8e45076..400d7521d 100644
> > --- a/grub-core/partmap/msdos.c
> > +++ b/grub-core/partmap/msdos.c
> > @@ -236,7 +236,8 @@ static grub_err_t
> >  pc_partition_map_embed (struct grub_disk *disk, unsigned int *nsectors,
> >   unsigned int max_nsectors,
> >   grub_embed_type_t embed_type,
> > - grub_disk_addr_t **sectors)
> > + grub_disk_addr_t **sectors,
> > + int warn_short)
> >  {
> >grub_disk_addr_t end = ~0ULL;
> >struct grub_msdos_partition_mbr mbr;
> > @@ -390,6 +391,10 @@ pc_partition_map_embed (struct grub_disk *disk,
> > unsigned int *nsectors,
> >return GRUB_ERR_NONE;
> >  }
> >
> > +  if (end < GRUB_MIN_RECOMMENDED_MBRGAP && warn_short) {
> > +grub_util_warn("You have a short MBR gap and use advanced config.
> > Please increase post-MBR gap");
> > +  }
> > +
> >if (end <= 1)
> >  return grub_error (GRUB_ERR_FILE_NOT_FOUND,
> >  N_("this msdos-style partition label has no "
> > diff --git a/include/grub/partition.h b/include/grub/partition.h
> > index 7adb7ec6e..adc50d680 100644
> > --- a/include/grub/partition.h
> > +++ b/include/grub/partition.h
> > @@ -52,10 +52,12 @@ struct grub_partition_map
> >   grub_partition_iterate_hook_t hook, void *hook_data);
> >  #ifdef GRUB_UTIL
> >/* Determine sectors available for embedding.  */
> > +#define GRUB_MIN_RECOMMENDED_MBRGAP 1900
> >grub_err_t (*embed) (struct grub_disk *disk, unsigned int *nsectors,
> >  unsigned int max_nsectors,
> >  grub_embed_type_t embed_type,
> > -grub_disk_addr_t **sectors);
> > +grub_disk_addr_t **sectors,
> > +int warn_short);
> >  #endif
> >  };
> >  typedef struct grub_partition_map *grub_partition_map_t;
> > diff --git a/include/grub/util/install.h b/include/grub/util/install.h
> > index 2631b1074..640c71d28 100644
> > --- a/include/grub/util/install.h
> > +++ b/include/grub/util/install.h
> > @@ -193,13 +193,13 @@ grub_util_bios_setup (const char *dir,
> > const char *boot_file, const char *core_file,
> > const char *dest, int force,
> > int fs_probe, int allow_floppy,
> > -   int add_rs_codes);
> > +   int add_rs_codes, int warn_short_mbr_gap);
> >  void
> >  grub_util_sparc_setup (const char *dir,
> >  const char *boot_file, const char *core_file,
> >  const char *dest, int force,
> >  int fs_probe, int allow_floppy,
> > -int add_rs_codes);
> > +int add_rs_codes, int warn_short_mbr_gap);
> >
> >  char *
> >  grub_install_get_image_targets_string (void);
> > @@ -265,4 +265,7 @@ grub_util_get_target_name (const struct
> > grub_install_image_target_desc *t);
> >  extern char *grub_

Re: [PATCH v2] Warn if MBR gap is small and user uses advanced modules

2020-05-29 Thread Vladimir 'phcoder' Serbinenko 
Signed-off-by: Bladimir Serbinenko 

On Fri, May 29, 2020 at 3:09 PM Vladimir 'phcoder' Serbinenko
 wrote:
>
> On Tue, May 12, 2020 at 1:48 PM Daniel Kiper  wrote:
> >
> > On Mon, Apr 27, 2020 at 05:51:34PM +0200, Vladimir 'phcoder' Serbinenko 
> > wrote:
> > > We don't want to support small MBR gap in pair with anything but
> > > the simplest config of biosdisk+part_msdos+simple filesystem. In this
> > > path "simple filesystems" are all current filesystems except zfs and
> > > btrfs.
> >
> > The patch LGTM. If there are no objections I will take it in a week or so.
> >
> > Additionally, I hope that Vladimir will be able to prepare a doc patch
> > describing the issue and why we introduced these warnings.
> >
> > Daniel
> >
> > > ---
> > >  grub-core/partmap/gpt.c |  9 -
> > >  grub-core/partmap/msdos.c   |  7 ++-
> > >  include/grub/partition.h|  4 +++-
> > >  include/grub/util/install.h |  7 +--
> > >  util/grub-install-common.c  | 24 
> > >  util/grub-install.c | 10 +++---
> > >  util/grub-setup.c   |  2 +-
> > >  util/setup.c|  5 +++--
> > >  8 files changed, 57 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/grub-core/partmap/gpt.c b/grub-core/partmap/gpt.c
> > > index 103f6796f..6b979f7c3 100644
> > > --- a/grub-core/partmap/gpt.c
> > > +++ b/grub-core/partmap/gpt.c
> > > @@ -25,6 +25,9 @@
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#ifdef GRUB_UTIL
> > > +#include 
> > > +#endif
> > >
> > >  GRUB_MOD_LICENSE ("GPLv3+");
> > >
> > > @@ -169,7 +172,8 @@ static grub_err_t
> > >  gpt_partition_map_embed (struct grub_disk *disk, unsigned int *nsectors,
> > >   unsigned int max_nsectors,
> > >   grub_embed_type_t embed_type,
> > > - grub_disk_addr_t **sectors)
> > > + grub_disk_addr_t **sectors,
> > > + int warn_short)
> > >  {
> > >struct gpt_partition_map_embed_ctx ctx = {
> > >  .start = 0,
> > > @@ -191,6 +195,9 @@ gpt_partition_map_embed (struct grub_disk *disk,
> > > unsigned int *nsectors,
> > >  N_("this GPT partition label contains no BIOS Boot Partition;"
> > > " embedding won't be possible"));
> > >
> > > +  if (ctx.len < GRUB_MIN_RECOMMENDED_MBRGAP) {
> > > +grub_util_warn("Your BIOS Boot Partition is under 1 MiB, please
> > > increase its size.");
> > > +  }
> > >if (ctx.len < *nsectors)
> > >  return grub_error (GRUB_ERR_OUT_OF_RANGE,
> > >  N_("your BIOS Boot Partition is too small;"
> > > diff --git a/grub-core/partmap/msdos.c b/grub-core/partmap/msdos.c
> > > index 7b8e45076..400d7521d 100644
> > > --- a/grub-core/partmap/msdos.c
> > > +++ b/grub-core/partmap/msdos.c
> > > @@ -236,7 +236,8 @@ static grub_err_t
> > >  pc_partition_map_embed (struct grub_disk *disk, unsigned int *nsectors,
> > >   unsigned int max_nsectors,
> > >   grub_embed_type_t embed_type,
> > > - grub_disk_addr_t **sectors)
> > > + grub_disk_addr_t **sectors,
> > > + int warn_short)
> > >  {
> > >grub_disk_addr_t end = ~0ULL;
> > >struct grub_msdos_partition_mbr mbr;
> > > @@ -390,6 +391,10 @@ pc_partition_map_embed (struct grub_disk *disk,
> > > unsigned int *nsectors,
> > >return GRUB_ERR_NONE;
> > >  }
> > >
> > > +  if (end < GRUB_MIN_RECOMMENDED_MBRGAP && warn_short) {
> > > +grub_util_warn("You have a short MBR gap and use advanced config.
> > > Please increase post-MBR gap");
> > > +  }
> > > +
> > >if (end <= 1)
> > >  return grub_error (GRUB_ERR_FILE_NOT_FOUND,
> > >  N_("this msdos-style partition label has no "
> > > diff --git a/include/grub/partition.h b/include/grub/partition.h
> > > index 7adb7ec6e..adc50d680 100644
> > > --- a/include/grub/partition.h
> > > +++ b/include/grub/partition.h
> > > @@ -52,10 +52,12 @@ struct grub_partition_map
> > >   grub_partition_iterate_hook_t hook, void *hook_data);
> > >  #ifdef GRUB_UTIL
> > >/* Determine sectors available for embedding.  */
> > > +#define GRUB_MIN_RECOMMENDED_MBRGAP 1900
> > >grub_err_t (*embed) (struct grub_disk *disk, unsigned int *nsectors,
> > >  unsigned int max_nsectors,
> > >  grub_embed_type_t embed_type,
> > > -grub_disk_addr_t **sectors);
> > > +grub_disk_addr_t **sectors,
> > > +int warn_short);
> > >  #endif
> > >  };
> > >  typedef struct grub_partition_map *grub_partition_map_t;
> > > diff --git a/include/grub/util/install.h b/include/grub/util/install.h
> > > index 2631b1074..640c71d28 100644
> > > --- a/include/grub/util/install.h
> > > +++ b/include/grub/util/install.h
> > > @@ -193,13 +193,13 @@ grub_util_bios_setup (const char *dir,
> > > const char *boot_file, const char *core_file,
> > > const char *dest, int force,
> > > int fs_probe, int allow_floppy,
> > > -   int add_rs_codes);
> > > +   int add_rs_codes, int warn_short_mbr_gap);
> > >  void
> > >  grub_util_sparc_setup (const char *dir,
> > >  const char *boot_file, const char *core_file,
> > >

Re: [PATCH REBASED] verify: search keyid in hashed signature subpackets

2020-05-29 Thread Daniel Axtens 
Daniel Kiper  writes:

> On Fri, May 29, 2020 at 02:10:46PM +1000, Daniel Axtens wrote:
>> Charles Duffy  writes:
>>
>> > Amended the test repo to apply this patch; it applies and works-as-intended
>> > on both 2.04 and current master.
>> >
>> > As for the DCO assertions, my portion of the contribution was implemented
>> > strictly on personal time/equipment, so I'm able to to make the relevant
>> > assertions in my individual capacity; amended below thusly.
>>
>> Awesome, me too.
>
> Oh, nice to see that work revived...
>
>> >> (Add further description per thread at
>> >> https://lists.gnu.org/archive/html/grub-devel/2016-11/msg00073.html)
>>
>> I will leave doing further revisions to you - looking through the thread
>> from 2016 it looks like the commit message needs more details and maybe
>> some variable names and constants need to be cleaned up etc. Now that we
>> have all the relevant Signed-off-bys, that should all be just a matter of
>> programming. My understanding is that you should maintain all three
>
> You mean that I have to wait for next version of it...

I looked back at the 2016 thread and you had some comments there about
the clarity of the code and the details in the commit message. I imagine
those comments still stand. I was just trying to be clear to Charles
that I wasn't going to take on the task of addressing those comments,
and that he should address those and respin the patch.

>
>> S-O-Bs in the commit message for future spins, but I've never been clear
>> on what order they should be in if you make further revisions.
>
> Well, it seems to me that it depends on the project and maintainers
> preference. I prefer the oldest SOB at the top. So, in this case:
>
>   Signed-off-by: Ignat Korchagin 
>   Signed-off-by: Charles Duffy 
>   Signed-off-by: Daniel Axtens 

Noted.

Regards,
Daniel

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

Re: [PATCH] json: Remove invalid typedef redefinition

2020-05-29 Thread Daniel Axtens 
Hi Patrick,

> The C standard does not allow for typedef redefinitions, even if they
> map to the same underlying type. In order to avoid including the
> "jsmn.h" in "json.h" and thus exposing jsmn's internals, we have exactly
> such a forward-declaring typedef in "json.h". If enforcing the GNU99 C
> standard, clang may generate a warning about this non-standard
> construct.
>
> Fix the issue by using a simple `struct jsmntok` forward declaration
> instead of using a typedef.
>
> Signed-off-by: Patrick Steinhardt 

Tested-by: Daniel Axtens  # clang build test for emu target 
only

With this patch, clang can compile master again. Thanks Patrick!

Regards,
Daniel

> ---
>
> Sorry for the late response, didn't notice at first that this directly
> impacts code I wrote. I don't have Clang available on my computer and
> GCC seems to lack the equivalent option for this, so I wasn't able to
> reproduce the warning. Below patch should fix the issue, though, as we
> simply avoid using a typedef for the forward declaration.
>
>  grub-core/lib/json/json.h | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/lib/json/json.h b/grub-core/lib/json/json.h
> index d9f99454d..2e2b0bd07 100644
> --- a/grub-core/lib/json/json.h
> +++ b/grub-core/lib/json/json.h
> @@ -36,13 +36,14 @@ enum grub_json_type
>  };
>  typedef enum grub_json_type grub_json_type_t;
>  
> -typedef struct jsmntok jsmntok_t;
> +/* Forward-declaration to avoid including "jsmn.h" */
> +struct jsmntok;
>  
>  struct grub_json
>  {
> -  jsmntok_t   *tokens;
> -  char *string;
> -  grub_size_t idx;
> +  struct jsmntok *tokens;
> +  char*string;
> +  grub_size_t idx;
>  };
>  typedef struct grub_json grub_json_t;
>  
> -- 
> 2.26.2

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

Re: [BOOTLOADER SPECIFICATION RFC] The bootloader log format for TrenchBoot and others

2020-05-29 Thread Andy Lutomirski 


> On May 29, 2020, at 4:30 AM, Daniel Kiper  wrote:
> 
> Hey,
> 
> Below you can find my rough idea of the bootloader log format which is
> generic thing but initially will be used for TrenchBoot work. I discussed
> this proposal with Ross and Daniel S. So, the idea went through initial
> sanitization. Now I would like to take feedback from other folks too.
> So, please take a look and complain...

> In general we want to pass the messages produced by the bootloader to the OS
> kernel and finally to the userspace for further processing and analysis. Below
> is the description of the structures which will be used for this thing.

Is the intent for a human to read these, or to get them into the system log 
file, or are they intended to actually change the behavior of the system?

IOW, what is this for?
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel