Re: [Xen-devel] [PATCH v4 12/17] x86/VPMU: Handle PMU interrupts for PV guests

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 02/04/2014 06:22 AM, Jan Beulich wrote:
> > > > On 21.01.14 at 20:08, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> >   int vpmu_do_interrupt(struct cpu_user_regs *regs)
> >   {
> >       struct vcpu *v = current;
> > -    struct vpmu_struct *vpmu = vcpu_vpmu(v);
> > +    struct vpmu_struct *vpmu;
> >   
> > -    if ( vpmu->arch_vpmu_ops )
> > +    /* dom0 will handle this interrupt */
> > +    if ( v->domain->domain_id >= DOMID_FIRST_RESERVED )
> > +        v = dom0->vcpu[smp_processor_id() % dom0->max_vcpus];
> > +
> > +    vpmu = vcpu_vpmu(v);
> > +    if ( !is_hvm_domain(v->domain) )
> > +    {
> > +        /* PV guest or dom0 is doing system profiling */
> > +        const struct cpu_user_regs *gregs;
> > +        int err;
> > +
> > +        if ( v->arch.vpmu.xenpmu_data->pmu_flags & PMU_CACHED )
> > +            return 1;
> > +
> > +        /* PV guest will be reading PMU MSRs from xenpmu_data */
> > +        vpmu_set(vpmu, VPMU_CONTEXT_SAVE | VPMU_CONTEXT_LOADED);
> > +        err = vpmu->arch_vpmu_ops->arch_vpmu_save(v);
> > +        vpmu_reset(vpmu, VPMU_CONTEXT_SAVE | VPMU_CONTEXT_LOADED);
> > +
> > +        /* Store appropriate registers in xenpmu_data */
> > +        if ( is_pv_32bit_domain(current->domain) )
> > +        {
> > +            /*
> > +             * 32-bit dom0 cannot process Xen's addresses (which are 64 bit)
> > +             * and therefore we treat it the same way as a non-priviledged
> > +             * PV 32-bit domain.
> > +             */
> > +            struct compat_cpu_user_regs *cmp;
> > +
> > +            gregs = guest_cpu_user_regs();
> > +
> > +            cmp = (struct compat_cpu_user_regs *)
> > +                    &v->arch.vpmu.xenpmu_data->pmu.r.regs;
> Deliberate type changes like this can easily (and more readably as
> well as more forward compatibly) be done using (void *).
>
> > +            XLAT_cpu_user_regs(cmp, gregs);
> > +        }
> > +        else if ( !is_control_domain(current->domain) &&
> > +                 !is_idle_vcpu(current) )
> > +        {
> > +            /* PV guest */
> > +            gregs = guest_cpu_user_regs();
> > +            memcpy(&v->arch.vpmu.xenpmu_data->pmu.r.regs,
> > +                   gregs, sizeof(struct cpu_user_regs));
> > +        }
> > +        else
> > +            memcpy(&v->arch.vpmu.xenpmu_data->pmu.r.regs,
> > +                   regs, sizeof(struct cpu_user_regs));
> > +
> > +        v->arch.vpmu.xenpmu_data->domain_id = current->domain->domain_id;
> > +        v->arch.vpmu.xenpmu_data->vcpu_id = current->vcpu_id;
> > +        v->arch.vpmu.xenpmu_data->pcpu_id = smp_processor_id();
> > +
> > +        v->arch.vpmu.xenpmu_data->pmu_flags |= PMU_CACHED;
> > +        apic_write(APIC_LVTPC, vpmu->hw_lapic_lvtpc | APIC_LVT_MASKED);
> > +        vpmu->hw_lapic_lvtpc |= APIC_LVT_MASKED;
> > +
> > +        send_guest_vcpu_virq(v, VIRQ_XENPMU);
> > +
> > +        return 1;
> > +    }
> > +    else if ( vpmu->arch_vpmu_ops )
> If the previous (and only) if() branch returns unconditionally, using
> "else if" is more confusing then clarifying imo (and in any case
> needlessly growing the patch, even if just by a bit).
Not sure I understand what you are saying here.

Here is the code structure:

int vpmu_do_interrupt(struct cpu_user_regs *regs)
{
     if ( !is_hvm_domain(v->domain) || (vpmu_mode & XENPMU_MODE_PRIV) )
{
        // work
        return 1;
}
    else if ( vpmu->arch_vpmu_ops )
{
        if ( !vpmu->arch_vpmu_ops->do_interrupt(regs) )
            return 0;

        // other work
        return 1;
}

    return 0;
}

What do you propose?


-boris

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