Re: [PATCH 1/2] KVM: x86: Synthesize at most one PMI per VM-exit

[Sean Christopherson <seanjc@google.com>]

On Mon, Sep 25, 2023, Mingwei Zhang wrote:
> On Sun, Sep 24, 2023 at 11:09 PM Mingwei Zhang <mizhang@google.com> wrote:
> >
> > Hi Sean,
> >
> > On Fri, Sep 22, 2023 at 4:00 PM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Fri, Sep 22, 2023, Mingwei Zhang wrote:
> > > > So yes, they could be put together and they could be put separately.
> > > > But I don't see why they _cannot_ be together or cause confusion.
> > >
> > > Because they don't need to be put together.  Roman's patch kinda sorta overlaps
> > > with the prev_counter mess, but Jim's fixes are entirely orthogonal.
> > >
> > > If one person initially posted such a series with everything together I probably
> > > wouldn't care *too* much, but combining patches and/or series that aren't tightly
> > > coupled or dependent in some way usually does more harm than good.  E.g. if a
> > > maintainer has complaints against only one or two patches in series of unrelated
> > > patches, then grabbing the "good" patches is unnecessarily difficult.  It's not
> > > truly hard on the maintainer's end, but little bits of avoidable friction in the
> > > process adds up across hundreds and thousands of patches.
> > >
> > > FWIW, my plan is to apply Roman's patch pretty much as-is, grab v2 from Jim, and
> > > post my cleanups as a separate series on top (maybe two series, really haven't
> > > thought about it yet).  The only reason I have them all in a single branch is
> > > because there are code conflicts and I know I will apply the patches from Roman
> > > and Jim first, i.e. I didn't want to develop on a base that I knew would become
> > > stale.
> > >
> > > > So, I would like to put them together in the same context with a cover letter
> > > > fully describing the details.
> > >
> > > I certainly won't object to a thorough bug report/analysis, but I'd prefer that
> > > Jim's series be posted separately (though I don't care if it's you or Jim that
> > > posts it).
> >
> > Thanks for agreeing to put things together. In fact, everything
> > together means all relevant fix patches for the same bug need to be
> > together. But I will put my patch explicitly as _optional_ mentioned
> > in the cover letter.

No, please do not post your version of the emulated_counter patch.  I am more
than happy to give you primary author credit (though I need your SoB), all I care
about is minimizing the amount of effort and overhead on my end.  At this point,
posting your version at this time will only generate more noise and make my job
harder.  To tie everything together in the cover letter, just include lore links
to the relevant pseudo-patches.

Assuming you are taking over Jim's series, please post v2 asap.  I want to get
the critical fixes applied sooner than later.

> > If the series causes inconvenience, please accept my apology. For the
> > sense of responsibility, I think I could just use this opportunity to
> > send my updated version with your comment fixed. I will also use this
> > chance to update your fix to Jim's patches.
> >
> > One last thing, breaking the kvm-unit-test/pmu still surprises me.
> > Please test it again when you have a chance. Maybe adding more fixes
> > on top. With the series sent, I will hand it over to you.
> >
> 
> Never, this is a test failure that we already solved internally.
> Applying the following fix to kvm-unit-tests/pmu remove the failures:
> 
> diff --git a/x86/pmu.c b/x86/pmu.c
> index 0def2869..667e6233 100644
> --- a/x86/pmu.c
> +++ b/x86/pmu.c
> @@ -68,6 +68,7 @@ volatile uint64_t irq_received;
>  static void cnt_overflow(isr_regs_t *regs)
>  {
>         irq_received++;
> +       apic_write(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);
>         apic_write(APIC_EOI, 0);
>  }
> 
> Since KVM vPMU adds a mask when injecting the PMI, it is the
> responsibility of the guest PMI handler to remove the mask and allow
> subsequent PMIs delivered.
> 
> We should upstream the above fix some time.

Please post the above asap.  And give pmu_pebs.c's cnt_overflow() the same
treatment when you do.  Or just give my your SoB and I'll write the changelog.