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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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).

Jan


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