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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 02/04/2014 06:22 AM, Jan Beulich wrote:
>>>> On 21.01.14 at 20:08, Boris Ostrovsky <boris.ostrovsky@oracle.com> 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