| Message ID | a112f0fbbb333fc29a35d0a81853d59409a33fde.1690798460.git.simon@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [XEN,v2] x86/hpet: Disable legacy replacement mode after IRQ test | expand |
On 31.07.2023 12:32, Simon Gaiser wrote: > --- a/xen/arch/x86/io_apic.c > +++ b/xen/arch/x86/io_apic.c > @@ -1967,6 +1967,8 @@ static void __init check_timer(void) > > if ( timer_irq_works() ) > { > + 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; > } I'm not sure of the value of the log message. Furthermore, considering plans to make XENLOG_INFO visible by default in release builds, when not purely a debugging message. I'm curious what other x86 maintainers think ... Jan
Jan Beulich: > On 31.07.2023 12:32, Simon Gaiser wrote: >> --- a/xen/arch/x86/io_apic.c >> +++ b/xen/arch/x86/io_apic.c >> @@ -1967,6 +1967,8 @@ static void __init check_timer(void) >> >> if ( timer_irq_works() ) >> { >> + 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; >> } > > I'm not sure of the value of the log message. Furthermore, considering > plans to make XENLOG_INFO visible by default in release builds, when > not purely a debugging message. I'm curious what other x86 maintainers > think ... As far as I can see nobody has so far voiced an opinion. How about you tell me the level and message you would prefer and I send a new version of the patch with it. Simon
On 07.08.2023 11:47, Simon Gaiser wrote: > Jan Beulich: >> On 31.07.2023 12:32, Simon Gaiser wrote: >>> --- a/xen/arch/x86/io_apic.c >>> +++ b/xen/arch/x86/io_apic.c >>> @@ -1967,6 +1967,8 @@ static void __init check_timer(void) >>> >>> if ( timer_irq_works() ) >>> { >>> + 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; >>> } >> >> I'm not sure of the value of the log message. Furthermore, considering >> plans to make XENLOG_INFO visible by default in release builds, when >> not purely a debugging message. I'm curious what other x86 maintainers >> think ... > > As far as I can see nobody has so far voiced an opinion. How about you > tell me the level and message you would prefer and I send a new version > of the patch with it. XENLOG_DEBUG or even dprintk(). Also note that generally we avoid full stops in log messages. Jan
On 07.08.2023 13:28, Simon Gaiser wrote: > As far as I understand the HPET legacy mode is not required after the > timer IRQ test. For previous discussion see [1] and [2]. Keeping it > enabled prevents reaching deeper C-states on some systems and thereby > also S0ix residency. So disable it after the timer IRQ test worked. Note > that this code path is only reached when opt_hpet_legacy_replacement < 0, > so explicit user choice is still honored. > > Link: https://lore.kernel.org/xen-devel/cb408368-077d-edb5-b4ad-f80086db48c1@invisiblethingslab.com/ # [1] > Link: https://lore.kernel.org/xen-devel/20230718122603.2002-1-simon@invisiblethingslab.com/ # [2] > Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> Albeit I'll likely wrap the printk() slightly while committing. Jan
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c index 041233b9b7..b4b4cd5939 100644 --- a/xen/arch/x86/io_apic.c +++ b/xen/arch/x86/io_apic.c @@ -1967,6 +1967,8 @@ static void __init check_timer(void) if ( timer_irq_works() ) { + 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 after the timer IRQ test. For previous discussion see [1] and [2]. Keeping it enabled prevents reaching deeper C-states on some systems and thereby also S0ix residency. So disable it after the timer IRQ test worked. Note that this code path is only reached when opt_hpet_legacy_replacement < 0, so explicit user choice is still honored. Link: https://lore.kernel.org/xen-devel/cb408368-077d-edb5-b4ad-f80086db48c1@invisiblethingslab.com/ # [1] Link: https://lore.kernel.org/xen-devel/20230718122603.2002-1-simon@invisiblethingslab.com/ # [2] Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com> --- Changes in v2: - Always disable legacy mode after test, not only when ARAT is available. See [3] for reasoning. [3]: https://lore.kernel.org/xen-devel/ac77ecba-6804-1d16-60dc-f184e5d31dcb@invisiblethingslab.com/ --- xen/arch/x86/io_apic.c | 2 ++ 1 file changed, 2 insertions(+)