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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.
> 
> 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.

Jan