On Mon, 23 Oct 2023 18:42:43 +0100,
Raghavendra Rao Ananta <rananta@xxxxxxxxxx> wrote:
>
> On Mon, Oct 23, 2023 at 5:42 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> >
> > On Fri, 20 Oct 2023 22:40:46 +0100,
> > Raghavendra Rao Ananta <rananta@xxxxxxxxxx> wrote:
> > >
> > > For unimplemented counters, the registers PM{C,I}NTEN{SET,CLR}
> > > and PMOVS{SET,CLR} are expected to have the corresponding bits RAZ.
> > > Hence to ensure correct KVM's PMU emulation, mask out the bits in
> > > these registers for these unimplemented counters before the first
> > > vCPU run.
> > >
> > > Signed-off-by: Raghavendra Rao Ananta <rananta@xxxxxxxxxx>
> > > ---
> > > arch/arm64/kvm/arm.c | 2 +-
> > > arch/arm64/kvm/pmu-emul.c | 11 +++++++++++
> > > include/kvm/arm_pmu.h | 2 ++
> > > 3 files changed, 14 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > > index e3074a9e23a8b..3c0bb80483fb1 100644
> > > --- a/arch/arm64/kvm/arm.c
> > > +++ b/arch/arm64/kvm/arm.c
> > > @@ -857,7 +857,7 @@ static int check_vcpu_requests(struct kvm_vcpu *vcpu)
> > > }
> > >
> > > if (kvm_check_request(KVM_REQ_RELOAD_PMU, vcpu))
> > > - kvm_pmu_handle_pmcr(vcpu, kvm_vcpu_read_pmcr(vcpu));
> > > + kvm_vcpu_handle_request_reload_pmu(vcpu);
> >
> > Please rename this to kvm_vcpu_reload_pmu(). That's long enough. But
> > see below.
> >
> Sounds good.
>
> > >
> > > if (kvm_check_request(KVM_REQ_RESYNC_PMU_EL0, vcpu))
> > > kvm_vcpu_pmu_restore_guest(vcpu);
> > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > > index 9e24581206c24..31e4933293b76 100644
> > > --- a/arch/arm64/kvm/pmu-emul.c
> > > +++ b/arch/arm64/kvm/pmu-emul.c
> > > @@ -788,6 +788,17 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
> > > return val & mask;
> > > }
> > >
> > > +void kvm_vcpu_handle_request_reload_pmu(struct kvm_vcpu *vcpu)
> > > +{
> > > + u64 mask = kvm_pmu_valid_counter_mask(vcpu);
> > > +
> > > + kvm_pmu_handle_pmcr(vcpu, kvm_vcpu_read_pmcr(vcpu));
> > > +
> > > + __vcpu_sys_reg(vcpu, PMOVSSET_EL0) &= mask;
> > > + __vcpu_sys_reg(vcpu, PMINTENSET_EL1) &= mask;
> > > + __vcpu_sys_reg(vcpu, PMCNTENSET_EL0) &= mask;
> > > +}
> >
> > Why is this done on a vcpu request? Why can't it be done upfront, when
> > we're requesting the reload? Or when assigning the PMU? Or when
> > setting PMCR_EL0?
> >
> The idea was to do this only once, after userspace has configured the
> PMCR.N (and has no option to change it), but before we run the guest
> for the first time. So, I guess this can be done when we are
> requesting the reload, if you prefer.
Well, I'm trying to limit the proliferation of these one-off "helpers"
that make the code hard to follow. So it isn't "what I prefer", but
what makes the code easier to understand without having to follow a
maze of pointless abstraction.
> When assigning the PMU, it could be too early to sanitize as the
> userspace would not have configured the PMCR.N yet.
> It can probably be done when userspace configures PMCR.N, but since
> this field is per-guest, we may have to apply the setting for all the
> vCPUs during the ioctl, which may get a little ugly.
Right. So it has to happen at the point where userspace cannot write
to PMCR_EL0 anymore, for which any of the options I mentioned is too
early. What you have is thus correct. But it would have helped if that
rationale was captured in the commit message.
M.
--
Without deviation from the norm, progress is not possible.
