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

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

[Alexander Graf]

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@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




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

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

[Andrew Jones]

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@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?

Silently changing the user's input, which I see we also do for e.g. MPIDR,
isn't super user friendly.

Thanks,
drew

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

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

[Alexander Graf]

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@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.

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

Re: [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@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
> 
> 
> 

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

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

[Andrew Jones]

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@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
>

Reviewed-by: Andrew Jones <drjones@redhat.com> 

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

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

[Marc Zyngier]

On 2020-09-10 17:42, 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@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.

I'm afraid you may need a bit more than just this hack. At the moment,
although we can write junk into the shadow copy of PMCR_EL0, the reset
will make that behave correctly. With this patch, the junk sticks and
gets exposed to the guest.

You need at least a .set_user callback to the handling of PMCR_EL0
so that the value stored is legal, follows the architectural
behaviour, and matches the host capabilities.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm