Re: [PATCH v2 1/4] KVM: x86: Clean up KVM APIC LVT logic.

[Jue Wang <juew@google.com>]

Thanks a lot, Sean!


On Wed, May 11, 2022 at 10:38 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Apr 12, 2022, Jue Wang wrote:
> > This is in preparation to add APIC_LVTCMCI support.
>
> There is not nearly enough information in this changelog.  Same goes for all other
> patches in the series.  And when you start writing changelogs to explain what is
> being done and why, I suspect you'll find that this should be further broken up
> into multiple patches.
I realized the general lack of enough information and background in
the changelog and I am working to rewrite it with necessary
background.

>
>  1. Make APIC_VERSION capture only the magic 0x14UL
>  2. Fill apic_lvt_mask with enums / explicit entries.
>  3. Add APIC_LVTx() macro
>
> And proper upstream etiquette would be to add
>
>   Suggested-by: Sean Christopherson <seanjc@google.com>
>
> for #2 and #3.  I don't care much about the attribution (though that's nice too),
> but more importantly it provides a bit of context for others that get involved
> later in the series (sometimes unwillingly).  E.g. if someone encounters a bug
> with a patch, the Suggested-by gives them one more person to loop into the
> discussion.  Ditto for other reviewers, e.g. if someone starts reviewing the
> series at v3 or whatever, it provides some background on how the series got to
> v3 without them having to actually look at v1 or v2.

Thanks, for walking me through this process and practices. I am
breaking this patch into 3 and adding "Suggested-by: Sean
Christopherson ...." to them. Is it OK that I add you as
"Suggested-by" to the later patches in this series?


>
> > Signed-off-by: Jue Wang <juew@google.com>
> > ---
> >  arch/x86/kvm/lapic.c | 33 +++++++++++++++++++--------------
> >  arch/x86/kvm/lapic.h | 19 ++++++++++++++++++-
> >  2 files changed, 37 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 9322e6340a74..2c770e4c0e6c 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -54,7 +54,7 @@
> >  #define PRIo64 "o"
> >
> >  /* 14 is the version for Xeon and Pentium 8.4.8*/
> > -#define APIC_VERSION                 (0x14UL | ((KVM_APIC_LVT_NUM - 1) << 16))
> > +#define APIC_VERSION                 0x14UL
> >  #define LAPIC_MMIO_LENGTH            (1 << 12)
> >  /* followed define is not in apicdef.h */
> >  #define MAX_APIC_VECTOR                      256
> > @@ -364,10 +364,15 @@ static inline int apic_lvt_nmi_mode(u32 lvt_val)
> >       return (lvt_val & (APIC_MODE_MASK | APIC_LVT_MASKED)) == APIC_DM_NMI;
> >  }
> >
> > +static inline int kvm_apic_get_nr_lvt_entries(struct kvm_vcpu *vcpu)
> > +{
> > +     return KVM_APIC_MAX_NR_LVT_ENTRIES;
> > +}
>
> I think it makes sense to introduce this helper with the CMCI patch.  Until then,
> requiring @vcpu to get the max number of entries is misleading and unnecessary.
>
> Case in point, this patch is broken in that the APIC_SPIV path in kvm_lapic_reg_write()
> uses the #define directly, which necessitates fixup in the CMCI patch to use this
> helper.
>
Ack, will incorporate this and comment below into V3.
> > +
> >  void kvm_apic_set_version(struct kvm_vcpu *vcpu)
> >  {
> >       struct kvm_lapic *apic = vcpu->arch.apic;
> > -     u32 v = APIC_VERSION;
> > +     u32 v = APIC_VERSION | ((kvm_apic_get_nr_lvt_entries(vcpu) - 1) << 16);
> >
> >       if (!lapic_in_kernel(vcpu))
> >               return;
> > @@ -385,12 +390,13 @@ void kvm_apic_set_version(struct kvm_vcpu *vcpu)
> >       kvm_lapic_set_reg(apic, APIC_LVR, v);
> >  }
> >
> > -static const unsigned int apic_lvt_mask[KVM_APIC_LVT_NUM] = {
> > -     LVT_MASK ,      /* part LVTT mask, timer mode mask added at runtime */
> > -     LVT_MASK | APIC_MODE_MASK,      /* LVTTHMR */
> > -     LVT_MASK | APIC_MODE_MASK,      /* LVTPC */
> > -     LINT_MASK, LINT_MASK,   /* LVT0-1 */
> > -     LVT_MASK                /* LVTERR */
> > +static const unsigned int apic_lvt_mask[KVM_APIC_MAX_NR_LVT_ENTRIES] = {
> > +     [LVT_TIMER] = LVT_MASK,      /* timer mode mask added at runtime */
> > +     [LVT_THERMAL_MONITOR] = LVT_MASK | APIC_MODE_MASK,
> > +     [LVT_PERFORMANCE_COUNTER] = LVT_MASK | APIC_MODE_MASK,
> > +     [LVT_LINT0] = LINT_MASK,
> > +     [LVT_LINT1] = LINT_MASK,
> > +     [LVT_ERROR] = LVT_MASK
> >  };
> >
> >  static int find_highest_vector(void *bitmap)
> > @@ -2039,10 +2045,9 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
> >                       int i;
> >                       u32 lvt_val;
> >
> > -                     for (i = 0; i < KVM_APIC_LVT_NUM; i++) {
> > -                             lvt_val = kvm_lapic_get_reg(apic,
> > -                                                    APIC_LVTT + 0x10 * i);
> > -                             kvm_lapic_set_reg(apic, APIC_LVTT + 0x10 * i,
> > +                     for (i = 0; i < KVM_APIC_MAX_NR_LVT_ENTRIES; i++) {
> > +                             lvt_val = kvm_lapic_get_reg(apic, APIC_LVTx(i));
> > +                             kvm_lapic_set_reg(apic, APIC_LVTx(i),
> >                                            lvt_val | APIC_LVT_MASKED);
> >                       }
> >                       apic_update_lvtt(apic);
> > @@ -2341,8 +2346,8 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event)
> >               kvm_apic_set_xapic_id(apic, vcpu->vcpu_id);
> >       kvm_apic_set_version(apic->vcpu);
> >
> > -     for (i = 0; i < KVM_APIC_LVT_NUM; i++)
> > -             kvm_lapic_set_reg(apic, APIC_LVTT + 0x10 * i, APIC_LVT_MASKED);
> > +     for (i = 0; i < KVM_APIC_MAX_NR_LVT_ENTRIES; i++)
> > +             kvm_lapic_set_reg(apic, APIC_LVTx(i), APIC_LVT_MASKED);
> >       apic_update_lvtt(apic);
> >       if (kvm_vcpu_is_reset_bsp(vcpu) &&
> >           kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_LINT0_REENABLED))
> > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> > index 2b44e533fc8d..5666441d5d1b 100644
> > --- a/arch/x86/kvm/lapic.h
> > +++ b/arch/x86/kvm/lapic.h
> > @@ -10,7 +10,6 @@
> >
> >  #define KVM_APIC_INIT                0
> >  #define KVM_APIC_SIPI                1
> > -#define KVM_APIC_LVT_NUM     6
> >
> >  #define APIC_SHORT_MASK                      0xc0000
> >  #define APIC_DEST_NOSHORT            0x0
> > @@ -29,6 +28,24 @@ enum lapic_mode {
> >       LAPIC_MODE_X2APIC = MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE,
> >  };
> >
> > +enum lapic_lvt_entry {
> > +     LVT_TIMER,
> > +     LVT_THERMAL_MONITOR,
> > +     LVT_PERFORMANCE_COUNTER,
> > +     LVT_LINT0,
> > +     LVT_LINT1,
> > +     LVT_ERROR,
> > +
> > +     KVM_APIC_MAX_NR_LVT_ENTRIES,
> > +};
> > +
> > +
> > +#define APIC_LVTx(x)                                                    \
> > +({                                                                      \
> > +     int __apic_reg = APIC_LVTT + 0x10 * (x);                        \
>
> An intermediate variable is completely unnecessary.  This should do just fine.
>
>   #define APIC_LVTx(x) (APIC_LVTT + 0x10 * (x))
>
> Yes, the macro _may_ eventually becomes a multi-line beast with a variable when
> CMCI support is added, but again that belongs in the CMCI patch.  That way this
> patch doesn't need to change if we decide that even the CMCI-aware version can
> just be:
>
>   #define APIC_LVTx(x) ((x) == LVT_CMCI ? APIC_LVTCMCI : APIC_LVTT + 0x10 * (x))
>
>
> > +     __apic_reg;                                                     \
> > +})
> > +
> >  struct kvm_timer {
> >       struct hrtimer timer;
> >       s64 period;                             /* unit: ns */
> > --
> > 2.35.1.1178.g4f1659d476-goog
> >