Re: [PATCH 1/3] KVM: arm64: Narrow PMU sysreg reset values to architectural requirements

[Marc Zyngier <maz@kernel.org>]

Hi Alex,

On Wed, 14 Jul 2021 16:48:07 +0100,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hi Marc,
> 
> On 7/13/21 2:58 PM, Marc Zyngier wrote:
> > A number of the PMU sysregs expose reset values that are not in
> > compliant with the architecture (set bits in the RES0 ranges,
> > for example).
> >
> > This in turn has the effect that we need to pointlessly mask
> > some register when using them.
> >
> > Let's start by making sure we don't have illegal values in the
> > shadow registers at reset time. This affects all the registers
> > that dedicate one bit per counter, the counters themselves,
> > PMEVTYPERn_EL0 and PMSELR_EL0.
> >
> > Reported-by: Alexandre Chartre <alexandre.chartre@oracle.com>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/sys_regs.c | 46 ++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 43 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index f6f126eb6ac1..95ccb8f45409 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -603,6 +603,44 @@ static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu,
> >  	return REG_HIDDEN;
> >  }
> >  
> > +static void reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > +{
> > +	u64 n, mask;
> > +
> > +	/* No PMU available, any PMU reg may UNDEF... */
> > +	if (!kvm_arm_support_pmu_v3())
> > +		return;
> > +
> > +	n = read_sysreg(pmcr_el0) >> ARMV8_PMU_PMCR_N_SHIFT;
> 
> Isn't this going to cause a lot of unnecessary traps with NV? Is
> that going to be a problem?

We'll get a new traps at L2 VM creation if we expose a PMU to the L1
guest, and if L2 gets one too. I don't think that's a real problem, as
the performance of an L2 PMU is bound to be hilarious, and if we are
really worried about that, we can always cache it locally. Which is
likely the best thing to do if you think of big-little.

Let's not think of big-little.

Another thing is that we could perfectly ignore the number of counter
on the host and always expose the architectural maximum, given that
the PMU is completely emulated. With that, no trap.

> Because at the moment I can't think of an elegant way to avoid it,
> other than special casing PMCR_EL0 in kvm_reset_sys_regs() and using
> here __vcpu_sys_reg(vcpu, PMCR_EL0). Or, even better, using
> kvm_pmu_valid_counter_mask(vcpu), since this is identical to what
> that function does.

I looked into that and bailed out, as it creates interesting ordering
problems...

> 
> > +	n &= ARMV8_PMU_PMCR_N_MASK;
> > +
> > +	reset_unknown(vcpu, r);
> > +
> > +	mask = BIT(ARMV8_PMU_CYCLE_IDX);
> 
> PMSWINC_EL0 has bit 31 RES0. Other than that, looked at all the PMU
> registers and everything looks correct to me.

PMSWINC_EL0 is a RAZ/WO register, which really shouldn't have a shadow
counterpart (the storage is completely unused). Let me get rid on this
sucker in v2.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm