[PATCH v3] KVM: arm64: Preserve PMCR immutable values across reset

[Andrew Jones]

On Fri, Sep 11, 2020 at 09:40:04AM +0200, Alexander Graf wrote:
> 
> 
> On 10.09.20 19:36, Andrew Jones wrote:
> > 
> > On Thu, Sep 10, 2020 at 06:42:43PM +0200, Alexander Graf wrote:
> > > We allow user space to set the PMCR register to any value. However,
> > > when time comes for a vcpu reset (for example on PSCI online), PMCR
> > > is reset to the hardware capabilities.
> > > 
> > > I would like to explicitly expose different PMU capabilities (number
> > > of supported event counters) to the guest than hardware supports.
> > > Ideally across vcpu resets.
> > > 
> > > So this patch adopts the reset path to only populate the immutable
> > > PMCR register bits from hardware when they were not initialized
> > > previously. This effectively means that on a normal reset, only the
> > > guest settable fields are reset, while on vcpu creation the register
> > > gets populated from hardware like before.
> > > 
> > > With this in place and a change in user space to invoke SET_ONE_REG
> > > on the PMCR for every vcpu, I can reliably set the PMU event counter
> > > number to arbitrary values.
> > > 
> > > Signed-off-by: Alexander Graf <graf at amazon.com>
> > > ---
> > >   arch/arm64/kvm/sys_regs.c | 9 ++++++++-
> > >   1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > index 20ab2a7d37ca..28f67550db7f 100644
> > > --- a/arch/arm64/kvm/sys_regs.c
> > > +++ b/arch/arm64/kvm/sys_regs.c
> > > @@ -663,7 +663,14 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > >   {
> > >        u64 pmcr, val;
> > > 
> > > -     pmcr = read_sysreg(pmcr_el0);
> > > +     /*
> > > +      * If we already received PMCR from a previous ONE_REG call,
> > > +      * maintain its immutable flags
> > > +      */
> > > +     pmcr = __vcpu_sys_reg(vcpu, r->reg);
> > > +     if (!__vcpu_sys_reg(vcpu, r->reg))
> > > +             pmcr = read_sysreg(pmcr_el0);
> > > +
> > >        /*
> > >         * Writable bits of PMCR_EL0 (ARMV8_PMU_PMCR_MASK) are reset to UNKNOWN
> > >         * except PMCR.E resetting to zero.
> > > --
> > > 2.16.4
> > > 
> > 
> > Aha, a much simpler patch than I expected. With this approach we don't
> > need a get_user() function, or to use 'val', but don't we still want to
> > add sanity checks with a set_user() function? At least to ensure immutable
> > flags match and that PMCR_EL0.N isn't too big?
> 
> We don't check for any flags today, so in a way adding checks would be ABI
> breakage.
> 
> And as Marc pointed out, all of the counters are basically virtual through
> perf. So if you report 31 counters, you end up spawning 31 perf counters
> which get multiplexed, so it would work (albeit not be terribly accurate).
> 
> That leaves identification bits as something we can check for. But do we
> really have to? What's the worst thing that can happen? KVM user space can
> shoot themselves in the foot. Well, they can also set PC to an invalid
> value. If you do bad things you get bad results :). As long as it's not a
> security risk, I'm not sure the benefits of checking outweigh the risks.

That's a reasonable justification.

Thanks,
drew

> 
> > Silently changing the user's input, which I see we also do for e.g. MPIDR,
> > isn't super user friendly.
> 
> Yes :).
> 
> 
> Alex
> 
> 
> 
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
> Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
> Sitz: Berlin
> Ust-ID: DE 289 237 879
> 
> 
>