[PATCH] x86: suppress HPET broadcast initialization in the presence of ARAT

[PATCH] x86: suppress HPET broadcast initialization in the presence of ARAT

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 731 bytes --]

This follows Linux commit 39fe05e58c5e448601ce46e6b03900d5bf31c4b0,
noticing that all this setup is pointless when ARAT support is there,
and knowing that on SLED11's native kernel it has actually caused S3
resume issues.

A question would be whether HPET legacy interrupts should be forced
off in this case (rather than leaving whatever came from firmware).

Signed-off-by: Jan Beulich <jbeulich@novell.com>

--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -557,6 +557,9 @@ void hpet_broadcast_init(void)
     u32 hpet_id, cfg;
     int i;
 
+    if ( boot_cpu_has(X86_FEATURE_ARAT) )
+        return;
+
     if ( irq_channel == NULL )
     {
         irq_channel = xmalloc_array(int, nr_irqs);




[-- Attachment #2: x86-arat-no-hpet-irq.patch --]
[-- Type: text/plain, Size: 725 bytes --]

This follows Linux commit 39fe05e58c5e448601ce46e6b03900d5bf31c4b0,
noticing that all this setup is pointless when ARAT support is there,
and knowing that on SLED11's native kernel it has actually caused S3
resume issues.

A question would be whether HPET legacy interrupts should be forced
off in this case (rather than leaving whatever came from firmware).

Signed-off-by: Jan Beulich <jbeulich@novell.com>

--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -557,6 +557,9 @@ void hpet_broadcast_init(void)
     u32 hpet_id, cfg;
     int i;
 
+    if ( boot_cpu_has(X86_FEATURE_ARAT) )
+        return;
+
     if ( irq_channel == NULL )
     {
         irq_channel = xmalloc_array(int, nr_irqs);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

RE: [PATCH] x86: suppress HPET broadcast initialization in the presence of ARAT

[Wei, Gang]

Jan Beulich wrote on 2011-02-10:
> This follows Linux commit 39fe05e58c5e448601ce46e6b03900d5bf31c4b0,
> noticing that all this setup is pointless when ARAT support is there, 
> and knowing that on SLED11's native kernel it has actually caused S3 resume issues.
> 
> A question would be whether HPET legacy interrupts should be forced 
> off in this case (rather than leaving whatever came from firmware).
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> 
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -557,6 +557,9 @@ void hpet_broadcast_init(void)
>      u32 hpet_id, cfg;
>      int i;
> +    if ( boot_cpu_has(X86_FEATURE_ARAT) )
> +        return;
> +
>      if ( irq_channel == NULL )
>      {
>          irq_channel = xmalloc_array(int, nr_irqs);
>

Although this patch was already checked in, I still have to say it is not necessary for Xen. Because hpet_broadcast_init() fn is only called if (xen_cpuidle && !boot_cpu_has(X86_FEATURE_ARAT)) in disable_pit_irq(). Of course I agree to keep it as a never used double check.

Jimmy

Re: [PATCH] x86: suppress HPET broadcast initialization in the presence of ARAT

[Keir Fraser]

On 12/02/2011 07:15, "Wei, Gang" <gang.wei@intel.com> wrote:

> Jan Beulich wrote on 2011-02-10:
>> This follows Linux commit 39fe05e58c5e448601ce46e6b03900d5bf31c4b0,
>> noticing that all this setup is pointless when ARAT support is there,
>> and knowing that on SLED11's native kernel it has actually caused S3 resume
>> issues.
>> 
> 
> Although this patch was already checked in, I still have to say it is not
> necessary for Xen. Because hpet_broadcast_init() fn is only called if
> (xen_cpuidle && !boot_cpu_has(X86_FEATURE_ARAT)) in disable_pit_irq(). Of
> course I agree to keep it as a never used double check.

Hmm I didn't spot that. Actually it is part of a more complex series of
checks in the caller, so I wonder whether repeating just that one check in
the function itself really makes much sense. I'm somewhat inclibned to
revert it.

 -- Keir

> Jimmy
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

RE: [PATCH] x86: suppress HPET broadcast initialization in the presence of ARAT

[Wei, Gang]

Keir Fraser wrote on 2011-02-12:
> On 12/02/2011 07:15, "Wei, Gang" <gang.wei@intel.com> wrote:
>> Jan Beulich wrote on 2011-02-10:
>>> This follows Linux commit 39fe05e58c5e448601ce46e6b03900d5bf31c4b0,
>>> noticing that all this setup is pointless when ARAT support is
>>> there, and knowing that on SLED11's native kernel it has actually
>>> caused S3 resume issues.
>> 
>> Although this patch was already checked in, I still have to say it
>> is not necessary for Xen. Because hpet_broadcast_init() fn is only
>> called if (xen_cpuidle && !boot_cpu_has(X86_FEATURE_ARAT)) in
>> disable_pit_irq(). Of course I agree to keep it as a never used double check.
> 
> Hmm I didn't spot that. Actually it is part of a more complex series
> of checks in the caller, so I wonder whether repeating just that one
> check in the function itself really makes much sense. I'm somewhat inclibned to revert it.

Revert it would be better.

Jimmy

Re: [PATCH] x86: suppress HPET broadcast initialization in the presence of ARAT

[Jan Beulich]

>>> On 12.02.11 at 08:35, Keir Fraser <keir@xen.org> wrote:
> On 12/02/2011 07:15, "Wei, Gang" <gang.wei@intel.com> wrote:
> 
>> Jan Beulich wrote on 2011-02-10:
>>> This follows Linux commit 39fe05e58c5e448601ce46e6b03900d5bf31c4b0,
>>> noticing that all this setup is pointless when ARAT support is there,
>>> and knowing that on SLED11's native kernel it has actually caused S3 resume
>>> issues.
>>> 
>> 
>> Although this patch was already checked in, I still have to say it is not
>> necessary for Xen. Because hpet_broadcast_init() fn is only called if
>> (xen_cpuidle && !boot_cpu_has(X86_FEATURE_ARAT)) in disable_pit_irq(). Of
>> course I agree to keep it as a never used double check.
> 
> Hmm I didn't spot that. Actually it is part of a more complex series of
> checks in the caller, so I wonder whether repeating just that one check in
> the function itself really makes much sense. I'm somewhat inclibned to
> revert it.

Yes, revert it - somehow I managed to not notice we do this check
already.

Jan