Re: [PATCH v2] kern/efi: Adding efi-watchdog command

[Daniel Kiper]

On Fri, Sep 10, 2021 at 12:34:54PM +0200, Erwan Velu wrote:
> Thanks for your review, I'll rework my patch according to your comments.
> I just kept the open points in this thread.
>
> Le 09/09/2021 à 17:46, Daniel Kiper a écrit :
> >
> > +static grub_err_t grub_efi_set_watchdog_timer(unsigned long timeout)
> > Please use grub_efi_uintn_t instead of unsigned long. However, please
> > remember that its size depends on architecture. So, it can be 32-bits or
> > 64-bits...
> I'll follow the uefi spec here so grub_efi_uintn_ is fine.

At the end of my previous email I suggested it should be u32, i.e. 
grub_efi_uint32_t.
I think we should stick with it and only do conversion during UEFI watchdog 
call.
Of course it means you have to be very careful what you get from grub_strtoul().

> > > +    The numeric code to log on a watchdog timer timeout event.
> > > +    The firmware reserves codes 0x0000 to 0xFFFF.
> > > +    Loaders and operating systems may use other timeout codes.
> > This comment says you cannot use 0xb007. I would use 0xdeadb007deadb007
> > by default. However, I would give an option to change the default by user.
>
> Stupid me ....
>
> I was wondering if the code should be configurable.
>
> In the initial patch it was the case but though this would give an easier
> command line interface to users.
>
> I mean, the aim of this code is to report mostly 'who' manipulated the
> watchdog, so sound good to me to get a value that is more grub depend than
> user centric.

The GRUB should provide default but user should be able to change it.

> > > +  */
> > Please format comments according to [2].
> >
> > > +  grub_efi_uint64_t code = 0xB007;
> > > +  grub_efi_status_t status = 0;
> > > +
> > > +  if (timeout >= 0xFFFF)
> > > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("efi-watchdog: timeout 
> > > must be lower than 65535"));
> > Why do you artificially limit the timeout value? Please do not do that.
>
> Mistake of my own, make sense to remove that.

Please be aware of my note WRT grub_efi_uint32_t type above.

> > > +  if (timeout > 0)
> > > +    grub_printf("grub %s: Arming EFI watchdog at %lu seconds\n", 
> > > PACKAGE_VERSION, timeout);
> > If you want debug messages please use grub_dprintf() instead of 
> > grub_printf().
>
> Maybe my print isn't ideal but I found important to warn the user a watchdog
> is armed.
>
> If the user is not informed and the watchdog fires, the user could think
> there is a hardware issue on the host.
>
> So to me, this message is mandatory to say users : "beware, the system will
> be reset within x seconds if you don't boot"

If user have problems with watchdog they can enable debugging and see what
is going on. That is why I suggested grub_dprintf() usage.

Daniel