| Message ID | 20230718122603.2002-1-simon@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [XEN] x86/hpet: Disable legacy replacement mode after IRQ test if not needed | expand |
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.
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
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
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
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
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; }
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(+)