> On 15 Nov 2019, at 3:27, Liran Alon <liran.alon@xxxxxxxxxx> wrote:
>
>
>
>> On 15 Nov 2019, at 3:14, Liran Alon <liran.alon@xxxxxxxxxx> wrote:
>>
>>
>>
>>> On 15 Nov 2019, at 3:05, Jim Mattson <jmattson@xxxxxxxxxx> wrote:
>>>
>>> On Thu, Nov 14, 2019 at 4:35 PM Liran Alon <liran.alon@xxxxxxxxxx> wrote:
>>>>
>>>>
>>>>
>>>>> On 15 Nov 2019, at 0:07, Jim Mattson <jmattson@xxxxxxxxxx> wrote:
>>>>>
>>>>> On Sat, Sep 8, 2018 at 5:58 PM Liran Alon <liran.alon@xxxxxxxxxx> wrote:
>>>>>>
>>>>>>
>>>>>>> On 7 Sep 2018, at 21:37, Jim Mattson <jmattson@xxxxxxxxxx> 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?
