Re: [PATCH 1/2] xen/x86: io_apic: Introduce a command line option to skip timer check

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Sep 15, 2023 at 03:27:40PM +0100, Julien Grall wrote:
> Hi Roger,
> 
> On 15/09/2023 14:54, Roger Pau Monné wrote:
> > On Fri, Aug 18, 2023 at 02:44:40PM +0100, Julien Grall wrote:
> > > From: Julien Grall <jgrall@xxxxxxxxxx>
> > > 
> > > Currently, Xen will spend ~100ms to check if the timer works. If the
> > > Admin knows their platform have a working timer, then it would be
> > > handy to be able to bypass the check.
> > 
> > I'm of the opinion that the current code is at least dubious.
> > 
> > Specifically attempts to test the PIT timer, even when the hypervisor
> > might be using a different timer.  At which point it mostly attempts
> > to test that the interrupt routing from the PIT (or it's replacement)
> > is correct, and that Xen can receive such interrupts.
> > 
> > We go to great lengths in order to attempt to please this piece of
> > code, even when no PIT is available, at which point we switch the HPET
> > to legacy replacement mode in order to satisfy the checks.
> > 
> > I think the current code is not useful, and I would be fine if it was
> > just removed.
> 
> I am afraid I don't know enough the code to be able to say if it can be
> removed.
> 
> I also have no idea how common are such platforms nowadays (I suspect it is
> rather small). Which I why I went with a command line option.

It was more of a grumble rather than a request for you to remove the
code.  I've been meaning to look into this myself for some time, as we
just keep accumulating bodges in order to keep the test happy.

We don't seem to perform such tests for any other interrupt sources
that Xen uses (or timer), so I find it kind of odd.  I guess at one
point the PIT was always used, and hence it was relevant to test for
it unconditionally, but that's not the case anymore.

> 
> > 
> > > 
> > > Introduce a command line option 'no_timer_check' (the name is
> > > matching the Linux parameter) for this purpose.
> > > 
> > > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> > > ---
> > >   docs/misc/xen-command-line.pandoc |  7 +++++++
> > >   xen/arch/x86/io_apic.c            | 11 +++++++++++
> > >   2 files changed, 18 insertions(+)
> > > 
> > > diff --git a/docs/misc/xen-command-line.pandoc 
> > > b/docs/misc/xen-command-line.pandoc
> > > index 4872b9098e83..1f9d3106383f 100644
> > > --- a/docs/misc/xen-command-line.pandoc
> > > +++ b/docs/misc/xen-command-line.pandoc
> > > @@ -1896,6 +1896,13 @@ This option is ignored in **pv-shim** mode.
> > >   ### nr_irqs (x86)
> > >   > `= <integer>`
> > > +### no_timer_works (x86)
> > 
> > I find the naming confusing, and I would rather avoid negative named
> > booleans.
> > 
> > Maybe it should be `check_pit_intr` and default to enabled for the
> > time being?
> Jan suggested to use timer-irq-works. Would you be happy with the name?

Hm, pit_irq_works might be better IMO, but I could live with
timer_irq_works (with either _ or -).

> > Note that if you don't want to run the test in timer_irq_works() you
> > can possibly avoid the code in preinit_pit(), as there no need to
> > setup channel 0 in periodic mode then.
> 
> The channel also seems to be used as a fallback method for calibrating the
> APIC (see calibrate_APIC_clock()). AFAICT, the fallback method should only
> be used when the PIT is enabled.

Yes, I see.  I think most systems from the last decade will have TSC
deadline timer support, and hence don't engage in the calibration.  We
should see about switching the calibration to use the selected timer,
instead of forcing the usage of the PIT.

> I think it would still be feasible to avoid running the IRQ tests even when
> PIT is used. So it means, we cannot skip preinit_pit().

Yeah, so we seem to have a couple of usages of the Channel 0 counter
that don't rely on the interrupt at all.

> As a side note, I noticed that preinit_pit() is called during resume. But I
> can't find any use of channel 0 after boot. So maybe the call could be
> removed?

See _disable_pit_irq(), there's a relation between the PIT and
cpuidle.

Thanks, Roger.