Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230504120042.785651-1-rkagan@amazon.de> <ZH6DJ8aFq/LM6Bk9@google.com>
In-Reply-To: <ZH6DJ8aFq/LM6Bk9@google.com>
From:   Jim Mattson <jmattson@google.com>
Date:   Thu, 29 Jun 2023 14:16:53 -0700
Message-ID: <CALMp9eS3F08cwUJbKjTRAEL0KyZ=MC==YSH+DW-qsFkNfMpqEQ@mail.gmail.com>
Subject: Re: [PATCH] KVM: x86: vPMU: truncate counter value to allowed width
To:     Sean Christopherson <seanjc@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Eric Hankland <ehankland@google.com>
Cc:     Roman Kagan <rkagan@amazon.de>, kvm@vger.kernel.org,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Like Xu <likexu@tencent.com>, x86@kernel.org,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
        Borislav Petkov <bp@alien8.de>, Ingo Molnar <mingo@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jun 5, 2023 at 5:51=E2=80=AFPM Sean Christopherson <seanjc@google.c=
om> wrote:
>
> On Thu, May 04, 2023, Roman Kagan wrote:
> > diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> > index 5c7bbf03b599..6a91e1afef5a 100644
> > --- a/arch/x86/kvm/pmu.h
> > +++ b/arch/x86/kvm/pmu.h
> > @@ -60,6 +60,12 @@ static inline u64 pmc_read_counter(struct kvm_pmc *p=
mc)
> >       return counter & pmc_bitmask(pmc);
> >  }
> >
> > +static inline void pmc_set_counter(struct kvm_pmc *pmc, u64 val)
> > +{
> > +     pmc->counter +=3D val - pmc_read_counter(pmc);
>
> Ugh, not your code, but I don't see how the current code can possibly be =
correct.
>
> The above unpacks to
>
>         counter =3D pmc->counter;
>         if (pmc->perf_event && !pmc->is_paused)
>                 counter +=3D perf_event_read_value(pmc->perf_event,
>                                                  &enabled, &running);
>         pmc->counter +=3D val - (counter & pmc_bitmask(pmc));
>
> which distills down to
>
>         counter =3D 0;
>         if (pmc->perf_event && !pmc->is_paused)
>                 counter +=3D perf_event_read_value(pmc->perf_event,
>                                                  &enabled, &running);
>         pmc->counter =3D val - (counter & pmc_bitmask(pmc));
>
> or more succinctly
>
>         if (pmc->perf_event && !pmc->is_paused)
>                 val -=3D perf_event_read_value(pmc->perf_event, &enabled,=
 &running);
>
>         pmc->counter =3D val;
>
> which is obviously wrong.  E.g. if the guest writes '0' to an active coun=
ter, the
> adjustment will cause pmc->counter to be loaded with a large (in unsigned=
 terms)
> value, and thus quickly overflow after a write of '0'.

This weird construct goes all the way back to commit f5132b01386b
("KVM: Expose a version 2 architectural PMU to a guests"). Paolo
killed it in commit 2924b52117b2 ("KVM: x86/pmu: do not mask the value
that is written to fixed PMUs"), perhaps by accident. Eric then
resurrected it in commit 4400cf546b4b ("KVM: x86: Fix perfctr WRMSR
for running counters").

It makes no sense to me. WRMSR should just set the new value of the
counter, regardless of the old value or whether or not it is running.

> I assume the intent it to purge any accumulated counts that occurred sinc=
e the
> last read, but *before* the write, but then shouldn't this just be:
>
>         /* Purge any events that were acculumated before the write. */
>         if (pmc->perf_event && !pmc->is_paused)
>                 (void)perf_event_read_value(pmc->perf_event, &enabled, &r=
unning);
>
>         pmc->counter =3D val & pmc_bitmask(pmc);
>
> Am I missing something?
>
> I'd like to get this sorted out before applying this patch, because I rea=
lly want
> to document what on earth this new helper is doing.  Seeing a bizarre par=
tial
> RMW operation in a helper with "set" as the verb is super weird.