Maxim Levitsky <mlevitsk@xxxxxxxxxx> writes:
> On Wed, 2020-05-20 at 18:33 +0200, Vitaly Kuznetsov wrote:
>> Maxim Levitsky <mlevitsk@xxxxxxxxxx> writes:
>>
>> > This msr is only available when the host supports WAITPKG feature.
>> >
>> > This breaks a nested guest, if the L1 hypervisor is set to ignore
>> > unknown msrs, because the only other safety check that the
>> > kernel does is that it attempts to read the msr and
>> > rejects it if it gets an exception.
>> >
>> > Fixes: 6e3ba4abce KVM: vmx: Emulate MSR IA32_UMWAIT_CONTROL
>> >
>> > Signed-off-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
>> > ---
>> > arch/x86/kvm/x86.c | 4 ++++
>> > 1 file changed, 4 insertions(+)
>> >
>> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> > index fe3a24fd6b263..9c507b32b1b77 100644
>> > --- a/arch/x86/kvm/x86.c
>> > +++ b/arch/x86/kvm/x86.c
>> > @@ -5314,6 +5314,10 @@ static void kvm_init_msr_list(void)
>> > if (msrs_to_save_all[i] - MSR_ARCH_PERFMON_EVENTSEL0 >=
>> > min(INTEL_PMC_MAX_GENERIC, x86_pmu.num_counters_gp))
>> > continue;
>> > + break;
>> > + case MSR_IA32_UMWAIT_CONTROL:
>> > + if (!kvm_cpu_cap_has(X86_FEATURE_WAITPKG))
>> > + continue;
>>
>> I'm probably missing something but (if I understand correctly) the only
>> effect of dropping MSR_IA32_UMWAIT_CONTROL from msrs_to_save would be
>> that KVM userspace won't see it in e.g. KVM_GET_MSR_INDEX_LIST. But why
>> is this causing an issue? I see both vmx_get_msr()/vmx_set_msr() have
>> 'host_initiated' check:
>>
>> case MSR_IA32_UMWAIT_CONTROL:
>> if (!msr_info->host_initiated && !vmx_has_waitpkg(vmx))
>> return 1;
>
> Here it fails like that:
>
> 1. KVM_GET_MSR_INDEX_LIST returns this msrs, and qemu notes that
> it is supported in 'has_msr_umwait' global var
>
> 2. Qemu does kvm_arch_get/put_registers->kvm_get/put_msrs->ioctl(KVM_GET_MSRS)
> and while doing this it adds MSR_IA32_UMWAIT_CONTROL to that msr list.
> That reaches 'svm_get_msr', and this one knows nothing about MSR_IA32_UMWAIT_CONTROL.
>
> So the difference here is that vmx_get_msr not called at all.
> I can add this msr to svm_get_msr instead but that feels wrong since this feature
> is not yet supported on AMD.
> When AMD adds support for this feature, then the VMX specific code can be moved to
> kvm_get_msr_common I guess.
>
>
Oh, SVM, I missed that completely)
>
>>
>> so KVM userspace should be able to read/write this MSR even when there's
>> no hardware support for it. Or who's trying to read/write it?
>>
>> Also, kvm_cpu_cap_has() check is not equal to vmx_has_waitpkg() which
>> checks secondary execution controls.
>
> I was afraid that something like that will happen, but in this particular
> case we can only check CPUID support and if supported, the then it means
> we are dealing with intel system and thus vmx_get_msr will be called and
> ignore that msr.
>
> Calling vmx_has_waitpkg from the common code doesn't seem right, and besides,
> it checks the secondary controls which are set by the host and can change,
> at least in theory during runtime (I don't know if KVM does this).
>
> Note that if I now understand correctly, the 'host_initiated' means
> that MSR read/write is done by the host itself and not on behalf of the guest.
Yes, it does that.
We have kvm_x86_ops.has_emulated_msr() mechanism, can we use it here?
E.g. completely untested
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 38f6aeefeb55..c19a9542e6c3 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3471,6 +3471,8 @@ static bool svm_has_emulated_msr(int index)
case MSR_IA32_MCG_EXT_CTL:
case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
return false;
+ case MSR_IA32_UMWAIT_CONTROL:
+ return false;
default:
break;
}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d786c7d27ce5..f45153ef3b81 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1183,7 +1183,6 @@ static const u32 msrs_to_save_all[] = {
MSR_IA32_RTIT_ADDR1_A, MSR_IA32_RTIT_ADDR1_B,
MSR_IA32_RTIT_ADDR2_A, MSR_IA32_RTIT_ADDR2_B,
MSR_IA32_RTIT_ADDR3_A, MSR_IA32_RTIT_ADDR3_B,
- MSR_IA32_UMWAIT_CONTROL,
MSR_ARCH_PERFMON_FIXED_CTR0, MSR_ARCH_PERFMON_FIXED_CTR1,
MSR_ARCH_PERFMON_FIXED_CTR0 + 2, MSR_ARCH_PERFMON_FIXED_CTR0 + 3,
@@ -1266,6 +1265,7 @@ static const u32 emulated_msrs_all[] = {
MSR_IA32_VMX_PROCBASED_CTLS2,
MSR_IA32_VMX_EPT_VPID_CAP,
MSR_IA32_VMX_VMFUNC,
+ MSR_IA32_UMWAIT_CONTROL,
MSR_K7_HWCR,
MSR_KVM_POLL_CONTROL,
--
Vitaly
