Re: [PATCH v1.1 2/2] x86/hpet: Don't enable legacy replacement mode unconditionally

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Mar 26, 2021 at 01:15:42PM +0000, Ian Jackson wrote:
> Tamas K Lengyel writes ("Re: [PATCH v1.1 2/2] x86/hpet: Don't enable legacy 
> replacement mode unconditionally"):
> > The problem from my perspective is that the end-users have no way to
> > determine when that boot option is needing to be set. Having an
> > installation step of "check if things explode when you reboot" is just
> > plain bad. Many times you don't even have access to a remote serial
> > console to check why Xen didn't boot. So that's not a realistic route
> > that can be taken. If Jan's patch is applied then the only thing I'll
> > be able to do is make all installations always-enable this option even
> > on systems that would have booted fine otherwise without it. It is
> > unclear if that has any downsides of its own and could very well just
> > kick the can down the road and lead to other issues.
> 
> Thanks for the clear explanation.
> 
> I think our options are:
> 
>  1. What is currently in xen.git#staging-4.15: some different set of
>     machines do not work (reliably? at all?), constituting a
>     regression on older hardware.
> 
>  2. Jan's patch, with the consequences you describe.  Constituing a
>     continued failure to properly support the newer hardware.
> 
>  3. Andy's patches which are not finished yet and are therefore high
>     risk.  Ie, delay the release.

I do have several confirmations that the "x86/timer: Fix boot on Intel
systems using ITSSPRC static PIT clock gating" patch indeed unbreaks
several Intel systems. And only one report about it causing a regression
on some AMD (although I may miss some others on the list).
Reverting to the previous default behavior I would also call a
regression.

I have tested Andy's patches on several machines and I can confirm they
fixed the issue - both keep the original issue fixed and fixes the
regression.
I see also Frédéric (who originally reported the regression) also
confirms it fixes it for him.

> Please let me know if you think this characterisation of the situation
> is inaccurate or misleading.

Both versions (with "x86/timer: Fix boot on Intel systems using ITSSPRC
static PIT clock gating" and without it) got significant testing and
results are as you summarize - each of those variants alone is broken on
some subset of hardware. What Andrew's patches do is to combine both
versions into one, to choose the right behavior depending on the
hardware. Specifically, apply the workaround in place of direct panic.
This isn't some completely new behavior. I think it is reasonably safe
to have it included in the release, even at such late time.

> This is not a good set of options.
> 
> Of them, I still think I would choose (2).  But I would love it if
> someone were to come up with a better suggestion (perhaps a variant on
> one of the above).


-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature