| Message ID | 20200128092715.69429-4-oupton@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Handle monitor trap flag during instruction emulation | expand |
On Tue, Jan 28, 2020 at 01:27:13AM -0800, Oliver Upton wrote: > KVM doesn't utilize exception payloads by default, as this behavior > diverges from the expectations of the userspace API. However, this > constraint only applies if KVM is servicing a KVM_GET_VCPU_EVENTS ioctl > before delivering the exception. > > Use exception payloads unconditionally if the vcpu is in guest mode. This sentence is super confusing. It doesn't align with the code, which is clearly handling "not is in guest mode". And KVM already uses payloads unconditionally, it's only the deferring behavior that is changing. > Deliver the exception payload before completing a KVM_GET_VCPU_EVENTS > to ensure API compatibility. > > Signed-off-by: Oliver Upton <oupton@google.com> > --- > arch/x86/kvm/x86.c | 29 ++++++++++++++++------------- > 1 file changed, 16 insertions(+), 13 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 7a341c0c978a..9f080101618c 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -497,19 +497,7 @@ static void kvm_multiple_exception(struct kvm_vcpu *vcpu, > vcpu->arch.exception.error_code = error_code; > vcpu->arch.exception.has_payload = has_payload; > vcpu->arch.exception.payload = payload; > - /* > - * In guest mode, payload delivery should be deferred, > - * so that the L1 hypervisor can intercept #PF before > - * CR2 is modified (or intercept #DB before DR6 is > - * modified under nVMX). However, for ABI > - * compatibility with KVM_GET_VCPU_EVENTS and > - * KVM_SET_VCPU_EVENTS, we can't delay payload > - * delivery unless userspace has enabled this > - * functionality via the per-VM capability, > - * KVM_CAP_EXCEPTION_PAYLOAD. > - */ > - if (!vcpu->kvm->arch.exception_payload_enabled || > - !is_guest_mode(vcpu)) > + if (!is_guest_mode(vcpu)) > kvm_deliver_exception_payload(vcpu); > return; > } > @@ -3790,6 +3778,21 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu, > { > process_nmi(vcpu); > > + /* > + * In guest mode, payload delivery should be deferred, > + * so that the L1 hypervisor can intercept #PF before > + * CR2 is modified (or intercept #DB before DR6 is > + * modified under nVMX). However, for ABI > + * compatibility with KVM_GET_VCPU_EVENTS and > + * KVM_SET_VCPU_EVENTS, we can't delay payload > + * delivery unless userspace has enabled this > + * functionality via the per-VM capability, > + * KVM_CAP_EXCEPTION_PAYLOAD. > + */ This comment needs to be rewritten. It makes sense in the context of kvm_multiple_exception(), here it's just confusing. > + if (!vcpu->kvm->arch.exception_payload_enabled && > + vcpu->arch.exception.pending && vcpu->arch.exception.has_payload) > + kvm_deliver_exception_payload(vcpu); Crushing CR2/DR6 just because userspace is retrieving info can't possibly be correct. If it somehow is correct then this needs big fat comment. What is the ABI compatibility issue? E.g. can KVM simply hide the payload info on KVM_GET_VCPU_EVENT and then drop it on KVM_SET_VCPU_EVENTS? > + > /* > * The API doesn't provide the instruction length for software > * exceptions, so don't report them. As long as the guest RIP > -- > 2.25.0.341.g760bfbb309-goog >
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 7a341c0c978a..9f080101618c 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -497,19 +497,7 @@ static void kvm_multiple_exception(struct kvm_vcpu *vcpu, vcpu->arch.exception.error_code = error_code; vcpu->arch.exception.has_payload = has_payload; vcpu->arch.exception.payload = payload; - /* - * In guest mode, payload delivery should be deferred, - * so that the L1 hypervisor can intercept #PF before - * CR2 is modified (or intercept #DB before DR6 is - * modified under nVMX). However, for ABI - * compatibility with KVM_GET_VCPU_EVENTS and - * KVM_SET_VCPU_EVENTS, we can't delay payload - * delivery unless userspace has enabled this - * functionality via the per-VM capability, - * KVM_CAP_EXCEPTION_PAYLOAD. - */ - if (!vcpu->kvm->arch.exception_payload_enabled || - !is_guest_mode(vcpu)) + if (!is_guest_mode(vcpu)) kvm_deliver_exception_payload(vcpu); return; } @@ -3790,6 +3778,21 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu, { process_nmi(vcpu); + /* + * In guest mode, payload delivery should be deferred, + * so that the L1 hypervisor can intercept #PF before + * CR2 is modified (or intercept #DB before DR6 is + * modified under nVMX). However, for ABI + * compatibility with KVM_GET_VCPU_EVENTS and + * KVM_SET_VCPU_EVENTS, we can't delay payload + * delivery unless userspace has enabled this + * functionality via the per-VM capability, + * KVM_CAP_EXCEPTION_PAYLOAD. + */ + if (!vcpu->kvm->arch.exception_payload_enabled && + vcpu->arch.exception.pending && vcpu->arch.exception.has_payload) + kvm_deliver_exception_payload(vcpu); + /* * The API doesn't provide the instruction length for software * exceptions, so don't report them. As long as the guest RIP
KVM doesn't utilize exception payloads by default, as this behavior diverges from the expectations of the userspace API. However, this constraint only applies if KVM is servicing a KVM_GET_VCPU_EVENTS ioctl before delivering the exception. Use exception payloads unconditionally if the vcpu is in guest mode. Deliver the exception payload before completing a KVM_GET_VCPU_EVENTS to ensure API compatibility. Signed-off-by: Oliver Upton <oupton@google.com> --- arch/x86/kvm/x86.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-)