Re: KVM_GET_MSR_INDEX_LIST vs KVM_GET_MSR_FEATURE_INDEX_LIST

Re: KVM_GET_MSR_INDEX_LIST vs KVM_GET_MSR_FEATURE_INDEX_LIST

[Jim Mattson]

On Sat, Sep 8, 2018 at 5:58 PM Liran Alon <liran.alon@oracle.com> wrote:
>
>
> > On 7 Sep 2018, at 21:37, Jim Mattson <jmattson@google.com> wrote:
> >
> > Are these two lists intended to be disjoint? Is it a bug that
> > IA32_ARCH_CAPABILITIES appears in both?
>
> From what I understand, the difference between these two lists is the following:
> * KVM_GET_MSR_INDEX_LIST is list of MSRs that are emulated/virtualized by KVM and
>   their value can be get/set via KVM_GET_MSRS/KVM_SET_MSRS on a *vCPU*.
> * KVM_GET_MSR_FEATURE_INDEX_LIST is a list of MSRs that are emulated/virtualized by KVM
>   and their value can be get via KVM_GET_MSRS on */dev/kvm*.
>   (But can be set via KVM_SET_MSRS on a *vCPU*).
>
> It seems that the KVM_GET_MSR_FEATURE_INDEX_LIST exists only for the purpose of
> userspace to be able to know how KVM plans to expose these MSRs values to newly created vCPUs
> based on KVM capabilities and host CPU capabilities.
>
> If the above is correct, it seems to me that the MSRs in KVM_GET_MSR_FEATURE_INDEX_LIST are actually
> suppose to be a subset of KVM_GET_MSR_INDEX_LIST. So it shouldn’t be an issue that IA32_ARCH_CAPABILITIES
> Exist in both of them.
> However, it seems that I am wrong because KVM_GET_MSR_FEATURE_INDEX_LIST is not
> a subset of KVM_GET_MSR_INDEX_LIST at all. In fact, it seems that IA32_ARCH_CAPABILITIES is the only one
> which exists in both lists…
> Also, MSR_IA32_UCODE_REV exists only in KVM_GET_MSR_FEATURE_INDEX_LIST but one can clearly see
> how it’s value can be get via KVM_GET_MSRS on a *vCPU* so it was also suppose to be part of KVM_GET_MSR_INDEX_LIST
> but it’s not…
>
> So I’m left pretty confused regarding this interface. Would like clarification as-well :)

Here's a more basic question: Should any MSR that can be read and
written by a guest appear in KVM_GET_MSR_INDEX_LIST? If not, what's
the point of this ioctl?

Re: KVM_GET_MSR_INDEX_LIST vs KVM_GET_MSR_FEATURE_INDEX_LIST

[Liran Alon]

> On 15 Nov 2019, at 0:07, Jim Mattson <jmattson@google.com> wrote:
> 
> On Sat, Sep 8, 2018 at 5:58 PM Liran Alon <liran.alon@oracle.com> wrote:
>> 
>> 
>>> On 7 Sep 2018, at 21:37, Jim Mattson <jmattson@google.com> wrote:
>>> 
>>> Are these two lists intended to be disjoint? Is it a bug that
>>> IA32_ARCH_CAPABILITIES appears in both?
> 
> Here's a more basic question: Should any MSR that can be read and
> written by a guest appear in KVM_GET_MSR_INDEX_LIST? If not, what's
> the point of this ioctl?

I think the point of KVM_GET_MSR_INDEX_LIST ioctl is for userspace to know what are all the MSR values it needs to save/restore on migration.
Therefore, any MSR that is exposed to guest read/write and isn’t determined on VM provisioning time, should be returned from this ioctl.

In contrast, KVM_GET_MSR_FEATURE_INDEX_LIST ioctl is meant to be used by userspace to query KVM capabilities based on host MSRs and KVM support and use that information to validate the CPU features that the user have requested to expose to guest.

For example, MSR_IA32_UCODE_REV is specified only in KVM_GET_MSR_FEATURE_INDEX_LIST. This is because it is determined by userspace on provisioning time (No need to save/restore on migration) and userspace may require to know it’s host value to define guest value appropriately. MSR_IA32_PERF_STATUS is not specified in neither ioctls because KVM returns constant value for it (not required to be saved/restored).

However, I’m also not sure about above mentioned definitions… As they are some bizarre things that seems to contradict it:
1) MSR_IA32_ARCH_CAPABILITIES is specified in KVM_GET_MSR_FEATURE_INDEX_LIST to allow userspace to know which vulnerabilities apply to CPU. By default, vCPU MSR_IA32_ARCH_CAPABILITIES value will be set by host value (See kvm_arch_vcpu_setup()) but it’s possible for host userspace to override value exposed to guest (See kvm_set_msr_common()). *However*, it seems to me to be wrong that this MSR is specified in KVM_GET_MSR_INDEX_LIST as it should be determined in VM provisioning time and thus not need to be saved/restore on migration. i.e. How is it different from MSR_IA32_UCODE_REV?
2) MSR_EFER should be saved/restored and thus returned by KVM_GET_MSR_INDEX_LIST. But it’s not. Probably because it can be saved/restored via KVM_{GET,SET}_SREGS but this is inconsistent with semantic definitions of KVM_GET_MSR_INDEX_LIST ioctl...
3) MSR_AMD64_OSVW_ID_LENGTH & MSR_AMD64_OSVW_STATUS can be set by guest but it doesn’t seem to be specified in emulated_msrs[] and therefore not returned by KVM_GET_MSR_INDEX_LIST ioctl. I think this is a migration bug...

Unless someone disagrees, I think I will submit a patch for (1) and (3).

-Liran

Re: KVM_GET_MSR_INDEX_LIST vs KVM_GET_MSR_FEATURE_INDEX_LIST

[Jim Mattson]

On Thu, Nov 14, 2019 at 4:35 PM Liran Alon <liran.alon@oracle.com> wrote:
>
>
>
> > On 15 Nov 2019, at 0:07, Jim Mattson <jmattson@google.com> wrote:
> >
> > On Sat, Sep 8, 2018 at 5:58 PM Liran Alon <liran.alon@oracle.com> wrote:
> >>
> >>
> >>> On 7 Sep 2018, at 21:37, Jim Mattson <jmattson@google.com> wrote:
> >>>
> >>> Are these two lists intended to be disjoint? Is it a bug that
> >>> IA32_ARCH_CAPABILITIES appears in both?
> >
> > Here's a more basic question: Should any MSR that can be read and
> > written by a guest appear in KVM_GET_MSR_INDEX_LIST? If not, what's
> > the point of this ioctl?
>
> I think the point of KVM_GET_MSR_INDEX_LIST ioctl is for userspace to know what are all the MSR values it needs to save/restore on migration.
> Therefore, any MSR that is exposed to guest read/write and isn’t determined on VM provisioning time, should be returned from this ioctl.
>
> In contrast, KVM_GET_MSR_FEATURE_INDEX_LIST ioctl is meant to be used by userspace to query KVM capabilities based on host MSRs and KVM support and use that information to validate the CPU features that the user have requested to expose to guest.
>
> For example, MSR_IA32_UCODE_REV is specified only in KVM_GET_MSR_FEATURE_INDEX_LIST. This is because it is determined by userspace on provisioning time (No need to save/restore on migration) and userspace may require to know it’s host value to define guest value appropriately. MSR_IA32_PERF_STATUS is not specified in neither ioctls because KVM returns constant value for it (not required to be saved/restored).
>
> However, I’m also not sure about above mentioned definitions… As they are some bizarre things that seems to contradict it:
> 1) MSR_IA32_ARCH_CAPABILITIES is specified in KVM_GET_MSR_FEATURE_INDEX_LIST to allow userspace to know which vulnerabilities apply to CPU. By default, vCPU MSR_IA32_ARCH_CAPABILITIES value will be set by host value (See kvm_arch_vcpu_setup()) but it’s possible for host userspace to override value exposed to guest (See kvm_set_msr_common()). *However*, it seems to me to be wrong that this MSR is specified in KVM_GET_MSR_INDEX_LIST as it should be determined in VM provisioning time and thus not need to be saved/restore on migration. i.e. How is it different from MSR_IA32_UCODE_REV?
> 2) MSR_EFER should be saved/restored and thus returned by KVM_GET_MSR_INDEX_LIST. But it’s not. Probably because it can be saved/restored via KVM_{GET,SET}_SREGS but this is inconsistent with semantic definitions of KVM_GET_MSR_INDEX_LIST ioctl...
> 3) MSR_AMD64_OSVW_ID_LENGTH & MSR_AMD64_OSVW_STATUS can be set by guest but it doesn’t seem to be specified in emulated_msrs[] and therefore not returned by KVM_GET_MSR_INDEX_LIST ioctl. I think this is a migration bug...
>
> Unless someone disagrees, I think I will submit a patch for (1) and (3).

I assume that we're also skipping the x2APIC MSRs because they can be
read/modified with KVM_{GET,SET}_LAPIC. At least until you start
thinking about userspace instruction emulation.

What about MSR_F15H_PERF_*, MSR_K7_EVNTSEL*, MSR_K7_PERFCTR*,
MSR_MTRR*, HV_X64_MSR_STIMER[12]_CONFIG,
HV_X64_MSR_STIMER[0123]_COUNT, MSR_VM_CR, and possibly others I'm
missing on the first pass?

Re: KVM_GET_MSR_INDEX_LIST vs KVM_GET_MSR_FEATURE_INDEX_LIST

[Liran Alon]

> On 15 Nov 2019, at 3:05, Jim Mattson <jmattson@google.com> wrote:
> 
> On Thu, Nov 14, 2019 at 4:35 PM Liran Alon <liran.alon@oracle.com> wrote:
>> 
>> 
>> 
>>> On 15 Nov 2019, at 0:07, Jim Mattson <jmattson@google.com> wrote:
>>> 
>>> On Sat, Sep 8, 2018 at 5:58 PM Liran Alon <liran.alon@oracle.com> wrote:
>>>> 
>>>> 
>>>>> On 7 Sep 2018, at 21:37, Jim Mattson <jmattson@google.com> wrote:
>>>>> 
>>>>> Are these two lists intended to be disjoint? Is it a bug that
>>>>> IA32_ARCH_CAPABILITIES appears in both?
>>> 
>>> Here's a more basic question: Should any MSR that can be read and
>>> written by a guest appear in KVM_GET_MSR_INDEX_LIST? If not, what's
>>> the point of this ioctl?
>> 
>> I think the point of KVM_GET_MSR_INDEX_LIST ioctl is for userspace to know what are all the MSR values it needs to save/restore on migration.
>> Therefore, any MSR that is exposed to guest read/write and isn’t determined on VM provisioning time, should be returned from this ioctl.
>> 
>> In contrast, KVM_GET_MSR_FEATURE_INDEX_LIST ioctl is meant to be used by userspace to query KVM capabilities based on host MSRs and KVM support and use that information to validate the CPU features that the user have requested to expose to guest.
>> 
>> For example, MSR_IA32_UCODE_REV is specified only in KVM_GET_MSR_FEATURE_INDEX_LIST. This is because it is determined by userspace on provisioning time (No need to save/restore on migration) and userspace may require to know it’s host value to define guest value appropriately. MSR_IA32_PERF_STATUS is not specified in neither ioctls because KVM returns constant value for it (not required to be saved/restored).
>> 
>> However, I’m also not sure about above mentioned definitions… As they are some bizarre things that seems to contradict it:
>> 1) MSR_IA32_ARCH_CAPABILITIES is specified in KVM_GET_MSR_FEATURE_INDEX_LIST to allow userspace to know which vulnerabilities apply to CPU. By default, vCPU MSR_IA32_ARCH_CAPABILITIES value will be set by host value (See kvm_arch_vcpu_setup()) but it’s possible for host userspace to override value exposed to guest (See kvm_set_msr_common()). *However*, it seems to me to be wrong that this MSR is specified in KVM_GET_MSR_INDEX_LIST as it should be determined in VM provisioning time and thus not need to be saved/restore on migration. i.e. How is it different from MSR_IA32_UCODE_REV?
>> 2) MSR_EFER should be saved/restored and thus returned by KVM_GET_MSR_INDEX_LIST. But it’s not. Probably because it can be saved/restored via KVM_{GET,SET}_SREGS but this is inconsistent with semantic definitions of KVM_GET_MSR_INDEX_LIST ioctl...
>> 3) MSR_AMD64_OSVW_ID_LENGTH & MSR_AMD64_OSVW_STATUS can be set by guest but it doesn’t seem to be specified in emulated_msrs[] and therefore not returned by KVM_GET_MSR_INDEX_LIST ioctl. I think this is a migration bug...
>> 
>> Unless someone disagrees, I think I will submit a patch for (1) and (3).
> 
> I assume that we're also skipping the x2APIC MSRs because they can be
> read/modified with KVM_{GET,SET}_LAPIC. At least until you start
> thinking about userspace instruction emulation.

Probably...

> 
> What about MSR_F15H_PERF_*, MSR_K7_EVNTSEL*, MSR_K7_PERFCTR*,
> MSR_MTRR*, HV_X64_MSR_STIMER[12]_CONFIG,
> HV_X64_MSR_STIMER[0123]_COUNT, MSR_VM_CR, and possibly others I'm
> missing on the first pass?

LOL, yes all of the above seem to be right...
This is indeed extremely error-prone. :\

-Liran

Re: KVM_GET_MSR_INDEX_LIST vs KVM_GET_MSR_FEATURE_INDEX_LIST

[Liran Alon]

> On 15 Nov 2019, at 3:14, Liran Alon <liran.alon@oracle.com> wrote:
> 
> 
> 
>> On 15 Nov 2019, at 3:05, Jim Mattson <jmattson@google.com> wrote:
>> 
>> On Thu, Nov 14, 2019 at 4:35 PM Liran Alon <liran.alon@oracle.com> wrote:
>>> 
>>> 
>>> 
>>>> On 15 Nov 2019, at 0:07, Jim Mattson <jmattson@google.com> wrote:
>>>> 
>>>> On Sat, Sep 8, 2018 at 5:58 PM Liran Alon <liran.alon@oracle.com> wrote:
>>>>> 
>>>>> 
>>>>>> On 7 Sep 2018, at 21:37, Jim Mattson <jmattson@google.com> wrote:
>>>>>> 
>>>>>> Are these two lists intended to be disjoint? Is it a bug that
>>>>>> IA32_ARCH_CAPABILITIES appears in both?
>>>> 
>>>> Here's a more basic question: Should any MSR that can be read and
>>>> written by a guest appear in KVM_GET_MSR_INDEX_LIST? If not, what's
>>>> the point of this ioctl?
>>> 
>>> I think the point of KVM_GET_MSR_INDEX_LIST ioctl is for userspace to know what are all the MSR values it needs to save/restore on migration.
>>> Therefore, any MSR that is exposed to guest read/write and isn’t determined on VM provisioning time, should be returned from this ioctl.
>>> 
>>> In contrast, KVM_GET_MSR_FEATURE_INDEX_LIST ioctl is meant to be used by userspace to query KVM capabilities based on host MSRs and KVM support and use that information to validate the CPU features that the user have requested to expose to guest.
>>> 
>>> For example, MSR_IA32_UCODE_REV is specified only in KVM_GET_MSR_FEATURE_INDEX_LIST. This is because it is determined by userspace on provisioning time (No need to save/restore on migration) and userspace may require to know it’s host value to define guest value appropriately. MSR_IA32_PERF_STATUS is not specified in neither ioctls because KVM returns constant value for it (not required to be saved/restored).
>>> 
>>> However, I’m also not sure about above mentioned definitions… As they are some bizarre things that seems to contradict it:
>>> 1) MSR_IA32_ARCH_CAPABILITIES is specified in KVM_GET_MSR_FEATURE_INDEX_LIST to allow userspace to know which vulnerabilities apply to CPU. By default, vCPU MSR_IA32_ARCH_CAPABILITIES value will be set by host value (See kvm_arch_vcpu_setup()) but it’s possible for host userspace to override value exposed to guest (See kvm_set_msr_common()). *However*, it seems to me to be wrong that this MSR is specified in KVM_GET_MSR_INDEX_LIST as it should be determined in VM provisioning time and thus not need to be saved/restore on migration. i.e. How is it different from MSR_IA32_UCODE_REV?
>>> 2) MSR_EFER should be saved/restored and thus returned by KVM_GET_MSR_INDEX_LIST. But it’s not. Probably because it can be saved/restored via KVM_{GET,SET}_SREGS but this is inconsistent with semantic definitions of KVM_GET_MSR_INDEX_LIST ioctl...
>>> 3) MSR_AMD64_OSVW_ID_LENGTH & MSR_AMD64_OSVW_STATUS can be set by guest but it doesn’t seem to be specified in emulated_msrs[] and therefore not returned by KVM_GET_MSR_INDEX_LIST ioctl. I think this is a migration bug...
>>> 
>>> Unless someone disagrees, I think I will submit a patch for (1) and (3).
>> 
>> I assume that we're also skipping the x2APIC MSRs because they can be
>> read/modified with KVM_{GET,SET}_LAPIC. At least until you start
>> thinking about userspace instruction emulation.
> 
> Probably...
> 
>> 
>> What about MSR_F15H_PERF_*, MSR_K7_EVNTSEL*, MSR_K7_PERFCTR*,
>> MSR_MTRR*, HV_X64_MSR_STIMER[12]_CONFIG,
>> HV_X64_MSR_STIMER[0123]_COUNT, MSR_VM_CR, and possibly others I'm
>> missing on the first pass?
> 
> LOL, yes all of the above seem to be right...
> This is indeed extremely error-prone. :\
> 
> -Liran
> 

BTW… Looking at QEMU code, one can see the following:
* If kvm_get_supported_msrs(), which invoke KVM_GET_MSR_INDEX_LIST, sees HV_X64_MSR_STIMER0_CONFIG returned, it signals a flag to QEMU that also all the rest of HV_X86_MSR_STIMER[0123] exists and needs to be saved/restored.
* Similarly, MSR_MTRR* are saved/restored in case guest is exposed with MTRR in CPUID[1].EDX. Regardless of KVM_GET_MSR_INDEX_LIST ioctl.

These all seem to be rather arbitrary decisions and I agree with your observation that it seems that these IOCTLs interface semantics are not well-defined or abused.

Paolo, what do you think?
Should we submit patches to fix above mentioned issues in KVM and clarify IOCTLs documentation? Should we change the interface?

-Liran

Re: KVM_GET_MSR_INDEX_LIST vs KVM_GET_MSR_FEATURE_INDEX_LIST

[Liran Alon]

+Paolo

> On 15 Nov 2019, at 3:27, Liran Alon <liran.alon@oracle.com> wrote:
> 
> 
> 
>> On 15 Nov 2019, at 3:14, Liran Alon <liran.alon@oracle.com> wrote:
>> 
>> 
>> 
>>> On 15 Nov 2019, at 3:05, Jim Mattson <jmattson@google.com> wrote:
>>> 
>>> On Thu, Nov 14, 2019 at 4:35 PM Liran Alon <liran.alon@oracle.com> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On 15 Nov 2019, at 0:07, Jim Mattson <jmattson@google.com> wrote:
>>>>> 
>>>>> On Sat, Sep 8, 2018 at 5:58 PM Liran Alon <liran.alon@oracle.com> wrote:
>>>>>> 
>>>>>> 
>>>>>>> On 7 Sep 2018, at 21:37, Jim Mattson <jmattson@google.com> wrote:
>>>>>>> 
>>>>>>> Are these two lists intended to be disjoint? Is it a bug that
>>>>>>> IA32_ARCH_CAPABILITIES appears in both?
>>>>> 
>>>>> Here's a more basic question: Should any MSR that can be read and
>>>>> written by a guest appear in KVM_GET_MSR_INDEX_LIST? If not, what's
>>>>> the point of this ioctl?
>>>> 
>>>> I think the point of KVM_GET_MSR_INDEX_LIST ioctl is for userspace to know what are all the MSR values it needs to save/restore on migration.
>>>> Therefore, any MSR that is exposed to guest read/write and isn’t determined on VM provisioning time, should be returned from this ioctl.
>>>> 
>>>> In contrast, KVM_GET_MSR_FEATURE_INDEX_LIST ioctl is meant to be used by userspace to query KVM capabilities based on host MSRs and KVM support and use that information to validate the CPU features that the user have requested to expose to guest.
>>>> 
>>>> For example, MSR_IA32_UCODE_REV is specified only in KVM_GET_MSR_FEATURE_INDEX_LIST. This is because it is determined by userspace on provisioning time (No need to save/restore on migration) and userspace may require to know it’s host value to define guest value appropriately. MSR_IA32_PERF_STATUS is not specified in neither ioctls because KVM returns constant value for it (not required to be saved/restored).
>>>> 
>>>> However, I’m also not sure about above mentioned definitions… As they are some bizarre things that seems to contradict it:
>>>> 1) MSR_IA32_ARCH_CAPABILITIES is specified in KVM_GET_MSR_FEATURE_INDEX_LIST to allow userspace to know which vulnerabilities apply to CPU. By default, vCPU MSR_IA32_ARCH_CAPABILITIES value will be set by host value (See kvm_arch_vcpu_setup()) but it’s possible for host userspace to override value exposed to guest (See kvm_set_msr_common()). *However*, it seems to me to be wrong that this MSR is specified in KVM_GET_MSR_INDEX_LIST as it should be determined in VM provisioning time and thus not need to be saved/restore on migration. i.e. How is it different from MSR_IA32_UCODE_REV?
>>>> 2) MSR_EFER should be saved/restored and thus returned by KVM_GET_MSR_INDEX_LIST. But it’s not. Probably because it can be saved/restored via KVM_{GET,SET}_SREGS but this is inconsistent with semantic definitions of KVM_GET_MSR_INDEX_LIST ioctl...
>>>> 3) MSR_AMD64_OSVW_ID_LENGTH & MSR_AMD64_OSVW_STATUS can be set by guest but it doesn’t seem to be specified in emulated_msrs[] and therefore not returned by KVM_GET_MSR_INDEX_LIST ioctl. I think this is a migration bug...
>>>> 
>>>> Unless someone disagrees, I think I will submit a patch for (1) and (3).
>>> 
>>> I assume that we're also skipping the x2APIC MSRs because they can be
>>> read/modified with KVM_{GET,SET}_LAPIC. At least until you start
>>> thinking about userspace instruction emulation.
>> 
>> Probably...
>> 
>>> 
>>> What about MSR_F15H_PERF_*, MSR_K7_EVNTSEL*, MSR_K7_PERFCTR*,
>>> MSR_MTRR*, HV_X64_MSR_STIMER[12]_CONFIG,
>>> HV_X64_MSR_STIMER[0123]_COUNT, MSR_VM_CR, and possibly others I'm
>>> missing on the first pass?
>> 
>> LOL, yes all of the above seem to be right...
>> This is indeed extremely error-prone. :\
>> 
>> -Liran
>> 
> 
> BTW… Looking at QEMU code, one can see the following:
> * If kvm_get_supported_msrs(), which invoke KVM_GET_MSR_INDEX_LIST, sees HV_X64_MSR_STIMER0_CONFIG returned, it signals a flag to QEMU that also all the rest of HV_X86_MSR_STIMER[0123] exists and needs to be saved/restored.
> * Similarly, MSR_MTRR* are saved/restored in case guest is exposed with MTRR in CPUID[1].EDX. Regardless of KVM_GET_MSR_INDEX_LIST ioctl.
> 
> These all seem to be rather arbitrary decisions and I agree with your observation that it seems that these IOCTLs interface semantics are not well-defined or abused.
> 
> Paolo, what do you think?
> Should we submit patches to fix above mentioned issues in KVM and clarify IOCTLs documentation? Should we change the interface?
> 
> -Liran
> 
> 
> 
> 
> 

Re: KVM_GET_MSR_INDEX_LIST vs KVM_GET_MSR_FEATURE_INDEX_LIST

[Liran Alon]

> On 15 Nov 2019, at 3:27, Liran Alon <liran.alon@oracle.com> wrote:
> 
> 
> 
>> On 15 Nov 2019, at 3:14, Liran Alon <liran.alon@oracle.com> wrote:
>> 
>> 
>> 
>>> On 15 Nov 2019, at 3:05, Jim Mattson <jmattson@google.com> wrote:
>>> 
>>> On Thu, Nov 14, 2019 at 4:35 PM Liran Alon <liran.alon@oracle.com> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On 15 Nov 2019, at 0:07, Jim Mattson <jmattson@google.com> wrote:
>>>>> 
>>>>> On Sat, Sep 8, 2018 at 5:58 PM Liran Alon <liran.alon@oracle.com> wrote:
>>>>>> 
>>>>>> 
>>>>>>> On 7 Sep 2018, at 21:37, Jim Mattson <jmattson@google.com> wrote:
>>>>>>> 
>>>>>>> Are these two lists intended to be disjoint? Is it a bug that
>>>>>>> IA32_ARCH_CAPABILITIES appears in both?
>>>>> 
>>>>> Here's a more basic question: Should any MSR that can be read and
>>>>> written by a guest appear in KVM_GET_MSR_INDEX_LIST? If not, what's
>>>>> the point of this ioctl?
>>>> 
>>>> I think the point of KVM_GET_MSR_INDEX_LIST ioctl is for userspace to know what are all the MSR values it needs to save/restore on migration.
>>>> Therefore, any MSR that is exposed to guest read/write and isn’t determined on VM provisioning time, should be returned from this ioctl.
>>>> 
>>>> In contrast, KVM_GET_MSR_FEATURE_INDEX_LIST ioctl is meant to be used by userspace to query KVM capabilities based on host MSRs and KVM support and use that information to validate the CPU features that the user have requested to expose to guest.
>>>> 
>>>> For example, MSR_IA32_UCODE_REV is specified only in KVM_GET_MSR_FEATURE_INDEX_LIST. This is because it is determined by userspace on provisioning time (No need to save/restore on migration) and userspace may require to know it’s host value to define guest value appropriately. MSR_IA32_PERF_STATUS is not specified in neither ioctls because KVM returns constant value for it (not required to be saved/restored).
>>>> 
>>>> However, I’m also not sure about above mentioned definitions… As they are some bizarre things that seems to contradict it:
>>>> 1) MSR_IA32_ARCH_CAPABILITIES is specified in KVM_GET_MSR_FEATURE_INDEX_LIST to allow userspace to know which vulnerabilities apply to CPU. By default, vCPU MSR_IA32_ARCH_CAPABILITIES value will be set by host value (See kvm_arch_vcpu_setup()) but it’s possible for host userspace to override value exposed to guest (See kvm_set_msr_common()). *However*, it seems to me to be wrong that this MSR is specified in KVM_GET_MSR_INDEX_LIST as it should be determined in VM provisioning time and thus not need to be saved/restore on migration. i.e. How is it different from MSR_IA32_UCODE_REV?
>>>> 2) MSR_EFER should be saved/restored and thus returned by KVM_GET_MSR_INDEX_LIST. But it’s not. Probably because it can be saved/restored via KVM_{GET,SET}_SREGS but this is inconsistent with semantic definitions of KVM_GET_MSR_INDEX_LIST ioctl...
>>>> 3) MSR_AMD64_OSVW_ID_LENGTH & MSR_AMD64_OSVW_STATUS can be set by guest but it doesn’t seem to be specified in emulated_msrs[] and therefore not returned by KVM_GET_MSR_INDEX_LIST ioctl. I think this is a migration bug...
>>>> 
>>>> Unless someone disagrees, I think I will submit a patch for (1) and (3).
>>> 
>>> I assume that we're also skipping the x2APIC MSRs because they can be
>>> read/modified with KVM_{GET,SET}_LAPIC. At least until you start
>>> thinking about userspace instruction emulation.
>> 
>> Probably...
>> 
>>> 
>>> What about MSR_F15H_PERF_*, MSR_K7_EVNTSEL*, MSR_K7_PERFCTR*,
>>> MSR_MTRR*, HV_X64_MSR_STIMER[12]_CONFIG,
>>> HV_X64_MSR_STIMER[0123]_COUNT, MSR_VM_CR, and possibly others I'm
>>> missing on the first pass?
>> 
>> LOL, yes all of the above seem to be right...
>> This is indeed extremely error-prone. :\
>> 
>> -Liran
>> 
> 
> BTW… Looking at QEMU code, one can see the following:
> * If kvm_get_supported_msrs(), which invoke KVM_GET_MSR_INDEX_LIST, sees HV_X64_MSR_STIMER0_CONFIG returned, it signals a flag to QEMU that also all the rest of HV_X86_MSR_STIMER[0123] exists and needs to be saved/restored.
> * Similarly, MSR_MTRR* are saved/restored in case guest is exposed with MTRR in CPUID[1].EDX. Regardless of KVM_GET_MSR_INDEX_LIST ioctl.
> 
> These all seem to be rather arbitrary decisions and I agree with your observation that it seems that these IOCTLs interface semantics are not well-defined or abused.
> 
> Paolo, what do you think?
> Should we submit patches to fix above mentioned issues in KVM and clarify IOCTLs documentation? Should we change the interface?
> 
> -Liran
> 

Paolo, ping?