From mboxrd@z Thu Jan  1 00:00:00 1970
From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Subject: Re: [PATCH v21 11/14] x86/VPMU: Handle PMU interrupts
 for PV(H) guests
Date: Mon, 18 May 2015 11:11:03 -0400
Message-ID: <555A0107.9020903@oracle.com>
References: <1431119174-1947-1-git-send-email-boris.ostrovsky@oracle.com>
	<1431119174-1947-12-git-send-email-boris.ostrovsky@oracle.com>
	<2290839.Ioueb7rBvb@amur>
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: <2290839.Ioueb7rBvb@amur>
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: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>, xen-devel@lists.xen.org
Cc: kevin.tian@intel.com, JBeulich@suse.com, jun.nakajima@intel.com, andrew.cooper3@citrix.com, tim@xen.org, Aravind.Gopalakrishnan@amd.com, suravee.suthikulpanit@amd.com, dgdegra@tycho.nsa.gov
List-Id: xen-devel@lists.xenproject.org

On 05/18/2015 05:43 AM, Dietmar Hahn wrote:
> Am Freitag 08 Mai 2015, 17:06:11 schrieb Boris Ostrovsky:
>>
>> +    if ( !is_hvm_vcpu(sampling) )
>> +    {
>> +        /* PV(H) guest */
>> +        const struct cpu_user_regs *cur_regs;
>> +        uint64_t *flags = &vpmu->xenpmu_data->pmu.pmu_flags;
>> +        domid_t domid = DOMID_SELF;
>> +
>> +        if ( !vpmu->xenpmu_data )
>> +            return;
>> +
>> +        if ( is_pvh_vcpu(sampling) &&
>> +             !vpmu->arch_vpmu_ops->do_interrupt(regs) )
>
> Here you expect vpmu->arch_vpmu_ops != NULL but ...
>
>> +            return;
>> +
>> +        if ( *flags & PMU_CACHED )
>> +            return;
>> +


...

>> +
>> +        return;
>> +    }
>>
>>       if ( vpmu->arch_vpmu_ops )
>
> ... here is a check.
> Maybe this check here is unnecessary because you would never get this interrupt
> without having arch_vpmu_ops != NULL to switch on the machinery?
>
> There are some other locations too with checks before calling
> vpmu->arch_vpmu_ops->... and some without. Maybe it would make sense to force
> always a complete set of arch_vpmu_ops - functions to avoid this?


I was actually thinking about (eventually) dropping ops tests and 
checking that all of them exist during VPMU initialization.

As for this particular test, it may be worth moving it to the beginning 
of the routine, mostly to guard against spurious interrupts (but also to 
avoid performing it more than once)


>>   }
>>
>> -void vpmu_load(struct vcpu *v)
>> +int vpmu_load(struct vcpu *v, bool_t verify)
>
> vpmu_load uses "verify" but within the arch_vpmu_load functions
> (core2_vpmu_load() and amd_vpmu_load()) you use "from_guest" for the same
> meaning. This is a little bit confusing.
> Always using "verify" would be clearer I think.


Then this will not be consistent with the save part (which doesn't use 
the flag to verify the context but rather to only state that the routine 
should copy it). So I think renaming 'verify' to 'from_guest' and 
keeping arch ops as they are now would be more consistent.

Thanks.
-boris