Re: [PATCH v8 06/13] KVM: arm64: Sanitize PM{C,I}NTEN{SET,CLR}, PMOVS{SET,CLR} before first run

[Raghavendra Rao Ananta]

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.

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.

Thank you.
Raghavendra