1. Patchew
  2. Xen
  3. View series

[XEN PATCH v2] x86/hpet: Disable legacy replacement mode after IRQ test

Simon Gaiser posted 1 patch 9 months, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/a112f0fbbb333fc29a35d0a81853d59409a33fde.1690798460.git.simon@invisiblethingslab.com
There is a newer version of this series
xen/arch/x86/io_apic.c | 2 ++
1 file changed, 2 insertions(+)
[XEN PATCH v2] x86/hpet: Disable legacy replacement mode after IRQ test
Posted by Simon Gaiser 9 months, 2 weeks ago 
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(+)

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;
             }
-- 
2.40.1
Re: [XEN PATCH v3] x86/hpet: Disable legacy replacement mode after IRQ test
Posted by Jan Beulich 8 months, 3 weeks ago 
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
Re: [XEN PATCH v2] x86/hpet: Disable legacy replacement mode after IRQ test
Posted by Jan Beulich 9 months, 2 weeks ago 
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
Re: [XEN PATCH v2] x86/hpet: Disable legacy replacement mode after IRQ test
Posted by Simon Gaiser 9 months, 1 week ago 
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
Re: [XEN PATCH v2] x86/hpet: Disable legacy replacement mode after IRQ test
Posted by Jan Beulich 9 months, 1 week ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.