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

From: Jan Beulich <jbeulich@suse.com>
To: "Julien Grall" <julien@xen.org>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Cc: Julien Grall <jgrall@amazon.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2 1/2] xen/x86: io_apic: Introduce a command line option to skip timer check
Date: Thu, 4 Jan 2024 09:54:30 +0100	[thread overview]
Message-ID: <3a0b6d8a-d9f3-41d0-9acc-e84b5d9d3e33@suse.com> (raw)
In-Reply-To: <d0daf858-eda8-4b2a-9cfe-82fff834df8f@xen.org>

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?

Jan

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