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@xxxxxxxxxx>
> > > ---
> > > 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
>
>
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
