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

From: Julien Grall <julien@xen.org>
To: Jan Beulich <jbeulich@suse.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>, "Roger Pau Monné" <roger.pau@citrix.com>,
	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, 14 Dec 2023 10:14:35 +0000	[thread overview]
Message-ID: <8b8a62a0-f854-405e-b256-5eee4bfdcb6c@xen.org> (raw)
In-Reply-To: <b17bb7d1-1206-4ad1-96b1-7b903a740c83@suse.com>

Hi,

On 14/12/2023 10:10, Jan Beulich wrote:
> On 11.12.2023 13:23, Julien Grall wrote:
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -2535,6 +2535,17 @@ pages) must also be specified via the tbuf_size parameter.
>>   ### tickle_one_idle_cpu
>>   > `= <boolean>`
>>   
>> +### pit-irq-works (x86)
>> +> `=<boolean>`
>> +
>> +> Default: `false`
>> +
>> +Disables the code which tests for broken timer IRQ sources. Enabling
>> +this option will reduce boot time on HW where the timer works properly.
>> +
>> +If the system is unstable when enabling the option, then it means you
>> +may have a broken HW and therefore the testing cannot be be skipped.
>> +
>>   ### timer_slop
>>   > `= <integer>`
> 
> With the rename this now needs to move up to retain sorting.

Ok.

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

After all, this code is heavily borrowed from Linux. So shouldn't we 
follow what they are doing?

Cheers,

-- 
Julien Grall