On Fri, Jun 3, 2022 at 11:58 AM David Matlack <dmatlack@xxxxxxxxxx> wrote:
>
> On Fri, May 20, 2022 at 10:36 AM Jue Wang <juew@xxxxxxxxxx> wrote:
> >
> > To implement Corrected Machine Check Interrupt (CMCI) as another
> > LVT vector, the APIC LVT logic needs to be able to handle an additional
> > LVT vector conditioned on whether MCG_CMCI_P is enabled on the vCPU,
> > this is because CMCI signaling can only be enabled when the CPU's
> > MCG_CMCI_P bit is set (Intel SDM, section 15.3.1.1).
> >
> > This patch factors out the dependency on KVM_APIC_LVT_NUM from the
> > APIC_VERSION macro. In later patches, KVM_APIC_LVT_NUM will be replaced
> > with a helper kvm_apic_get_nr_lvt_entries that reports different LVT
> > number conditioned on whether MCG_CMCI_P is enabled on the vCPU.
>
> Prefer to state what the patch does first, then explain why. Also
> please to use more precise language, especially when referring to
> architectural concepts. For example, I don't believe there is any such
> thing as an "LVT vector".
BTW, these suggestions apply to the entire series.
>
> Putting that together, how about something like this:
>
> Refactor APIC_VERSION so that the maximum number of LVT entries is
> inserted at runtime rather than compile time. This will be used in a
> subsequent commit to expose the LVT CMCI Register to VMs that support
> Corrected Machine Check error counting/signaling
> (IA32_MCG_CAP.MCG_CMCI_P=1).
>
> >
> > Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > Signed-off-by: Jue Wang <juew@xxxxxxxxxx>
> > ---
> > arch/x86/kvm/lapic.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 66b0eb0bda94..a5caa77e279f 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
> > @@ -401,7 +401,7 @@ static inline int apic_lvt_nmi_mode(u32 lvt_val)
> > 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_LVT_NUM - 1) << 16);
> >
> > if (!lapic_in_kernel(vcpu))
> > return;
> > --
> > 2.36.1.124.g0e6072fb45-goog
> >
