Re: [PATCH v2] target/i386/kvm: Refine VMX controls setting for backward compatibility

[Ewan Hai]

On 2/20/24 03:32, Xiaoyao Li wrote:
> > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> > index 11b8177eff..c8f6c0b531 100644
> > --- a/target/i386/kvm/kvm.c
> > +++ b/target/i386/kvm/kvm.c
> > @@ -2296,6 +2296,7 @@ void kvm_arch_do_init_vcpu(X86CPU *cpu)
> >   static int kvm_get_supported_feature_msrs(KVMState *s)
> >   {
> >       int ret = 0;
> > +    int i;
> >
> >       if (kvm_feature_msrs != NULL) {
> >           return 0;
> > @@ -2330,6 +2331,19 @@ static int 
> > kvm_get_supported_feature_msrs(KVMState *s)
> >           return ret;
> >       }
> >
> > +    /*
> > +     * Compatibility fix:
> > +     * Older Linux kernels(<5.3) include the 
> > MSR_IA32_VMX_PROCBASED_CTLS2
>
> we can be more accurate, that kernel version 4.17 to 5.2, reports
> MSR_IA32_VMX_PROCBASED_CTLS2 in KVM_GET_MSR_FEATURE_INDEX_LIST but not
> KVM_GET_MSR_INDEX_LIST.
Yeah, I'll add this more precise comment to the next patch.
> > +     * only in feature msr list, but not in regular msr list. This 
> > lead to
> > +     * an issue in older kernel versions where QEMU, through the 
> > regular
> > +     * MSR list check, assumes the kernel doesn't maintain this msr,
> > +     * resulting in incorrect settings by QEMU for this msr.
> > +     */
> > +    for (i = 0; i < kvm_feature_msrs->nmsrs; i++) {
> > +        if (kvm_feature_msrs->indices[i] == 
> > MSR_IA32_VMX_PROCBASED_CTLS2) {
> > +            has_msr_vmx_procbased_ctls2 = true;
> > +        }
> > +    }
>
> I'm wondering should we move all the initialization of has_msr_*, that
> associated with feature MSRs, to here. e.g., has_msr_arch_capabs,
> has_msr_vmx_vmfunc,...
I believe this is a more elegant way to fix the issue, which will be 
reflected in my next patch.