| Message ID | 1478607416-25813-1-git-send-email-rcojocaru@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 08.11.16 at 13:16, <rcojocaru@bitdefender.com> wrote: > xc_hvm_inject_trap() sets v->arch.hvm_vcpu.inject_trap.vector, > which is then checked in hvm_do_resume(), and if != -1, a trap > is injected, regardless of whether vmx_idtv_reinject() has written > VM_ENTRY_INTR_INFO directly. If that's the case, the toolstack > injected interrupt will overwrite the reinjected one, which will > get lost forever. This patch returns -EBUSY not only if > v->arch.hvm_vcpu.inject_trap.vector != -1, but also if > hvm_event_pending(v). Considering the earlier discussion I don't understand why this was put together and submitted: It is my understanding that it won't cover all possible cases of actual injection not succeeding, and I can't see what good a partial solution will do for you or anyone else. Jan
On 11/08/2016 03:10 PM, Jan Beulich wrote: >>>> On 08.11.16 at 13:16, <rcojocaru@bitdefender.com> wrote: >> xc_hvm_inject_trap() sets v->arch.hvm_vcpu.inject_trap.vector, >> which is then checked in hvm_do_resume(), and if != -1, a trap >> is injected, regardless of whether vmx_idtv_reinject() has written >> VM_ENTRY_INTR_INFO directly. If that's the case, the toolstack >> injected interrupt will overwrite the reinjected one, which will >> get lost forever. This patch returns -EBUSY not only if >> v->arch.hvm_vcpu.inject_trap.vector != -1, but also if >> hvm_event_pending(v). > > Considering the earlier discussion I don't understand why this was > put together and submitted: It is my understanding that it won't > cover all possible cases of actual injection not succeeding, and I > can't see what good a partial solution will do for you or anyone > else. Well, the previous discussion also moved quite a bit through proposed solutions, and this is the smallest patch that helps. We're always injecting traps when the VCPU is paused (i.e. when handling a sync vm_event), and at that time I believe that xc_hvm_inject_trap() succeeding with this patch would guarantee that the interrupt can be delivered, and it failing immediately will notify us that injection is not currently possible. So for the flow which prompted the discussion, this is a solution. Also, the patch is RFC - I thought seeing a concrete patch would help, rather than going on with more abstract concerns. I'll go back to the vm_event patch then. Thanks, Razvan
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 704fd64..cf01ae4 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -5938,7 +5938,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg) if ( tr.vcpuid >= d->max_vcpus || (v = d->vcpu[tr.vcpuid]) == NULL ) goto injtrap_fail; - if ( v->arch.hvm_vcpu.inject_trap.vector != -1 ) + if ( v->arch.hvm_vcpu.inject_trap.vector != -1 || hvm_event_pending(v) ) rc = -EBUSY; else { diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 9a8f694..f50a593 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -1790,8 +1790,9 @@ static int vmx_event_pending(struct vcpu *v) { unsigned long intr_info; - ASSERT(v == current); + vmx_vmcs_enter(v); __vmread(VM_ENTRY_INTR_INFO, &intr_info); + vmx_vmcs_exit(v); return intr_info & INTR_INFO_VALID_MASK; }
xc_hvm_inject_trap() sets v->arch.hvm_vcpu.inject_trap.vector, which is then checked in hvm_do_resume(), and if != -1, a trap is injected, regardless of whether vmx_idtv_reinject() has written VM_ENTRY_INTR_INFO directly. If that's the case, the toolstack injected interrupt will overwrite the reinjected one, which will get lost forever. This patch returns -EBUSY not only if v->arch.hvm_vcpu.inject_trap.vector != -1, but also if hvm_event_pending(v). hvm_event_pending() has also been modified to be able to run on a VCPU which is not current. Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> --- xen/arch/x86/hvm/hvm.c | 2 +- xen/arch/x86/hvm/vmx/vmx.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-)