Re: [PATCH v2 1/2] KVM: x86: Initialize nr_lvt_entries to a proper default value

[Jue Wang <juew@google.com>]

Thanks Sean.

+cc another email to stay in the loop.

On Fri, Jul 8, 2022 at 3:56 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Jul 06, 2022, Jue Wang wrote:
> > Set the default value of nr_lvt_entries to KVM_APIC_MAX_NR_LVT_ENTRIES-1
> > to address the cases when KVM_X86_SETUP_MCE is not called.
> >
> > Fixes: 4b903561ec49 ("KVM: x86: Add Corrected Machine Check Interrupt (CMCI) emulation to lapic.")
> > Signed-off-by: Jue Wang <juew@google.com>
> > ---
> >  arch/x86/kvm/lapic.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 8537b66cc646..257366b8e3ae 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -2524,6 +2524,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu, int timer_advance_ns)
> >
> >       vcpu->arch.apic = apic;
> >
> > +     apic->nr_lvt_entries = KVM_APIC_MAX_NR_LVT_ENTRIES - 1;
>
> This works, but I don't love the subtle math nor the reliance on mcg_cap.MCG_CMCI_P
> being clear by default.  I'll properly post the below patch next week (compile tested
> only at this point).
>
> From: Sean Christopherson <seanjc@google.com>
> Date: Fri, 8 Jul 2022 15:38:51 -0700
> Subject: [PATCH] KVM: x86: Initialize number of APIC LVT entries during APIC
>  creation
>
> Initialize the number of LVT entries during APIC creation, else the field
> will be incorrectly left '0' if userspace never invokes KVM_X86_SETUP_MCE.
>
> Add and use a helper to calculate the number of entries even though
> MCG_CMCI_P is not set by default in vcpu->arch.mcg_cap.  Relying on that
> to always be true is unnecessarily risky, and subtle/confusing as well.
>
> Fixes: 4b903561ec49 ("KVM: x86: Add Corrected Machine Check Interrupt (CMCI) emulation to lapic.")
> Reported-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Cc: Jue Wang <juew@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/lapic.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 6ff17d5a2ae3..1540d01ecb67 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -405,6 +405,11 @@ static inline bool kvm_lapic_lvt_supported(struct kvm_lapic *apic, int lvt_index
>         return apic->nr_lvt_entries > lvt_index;
>  }
>
> +static inline int kvm_apic_calc_nr_lvt_entries(struct kvm_vcpu *vcpu)
> +{
> +       return KVM_APIC_MAX_NR_LVT_ENTRIES - !(vcpu->arch.mcg_cap & MCG_CMCI_P);
> +}
> +
>  void kvm_apic_set_version(struct kvm_vcpu *vcpu)
>  {
>         struct kvm_lapic *apic = vcpu->arch.apic;
> @@ -2561,6 +2566,8 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu, int timer_advance_ns)
>         }
>         apic->vcpu = vcpu;
>
> +       apic->nr_lvt_entries = kvm_apic_calc_nr_lvt_entries(vcpu);
> +
>         hrtimer_init(&apic->lapic_timer.timer, CLOCK_MONOTONIC,
>                      HRTIMER_MODE_ABS_HARD);
>         apic->lapic_timer.timer.function = apic_timer_fn;
>
> base-commit: 4a627b0b162b9495f3646caa6edb0e0f97d8f2de
> --
>