Re: [Xen-devel] [PATCH] Avoid race conditions in HPET initialization

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 03/01/14 13:15, Frediano Ziglio wrote:
> Avoid turning on legacy interrupts before hpet_event has been set up.
> Particularly, the spinlock can be uninitialised at the point at which
> the interrupt first arrives.
>
> Also, fix a memory leak of a cpumask in the unlikely event that
> hpet_assign_irq() fails.
>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>

Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

For reference, this was the underlying issue behind my patch "[Patch]
xen/spinlock: Improvements to check_lock()", Message ID
"1387816747-21470-1-git-send-email-andrew.cooper3@xxxxxxxxxx"

The hpet code was receiving a legacy interrupt between enabling
interrupts and initialising the spinlock, and check_lock() was mistaking
the 0 in lock->debug.irq_safe for "not IRQ-safe".

~Andrew

> ---
>  xen/arch/x86/hpet.c |   59 
> +++++++++++++++++++++++++++------------------------
>  1 file changed, 31 insertions(+), 28 deletions(-)
>
> diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
> index 3a4f7e8..0dedfb7 100644
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -387,6 +387,26 @@ static int __init hpet_assign_irq(struct 
> hpet_event_channel *ch)
>      return 0;
>  }
>  
> +static void __init hpet_init_channel(struct hpet_event_channel *ch)
> +{
> +    u64 hpet_rate = hpet_setup();
> +
> +    /*
> +     * The period is a femto seconds value. We need to calculate the scaled
> +     * math multiplication factor for nanosecond to hpet tick conversion.
> +     */
> +    ch->mult = div_sc((unsigned long)hpet_rate,
> +                                     1000000000ul, 32);
> +    ch->shift = 32;
> +    ch->next_event = STIME_MAX;
> +    spin_lock_init(&ch->lock);
> +    ch->event_handler = handle_hpet_broadcast;
> +
> +    ch->msi.irq = -1;
> +    ch->msi.msi_attrib.maskbit = 1;
> +    ch->msi.msi_attrib.pos = MSI_TYPE_HPET;
> +}
> +
>  static void __init hpet_fsb_cap_lookup(void)
>  {
>      u32 id;
> @@ -423,11 +443,15 @@ static void __init hpet_fsb_cap_lookup(void)
>              break;
>          }
>  
> +        hpet_init_channel(ch);
> +
>          ch->flags = 0;
>          ch->idx = i;
>  
>          if ( hpet_assign_irq(ch) == 0 )
>              num_hpets_used++;
> +        else
> +            free_cpumask_var(ch->cpumask);
>      }
>  
>      printk(XENLOG_INFO "HPET: %u timers usable for broadcast (%u total)\n",
> @@ -553,7 +577,6 @@ void __init hpet_broadcast_init(void)
>  {
>      u64 hpet_rate = hpet_setup();
>      u32 hpet_id, cfg;
> -    unsigned int i, n;
>  
>      if ( hpet_rate == 0 || hpet_broadcast_is_available() )
>          return;
> @@ -565,7 +588,6 @@ void __init hpet_broadcast_init(void)
>      {
>          /* Stop HPET legacy interrupts */
>          cfg &= ~HPET_CFG_LEGACY;
> -        n = num_hpets_used;
>      }
>      else
>      {
> @@ -577,11 +599,10 @@ void __init hpet_broadcast_init(void)
>              hpet_events = xzalloc(struct hpet_event_channel);
>          if ( !hpet_events || !zalloc_cpumask_var(&hpet_events->cpumask) )
>              return;
> -        hpet_events->msi.irq = -1;
> +        hpet_init_channel(hpet_events);
>  
>          /* Start HPET legacy interrupts */
>          cfg |= HPET_CFG_LEGACY;
> -        n = 1;
>  
>          if ( !force_hpet_broadcast )
>              pv_rtc_handler = handle_rtc_once;
> @@ -589,31 +610,13 @@ void __init hpet_broadcast_init(void)
>  
>      hpet_write32(cfg, HPET_CFG);
>  
> -    for ( i = 0; i < n; i++ )
> +    if ( cfg & HPET_CFG_LEGACY )
>      {
> -        if ( i == 0 && (cfg & HPET_CFG_LEGACY) )
> -        {
> -            /* set HPET T0 as oneshot */
> -            cfg = hpet_read32(HPET_Tn_CFG(0));
> -            cfg &= ~(HPET_TN_LEVEL | HPET_TN_PERIODIC);
> -            cfg |= HPET_TN_ENABLE | HPET_TN_32BIT;
> -            hpet_write32(cfg, HPET_Tn_CFG(0));
> -        }
> -
> -        /*
> -         * The period is a femto seconds value. We need to calculate the 
> scaled
> -         * math multiplication factor for nanosecond to hpet tick conversion.
> -         */
> -        hpet_events[i].mult = div_sc((unsigned long)hpet_rate,
> -                                     1000000000ul, 32);
> -        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[i].msi.msi_attrib.maskbit = 1;
> -        hpet_events[i].msi.msi_attrib.pos = MSI_TYPE_HPET;
> +        /* set HPET T0 as oneshot */
> +        cfg = hpet_read32(HPET_Tn_CFG(0));
> +        cfg &= ~(HPET_TN_LEVEL | HPET_TN_PERIODIC);
> +        cfg |= HPET_TN_ENABLE | HPET_TN_32BIT;
> +        hpet_write32(cfg, HPET_Tn_CFG(0));
>      }
>  
>      if ( !num_hpets_used )


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