Re: [PATCH v5 2/4] arm64: KVM: add accessors to track guest/host only counters

From: Suzuki K Poulose <suzuki.poulose@arm.com>
To: Andrew Murray <andrew.murray@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Julien Thierry <julien.thierry@arm.com>,
	Marc Zyngier <marc.zyngier@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will.deacon@arm.com>,
	Christoffer Dall <christoffer.dall@arm.com>,
	kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v5 2/4] arm64: KVM: add accessors to track guest/host only counters
Date: Fri, 7 Dec 2018 14:08:02 +0000	[thread overview]
Message-ID: <441d603e-eb30-821b-a59b-eec1bd6c8e22@arm.com> (raw)
In-Reply-To: <20181206192108.GA51311@e119886-lin.cambridge.arm.com>

On 06/12/2018 19:21, Andrew Murray wrote:
> On Thu, Dec 06, 2018 at 06:51:37PM +0000, Suzuki K Poulose wrote:
>>
>>
>> On 06/12/2018 17:21, Suzuki K Poulose wrote:
>>> Hi Andrew,
>>>
>>> On 05/12/2018 15:30, Andrew Murray wrote:
>>>> In order to effeciently enable/disable guest/host only perf counters
>>>> at guest entry/exit we add bitfields to kvm_cpu_context for guest and
>>>> host events as well as accessors for updating them.
>>>>
>>>> Signed-off-by: Andrew Murray <andrew.murray@arm.com>
>>>> ---
>>>>     arch/arm64/include/asm/kvm_host.h | 24 ++++++++++++++++++++++++
>>>>     1 file changed, 24 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>>>> index 1550192..800c87b 100644
>>>> --- a/arch/arm64/include/asm/kvm_host.h
>>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>>> @@ -203,6 +203,8 @@ struct kvm_cpu_context {
>>>>     	};
>>>>     	struct kvm_vcpu *__hyp_running_vcpu;
>>>> +	u32 events_host;
>>>> +	u32 events_guest;
>>>>     };
>>>>     typedef struct kvm_cpu_context kvm_cpu_context_t;
>>>> @@ -467,11 +469,33 @@ void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu);
>>>>     void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu);
>>>>     void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu);
>>>> +#define KVM_PMU_EVENTS_HOST	1
>>>> +#define KVM_PMU_EVENTS_GUEST	2
>>>> +
>>>>     #ifdef CONFIG_KVM /* Avoid conflicts with core headers if CONFIG_KVM=n */
>>>>     static inline int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
>>>>     {
>>>>     	return kvm_arch_vcpu_run_map_fp(vcpu);
>>>>     }
>>>> +static inline void kvm_set_pmu_events(u32 set, int flags)
>>>> +{
>>>> +	kvm_cpu_context_t *ctx = this_cpu_ptr(&kvm_host_cpu_state);
>>>> +
>>>> +	if (flags & KVM_PMU_EVENTS_HOST)
>>>> +		ctx->events_host |= set;
>>>> +	if (flags & KVM_PMU_EVENTS_GUEST)
>>>> +		ctx->events_guest |= set;
>>>
>>> You seem to be passing a single flag at a time ever, in the next patch.
>>> Either we can batch the calls in the next patch, or make this a real number and
>>> not a flag. g.g, enum { HOST,  GUEST } or even a bool.
>>>
>>> I think the former sounds better.
>>
>> Having another look at patch-3, we can never club set() calls for host &
>> guest events, we should go for the latter.
> 
> Well we could always do this..
> 
>          u32 flags = 0;
>          if (!attr->exclude_host)
>                  flags |= KVM_PMU_EVENTS_HOST;
>          if (!attr->exclude_guest)
>                  flags |= KVM_PMU_EVENTS_GUEST;
> 
>          kvm_set_pmu_events(counter_bits, flags);

Yes, I think this should be fine.

Cheers
Suzuki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel