Thanks a lot, Sean!
On Wed, May 11, 2022 at 10:38 AM Sean Christopherson <seanjc@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> > ---
> > 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
> >
