[PATCH 0/2] xen/x86: Optimize timer_irq_works()

[PATCH 0/2] xen/x86: Optimize timer_irq_works()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Hi all,

At the moment timer_irq_works() will always wait 100ms even with
enough ticks elapsed. This is a bit wasteful when most of the HW
should not be buggy.

The admin may also know that their HW is not buggy so they could
decide to skip the full 100ms. Also, looking at Linux changes, it
was pointed out that the timer_irq_works() may always return false
on when using para-virtualized delay. It is not a problem for Xen,
but still it makes sense to provide an option to disable the timer
check.

Patch #1 of the series will add the command line option while patch
#2 will optimize timer_irq_works() for the good case.

Cheers,

Julien Grall (2):
  xen/x86: io_apic: Introduce a command line option to skip timer check
  xen/x86: ioapic: Bail out from timer_irq_works() as soon as possible

 docs/misc/xen-command-line.pandoc |  7 ++++++
 xen/arch/x86/io_apic.c            | 41 ++++++++++++++++++++++---------
 2 files changed, 37 insertions(+), 11 deletions(-)

-- 
2.40.1

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

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Currently, Xen will spend ~100ms to check if the timer works. If the
Admin knows their platform have a working timer, then it would be
handy to be able to bypass the check.

Introduce a command line option 'no_timer_check' (the name is
matching the Linux parameter) for this purpose.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 docs/misc/xen-command-line.pandoc |  7 +++++++
 xen/arch/x86/io_apic.c            | 11 +++++++++++
 2 files changed, 18 insertions(+)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 4872b9098e83..1f9d3106383f 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1896,6 +1896,13 @@ This option is ignored in **pv-shim** mode.
 ### nr_irqs (x86)
 > `= <integer>`
 
+### no_timer_works (x86)
+> `=<boolean>`
+
+> Default: `true`
+
+Disables the code which tests for broken timer IRQ sources.
+
 ### irq-max-guests (x86)
 > `= <integer>`
 
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index b3afef8933d7..4875bb97003f 100644
--- 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 no_timer_check;
+boolean_param("no_timer_check", no_timer_check);
+
 /*
  * 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 ( no_timer_check )
+        return 1;
+
     t1 = ACCESS_ONCE(pit0_ticks);
 
     local_save_flags(flags);
-- 
2.40.1

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

[Jan Beulich]

On 18.08.2023 15:44, Julien Grall wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1896,6 +1896,13 @@ This option is ignored in **pv-shim** mode.
>  ### nr_irqs (x86)
>  > `= <integer>`
>  
> +### no_timer_works (x86)
> +> `=<boolean>`
> +
> +> Default: `true`
> +
> +Disables the code which tests for broken timer IRQ sources.

In description and code it's "check", but here it's "works". Likely
just a typo. But I'd prefer if we didn't introduce any new "no*"
options which then can be negated to "no-no*". Make it "timer-check"
(also avoiding the underscore, no matter that Linux uses it), or
alternatively make it a truly positive option, e.g. "timer-irq-works".

I also think it wants emphasizing that if this option is used and then
something doesn't work, people are on their own.

Jan

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

[Julien Grall]

Hi,

On 07/09/2023 15:09, Jan Beulich wrote:
> On 18.08.2023 15:44, Julien Grall wrote:
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -1896,6 +1896,13 @@ This option is ignored in **pv-shim** mode.
>>   ### nr_irqs (x86)
>>   > `= <integer>`
>>   
>> +### no_timer_works (x86)
>> +> `=<boolean>`
>> +
>> +> Default: `true`
>> +
>> +Disables the code which tests for broken timer IRQ sources.
> 
> In description and code it's "check", but here it's "works". Likely
> just a typo. But I'd prefer if we didn't introduce any new "no*"
> options which then can be negated to "no-no*". Make it "timer-check"
> (also avoiding the underscore, no matter that Linux uses it), or
> alternatively make it a truly positive option, e.g. "timer-irq-works".

I don't mind too much about using - over _ but it is never clear why you 
strongly push for it (and whether the others agrees). Is this documented 
somewhere? If not, can you do it so everyone can apply it consistently? 
(At least I would not remember to ask for it because I am happy with the _).

I will go for 'timer-irq-works'.

> 
> I also think it wants emphasizing that if this option is used and then
> something doesn't work, people are on their own.

I will do that.

Note that I will only resend a new version after the tree as branched 
because this is not meant for 4.18.

Cheers,

-- 
Julien Grall

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

[George Dunlap]

On Fri, Sep 15, 2023 at 2:18 PM Julien Grall <julien@xen.org> wrote:

> I don't mind too much about using - over _ but it is never clear why you
> strongly push for it (and whether the others agrees). Is this documented
> somewhere? If not, can you do it so everyone can apply it consistently?
> (At least I would not remember to ask for it because I am happy with the _).

FWIW I prefer `-` over `_`; I'd Ack a patch that documented it
somewhere (if it's not documented already).

 -George

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

[Jan Beulich]

On 15.09.2023 15:18, Julien Grall wrote:
> On 07/09/2023 15:09, Jan Beulich wrote:
>> On 18.08.2023 15:44, Julien Grall wrote:
>>> --- a/docs/misc/xen-command-line.pandoc
>>> +++ b/docs/misc/xen-command-line.pandoc
>>> @@ -1896,6 +1896,13 @@ This option is ignored in **pv-shim** mode.
>>>   ### nr_irqs (x86)
>>>   > `= <integer>`
>>>   
>>> +### no_timer_works (x86)
>>> +> `=<boolean>`
>>> +
>>> +> Default: `true`
>>> +
>>> +Disables the code which tests for broken timer IRQ sources.
>>
>> In description and code it's "check", but here it's "works". Likely
>> just a typo. But I'd prefer if we didn't introduce any new "no*"
>> options which then can be negated to "no-no*". Make it "timer-check"
>> (also avoiding the underscore, no matter that Linux uses it), or
>> alternatively make it a truly positive option, e.g. "timer-irq-works".
> 
> I don't mind too much about using - over _ but it is never clear why you 
> strongly push for it (and whether the others agrees).

Informal feedback suggested that various people agree and no-one strongly
disagrees to the argument of underscore really only being an auxiliary
separator character, when no better one can be used, and it also being
two keypresses to type on most keyboards, when dash is just one.

> Is this documented 
> somewhere? If not, can you do it so everyone can apply it consistently? 
> (At least I would not remember to ask for it because I am happy with the _).

As to documenting - it's not clear to me where such documentation ought
to go. In a way this is coding style, so it could be ./CODING_STYLE, but
then my experience with proposing changes there has been at best mixed.

Jan

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

[Roger Pau Monné]

On Fri, Aug 18, 2023 at 02:44:40PM +0100, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Currently, Xen will spend ~100ms to check if the timer works. If the
> Admin knows their platform have a working timer, then it would be
> handy to be able to bypass the check.

I'm of the opinion that the current code is at least dubious.

Specifically attempts to test the PIT timer, even when the hypervisor
might be using a different timer.  At which point it mostly attempts
to test that the interrupt routing from the PIT (or it's replacement)
is correct, and that Xen can receive such interrupts.

We go to great lengths in order to attempt to please this piece of
code, even when no PIT is available, at which point we switch the HPET
to legacy replacement mode in order to satisfy the checks.

I think the current code is not useful, and I would be fine if it was
just removed.

> 
> Introduce a command line option 'no_timer_check' (the name is
> matching the Linux parameter) for this purpose.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
>  docs/misc/xen-command-line.pandoc |  7 +++++++
>  xen/arch/x86/io_apic.c            | 11 +++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> index 4872b9098e83..1f9d3106383f 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1896,6 +1896,13 @@ This option is ignored in **pv-shim** mode.
>  ### nr_irqs (x86)
>  > `= <integer>`
>  
> +### no_timer_works (x86)

I find the naming confusing, and I would rather avoid negative named
booleans.

Maybe it should be `check_pit_intr` and default to enabled for the
time being?

> +> `=<boolean>`
> +
> +> Default: `true`

I think the default is wrong here?  AFAICT no_timer_check will default
to false.

> +
> +Disables the code which tests for broken timer IRQ sources.

Hm, yes, it does check for one specific source, which doesn't need to
be the currently selected timer.

"Disables the code which tests interrupt injection from the legacy
i8259 timer."

Even that is not fully true, as it would resort to testing the HPET
legacy mode if PIT doesn't work, but one could argue the HPET legacy
mode is just a replacement for the i8254.

> +
>  ### irq-max-guests (x86)
>  > `= <integer>`
>  
> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> index b3afef8933d7..4875bb97003f 100644
> --- 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.

I would mention i8254 here, as said this is quite likely not testing
the currently in use timer.

Note that if you don't want to run the test in timer_irq_works() you
can possibly avoid the code in preinit_pit(), as there no need to
setup channel 0 in periodic mode then.

Thanks, Roger.

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

[Julien Grall]

Hi Roger,

On 15/09/2023 14:54, Roger Pau Monné wrote:
> On Fri, Aug 18, 2023 at 02:44:40PM +0100, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Currently, Xen will spend ~100ms to check if the timer works. If the
>> Admin knows their platform have a working timer, then it would be
>> handy to be able to bypass the check.
> 
> I'm of the opinion that the current code is at least dubious.
> 
> Specifically attempts to test the PIT timer, even when the hypervisor
> might be using a different timer.  At which point it mostly attempts
> to test that the interrupt routing from the PIT (or it's replacement)
> is correct, and that Xen can receive such interrupts.
> 
> We go to great lengths in order to attempt to please this piece of
> code, even when no PIT is available, at which point we switch the HPET
> to legacy replacement mode in order to satisfy the checks.
> 
> I think the current code is not useful, and I would be fine if it was
> just removed.

I am afraid I don't know enough the code to be able to say if it can be 
removed.

I also have no idea how common are such platforms nowadays (I suspect it 
is rather small). Which I why I went with a command line option.

> 
>>
>> Introduce a command line option 'no_timer_check' (the name is
>> matching the Linux parameter) for this purpose.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> ---
>>   docs/misc/xen-command-line.pandoc |  7 +++++++
>>   xen/arch/x86/io_apic.c            | 11 +++++++++++
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
>> index 4872b9098e83..1f9d3106383f 100644
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -1896,6 +1896,13 @@ This option is ignored in **pv-shim** mode.
>>   ### nr_irqs (x86)
>>   > `= <integer>`
>>   
>> +### no_timer_works (x86)
> 
> I find the naming confusing, and I would rather avoid negative named
> booleans.
> 
> Maybe it should be `check_pit_intr` and default to enabled for the
> time being?
Jan suggested to use timer-irq-works. Would you be happy with the name?

> 
>> +> `=<boolean>`
>> +
>> +> Default: `true`
> 
> I think the default is wrong here?  AFAICT no_timer_check will default
> to false.

Ah yes. In the downstream version, I went with setting to true by 
default as we don't support any platform with broken timer. I forgot to 
update the patch before sending.

> 
>> +
>> +Disables the code which tests for broken timer IRQ sources.
> 
> Hm, yes, it does check for one specific source, which doesn't need to
> be the currently selected timer.
> 
> "Disables the code which tests interrupt injection from the legacy
> i8259 timer."

I can update the comment.

> 
> Even that is not fully true, as it would resort to testing the HPET
> legacy mode if PIT doesn't work, but one could argue the HPET legacy
> mode is just a replacement for the i8254.
> 
>> +
>>   ### irq-max-guests (x86)
>>   > `= <integer>`
>>   
>> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
>> index b3afef8933d7..4875bb97003f 100644
>> --- 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.
> 
> I would mention i8254 here, as said this is quite likely not testing
> the currently in use timer.

Sure.

> Note that if you don't want to run the test in timer_irq_works() you
> can possibly avoid the code in preinit_pit(), as there no need to
> setup channel 0 in periodic mode then.

The channel also seems to be used as a fallback method for calibrating 
the APIC (see calibrate_APIC_clock()). AFAICT, the fallback method 
should only be used when the PIT is enabled.

I think it would still be feasible to avoid running the IRQ tests even 
when PIT is used. So it means, we cannot skip preinit_pit().

As a side note, I noticed that preinit_pit() is called during resume. 
But I can't find any use of channel 0 after boot. So maybe the call 
could be removed?

Cheers,

-- 
Julien Grall

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

[Roger Pau Monné]

On Fri, Sep 15, 2023 at 03:27:40PM +0100, Julien Grall wrote:
> Hi Roger,
> 
> On 15/09/2023 14:54, Roger Pau Monné wrote:
> > On Fri, Aug 18, 2023 at 02:44:40PM +0100, Julien Grall wrote:
> > > From: Julien Grall <jgrall@amazon.com>
> > > 
> > > Currently, Xen will spend ~100ms to check if the timer works. If the
> > > Admin knows their platform have a working timer, then it would be
> > > handy to be able to bypass the check.
> > 
> > I'm of the opinion that the current code is at least dubious.
> > 
> > Specifically attempts to test the PIT timer, even when the hypervisor
> > might be using a different timer.  At which point it mostly attempts
> > to test that the interrupt routing from the PIT (or it's replacement)
> > is correct, and that Xen can receive such interrupts.
> > 
> > We go to great lengths in order to attempt to please this piece of
> > code, even when no PIT is available, at which point we switch the HPET
> > to legacy replacement mode in order to satisfy the checks.
> > 
> > I think the current code is not useful, and I would be fine if it was
> > just removed.
> 
> I am afraid I don't know enough the code to be able to say if it can be
> removed.
> 
> I also have no idea how common are such platforms nowadays (I suspect it is
> rather small). Which I why I went with a command line option.

It was more of a grumble rather than a request for you to remove the
code.  I've been meaning to look into this myself for some time, as we
just keep accumulating bodges in order to keep the test happy.

We don't seem to perform such tests for any other interrupt sources
that Xen uses (or timer), so I find it kind of odd.  I guess at one
point the PIT was always used, and hence it was relevant to test for
it unconditionally, but that's not the case anymore.

> 
> > 
> > > 
> > > Introduce a command line option 'no_timer_check' (the name is
> > > matching the Linux parameter) for this purpose.
> > > 
> > > Signed-off-by: Julien Grall <jgrall@amazon.com>
> > > ---
> > >   docs/misc/xen-command-line.pandoc |  7 +++++++
> > >   xen/arch/x86/io_apic.c            | 11 +++++++++++
> > >   2 files changed, 18 insertions(+)
> > > 
> > > diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> > > index 4872b9098e83..1f9d3106383f 100644
> > > --- a/docs/misc/xen-command-line.pandoc
> > > +++ b/docs/misc/xen-command-line.pandoc
> > > @@ -1896,6 +1896,13 @@ This option is ignored in **pv-shim** mode.
> > >   ### nr_irqs (x86)
> > >   > `= <integer>`
> > > +### no_timer_works (x86)
> > 
> > I find the naming confusing, and I would rather avoid negative named
> > booleans.
> > 
> > Maybe it should be `check_pit_intr` and default to enabled for the
> > time being?
> Jan suggested to use timer-irq-works. Would you be happy with the name?

Hm, pit_irq_works might be better IMO, but I could live with
timer_irq_works (with either _ or -).

> > Note that if you don't want to run the test in timer_irq_works() you
> > can possibly avoid the code in preinit_pit(), as there no need to
> > setup channel 0 in periodic mode then.
> 
> The channel also seems to be used as a fallback method for calibrating the
> APIC (see calibrate_APIC_clock()). AFAICT, the fallback method should only
> be used when the PIT is enabled.

Yes, I see.  I think most systems from the last decade will have TSC
deadline timer support, and hence don't engage in the calibration.  We
should see about switching the calibration to use the selected timer,
instead of forcing the usage of the PIT.

> I think it would still be feasible to avoid running the IRQ tests even when
> PIT is used. So it means, we cannot skip preinit_pit().

Yeah, so we seem to have a couple of usages of the Channel 0 counter
that don't rely on the interrupt at all.

> As a side note, I noticed that preinit_pit() is called during resume. But I
> can't find any use of channel 0 after boot. So maybe the call could be
> removed?

See _disable_pit_irq(), there's a relation between the PIT and
cpuidle.

Thanks, Roger.

[PATCH 2/2] xen/x86: ioapic: Bail out from timer_irq_works() as soon as possible

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Currently timer_irq_works() will wait the full 100ms before checking
that pit0_ticks has been incremented at least 4 times.

However, the bulk of the BIOS/platform should not have a buggy timer.
So waiting for the full 100ms is a bit harsh.

Rework the logic to only wait until 100ms passed or we saw more than
4 ticks. So now, in the good case, this will reduce the wait time
to ~50ms.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/x86/io_apic.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index 4875bb97003f..0bd774962a68 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -1509,6 +1509,8 @@ static void __init setup_ioapic_ids_from_mpc(void)
 static int __init timer_irq_works(void)
 {
     unsigned long t1, flags;
+    /* Wait for maximum 10 ticks */
+    unsigned long msec = (10 * 1000) / HZ;
 
     if ( no_timer_check )
         return 1;
@@ -1517,19 +1519,25 @@ static int __init timer_irq_works(void)
 
     local_save_flags(flags);
     local_irq_enable();
-    /* Let ten ticks pass... */
-    mdelay((10 * 1000) / HZ);
-    local_irq_restore(flags);
 
-    /*
-     * Expect a few ticks at least, to be sure some possible
-     * glue logic does not lock up after one or two first
-     * ticks in a non-ExtINT mode.  Also the local APIC
-     * might have cached one ExtINT interrupt.  Finally, at
-     * least one tick may be lost due to delays.
-     */
-    if ( (ACCESS_ONCE(pit0_ticks) - t1) > 4 )
+    while ( msec-- )
+    {
+        mdelay(1);
+        /*
+         * Expect a few ticks at least, to be sure some possible
+         * glue logic does not lock up after one or two first
+         * ticks in a non-ExtINT mode.  Also the local APIC
+         * might have cached one ExtINT interrupt.  Finally, at
+         * least one tick may be lost due to delays.
+         */
+        if ( (ACCESS_ONCE(pit0_ticks) - t1) <= 4 )
+            continue;
+
+        local_irq_restore(flags);
         return 1;
+    }
+
+    local_irq_restore(flags);
 
     return 0;
 }
-- 
2.40.1

Re: [PATCH 2/2] xen/x86: ioapic: Bail out from timer_irq_works() as soon as possible

[Jan Beulich]

On 18.08.2023 15:44, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Currently timer_irq_works() will wait the full 100ms before checking
> that pit0_ticks has been incremented at least 4 times.
> 
> However, the bulk of the BIOS/platform should not have a buggy timer.
> So waiting for the full 100ms is a bit harsh.
> 
> Rework the logic to only wait until 100ms passed or we saw more than
> 4 ticks. So now, in the good case, this will reduce the wait time
> to ~50ms.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

In principle this is all fine. There's a secondary aspect though which
may call for a slight rework of the patch.

> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -1509,6 +1509,8 @@ static void __init setup_ioapic_ids_from_mpc(void)
>  static int __init timer_irq_works(void)
>  {
>      unsigned long t1, flags;
> +    /* Wait for maximum 10 ticks */
> +    unsigned long msec = (10 * 1000) / HZ;

(Minor remark: I don't think this needs to be unsigned long; unsigned
in will suffice.)

> @@ -1517,19 +1519,25 @@ static int __init timer_irq_works(void)
>  
>      local_save_flags(flags);
>      local_irq_enable();
> -    /* Let ten ticks pass... */
> -    mdelay((10 * 1000) / HZ);
> -    local_irq_restore(flags);
>  
> -    /*
> -     * Expect a few ticks at least, to be sure some possible
> -     * glue logic does not lock up after one or two first
> -     * ticks in a non-ExtINT mode.  Also the local APIC
> -     * might have cached one ExtINT interrupt.  Finally, at
> -     * least one tick may be lost due to delays.
> -     */
> -    if ( (ACCESS_ONCE(pit0_ticks) - t1) > 4 )
> +    while ( msec-- )
> +    {
> +        mdelay(1);
> +        /*
> +         * Expect a few ticks at least, to be sure some possible
> +         * glue logic does not lock up after one or two first
> +         * ticks in a non-ExtINT mode.  Also the local APIC
> +         * might have cached one ExtINT interrupt.  Finally, at
> +         * least one tick may be lost due to delays.
> +         */
> +        if ( (ACCESS_ONCE(pit0_ticks) - t1) <= 4 )
> +            continue;
> +
> +        local_irq_restore(flags);
>          return 1;
> +    }
> +
> +    local_irq_restore(flags);
>  
>      return 0;
>  }

While Andrew has a patch pending (not sure why it didn't go in yet)
to simplify local_irq_restore(), and while further it shouldn't really
need using here (local_irq_disable() ought to be fine), I can see that
you don't want to make such an adjustment here. But then I'd prefer if
we got away with just a single instance, adjusting the final return
statement accordingly (easiest would likely be to go from the value of
"msec").

Jan

Re: [PATCH 2/2] xen/x86: ioapic: Bail out from timer_irq_works() as soon as possible

[Julien Grall]

Hi Jan,

On 07/09/2023 15:28, Jan Beulich wrote:
> On 18.08.2023 15:44, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Currently timer_irq_works() will wait the full 100ms before checking
>> that pit0_ticks has been incremented at least 4 times.
>>
>> However, the bulk of the BIOS/platform should not have a buggy timer.
>> So waiting for the full 100ms is a bit harsh.
>>
>> Rework the logic to only wait until 100ms passed or we saw more than
>> 4 ticks. So now, in the good case, this will reduce the wait time
>> to ~50ms.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> In principle this is all fine. There's a secondary aspect though which
> may call for a slight rework of the patch.
> 
>> --- a/xen/arch/x86/io_apic.c
>> +++ b/xen/arch/x86/io_apic.c
>> @@ -1509,6 +1509,8 @@ static void __init setup_ioapic_ids_from_mpc(void)
>>   static int __init timer_irq_works(void)
>>   {
>>       unsigned long t1, flags;
>> +    /* Wait for maximum 10 ticks */
>> +    unsigned long msec = (10 * 1000) / HZ;
> 
> (Minor remark: I don't think this needs to be unsigned long; unsigned
> in will suffice.)

You are right. I can switch to unsigned int.

> 
>> @@ -1517,19 +1519,25 @@ static int __init timer_irq_works(void)
>>   
>>       local_save_flags(flags);
>>       local_irq_enable();
>> -    /* Let ten ticks pass... */
>> -    mdelay((10 * 1000) / HZ);
>> -    local_irq_restore(flags);
>>   
>> -    /*
>> -     * Expect a few ticks at least, to be sure some possible
>> -     * glue logic does not lock up after one or two first
>> -     * ticks in a non-ExtINT mode.  Also the local APIC
>> -     * might have cached one ExtINT interrupt.  Finally, at
>> -     * least one tick may be lost due to delays.
>> -     */
>> -    if ( (ACCESS_ONCE(pit0_ticks) - t1) > 4 )
>> +    while ( msec-- )
>> +    {
>> +        mdelay(1);
>> +        /*
>> +         * Expect a few ticks at least, to be sure some possible
>> +         * glue logic does not lock up after one or two first
>> +         * ticks in a non-ExtINT mode.  Also the local APIC
>> +         * might have cached one ExtINT interrupt.  Finally, at
>> +         * least one tick may be lost due to delays.
>> +         */
>> +        if ( (ACCESS_ONCE(pit0_ticks) - t1) <= 4 )
>> +            continue;
>> +
>> +        local_irq_restore(flags);
>>           return 1;
>> +    }
>> +
>> +    local_irq_restore(flags);
>>   
>>       return 0;
>>   }
> 
> While Andrew has a patch pending (not sure why it didn't go in yet)
> to simplify local_irq_restore(), and while further it shouldn't really
> need using here (local_irq_disable() ought to be fine)

Skimming through the code, the last call of timer_irq_works() in 
check_timer() happens after the interrupts masking state have been restored:

local_irq_restore(flags);

if ( timer_irq_works() )
   ...


So I think timer_irq_works() can be called with interrupts enabled and 
therefore we can't use local_irq_disable().

> I can see that
> you don't want to make such an adjustment here. But then I'd prefer if
> we got away with just a single instance, adjusting the final return
> statement accordingly (easiest would likely be to go from the value of
> "msec").

I was thinking to use 'msec > 0' as a condition to determine if the test 
passed. However, it would consider a failure if it tooks 10ms for the 
test to pass.

I will see if I can rework the loop.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] xen/x86: ioapic: Bail out from timer_irq_works() as soon as possible

[Jan Beulich]

On 15.09.2023 16:00, Julien Grall wrote:
> Hi Jan,
> 
> On 07/09/2023 15:28, Jan Beulich wrote:
>> On 18.08.2023 15:44, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> Currently timer_irq_works() will wait the full 100ms before checking
>>> that pit0_ticks has been incremented at least 4 times.
>>>
>>> However, the bulk of the BIOS/platform should not have a buggy timer.
>>> So waiting for the full 100ms is a bit harsh.
>>>
>>> Rework the logic to only wait until 100ms passed or we saw more than
>>> 4 ticks. So now, in the good case, this will reduce the wait time
>>> to ~50ms.
>>>
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> In principle this is all fine. There's a secondary aspect though which
>> may call for a slight rework of the patch.
>>
>>> --- a/xen/arch/x86/io_apic.c
>>> +++ b/xen/arch/x86/io_apic.c
>>> @@ -1509,6 +1509,8 @@ static void __init setup_ioapic_ids_from_mpc(void)
>>>   static int __init timer_irq_works(void)
>>>   {
>>>       unsigned long t1, flags;
>>> +    /* Wait for maximum 10 ticks */
>>> +    unsigned long msec = (10 * 1000) / HZ;
>>
>> (Minor remark: I don't think this needs to be unsigned long; unsigned
>> in will suffice.)
> 
> You are right. I can switch to unsigned int.
> 
>>
>>> @@ -1517,19 +1519,25 @@ static int __init timer_irq_works(void)
>>>   
>>>       local_save_flags(flags);
>>>       local_irq_enable();
>>> -    /* Let ten ticks pass... */
>>> -    mdelay((10 * 1000) / HZ);
>>> -    local_irq_restore(flags);
>>>   
>>> -    /*
>>> -     * Expect a few ticks at least, to be sure some possible
>>> -     * glue logic does not lock up after one or two first
>>> -     * ticks in a non-ExtINT mode.  Also the local APIC
>>> -     * might have cached one ExtINT interrupt.  Finally, at
>>> -     * least one tick may be lost due to delays.
>>> -     */
>>> -    if ( (ACCESS_ONCE(pit0_ticks) - t1) > 4 )
>>> +    while ( msec-- )
>>> +    {
>>> +        mdelay(1);
>>> +        /*
>>> +         * Expect a few ticks at least, to be sure some possible
>>> +         * glue logic does not lock up after one or two first
>>> +         * ticks in a non-ExtINT mode.  Also the local APIC
>>> +         * might have cached one ExtINT interrupt.  Finally, at
>>> +         * least one tick may be lost due to delays.
>>> +         */
>>> +        if ( (ACCESS_ONCE(pit0_ticks) - t1) <= 4 )
>>> +            continue;
>>> +
>>> +        local_irq_restore(flags);
>>>           return 1;
>>> +    }
>>> +
>>> +    local_irq_restore(flags);
>>>   
>>>       return 0;
>>>   }
>>
>> While Andrew has a patch pending (not sure why it didn't go in yet)
>> to simplify local_irq_restore(), and while further it shouldn't really
>> need using here (local_irq_disable() ought to be fine)
> 
> Skimming through the code, the last call of timer_irq_works() in 
> check_timer() happens after the interrupts masking state have been restored:
> 
> local_irq_restore(flags);
> 
> if ( timer_irq_works() )
>    ...
> 
> 
> So I think timer_irq_works() can be called with interrupts enabled and 
> therefore we can't use local_irq_disable().

Hmm, yes, you're right. That's inconsistent, but dealing with that is a
separate task.

Jan