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
