From mboxrd@z Thu Jan  1 00:00:00 1970
From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Subject: Re: [PATCH v4 12/17] x86/VPMU: Handle PMU interrupts
	for PV guests
Date: Tue, 04 Feb 2014 10:26:02 -0500
Message-ID: <52F1068A.6060500@oracle.com>
References: <1390331342-3967-1-git-send-email-boris.ostrovsky@oracle.com>
	<1390331342-3967-13-git-send-email-boris.ostrovsky@oracle.com>
	<52F0DB8F0200007800118F53@nat28.tlf.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <52F0DB8F0200007800118F53@nat28.tlf.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: keir@xen.org, suravee.suthikulpanit@amd.com, andrew.cooper3@citrix.com, eddie.dong@intel.com, dietmar.hahn@ts.fujitsu.com, xen-devel@lists.xen.org, jun.nakajima@intel.com
List-Id: xen-devel@lists.xenproject.org

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