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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 14/12/2023 10:35, Jan Beulich wrote:
> On 14.12.2023 11:14, Julien Grall wrote:
> > On 14/12/2023 10:10, Jan Beulich wrote:
> > > On 11.12.2023 13:23, Julien Grall wrote:
> > > > --- a/xen/arch/x86/io_apic.c
> > > > +++ b/xen/arch/x86/io_apic.c
> > > > @@ -57,6 +57,14 @@ bool __initdata ioapic_ack_forced;
> > > >    int __read_mostly nr_ioapic_entries[MAX_IO_APICS];
> > > >    int __read_mostly nr_ioapics;
> > > >    
> > > > +/*
> > > > + * The logic to check if the timer is working is expensive. So allow
> > > > + * the admin to bypass it if they know their platform doesn't have
> > > > + * a buggy timer.
> > > > + */
> > > > +static bool __initdata pit_irq_works;
> > > > +boolean_param("pit-irq-works", pit_irq_works);
> > > > +
> > > >    /*
> > > >     * Rough estimation of how many shared IRQs there are, can
> > > >     * be changed anytime.
> > > > @@ -1502,6 +1510,9 @@ static int __init timer_irq_works(void)
> > > >    {
> > > >        unsigned long t1, flags;
> > > >    
> > > > +    if ( pit_irq_works )
> > > > +        return 1;
> > > When the check is placed here, what exactly use of the option means is
> > > system dependent. I consider this somewhat risky, so I'd prefer if the
> > > check was put on the "normal" path in check_timer(). That way it'll
> > > affect only the one case which we can generally consider "known good",
> > > but not the cases where the virtual wire setups are being probed. I.e.
By "known good", do you mean the following:

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index c89fbed8d675..c39d39ee951a 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -1960,7 +1959,8 @@ static void __init check_timer(void)
          * Ok, does IRQ0 through the IOAPIC work?
          */
         unmask_IO_APIC_irq(irq_to_desc(0));
-        if (timer_irq_works()) {
+        if (pit_irq_works || timer_irq_works()) {
+            printk("====== pirq_irq_works %d =====\n", pit_irq_works);
             local_irq_restore(flags);
             return;
         }

> > I am not against restricting when we allow skipping the timer check. But
> > in that case, I wonder why Linux is doing it differently?
> Sadly Linux'es git history doesn't go back far enough (begins only at past
> 2.6.11), so I can't (easily) find the patch (and description) for the x86-64
> change. The later i386 change is justified mainly by paravirt needs, so
> isn't applicable here. I wouldn't therefore exclude that my point above
> wasn't even taken into consideration. Furthermore their command line option
> is "no_timer_check", which to me firmly says "don't check" without regard to
> whether the source (PIT) is actually okay. That's different with the option
> name you (imo validly) chose.
Just to note that the name was suggested by Roger. I have to admit that 
I didn't check if this made sense for the existing placement.
Anyway, I tested the change on the HW where I wanted to skip the timer 
check. And I can confirm this is still skipping the timer check.
So I will send a new version with the diff above and some updated comments.

Cheers,

--
Julien Grall