Re: [PATCH] KVM: x86: Deflect unknown MSR accesses to user space

[Alexander Graf <graf@amazon.com>]

On 29.07.20 11:22, Vitaly Kuznetsov wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> Alexander Graf <graf@amazon.com> writes:
> 
>> On 29.07.20 10:23, Vitaly Kuznetsov wrote:
>>>
>>>
>>>
>>> Jim Mattson <jmattson@google.com> writes:
>>>
>>>> On Tue, Jul 28, 2020 at 5:41 AM Alexander Graf <graf@amazon.com> wrote:
>>>>>
>>>
>>> ...
>>>
>>>>> While it does feel a bit overengineered, it would solve the problem that
>>>>> we're turning in-KVM handled MSRs into an ABI.
>>>>
>>>> It seems unlikely that userspace is going to know what to do with a
>>>> large number of MSRs. I suspect that a small enumerated list will
>>>> suffice.
>>>
>>> The list can also be 'wildcarded', i.e.
>>> {
>>>    u32 index;
>>>    u32 mask;
>>>    ...
>>> }
>>>
>>> to make it really short.
>>
>> I like the idea of wildcards, but I can't quite wrap my head around how
>> we would implement ignore_msrs in user space with them?
>>
> 
> For that I think we can still deflect all unknown MSR accesses to
> userspace (when the CAP is enabled of course ) but MSRs which are on the
> list will *have to be deflected*, i.e. KVM can't handle them internally
> without consulting with userspace.
> 
> We can make it tunable through a parameter for CAP enablement if needed.

That would still make the set of MSRs implemented in KVM a de-facto ABI, no?

Another thing that might be worth bringing up here is that we have an 
in-house mechanism to set up a allowlist for KVM handling MSR accesses. 
What if we combine the two?

int kvm_rdmsr(...)
{
     switch (msr) {
     [...]
     default:
         return -ENOENT;
     }
}

int rdmsr(...) {
     if (!has_allowlist || msr_read_is_allowed(msr))
         return kvm_rdmsr();

     return -ENOENT;
}

int handle_rdmsr(...)
{
     switch (rdmsr(msr)) {
     case 0:
         return 1;
     case 1:
         inject_gp();
         return 1;
     case -ENOENT:
         if (cap_msr_exit) {
             run->exit_reason = MSR;
             return 0;
         } else {
             inject_gp();
             return 1;
         }
     }
}

That way user space can either say "I don't care what you implement, 
just tell me all the MSRs you could not handle" or it says "I want you 
to handle this exact subset of MSRs, tell me any time there's an out of 
bounds access".

That would give us the best of both worlds, right?


Alex



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879