please revert c/s 17686

please revert c/s 17686

[Jan Beulich]

Switching HPET into legacy mode is not acceptable, as this doesn't only
cut off the PIT channel 0 interrupt, but also the RTC one. The former is
acceptable as no domain will ever gain control over it, but the latter isn't
as Dom0 may validly make use of it. I'm observing a failure of setting the
system clock correctly due to this issue (hwclock checks whether the RTC
update interrupt is occurring as expected), and I suppose other uses of
/dev/rtc would also suffer.

It is my understanding that using the HPET to overcome the APIC timer
stop issue is therefore impossible at present - you cannot use legacy
mode, and you also cannot use the individual routing mode as whatever
IRQ is chosen may turn out being in use by one or more other devices
(and hence would require sharing the IRQ between Xen and one or more
guests).

Thanks, Jan

Re: please revert c/s 17686

[Keir Fraser]

Probably best to just disable hpet_broadcast_init() for now, as this could
be fixed e.g., by using FSB delivery on some systems. Also the C-state
support looks buggy right now because we shouldn't use C3 (or deeper) unless
hpet_broadcast mechanism is available to us. Otherwise we can sleep forever.

 -- Keir

On 12/6/08 14:28, "Jan Beulich" <jbeulich@novell.com> wrote:

> Switching HPET into legacy mode is not acceptable, as this doesn't only
> cut off the PIT channel 0 interrupt, but also the RTC one. The former is
> acceptable as no domain will ever gain control over it, but the latter isn't
> as Dom0 may validly make use of it. I'm observing a failure of setting the
> system clock correctly due to this issue (hwclock checks whether the RTC
> update interrupt is occurring as expected), and I suppose other uses of
> /dev/rtc would also suffer.
> 
> It is my understanding that using the HPET to overcome the APIC timer
> stop issue is therefore impossible at present - you cannot use legacy
> mode, and you also cannot use the individual routing mode as whatever
> IRQ is chosen may turn out being in use by one or more other devices
> (and hence would require sharing the IRQ between Xen and one or more
> guests).
> 
> Thanks, Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: please revert c/s 17686

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 12.06.08 15:56 >>>
>Probably best to just disable hpet_broadcast_init() for now, as this could
>be fixed e.g., by using FSB delivery on some systems. Also the C-state
>support looks buggy right now because we shouldn't use C3 (or deeper) unless
>hpet_broadcast mechanism is available to us. Otherwise we can sleep forever.

Yes, that is something I noticed after I sent the original mail.

Jan

Re: please revert c/s 17686

[Keir Fraser]

I checked in c/s 17844 to address this issue. We do not call
hpet_broadcast_init() and this now has the knock-on effect that we never use
state C3.

 -- Keir

On 12/6/08 14:28, "Jan Beulich" <jbeulich@novell.com> wrote:

> Switching HPET into legacy mode is not acceptable, as this doesn't only
> cut off the PIT channel 0 interrupt, but also the RTC one. The former is
> acceptable as no domain will ever gain control over it, but the latter isn't
> as Dom0 may validly make use of it. I'm observing a failure of setting the
> system clock correctly due to this issue (hwclock checks whether the RTC
> update interrupt is occurring as expected), and I suppose other uses of
> /dev/rtc would also suffer.
> 
> It is my understanding that using the HPET to overcome the APIC timer
> stop issue is therefore impossible at present - you cannot use legacy
> mode, and you also cannot use the individual routing mode as whatever
> IRQ is chosen may turn out being in use by one or more other devices
> (and hence would require sharing the IRQ between Xen and one or more
> guests).
> 
> Thanks, Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

RE: please revert c/s 17686

[Wei, Gang]

Ok. Without hpet_broadcast, C3 can't work now. 

First, we need to add check for mechanism readiness before entering C3
to avoid sleeping forever as you mentioned.

Second, there are 3 options to reenable C3. 
  - Use pit_broadcast. Straightforward, may have some impacts on
tick-less effect.
  - Emulate RTC with legacy HPET. Need back porting from latest Linux
kernel.
  - Enable FSB delivery for HPET interrupt. Not all models support this
mode.

We would like to go with pit_broadcast first to ensure the C3 usability,
and look at other options later.

Thank Jan for your finding on this.

Jimmy

On Friday, June 13, 2008 12:23 AM, Keir Fraser wrote:
> I checked in c/s 17844 to address this issue. We do not call
> hpet_broadcast_init() and this now has the knock-on effect that we
never
> use state C3.
> 
>  -- Keir
> 
> On 12/6/08 14:28, "Jan Beulich" <jbeulich@novell.com> wrote:
> 
>> Switching HPET into legacy mode is not acceptable, as this doesn't
only
>> cut off the PIT channel 0 interrupt, but also the RTC one. The former
is
>> acceptable as no domain will ever gain control over it, but the
latter
>> isn't as Dom0 may validly make use of it. I'm observing a failure of
>> setting the system clock correctly due to this issue (hwclock checks
>> whether the RTC update interrupt is occurring as expected), and I
>> suppose other uses of /dev/rtc would also suffer. 
>> 
>> It is my understanding that using the HPET to overcome the APIC timer
>> stop issue is therefore impossible at present - you cannot use legacy
>> mode, and you also cannot use the individual routing mode as whatever
>> IRQ is chosen may turn out being in use by one or more other devices
>> (and hence would require sharing the IRQ between Xen and one or more
>> guests). 
>> 
>> Thanks, Jan
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: please revert c/s 17686

[Keir Fraser]

On 13/6/08 03:08, "Wei, Gang" <gang.wei@intel.com> wrote:

> Ok. Without hpet_broadcast, C3 can't work now.
> 
> First, we need to add check for mechanism readiness before entering C3
> to avoid sleeping forever as you mentioned.
> 
> Second, there are 3 options to reenable C3.
>   - Use pit_broadcast. Straightforward, may have some impacts on
> tick-less effect.
>   - Emulate RTC with legacy HPET. Need back porting from latest Linux
> kernel.
>   - Enable FSB delivery for HPET interrupt. Not all models support this
> mode.
> 
> We would like to go with pit_broadcast first to ensure the C3 usability,
> and look at other options later.

Will you keep the 10ms tick in this case? If that's acceptable it should be
a simple patch.

 -- Keir

Re: please revert c/s 17686

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 13.06.08 09:31 >>>
>On 13/6/08 03:08, "Wei, Gang" <gang.wei@intel.com> wrote:
>
>> Ok. Without hpet_broadcast, C3 can't work now.
>> 
>> First, we need to add check for mechanism readiness before entering C3
>> to avoid sleeping forever as you mentioned.
>> 
>> Second, there are 3 options to reenable C3.
>>   - Use pit_broadcast. Straightforward, may have some impacts on
>> tick-less effect.
>>   - Emulate RTC with legacy HPET. Need back porting from latest Linux
>> kernel.
>>   - Enable FSB delivery for HPET interrupt. Not all models support this
>> mode.
>> 
>> We would like to go with pit_broadcast first to ensure the C3 usability,
>> and look at other options later.
>
>Will you keep the 10ms tick in this case? If that's acceptable it should be
>a simple patch.

I think it would be nice it the tick was enabled only when at least one
CPU actually is about to enter or in C3. And I'm not certain whether
it wouldn't be possible to use a larger value than 10ms - at least in the
case where all CPUs are in C3 (but I see that this case doesn't really
seem to be expected anyway, given the warning handle_hpet_broadcast()
generates when the current CPU is in the channel's mask; I'm also
unclear about how the warning is avoided when the CPU currently in
charge of handling the timer interrupt is to enter C3 - maybe I'm
overlooking a place where the affinity get changed).

Jan

Re: please revert c/s 17686

[Keir Fraser]

On 13/6/08 08:45, "Jan Beulich" <jbeulich@novell.com> wrote:

>> Will you keep the 10ms tick in this case? If that's acceptable it should be
>> a simple patch.
> 
> I think it would be nice it the tick was enabled only when at least one
> CPU actually is about to enter or in C3. And I'm not certain whether
> it wouldn't be possible to use a larger value than 10ms - at least in the
> case where all CPUs are in C3 (but I see that this case doesn't really
> seem to be expected anyway, given the warning handle_hpet_broadcast()
> generates when the current CPU is in the channel's mask; I'm also
> unclear about how the warning is avoided when the CPU currently in
> charge of handling the timer interrupt is to enter C3 - maybe I'm
> overlooking a place where the affinity get changed).

I missed that warning printk. It does indeed look odd.

 -- Keir

RE: please revert c/s 17686

[Wei, Gang]

On Friday, June 13, 2008 3:51 PM, Keir Fraser wrote:
> On 13/6/08 08:45, "Jan Beulich" <jbeulich@novell.com> wrote:
> 
>>> Will you keep the 10ms tick in this case? If that's acceptable it
>>> should be a simple patch.

We have similar considerations as Jan mentioned below. We will try
larger interval & dynamically enable/disable.

>> 
>> I think it would be nice it the tick was enabled only when at least
one
>> CPU actually is about to enter or in C3. And I'm not certain whether
>> it wouldn't be possible to use a larger value than 10ms - at least in
the
>> case where all CPUs are in C3 (but I see that this case doesn't
really
>> seem to be expected anyway, given the warning handle_hpet_broadcast()
>> generates when the current CPU is in the channel's mask; I'm also
>> unclear about how the warning is avoided when the CPU currently in
>> charge of handling the timer interrupt is to enter C3 - maybe I'm
>> overlooking a place where the affinity get changed).

For the current implementation, the hpet_broadcast_exit() will be
executed before irq enabled, so the handle_hpet_broadcast() will always
get executed after the mask was cleared. We will look into whether it is
better to move hpet_broadcast_exit() after local_irq_enable().

> 
> I missed that warning printk. It does indeed look odd.

As to this warning printk, we can simply replace it with an assert.

Jimmy

RE: please revert c/s 17686

[Jan Beulich]

>> I missed that warning printk. It does indeed look odd.
>
>As to this warning printk, we can simply replace it with an assert.

That would make things worse, not better - the condition simply must be
allowed (as said before, otherwise you won't be able to bring all CPUs
at once into C3).

Jan

Re: please revert c/s 17686

[Keir Fraser]

On 13/6/08 09:12, "Jan Beulich" <jbeulich@novell.com> wrote:

>>> I missed that warning printk. It does indeed look odd.
>> 
>> As to this warning printk, we can simply replace it with an assert.
> 
> That would make things worse, not better - the condition simply must be
> allowed (as said before, otherwise you won't be able to bring all CPUs
> at once into C3).

I think that C2/C3 are entered with IRQs disabled, but IRQ pending will kick
the CPU out of C2/C3 nonetheless. That CPU will then execute
hpet_broadcast_exit() before local_irq_enable() and hence the warning printk
will never actually fire. So it would be correct as a BUG_ON().

Is this correct, Wei?

 -- Keir

RE: please revert c/s 17686

[Wei, Gang]

On Friday, June 13, 2008 4:25 PM, Keir Fraser wrote:
> On 13/6/08 09:12, "Jan Beulich" <jbeulich@novell.com> wrote:
> 
>>>> I missed that warning printk. It does indeed look odd.
>>> 
>>> As to this warning printk, we can simply replace it with an assert.
>> 
>> That would make things worse, not better - the condition simply must
be
>> allowed (as said before, otherwise you won't be able to bring all
CPUs
>> at once into C3).
> 
> I think that C2/C3 are entered with IRQs disabled, but IRQ pending
will
> kick the CPU out of C2/C3 nonetheless. That CPU will then execute
> hpet_broadcast_exit() before local_irq_enable() and hence the warning
> printk will never actually fire. So it would be correct as a BUG_ON().
> 
> Is this correct, Wei?

Absolutely right. And we will look into whether it is better to move
hpet_broadcast_exit() after local_irq_enable().

Jimmy