Re: [PATCH v3 05/12] KVM: x86: Add support for exiting to userspace on rdmsr or wrmsr

From: Alexander Graf <graf@amazon.com>
To: Jim Mattson <jmattson@google.com>
Cc: Aaron Lewis <aaronlewis@google.com>,
	Peter Shier <pshier@google.com>, Oliver Upton <oupton@google.com>,
	kvm list <kvm@vger.kernel.org>
Subject: Re: [PATCH v3 05/12] KVM: x86: Add support for exiting to userspace on rdmsr or wrmsr
Date: Mon, 24 Aug 2020 20:09:08 +0200	[thread overview]
Message-ID: <8851792a-d9f2-fe0c-33e7-cae7f3bb3919@amazon.com> (raw)
In-Reply-To: <CALMp9eQd4cmK2_2oEnTX7VUEA0N9gsEkdpKhLyWpQzWCQm4w-w@mail.gmail.com>

On 24.08.20 19:23, Jim Mattson wrote:
> 
> On Sun, Aug 23, 2020 at 6:35 PM Alexander Graf <graf@amazon.com> wrote:
>>
>>
>>
>> On 21.08.20 19:58, Jim Mattson wrote:
>>>
>>> On Thu, Aug 20, 2020 at 3:55 PM Jim Mattson <jmattson@google.com> wrote:
>>>>
>>>> On Thu, Aug 20, 2020 at 2:59 PM Alexander Graf <graf@amazon.com> wrote:
>>>>
>>>>> Do we really need to do all of this dance of differentiating in kernel
>>>>> space between an exit that's there because user space asked for the exit
>>>>> and an MSR access that would just generate a #GP?
>>>>>
>>>>> At the end of the day, user space *knows* which MSRs it asked to
>>>>> receive. It can filter for them super easily.
>>>>
>>>> If no one else has an opinion, I can let this go. :-)
>>>>
>>>> However, to make the right decision in kvm_emulate_{rdmsr,wrmsr}
>>>> (without the unfortunate before and after checks that Aaron added),
>>>> kvm_{get,set}_msr should at least distinguish between "permission
>>>> denied" and "raise #GP," so I can provide a deny list without asking
>>>> for userspace exits on #GP.
>>>
>>> Actually, I think this whole discussion is moot. You no longer need
>>> the first ioctl (ask for a userspace exit on #GP). The allow/deny list
>>> is sufficient. Moreover, the allow/deny list checks can be in
>>> kvm_emulate_{rdmsr,wrmsr} before the call to kvm_{get,set}_msr, so we
>>> needn't be concerned with distinguishable error values either.
>>>
>>
>> I also care about cases where I allow in-kernel handling, but for
>> whatever reason there still would be a #GP injected into the guest. I
>> want to record those events and be able to later have data that tell me
>> why something went wrong.
>>
>> So yes, for your use case you do not care about the distinction between
>> "deny MSR access" and "report invalid MSR access". However, I do care :).
> 
> In that case, I'm going to continue to hold a hard line on the
> distinction between a #GP for an invalid MSR access and the #GP for an
> unknown MSR. If, for instance, you wanted to implement ignore_msrs in
> userspace, as you've proposed in the past, this would be extremely
> helpful. Without it, userspace gets an exit because (1) the MSR access
> isn't in the allow list, (2) the MSR access is invalid, or (3) the MSR
> is unknown to kvm. As you've pointed out, it is easy for userspace to
> distinguish (1) from the others, since it provided the allow/deny list
> in the first place. But how do you distinguish (2) from (3) without
> replicating the logic in the kernel?
> 
>> My stance on this is again that it's trivial to handle a few invalid MSR
>> #GPs from user space and just not report anything. It should come at
>> almost negligible performance cost, no?
> 
> Yes, the performance cost should be negligible, but what is the point?
> We're trying to design a good API here, aren't we?
> 
>> As for your argumentation above, we have a second call chain into
>> kvm_{get,set}_msr from the x86 emulator which you'd also need to cover.
>>
>> One thing we could do I guess is to add a parameter to ENABLE_CAP on
>> KVM_CAP_X86_USER_SPACE_MSR so that it only bounces on certain return
>> values, such as -ENOENT. I still fail to see cases where that's
>> genuinely beneficial though.
> 
> I'd like to see two completely independent APIs, so that I can just
> request a bounce on -EPERM through a deny list.  I think it's useful

Where would that bounce to? Which user space event does that trigger? 
Yet another one? Wouldn't 4 exit reasons just for MSR traps be a bit 
much? :)

> to distinguish between -ENOENT and -EINVAL, but I have no issues wih
> both causing an exit to userspace, if userspace has requested exits on
> MSR #GPs.

So imagine we took the first argument to ENABLE_CAP as filter:

   (1<<0) REPORT_ENOENT
   (1<<1) REPORT_EINVAL
   (1<<2) REPORT_EPERM
   (1<<31) REPORT_ANY

Then we also add the reason to the kvm_run exit response and user space 
can differentiate easily between the different events.

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