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

[Sean Christopherson <seanjc@google.com>]

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.

 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.

> 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.

> +
>  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
>