Re: [Xen-devel] [PATCH v2 2/5] x86/hpet: Use singe apic vector rather than irq_descs for HPET interrupts

[Tim Deegan <tim@xxxxxxx>]

Hi,

At 15:28 +0000 on 07 Nov (1383834502), Andrew Cooper wrote:
> This involves rewriting most of the MSI related HPET code, and as a result
> this patch looks very complicated.  It is probably best viewed as an end
> result, with the following notes explaining what is going on.
> 
> The new logic is as follows:
>  * A single high priority vector is allocated and uses on all cpus.
>  * Reliance on the irq infrastructure is completely removed.
>  * Tracking of free hpet channels has changed.  It is now an individual
>    bitmap, and allocation is based on winning a test_and_clear_bit()
>    operation.
>  * There is a notion of strict ownership of hpet channels.
>  ** A cpu which owns an HPET channel can program it for a desired deadline.
>  ** A cpu which can't find a free HPET channel to own may register for being
>     woken up by another in-use HPET which will fire at an appropriate time.
>  * Some functions have been renamed to be more descriptive.  Some functions
>    have parameters changed to be more consistent.
>  * Any function with a __hpet prefix expectes the appropriate lock to be held.

It certainly looks like the code should be easier to understand after
this!  I'll try to read through the end result later in the week, but
I have a few questions from the patch:

> -static void handle_hpet_broadcast(struct hpet_event_channel *ch)
> +static void __hpet_interrupt(struct hpet_event_channel *ch)
>  {
[...]
> +    __hpet_program_time(ch, this_cpu(timer_deadline), NOW(), 1);
> +    raise_softirq(TIMER_SOFTIRQ);
>  }

What's the __hpet_program_time() doing?  It looks like we're
reprogramming the hpet for our next event, before we handle the event
we woke up for (i.e. always setting to to fire immediately).  Or have
I misunderstood?

> @@ -619,9 +425,8 @@ void __init hpet_broadcast_init(void)
>          hpet_events[i].shift = 32;
>          hpet_events[i].next_event = STIME_MAX;
>          spin_lock_init(&hpet_events[i].lock);
> -        wmb();
> -        hpet_events[i].event_handler = handle_hpet_broadcast;
>  
> +        hpet_events[1].msi.irq = -1;

Really [1] or DYM [i]?

>          hpet_events[i].msi.msi_attrib.maskbit = 1;
>          hpet_events[i].msi.msi_attrib.pos = MSI_TYPE_HPET;
>      }
> @@ -661,9 +466,6 @@ void hpet_broadcast_resume(void)
>  
>      for ( i = 0; i < n; i++ )
>      {
> -        if ( hpet_events[i].msi.irq >= 0 )
> -            __hpet_setup_msi_irq(irq_to_desc(hpet_events[i].msi.irq));
> -
>          /* set HPET Tn as oneshot */
>          cfg = hpet_read32(HPET_Tn_CFG(hpet_events[i].idx));
>          cfg &= ~(HPET_TN_LEVEL | HPET_TN_PERIODIC);
> @@ -706,36 +508,76 @@ void hpet_disable_legacy_broadcast(void)
>  void hpet_broadcast_enter(void)
>  {
>      unsigned int cpu = smp_processor_id();
> -    struct hpet_event_channel *ch = per_cpu(cpu_bc_channel, cpu);
> +    struct hpet_event_channel *ch = this_cpu(hpet_channel);
>      s_time_t deadline = this_cpu(timer_deadline);
>  
>      ASSERT(!local_irq_is_enabled());
> +    ASSERT(ch == NULL);
>  
>      if ( deadline == 0 )
>          return;
>  
> -    if ( !ch )
> -        ch = hpet_get_channel(cpu);
> +    ch = hpet_get_free_channel();
>  
> +    if ( ch )
> +    {
> +        /* This really should be an MSI channel by this point */
> +        ASSERT( !(ch->flags & HPET_EVT_LEGACY) );
> +
> +        spin_lock(&ch->lock);
> +
> +        this_cpu(hpet_channel) = ch;
> +        ch->cpu = cpu;
> +        cpumask_set_cpu(cpu, ch->cpumask);
> +
> +        __hpet_setup_msi(ch);
> +        __hpet_program_time(ch, deadline, NOW(), 1);
> +        __hpet_msi_unmask(ch);
> +
> +        spin_unlock(&ch->lock);
> +
> +    }
> +    else
> +    {
> +        /* TODO - this seems very ugly */
> +        unsigned i;
> +
> +        for ( i = 0; i < num_hpets_used; ++i )
> +        {
> +            ch = &hpet_events[i];
> +            spin_lock(&ch->lock);
> +
> +            if ( ch->cpu == -1 )
> +                goto continue_search;
> +
> +            if ( ch->next_event >= deadline - MICROSECS(50) &&
> +                 ch->next_event <= deadline )
> +                break;
> +
> +        continue_search:
> +            spin_unlock(&ch->lock);
> +            ch = NULL;
> +        }
> +
> +        if ( ch )
> +        {
> +            cpumask_set_cpu(cpu, ch->cpumask);
> +            this_cpu(hpet_channel) = ch;
> +            spin_unlock(&ch->lock);
> +        }
> +        else
> +            this_cpu(timer_deadline) = NOW();

Hmm.  So if we can't find a channel that fits the deadline we want,  
we give up?  I thought the plan was to cause some other channel to
fire early.

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel