On 20.08.20 20:17, Jim Mattson wrote:
On Tue, Aug 18, 2020 at 2:16 PM Aaron Lewis <aaronlewis@xxxxxxxxxx> wrote:Add support for exiting to userspace on a rdmsr or wrmsr instruction if the MSR being read from or written to is in the user_exit_msrs list. Signed-off-by: Aaron Lewis <aaronlewis@xxxxxxxxxx> --- v2 -> v3 - Refactored commit based on Alexander Graf's changes in the first commit in this series. Changes made were: - Updated member 'inject_gp' to 'error' based on struct msr in kvm_run. - Move flag 'vcpu->kvm->arch.user_space_msr_enabled' out of kvm_msr_user_space() to allow it to work with both methods that bounce to userspace (msr list and #GP fallback). Updated caller functions to account for this change. - trace_kvm_msr has been moved up and combine with a previous call in complete_emulated_msr() based on the suggestion made by Alexander Graf <graf@xxxxxxxxxx>. ---@@ -1653,9 +1663,6 @@ static int kvm_msr_user_space(struct kvm_vcpu *vcpu, u32 index, u32 exit_reason, u64 data, int (*completion)(struct kvm_vcpu *vcpu)) { - if (!vcpu->kvm->arch.user_space_msr_enabled) - return 0; - vcpu->run->exit_reason = exit_reason; vcpu->run->msr.error = 0; vcpu->run->msr.pad[0] = 0; @@ -1686,10 +1693,18 @@ int kvm_emulate_rdmsr(struct kvm_vcpu *vcpu) u64 data; int r; + if (kvm_msr_user_exit(vcpu->kvm, ecx)) { + kvm_get_msr_user_space(vcpu, ecx); + /* Bounce to user space */ + return 0; + } + + r = kvm_get_msr(vcpu, ecx, &data); /* MSR read failed? See if we should ask user space */ - if (r && kvm_get_msr_user_space(vcpu, ecx)) { + if (r && vcpu->kvm->arch.user_space_msr_enabled) { + kvm_get_msr_user_space(vcpu, ecx); /* Bounce to user space */ return 0; }The before and after bounce to userspace is unfortunate. If we can consolidate the allow/deny list checks at the top of kvm_get_msr(), and we can tell why kvm_get_msr() failed (e.g. -EPERM=disallowed, -ENOENT=unknown MSR, -EINVAL=illegal access), then we can eliminate the first bounce to userspace above. -EPERM would always go to userspace. -ENOENT would go to userspace if userspace asked to handle unknown MSRs. -EINVAL would go to userspace if userspace asked to handle all #GPs. (Yes; I'd still like to be able to distinguish between "unknown MSR" and "illegal value." Otherwise, it seems impossible for userspace to know how to proceed.) (You may ask, "why would you get -EINVAL on a RDMSR?" This would be the case if you tried to read a write-only MSR, like IA32_FLUSH_CMD.)
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.
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
