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

From: Marc Zyngier <maz@kernel.org>
To: Alexandre Chartre <alexandre.chartre@oracle.com>
Cc: will@kernel.org, catalin.marinas@arm.com,
	alexandru.elisei@arm.com, james.morse@arm.com,
	suzuki.poulose@arm.com, linux-arm-kernel@lists.infradead.org,
	kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
	konrad.wilk@oracle.com
Subject: Re: [PATCH] KVM: arm64: Disabling disabled PMU counters wastes a lot of time
Date: Tue, 06 Jul 2021 18:36:46 +0100	[thread overview]
Message-ID: <87wnq3739t.wl-maz@kernel.org> (raw)
In-Reply-To: <87y2aj7av5.wl-maz@kernel.org>

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.

	M.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/pmu/reset-values

-- 
Without deviation from the norm, progress is not possible.