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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

    if (pin1 != -1) {
        /*
         * Ok, does IRQ0 through the IOAPIC work?
         */
        unmask_IO_APIC_irq(irq_to_desc(0));
        if (pit_irq_works || timer_irq_works()) {
            local_irq_restore(flags);
            return;
        }

Plus this way changes to the various fallback paths can also be done
without needing to consider users who might be making use of the new
option.

Jan