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

[Daniel Kiper]

On Tue, Jul 19, 2022 at 01:38:51PM -0400, Robbie Harwood wrote:
> Daniel Kiper <dkiper@net-space.pl> writes:
> > On Thu, Jul 14, 2022 at 05:42:51PM -0400, Robbie Harwood wrote:
> >
> >> 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
>
> >> +/*
> >> + * 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
>
> >> +#ifndef GRUB_PMTIMER_IGNORE_BAD_READS
> >
> > I still do not understand why we need compile this code conditionally.
> > Why cannot we build it always and enable only when debug is set to
> > pmtimer/all? Or use another variable to enable this test?
>
> It's mentioned in the commit message and the comment I've quoted above
> that this is for testing - there isn't a use case for defining it
> otherwise.  (I also touched on this some in reply to Paul Menzel.)

I took a look at your reply to Paul. I think something like that should
be added to the commit message. Otherwise it is not (entirely) clear why
this part of code has to be commented out (or disabled if we agree it).

> As it changes more behavior than just debug prints, it doesn't make
> sense to me to tie it to debug.  More generally, I'd rather remove it
> than making the behavior runtime-configurable.  Would you prefer
> removing the #define entirely to the patch as written now?

If you say it is untested code I do not feel comfortable to run it
unconditionally too. However, I can see some reasons to have it compiled
in. Then everybody could enable additional tests without recompiling the
GRUB. It would be especially convenient on machines with secure boot
enabled. Though I agree we should not tie this to debug variable and use
separate environment variable to enable this tests.

> (Rest of comments make sense to me and I'll cut a v4 once we resolve
> this.)

Cool! Thanks!

Daniel