Re: [PATCH v2 05/15] arm64: KVM: add access handler for SPE system registers

[Sudeep Holla]

On Fri, May 24, 2019 at 12:36:24PM +0100, Julien Thierry wrote:
> Hi Sudeep,
> 
> On 23/05/2019 11:34, Sudeep Holla wrote:
> > SPE Profiling Buffer owning EL is configurable and when MDCR_EL2.E2PB
> > is configured to provide buffer ownership to EL1, the control registers
> > are trapped.
> > 
> > Add access handlers for the Statistical Profiling Extension(SPE)
> > Profiling Buffer controls registers. This is need to support profiling
> > using SPE in the guests.
> > 
> > Signed-off-by: Sudeep Holla <sudeep.holla@xxxxxxx>
> > ---
> >  arch/arm64/include/asm/kvm_host.h | 13 ++++++++++++
> >  arch/arm64/kvm/sys_regs.c         | 35 +++++++++++++++++++++++++++++++
> >  include/kvm/arm_spe.h             | 15 +++++++++++++
> >  3 files changed, 63 insertions(+)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 611a4884fb6c..559aa6931291 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -147,6 +147,19 @@ enum vcpu_sysreg {
> >  	MDCCINT_EL1,	/* Monitor Debug Comms Channel Interrupt Enable Reg */
> >  	DISR_EL1,	/* Deferred Interrupt Status Register */
> >  
> > +	/* Statistical Profiling Extension Registers */
> > +
> > +	PMSCR_EL1,
> > +	PMSICR_EL1,
> > +	PMSIRR_EL1,
> > +	PMSFCR_EL1,
> > +	PMSEVFR_EL1,
> > +	PMSLATFR_EL1,
> > +	PMSIDR_EL1,
> > +	PMBLIMITR_EL1,
> > +	PMBPTR_EL1,
> > +	PMBSR_EL1,
> > +
> >  	/* Performance Monitors Registers */
> >  	PMCR_EL0,	/* Control Register */
> >  	PMSELR_EL0,	/* Event Counter Selection Register */
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 857b226bcdde..dbf5056828d3 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -646,6 +646,30 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> >  	__vcpu_sys_reg(vcpu, PMCR_EL0) = val;
> >  }
> >  
> > +static bool access_pmsb_val(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> > +			    const struct sys_reg_desc *r)
> > +{
> > +	if (p->is_write)
> > +		vcpu_write_sys_reg(vcpu, p->regval, r->reg);
> > +	else
> > +		p->regval = vcpu_read_sys_reg(vcpu, r->reg);
> > +
> > +	return true;
> > +}
> > +
> > +static void reset_pmsb_val(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > +{
> > +	if (!kvm_arm_support_spe_v1()) {
> > +		__vcpu_sys_reg(vcpu, r->reg) = 0;
> > +		return;
> > +	}
> > +
> > +	if (r->reg == PMSIDR_EL1)
> 
> If only PMSIDR_EL1 has a non-zero reset value, it feels a bit weird to
> share the reset function for all these registers.
>

Ah, right. Initially I did have couple of other registers which were not
needed. So I removed them without observing that I could have just used
reset_val(0) for all except PMSIDR_EL1.

> I would suggest only having a reset_pmsidr() function, and just use
> reset_val() with sys_reg_desc->val set to 0 for all the others.
>

Thanks for pointing this out.

--
Regards,
Sudeep