Re: [PATCH] KVM: arm64: Disabling disabled PMU counters wastes a lot of time

From: Alexandre Chartre <alexandre.chartre@oracle.com>
To: Marc Zyngier <maz@kernel.org>
Cc: kvm@vger.kernel.org, catalin.marinas@arm.com,
	konrad.wilk@oracle.com, will@kernel.org,
	kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] KVM: arm64: Disabling disabled PMU counters wastes a lot of time
Date: Wed, 7 Jul 2021 14:48:50 +0200	[thread overview]
Message-ID: <78f4827c-71fb-5c95-e700-d2f479f4d07f@oracle.com> (raw)
In-Reply-To: <87wnq3739t.wl-maz@kernel.org>

On 7/6/21 7:36 PM, Marc Zyngier wrote:
> On Tue, 06 Jul 2021 15:52:46 +0100,
> Marc Zyngier <maz@kernel.org> wrote:
>>
>> On Tue, 06 Jul 2021 14:50:35 +0100,
>> Alexandre Chartre <alexandre.chartre@oracle.com> wrote:
>>>
>>>
>>> Hi Marc,
>>>
>>> On 6/29/21 3:16 PM, Alexandre Chartre wrote:
>>>> On 6/29/21 11:06 AM, Marc Zyngier wrote
>>>> [...]
>>>>> So the sysreg is the only thing we should consider, and I think we
>>>>> should drop the useless masking. There is at least another instance of
>>>>> this in the PMU code (kvm_pmu_overflow_status()), and apart from
>>>>> kvm_pmu_vcpu_reset(), only the sysreg accessors should care about the
>>>>> masking to sanitise accesses.
>>>>>
>>>>> What do you think?
>>>>>
>>>>
>>>> I think you are right. PMCNTENSET_EL0 is already masked with kvm_pmu_valid_counter_mask()
>>>> so there's effectively no need to mask it again when we use it. I will send an additional
>>>> patch (on top of this one) to remove useless masking. Basically, changes would be:
>>>
>>> I had a closer look and we can't remove the mask. The access
>>> functions (for pmcnten, pminten, pmovs), clear or set only the
>>> specified valid counter bits. This means that bits other than the
>>> valid counter bits never change in __vcpu_sys_reg(), and those bits
>>> are not necessarily zero because the initial value is
>>> 0x1de7ec7edbadc0deULL (set by reset_unknown()).
>>
>> That's a bug that should be fixed on its own. Bits that are RAZ/WI in
>> the architecture shouldn't be kept in the shadow registers the first
>> place. I'll have a look.
> 
> Please try this[1] for size, which is on top of Linus' tree as of this
> morning.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/pmu/reset-values
> 

I have tried [1] and it works fine. Also tested with my patch on top (without
masking) and it also works fine.

Thanks,

alex.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm