[XEN PATCH] x86/hpet: Disable legacy replacement mode after IRQ test if not needed

[XEN PATCH] x86/hpet: Disable legacy replacement mode after IRQ test if not needed

[Simon Gaiser]

As far as I understand the HPET legacy mode is not required on systems
with ARAT after the timer IRQ test. For previous discussion see [1].
Keeping it enabled prevents reaching S0ix residency.

Link: https://lore.kernel.org/xen-devel/cb408368-077d-edb5-b4ad-f80086db48c1@invisiblethingslab.com/ # [1]
Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com>
---
 xen/arch/x86/io_apic.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index 9b8a972cf5..ea98d717d0 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -1966,6 +1966,10 @@ static void __init check_timer(void)
 
             if ( timer_irq_works() )
             {
+                if ( boot_cpu_has(X86_FEATURE_ARAT) ) {
+                    printk(XENLOG_INFO "IRQ test with HPET Legacy Replacement Mode worked. Disabling it again.\n");
+                    hpet_disable_legacy_replacement_mode();
+                }
                 local_irq_restore(flags);
                 return;
             }
-- 
2.40.1

Re: [XEN PATCH] x86/hpet: Disable legacy replacement mode after IRQ test if not needed

[Roger Pau Monné]

On Tue, Jul 18, 2023 at 02:26:03PM +0200, Simon Gaiser wrote:
> As far as I understand the HPET legacy mode is not required on systems
> with ARAT after the timer IRQ test.

What's the relation with ARAT here?

It would seem to me that keeping legacy replacement enabled should
only be done when opt_hpet_legacy_replacement > 0, and the currently
modified block is already in a opt_hpet_legacy_replacement < 0 gated
chunk.

Thanks, Roger.

Re: [XEN PATCH] x86/hpet: Disable legacy replacement mode after IRQ test if not needed

[Simon Gaiser]

[-- Attachment #1.1: Type: text/plain, Size: 1158 bytes --]

Roger Pau Monné:
> On Tue, Jul 18, 2023 at 02:26:03PM +0200, Simon Gaiser wrote:
>> As far as I understand the HPET legacy mode is not required on systems
>> with ARAT after the timer IRQ test.
> 
> What's the relation with ARAT here?
> 
> It would seem to me that keeping legacy replacement enabled should
> only be done when opt_hpet_legacy_replacement > 0, and the currently
> modified block is already in a opt_hpet_legacy_replacement < 0 gated
> chunk.

I was concerned that on systems without ARAT cpuidle might rely on HPET
legacy mode being available. See _disable_pit_irq and lapic_timer_init.
But now that I stared at this again, I think that condition isn't
actually needed. If we reach that code we know that we have no working
PIT, but HPET is working. So _disable_pit_irq which is run after
check_timer (__start_xen first calls check_timer via smp_prepare_cpus
and only later disable_pit_irq via do_initcalls) will setup HPET
broadcast, which should succeed since HPET worked previously.

So I guess we can just drop the condition (please double check, that
code is quite tangled and I'm not familiar with it).

Simon

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [XEN PATCH] x86/hpet: Disable legacy replacement mode after IRQ test if not needed

[Jan Beulich]

On 18.07.2023 23:51, Simon Gaiser wrote:
> Roger Pau Monné:
>> On Tue, Jul 18, 2023 at 02:26:03PM +0200, Simon Gaiser wrote:
>>> As far as I understand the HPET legacy mode is not required on systems
>>> with ARAT after the timer IRQ test.
>>
>> What's the relation with ARAT here?
>>
>> It would seem to me that keeping legacy replacement enabled should
>> only be done when opt_hpet_legacy_replacement > 0, and the currently
>> modified block is already in a opt_hpet_legacy_replacement < 0 gated
>> chunk.
> 
> I was concerned that on systems without ARAT cpuidle might rely on HPET
> legacy mode being available. See _disable_pit_irq and lapic_timer_init.
> But now that I stared at this again, I think that condition isn't
> actually needed. If we reach that code we know that we have no working
> PIT, but HPET is working. So _disable_pit_irq which is run after
> check_timer (__start_xen first calls check_timer via smp_prepare_cpus
> and only later disable_pit_irq via do_initcalls) will setup HPET
> broadcast, which should succeed since HPET worked previously.
> 
> So I guess we can just drop the condition (please double check, that
> code is quite tangled and I'm not familiar with it).

What you want to respect instead though is opt_hpet_legacy_replacement.

Jan

Re: [XEN PATCH] x86/hpet: Disable legacy replacement mode after IRQ test if not needed

[Simon Gaiser]

[-- Attachment #1.1: Type: text/plain, Size: 1662 bytes --]

Jan Beulich:
> On 18.07.2023 23:51, Simon Gaiser wrote:
>> Roger Pau Monné:
>>> On Tue, Jul 18, 2023 at 02:26:03PM +0200, Simon Gaiser wrote:
>>>> As far as I understand the HPET legacy mode is not required on systems
>>>> with ARAT after the timer IRQ test.
>>>
>>> What's the relation with ARAT here?
>>>
>>> It would seem to me that keeping legacy replacement enabled should
>>> only be done when opt_hpet_legacy_replacement > 0, and the currently
>>> modified block is already in a opt_hpet_legacy_replacement < 0 gated
>>> chunk.
>>
>> I was concerned that on systems without ARAT cpuidle might rely on HPET
>> legacy mode being available. See _disable_pit_irq and lapic_timer_init.
>> But now that I stared at this again, I think that condition isn't
>> actually needed. If we reach that code we know that we have no working
>> PIT, but HPET is working. So _disable_pit_irq which is run after
>> check_timer (__start_xen first calls check_timer via smp_prepare_cpus
>> and only later disable_pit_irq via do_initcalls) will setup HPET
>> broadcast, which should succeed since HPET worked previously.
>>
>> So I guess we can just drop the condition (please double check, that
>> code is quite tangled and I'm not familiar with it).
> 
> What you want to respect instead though is opt_hpet_legacy_replacement.

Can you please explain what behavior you expect? As Roger pointed out
this code only runs with opt_hpet_legacy_replacement < 0 so the user
didn't make an explicit choice. In that case enabling the legacy mode
for the timer IRQ test and then disabling it to allow lower power modes
seems reasonable to me.

Simon

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [XEN PATCH] x86/hpet: Disable legacy replacement mode after IRQ test if not needed

[Jan Beulich]

On 24.07.2023 11:48, Simon Gaiser wrote:
> Jan Beulich:
>> On 18.07.2023 23:51, Simon Gaiser wrote:
>>> Roger Pau Monné:
>>>> On Tue, Jul 18, 2023 at 02:26:03PM +0200, Simon Gaiser wrote:
>>>>> As far as I understand the HPET legacy mode is not required on systems
>>>>> with ARAT after the timer IRQ test.
>>>>
>>>> What's the relation with ARAT here?
>>>>
>>>> It would seem to me that keeping legacy replacement enabled should
>>>> only be done when opt_hpet_legacy_replacement > 0, and the currently
>>>> modified block is already in a opt_hpet_legacy_replacement < 0 gated
>>>> chunk.
>>>
>>> I was concerned that on systems without ARAT cpuidle might rely on HPET
>>> legacy mode being available. See _disable_pit_irq and lapic_timer_init.
>>> But now that I stared at this again, I think that condition isn't
>>> actually needed. If we reach that code we know that we have no working
>>> PIT, but HPET is working. So _disable_pit_irq which is run after
>>> check_timer (__start_xen first calls check_timer via smp_prepare_cpus
>>> and only later disable_pit_irq via do_initcalls) will setup HPET
>>> broadcast, which should succeed since HPET worked previously.
>>>
>>> So I guess we can just drop the condition (please double check, that
>>> code is quite tangled and I'm not familiar with it).
>>
>> What you want to respect instead though is opt_hpet_legacy_replacement.
> 
> Can you please explain what behavior you expect? As Roger pointed out
> this code only runs with opt_hpet_legacy_replacement < 0 so the user
> didn't make an explicit choice.

Oh, I'm sorry. Please disregard my comment then.

Jan