On Fri, Sep 27, 2019 at 9:32 AM Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
>
> On 27/09/19 18:10, Jim Mattson wrote:
> > On Fri, Sep 27, 2019 at 9:06 AM Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
> >>
> >> On 27/09/19 17:58, Xiaoyao Li wrote:
> >>> Indeed, "KVM_GET_MSR_INDEX_LIST" returns the guest msrs that KVM supports and
> >>> they are free from different guest configuration since they're initialized when
> >>> kvm module is loaded.
> >>>
> >>> Even though some MSRs are not exposed to guest by clear their related cpuid
> >>> bits, they are still saved/restored by QEMU in the same fashion.
> >>>
> >>> I wonder should we change "KVM_GET_MSR_INDEX_LIST" per VM?
> >>
> >> We can add a per-VM version too, yes.
>
> There is one problem with that: KVM_SET_CPUID2 is a vCPU ioctl, not a VM
> ioctl.
>
> > Should the system-wide version continue to list *some* supported MSRs
> > and *some* unsupported MSRs, with no rhyme or reason? Or should we
> > codify what that list contains?
>
> The optimal thing would be for it to list only MSRs that are
> unconditionally supported by all VMs and are part of the runtime state.
> MSRs that are not part of the runtime state, such as the VMX
> capabilities, should be returned by KVM_GET_MSR_FEATURE_INDEX_LIST.
>
> This also means that my own commit 95c5c7c77c06 ("KVM: nVMX: list VMX
> MSRs in KVM_GET_MSR_INDEX_LIST", 2019-07-02) was incorrect.
> Unfortunately, that commit was done because userspace (QEMU) has a
> genuine need to detect whether KVM is new enough to support the
> IA32_VMX_VMFUNC MSR.
>
> Perhaps we can make all MSRs supported unconditionally if
> host_initiated. For unsupported performance counters it's easy to make
> them return 0, and allow setting them to 0, if host_initiated (BTW, how
> did you pick 32? is there any risk of conflicts with other MSRs?).
>
> I'm not sure of the best set of values to allow for VMX caps, especially
> with the default0/default1 stuff going on for execution controls. But
> perhaps that would be the simplest thing to do.
>
> One possibility would be to make a KVM_GET_MSR_INDEX_LIST variant that
> is a system ioctl and takes a CPUID vector. I'm worried that it would
> be tedious to get right and hardish to keep correct---so I'm not sure
> it's a good idea.
Even worse, CPUID alone isn't sufficient. For example, according to
the SDM, "The IA32_VMX_VMFUNC MSR exists only on processors that
support the 1-setting of the 'activate secondary controls'
VM-execution control (only if bit 63 of the IA32_VMX_PROCBASED_CTLS
MSR is 1) and the 1-setting of the 'enable VM functions' secondary
processor-based VM-execution control (only if bit 45 of the
IA32_VMX_PROCBASED_CTLS2 MSR is 1)."
