RE: [Xen-devel] bogus HPET initialization order on x86

["Jan Beulich" <JBeulich@xxxxxxxxxx>]

>>> On 11.03.11 at 03:43, "Wei, Gang" <gang.wei@xxxxxxxxx> wrote:
> Jan Beulich wrote on 2011-03-11:
>>>>> "Wei, Gang"  03/10/11 4:36 AM >>> Jan Beulich wrote on 2011-03-09:
>>>>> Further, with the channel getting enabled (down the
>>>>> hpet_fsb_cap_lookup() call tree) before hpet_events[] gets fully
>>>>> initialized, I'd also think it should be possible to hit the spurious
>>>>> warning in hpet_interrupt_handler() just because of improper
>>>>> initialization order.
>>>>> 
>>>>> If that's all impossible in practice, adding some meaningful
>>>>> comments to the code to describe why this is so would be much
>>>>> appreciated.
>>> 
>>> For normal booting case, hpet interrupts should not come before dom0
>>> start booting and pass ACPI tables to hypervisor, so that's impossible in
>>> practice in this case.
>> 
>> The legacy code path gets called from the timer interrupt, so what
>> would prevent this path being taken before Dom0 boots?
> 
> Oh, yes, you are right. The legacy code path may get called before PIT 
> interrupt being turned off. If we put the initialization of 
> legacy_hpet_event.event_handler at the end of hpet_broadcast_init(), things 
> should be ok.

Okay, I'll submit a patch to this respect then.

> Further, it would be better if we could disable PIT interrupt 
> before calling hpet_broadcast_init(), and re-enable PIT interrupt only while 
> PIT broadcast is needed.

But I'll leave that part to you.

>> The non-legacy interrupts also get fully set up before Dom0 boots - I
>> don't think I saw anything that specifically enables these interrupts
>> upon arrival of some ACPI data from Dom0.
> 
> The non-legacy interrupts will only come after some hpet channels was 
> programmed. Before hypervisor got ACPI data from Dom0 and be capable to enter 
> deep C states, hpet channels were not programmed.

Could you point out *where* this happens? My reading of the code
is that all channels get set fully up during hpet_broadcast_init()
(->hpet_fsb_cap_lookup() ->hpet_assign_irq() ->hpet_setup_msi_irq()
->request_irq()). The only thing done later are adjustments of the
interrupts' affinities.

>>> Do I miss any other cases? If not, I will cook a patch to add the
>>> required comments along with pulling spinlock/rwlock
>>> initialization before .event_handler settings.
>> 
>> Yes, please, though I could also fold this in my bigger cleanup patch
>> (properly separating init a resume paths) if I still didn't understand
>> some aspect of it and this turns out a mere cleanup.
> 
> Oh, if you like, you can definitely include these change in your cleanup 
> patch. It is your finding. You deserve the credit. I can just help to review 
> & 
> ack.

As it's a (latent) bug fix, I'll do it separately, so that it can also
make it into 4.0 and 4.1. Independent of the above, I'll probably
do the adjustment to both legacy and MSI paths, though, even
if the latter should turn out benign.

Jan


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