Re: [PATCH 3/4] KVM: nVMX: accurate emulation of MSR_IA32_CR{0,4}_FIXED1

[David Matlack]

On Wed, Nov 23, 2016 at 11:24 AM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
>
>
> On 23/11/2016 20:16, David Matlack wrote:
>> > Oh, I thought userspace would do that!  Doing it in KVM is fine as well,
>> > but then do we need to give userspace access to CR{0,4}_FIXED{0,1} at all?
>>
>> I think it should be safe for userspace to skip restoring CR4_FIXED1,
>> since it is 100% generated based on CPUID. But I'd prefer to keep it
>> accessible from userspace, for consistency with the other VMX MSRs and
>> for flexibility. The auditing should ensure userspace doesn't restore
>> a CR4_FIXED1 that is inconsistent with CPUID.
>
> Or would it just allow userspace to put anything into it, even if it's
> inconsistent with CPUID, as long as it's consistent with the host?

It would not allow anything inconsistent with guest CPUID. The
auditing on restore of CR4_FIXED1 compares the new value with
vmx->nested.nested_vmx_cr4_fixed1, which is updated as part of setting
the guest's CPUID.

>
>> Userspace should restore CR0_FIXED1 in case future CPUs change which
>> bits of CR0 are valid in VMX operation. Userspace should also restore
>> CR{0,4}_FIXED0 so we have the flexibility to change the defaults in
>> KVM. Both of these situations seem unlikely but we might as well play
>> it safe, the cost is small.
>
> I disagree, there is always a cost.  Besides the fact that it's
> unlikely that there'll be any future CR0 bits at all, any changes would
> most likely be keyed by a new CPUID bit (the same as CR4) or execution
> control (the same as unrestricted guest).

That's true. So CR0_FIXED1 would not need to be accessible from
userspace either. This patch would need to be a little different then:
vmx_cpuid_update should also update vmx->nested.nested_vmx_cr0_fixed1
to 0xffffffff.

A downside of this scheme is we'd have to remember to update
nested_vmx_cr4_fixed1_update() before giving VMs new CPUID bits. If we
forget, a VM could end up with different values for CR{0,4}_FIXED0 for
the same CPUID depending on which version of KVM you're running on.

Hm, now I'm thinking you were right in the beginning. Userspace should
generate CR{0,4}_FIXED1, not the kernel. And KVM should allow
userspace to save/restore them.

>
> In the end, since we assume that userspace (any) has no idea of what to
> do with it, I see no good reason to make the MSRs available.
>
> Paolo