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

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

On Thu, Jan 04, 2024 at 09:54:30AM +0100, Jan Beulich wrote:
> On 02.01.2024 20:09, Julien Grall wrote:
> > 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;
> >           }
> 
> Yes.
> 
> >>> 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.
> 
> Roger, thoughts?

Right, with the usage of HPET in legacy replacement mode we are no
longer exclusively testing the PIT, so might make sense to use a more
generic name, timer-irq-works or some such.

Thanks, Roger.