Re: [Xen-devel] [PATCH v12 for-xen-4.5 19/20] x86/VPMU: NMI-based VPMU support

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.09.14 at 21:28, <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);
> -            break;
> +        case 0:
> +            vpmu_mode = XENPMU_MODE_OFF;
> +            /* FALLTHROUGH */
> +        case 1:
> +            return;

If you do this much of redundant code folding, then I think you
should also go the final step and fold above five lines into the
code below:

> +        default:
> +            if ( !strcmp(s, "nmi") )
> +                vpmu_interrupt_type = APIC_DM_NMI;
> +            else if ( !strcmp(s, "bts") )
> +                vpmu_features |= XENPMU_FEATURE_INTEL_BTS;
> +            else
> +            {
> +                printk("VPMU: unknown flag: %s - vpmu disabled!\n", s);

case 0:

> +                vpmu_mode = XENPMU_MODE_OFF;

case 1:

> @@ -91,6 +113,24 @@ void vpmu_lvtpc_update(uint32_t val)
>          apic_write(APIC_LVTPC, vpmu->hw_lapic_lvtpc);
>  }
>  
> +static void vpmu_send_interrupt(struct vcpu *v)
> +{
> +    struct vlapic *vlapic;
> +    u32 vlapic_lvtpc;
> +
> +    ASSERT( is_hvm_vcpu(v) );
> +
> +    vlapic = vcpu_vlapic(v);
> +    if ( !is_vlapic_lvtpc_enabled(vlapic) )
> +        return;
> +
> +    vlapic_lvtpc = vlapic_get_reg(vlapic, APIC_LVTPC);
> +    if ( GET_APIC_DELIVERY_MODE(vlapic_lvtpc) == APIC_MODE_FIXED )
> +        vlapic_set_irq(vcpu_vlapic(v), vlapic_lvtpc & APIC_VECTOR_MASK, 0);
> +    else
> +        v->nmi_pending = 1;

Is APIC_MODE_NMI guaranteed to be the only alternative to
APIC_MODE_FIXED here (even for a buggy guest)? I don't recall
having seen code preventing other modes to be set, but even if
such code exists, an ASSERT() here seems quite desirable to me
(perhaps after re-structuring this to a switch() this could also be
a debug log message).

> @@ -232,8 +273,9 @@ int vpmu_do_interrupt(struct cpu_user_regs *regs)
>                  if ( sampled->arch.flags & TF_kernel_mode )
>                      r->cs &= ~3;
>              }
> -            else
> +            else if ( !(vpmu_interrupt_type & APIC_DM_NMI) )

Even if right now only APIC_DM_FIXED and APIC_DM_NMI are
possible, this is a latent bug: APIC_DM_NMI is not by itself a mask
(also elsewhere).

> --- a/xen/include/xen/softirq.h
> +++ b/xen/include/xen/softirq.h
> @@ -8,6 +8,7 @@ enum {
>      NEW_TLBFLUSH_CLOCK_PERIOD_SOFTIRQ,
>      RCU_SOFTIRQ,
>      TASKLET_SOFTIRQ,
> +    PMU_SOFTIRQ,
>      NR_COMMON_SOFTIRQS
>  };

Shouldn't this be an arch specific softirq?

Jan


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