Re: [Xen-devel] [PATCH v9 19/20] x86/VPMU: NMI-based VPMU support

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.08.14 at 18:55, <boris.ostrovsky@xxxxxxxxxx> wrote:
>  static void __init parse_vpmu_param(char *s)
>  {
> -    switch ( parse_bool(s) )
> -    {
> -    case 0:
> -        break;
> -    default:
> -        if ( !strcmp(s, "bts") )
> -            vpmu_features |= XENPMU_FEATURE_INTEL_BTS;
> -        else if ( *s )
> +    char *ss;
> +
> +    vpmu_mode = XENPMU_MODE_SELF;
> +    if (*s == '\0')
> +        return;
> +
> +    do {
> +        ss = strchr(s, ',');
> +        if ( ss )
> +            *ss = '\0';
> +
> +        switch  ( parse_bool(s) )
>          {
> -            printk("VPMU: unknown flag: %s - vpmu disabled!\n", s);
> +        case 0:
> +            vpmu_mode = XENPMU_MODE_OFF;
> +            return;
> +        case -1:

This should be "default".

> +            if ( !strcmp(s, "nmi") )
> +                vpmu_interrupt_type = APIC_DM_NMI;
> +            else if ( !strcmp(s, "bts") )
> +                vpmu_features |= XENPMU_FEATURE_INTEL_BTS;
> +            else if ( !strcmp(s, "all") )
> +            {
> +                vpmu_mode &= ~XENPMU_MODE_SELF;
> +                vpmu_mode |= XENPMU_MODE_ALL;
> +            }
> +            else
> +            {
> +                printk("VPMU: unknown flag: %s - vpmu disabled!\n", s);
> +                vpmu_mode = XENPMU_MODE_OFF;
> +                return;
> +            }
> +        default:

And this should be "case 1" and preceded by a fall-through comment.

> @@ -265,30 +316,30 @@ int vpmu_do_interrupt(struct cpu_user_regs *regs)
>          apic_write(APIC_LVTPC, vpmu->hw_lapic_lvtpc | APIC_LVT_MASKED);
>          vpmu->hw_lapic_lvtpc |= APIC_LVT_MASKED;
>  
> -        send_guest_vcpu_virq(sampling, VIRQ_XENPMU);
> +        if ( vpmu_interrupt_type & APIC_DM_NMI )
> +        {
> +            per_cpu(sampled_vcpu, smp_processor_id()) = sampled;

Please use this_cpu() instead of kind of open-coding it (also again
below, and who knows where else).

>  static int pvpmu_init(struct domain *d, xen_pmu_params_t *params)
>  {
>      struct vcpu *v;
>      struct page_info *page;
>      uint64_t gfn = params->val;
> +    static bool_t __read_mostly pvpmu_initted = 0;

Pointless initializer. Also I think pvpmu_init_done would be a better name.

> @@ -476,6 +578,26 @@ static int pvpmu_init(struct domain *d, xen_pmu_params_t 
> *params)
>          return -EINVAL;
>      }
>  
> +    spin_lock(&init_lock);
> +
> +    if ( !pvpmu_initted )
> +    {
> +        if ( reserve_lapic_nmi() == 0 )
> +            set_nmi_callback(pmu_nmi_interrupt);
> +        else
> +        {
> +            spin_unlock(&init_lock);
> +            printk(XENLOG_G_ERR "Failed to reserve PMU NMI\n");
> +            put_page(page);
> +            return -EBUSY;
> +        }
> +        open_softirq(PMU_SOFTIRQ, pmu_softnmi);
> +
> +        pvpmu_initted = 1;

Consistency please: Either you have an if and an else branch, and
each does all that is needed, or you have just an if branch ending in
"return" and the else branch is implicit.

Jan


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