>From looking at the code I cannot deduce why it wouldn''t be possiblefor hpet_interrupt_handler() or hpet_legacy_irq_tick() to be called while hpet_broadcast_init() is still executing. If that''s indeed possible, then the setting of .event_handler clearly has to happen *after* initializing the channel''s spinlock and rwlock. 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. Thanks, Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> On 09.03.11 at 15:45, "Jan Beulich" <JBeulich@novell.com> wrote: >> From looking at the code I cannot deduce why it wouldn''t be possible > for hpet_interrupt_handler() or hpet_legacy_irq_tick() to be called > while hpet_broadcast_init() is still executing. If that''s indeed possible, > then the setting of .event_handler clearly has to happen *after* > initializing the channel''s spinlock and rwlock. > > 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.Also, what''s the point of decrementing the per-CPU irq_count in hpet_legacy_irq_tick()? Thanks, Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 09/03/2011 14:45, "Jan Beulich" <JBeulich@novell.com> wrote:>> From looking at the code I cannot deduce why it wouldn''t be possible > for hpet_interrupt_handler() or hpet_legacy_irq_tick() to be called > while hpet_broadcast_init() is still executing. If that''s indeed possible, > then the setting of .event_handler clearly has to happen *after* > initializing the channel''s spinlock and rwlock. > > 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.Only someone at Intel could answer these questions. Cc''ing Yu Ke, who seems most involved in this aspect. -- Keir> 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
CC the author Wei Gang Regards Ke> -----Original Message----- > From: Keir Fraser [mailto:keir.xen@gmail.com] > Sent: Thursday, March 10, 2011 12:12 AM > To: Jan Beulich; xen-devel@lists.xensource.com > Cc: Yu, Ke > Subject: Re: [Xen-devel] bogus HPET initialization order on x86 > > On 09/03/2011 14:45, "Jan Beulich" <JBeulich@novell.com> wrote: > > >> From looking at the code I cannot deduce why it wouldn''t be possible > > for hpet_interrupt_handler() or hpet_legacy_irq_tick() to be called > > while hpet_broadcast_init() is still executing. If that''s indeed > > possible, then the setting of .event_handler clearly has to happen > > *after* initializing the channel''s spinlock and rwlock. > > > > 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. > > Only someone at Intel could answer these questions. Cc''ing Yu Ke, who seems > most involved in this aspect. > > -- Keir > > > 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
Jan Beulich wrote on 2011-03-09:>>> From looking at the code I cannot deduce why it wouldn''t be >>> possible >> for hpet_interrupt_handler() or hpet_legacy_irq_tick() to be called >> while hpet_broadcast_init() is still executing. If that''s indeed >> possible, then the setting of .event_handler clearly has to happen >> *after* initializing the channel''s spinlock and rwlock.First of all, pulling the code initializing the channel''s spinlock & rwlock before the setting of .event_handler should be a cleaner way anyway.>> 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. For S3 resume case, the hpet_broadcast_init() is called in device_power_up()->time_resume()->disable_pit_irq(), when the irq was disabled and all non-boot cpus not enabled. So that''s also impossible. 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.> Also, what''s the point of decrementing the per-CPU irq_count in > hpet_legacy_irq_tick()?The per-CPU irq_count are used for calculate the total irq number excluding hpet irqs in a past period to give a estimation of IO frequence. The hpet irq was counted in do_IRQ, so need to reduce it while handling it. You can see another one in hpet_interrupt_handler(). Jimmy _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> "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? 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.>For S3 resume case, the hpet_broadcast_init() is called in device_power_up()->time_resume()->disable_pit_irq(), when >the irq was disabled and all non-boot cpus not enabled. So that''s also impossible.No, the resume path is not that interesting, albeit it also looks bogus to initialize spinlock and rwlock again in that path, but that''ll be taken care of once init and resume paths get split.>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. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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. 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.> 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.>> 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. Jimmy _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> On 10.03.11 at 04:35, "Wei, Gang" <gang.wei@intel.com> wrote: > Jan Beulich wrote on 2011-03-09: >> Also, what''s the point of decrementing the per-CPU irq_count in >> hpet_legacy_irq_tick()? > > The per-CPU irq_count are used for calculate the total irq number excluding > hpet irqs in a past period to give a estimation of IO frequence. The hpet irq > was counted in do_IRQ, so need to reduce it while handling it. You can see > another one in hpet_interrupt_handler().I can understand the latter, but the one I was asking about decrements irq_count even if not calling the event handler (i.e. when returning zero from the function). In other words, I would have understood what this is for if it was sitting after the initial if(). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> On 11.03.11 at 03:43, "Wei, Gang" <gang.wei@intel.com> 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@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich wrote on 2011-03-11:>>>> On 10.03.11 at 04:35, "Wei, Gang" <gang.wei@intel.com> wrote: >> Jan Beulich wrote on 2011-03-09: >>> Also, what''s the point of decrementing the per-CPU irq_count in >>> hpet_legacy_irq_tick()? >> >> The per-CPU irq_count are used for calculate the total irq number >> excluding hpet irqs in a past period to give a estimation of IO >> frequence. The hpet irq was counted in do_IRQ, so need to reduce it >> while handling it. You can see another one in hpet_interrupt_handler(). > > I can understand the latter, but the one I was asking about decrements > irq_count even if not calling the event handler (i.e. > when returning zero from the function). In other words, I would have > understood what this is for if it was sitting after the initial if().The irq_count is added to count the total irq number excluding hpet/pit irqs, so even the real event handler was not called, the adjustment should still be done. Jimmy _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich wrote on 2011-03-11:>>>> On 11.03.11 at 03:43, "Wei, Gang" <gang.wei@intel.com> wrote: >> 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.I will send a patch for this.>>> 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.What you read is just the configuration of the hpet interrupt. The hpet programming is happen in below places: 1. hpet_broadcast_enter()->reprogram_hpet_evt_channel() 2. handle_hpet_broadcast()->reprogram_hpet_evt_channel() So the hpet interrupt should come after the first calling to hpet_broadcast_enter() (i.e. lapic_timer_off()).> >>>> 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.It is fine. I will have a look at it when the patch is ready. Jimmy _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Wei, Gang wrote on 2011-03-11:>>> 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. > > I will send a patch for this.Just recalled one point so that we may not have to do this. The HPET device was configured as legacy mode before we try to initialize the hpet channel 0 as the legacy_hpet_event. cfg = hpet_read32(HPET_CFG); cfg |= HPET_CFG_LEGACY; hpet_write32(cfg, HPET_CFG); Once HPET device was configured as legacy mode, the platform timer interrupt source was shifted to hpet channel 0 by chipset, and PIT can never trig a interrupt after that. HPET spec can tell details. Jimmy _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel