Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Date:   Mon, 23 Oct 2023 19:07:13 +0100
Message-ID: <86ttqh43hq.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Raghavendra Rao Ananta <rananta@google.com>
Cc:     Oliver Upton <oliver.upton@linux.dev>,
        Alexandru Elisei <alexandru.elisei@arm.com>,
        James Morse <james.morse@arm.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Zenghui Yu <yuzenghui@huawei.com>,
        Shaoqin Huang <shahuang@redhat.com>,
        Jing Zhang <jingzhangos@google.com>,
        Reiji Watanabe <reijiw@google.com>,
        Colton Lewis <coltonlewis@google.com>,
        linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Subject: Re: [PATCH v8 06/13] KVM: arm64: Sanitize PM{C,I}NTEN{SET,CLR}, PMOVS{SET,CLR} before first run
In-Reply-To: <CAJHc60xijk_QpxK-eDzdY2g2OXXaQ3En=fBfBe45hAjuJMFNUw@mail.gmail.com>
References: <20231020214053.2144305-1-rananta@google.com>
        <20231020214053.2144305-7-rananta@google.com>
        <86y1ft4ijm.wl-maz@kernel.org>
        <CAJHc60xijk_QpxK-eDzdY2g2OXXaQ3En=fBfBe45hAjuJMFNUw@mail.gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/29.1
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, 23 Oct 2023 18:42:43 +0100,
Raghavendra Rao Ananta <rananta@google.com> wrote:
>=20
> On Mon, Oct 23, 2023 at 5:42=E2=80=AFAM Marc Zyngier <maz@kernel.org> wro=
te:
> >
> > On Fri, 20 Oct 2023 22:40:46 +0100,
> > Raghavendra Rao Ananta <rananta@google.com> 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@google.com>
> > > ---
> > >  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 *v=
cpu)
> > >               }
> > >
> > >               if (kvm_check_request(KVM_REQ_RELOAD_PMU, vcpu))
> > > -                     kvm_pmu_handle_pmcr(vcpu, kvm_vcpu_read_pmcr(vc=
pu));
> > > +                     kvm_vcpu_handle_request_reload_pmu(vcpu);
> >
> > Please rename this to kvm_vcpu_reload_pmu(). That's long enough. But
> > see below.
> >
> Sounds good.
>=20
> > >
> > >               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, bo=
ol pmceid1)
> > >       return val & mask;
> > >  }
> > >
> > > +void kvm_vcpu_handle_request_reload_pmu(struct kvm_vcpu *vcpu)
> > > +{
> > > +     u64 mask =3D kvm_pmu_valid_counter_mask(vcpu);
> > > +
> > > +     kvm_pmu_handle_pmcr(vcpu, kvm_vcpu_read_pmcr(vcpu));
> > > +
> > > +     __vcpu_sys_reg(vcpu, PMOVSSET_EL0) &=3D mask;
> > > +     __vcpu_sys_reg(vcpu, PMINTENSET_EL1) &=3D mask;
> > > +     __vcpu_sys_reg(vcpu, PMCNTENSET_EL0) &=3D 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.

--=20
Without deviation from the norm, progress is not possible.