| Message ID | 1478684526-3678-1-git-send-email-rcojocaru@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 09.11.16 at 10:42, <rcojocaru@bitdefender.com> wrote: > Added support for a new event type, VM_EVENT_REASON_INTERRUPT, > which is now fired in a one-shot manner when enabled via the new > VM_EVENT_FLAG_GET_NEXT_INTERRUPT vm_event response flag. > The patch also fixes the behaviour of the xc_hvm_inject_trap() > hypercall, which would lead to non-architectural interrupts > overwriting pending (specifically reinjected) architectural ones. Looks quite okay, just some more or less mechanical comments. > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -532,11 +532,23 @@ void hvm_do_resume(struct vcpu *v) > } > } > > - /* Inject pending hw/sw trap */ > - if ( v->arch.hvm_vcpu.inject_trap.vector != -1 ) > - { > + /* Inject pending hw/sw trap if there are no other pending interrupts. */ > + if ( v->arch.hvm_vcpu.inject_trap.vector != -1 && !hvm_event_pending(v) ) > hvm_inject_trap(&v->arch.hvm_vcpu.inject_trap); > - v->arch.hvm_vcpu.inject_trap.vector = -1; > + > + v->arch.hvm_vcpu.inject_trap.vector = -1; I don't see why you pull this out of the if() body. > + if ( unlikely(v->arch.vm_event) && > + v->arch.vm_event->monitor_next_interrupt ) Hard tab. > --- a/xen/arch/x86/hvm/monitor.c > +++ b/xen/arch/x86/hvm/monitor.c > @@ -150,6 +150,21 @@ int hvm_monitor_cpuid(unsigned long insn_length, unsigned int leaf, > return monitor_traps(curr, 1, &req); > } > > +void hvm_monitor_interrupt(unsigned int vector, unsigned int type, > + unsigned int err, uint64_t cr2) > +{ > + struct vcpu *curr = current; Pointless local variable (used just once). > --- a/xen/arch/x86/hvm/svm/svm.c > +++ b/xen/arch/x86/hvm/svm/svm.c > @@ -2220,6 +2220,21 @@ static void svm_invlpg(struct vcpu *v, unsigned long vaddr) > svm_asid_g_invlpg(v, vaddr); > } > > +static bool svm_get_pending_event(struct vcpu *v, struct hvm_trap *info) > +{ > + struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; > + > + if ( vmcb->eventinj.fields.v ) > + return false; > + > + info->vector = vmcb->eventinj.fields.vector; > + info->type = vmcb->eventinj.fields.type; > + info->error_code = vmcb->eventinj.fields.errorcode; > + info->cr2 = v->arch.hvm_vcpu.guest_cr[2]; I'd prefer for this last part to be put into generic code (i.e. the wrapper). > --- a/xen/include/asm-arm/vm_event.h > +++ b/xen/include/asm-arm/vm_event.h > @@ -52,4 +52,10 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp) > /* Not supported on ARM. */ > } > > +static inline > +void vm_event_monitor_next_interrupt(struct vcpu *v) > +{ > + /* Not supported on ARM. */ > +} That's unfortunate. If it can't be implemented, shouldn't the caller at least be advised of this being unavailable? Wasn't there even some mechanism to report capabilities? > --- a/xen/include/asm-x86/hvm/hvm.h > +++ b/xen/include/asm-x86/hvm/hvm.h > @@ -237,6 +237,8 @@ struct hvm_function_table { > /* Architecture function to setup TSC scaling ratio */ > void (*setup)(struct vcpu *v); > } tsc_scaling; > + > + bool (*get_pending_event)(struct vcpu *v, struct hvm_trap *info); > }; Stylistically I think this would better go a little earlier. > --- a/xen/include/asm-x86/vm_event.h > +++ b/xen/include/asm-x86/vm_event.h > @@ -32,6 +32,7 @@ struct arch_vm_event { > struct vm_event_emul_insn_data insn; > } emul; > struct monitor_write_data write_data; > + bool monitor_next_interrupt; > }; I think there's a 32-bit padding hole before write_data, so the new field would better go earlier (perhaps even right after flags). > @@ -139,6 +144,8 @@ > * These kinds of events will be filtered out in future versions. > */ > #define VM_EVENT_REASON_PRIVILEGED_CALL 11 > +/* Result of toolstack-requested (non-architectural) trap injection. */ > +#define VM_EVENT_REASON_INTERRUPT 12 Considering the event reports all kinds of interruptions, I don't think the comment is appropriate. > @@ -259,6 +266,13 @@ struct vm_event_cpuid { > uint32_t _pad; > }; > > +struct vm_event_interrupt { > + uint32_t vector; > + uint32_t type; > + uint32_t error_code; > + uint64_t cr2; > +}; This being x86-specific, I think it should be named or union-ized accordingly. Jan
On 11/09/2016 01:17 PM, Jan Beulich wrote: >>>> On 09.11.16 at 10:42, <rcojocaru@bitdefender.com> wrote: >> Added support for a new event type, VM_EVENT_REASON_INTERRUPT, >> which is now fired in a one-shot manner when enabled via the new >> VM_EVENT_FLAG_GET_NEXT_INTERRUPT vm_event response flag. >> The patch also fixes the behaviour of the xc_hvm_inject_trap() >> hypercall, which would lead to non-architectural interrupts >> overwriting pending (specifically reinjected) architectural ones. > > Looks quite okay, just some more or less mechanical comments. > >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -532,11 +532,23 @@ void hvm_do_resume(struct vcpu *v) >> } >> } >> >> - /* Inject pending hw/sw trap */ >> - if ( v->arch.hvm_vcpu.inject_trap.vector != -1 ) >> - { >> + /* Inject pending hw/sw trap if there are no other pending interrupts. */ >> + if ( v->arch.hvm_vcpu.inject_trap.vector != -1 && !hvm_event_pending(v) ) >> hvm_inject_trap(&v->arch.hvm_vcpu.inject_trap); >> - v->arch.hvm_vcpu.inject_trap.vector = -1; >> + >> + v->arch.hvm_vcpu.inject_trap.vector = -1; > > I don't see why you pull this out of the if() body. That is intended, and covered by the "the patch also fixes the behaviour of the xc_hvm_inject_trap() hypercall, which would lead to non-architectural interrupts overwriting pending (specifically reinjected) architectural ones" part of the patch description. If we couldn't inject the trap because there was a pending event (i.e. the second if() condition, then not setting v->arch.hvm_vcpu.inject_trap.vector to -1 would lead to the trap being kept for injection at the first opportunity - and that could be when the context has changed and we shouldn't inject it anymore. So v->arch.hvm_vcpu.inject_trap.vector is therefore reset either way. >> + if ( unlikely(v->arch.vm_event) && >> + v->arch.vm_event->monitor_next_interrupt ) > > Hard tab. I'll fix it. >> --- a/xen/arch/x86/hvm/monitor.c >> +++ b/xen/arch/x86/hvm/monitor.c >> @@ -150,6 +150,21 @@ int hvm_monitor_cpuid(unsigned long insn_length, unsigned int leaf, >> return monitor_traps(curr, 1, &req); >> } >> >> +void hvm_monitor_interrupt(unsigned int vector, unsigned int type, >> + unsigned int err, uint64_t cr2) >> +{ >> + struct vcpu *curr = current; > > Pointless local variable (used just once). I'll remove it. >> --- a/xen/arch/x86/hvm/svm/svm.c >> +++ b/xen/arch/x86/hvm/svm/svm.c >> @@ -2220,6 +2220,21 @@ static void svm_invlpg(struct vcpu *v, unsigned long vaddr) >> svm_asid_g_invlpg(v, vaddr); >> } >> >> +static bool svm_get_pending_event(struct vcpu *v, struct hvm_trap *info) >> +{ >> + struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; >> + >> + if ( vmcb->eventinj.fields.v ) >> + return false; >> + >> + info->vector = vmcb->eventinj.fields.vector; >> + info->type = vmcb->eventinj.fields.type; >> + info->error_code = vmcb->eventinj.fields.errorcode; >> + info->cr2 = v->arch.hvm_vcpu.guest_cr[2]; > > I'd prefer for this last part to be put into generic code (i.e. the > wrapper). You mean setting CR2, which is common, right? I'll move it to the wrapper. >> --- a/xen/include/asm-arm/vm_event.h >> +++ b/xen/include/asm-arm/vm_event.h >> @@ -52,4 +52,10 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp) >> /* Not supported on ARM. */ >> } >> >> +static inline >> +void vm_event_monitor_next_interrupt(struct vcpu *v) >> +{ >> + /* Not supported on ARM. */ >> +} > > That's unfortunate. If it can't be implemented, shouldn't the caller at > least be advised of this being unavailable? Wasn't there even some > mechanism to report capabilities? Yes, I forgot to update the capabilities list. Good point, I'll see about that as well. >> --- a/xen/include/asm-x86/hvm/hvm.h >> +++ b/xen/include/asm-x86/hvm/hvm.h >> @@ -237,6 +237,8 @@ struct hvm_function_table { >> /* Architecture function to setup TSC scaling ratio */ >> void (*setup)(struct vcpu *v); >> } tsc_scaling; >> + >> + bool (*get_pending_event)(struct vcpu *v, struct hvm_trap *info); >> }; > > Stylistically I think this would better go a little earlier. I'll move it after event_pending. >> --- a/xen/include/asm-x86/vm_event.h >> +++ b/xen/include/asm-x86/vm_event.h >> @@ -32,6 +32,7 @@ struct arch_vm_event { >> struct vm_event_emul_insn_data insn; >> } emul; >> struct monitor_write_data write_data; >> + bool monitor_next_interrupt; >> }; > > I think there's a 32-bit padding hole before write_data, so the new > field would better go earlier (perhaps even right after flags). I'll move it there. >> @@ -139,6 +144,8 @@ >> * These kinds of events will be filtered out in future versions. >> */ >> #define VM_EVENT_REASON_PRIVILEGED_CALL 11 >> +/* Result of toolstack-requested (non-architectural) trap injection. */ >> +#define VM_EVENT_REASON_INTERRUPT 12 > > Considering the event reports all kinds of interruptions, I don't think > the comment is appropriate. True, I'll update it. >> @@ -259,6 +266,13 @@ struct vm_event_cpuid { >> uint32_t _pad; >> }; >> >> +struct vm_event_interrupt { >> + uint32_t vector; >> + uint32_t type; >> + uint32_t error_code; >> + uint64_t cr2; >> +}; > > This being x86-specific, I think it should be named or union-ized > accordingly. Right, I'll rename it. Thanks, Razvan
On 09/11/16 11:32, Razvan Cojocaru wrote: > On 11/09/2016 01:17 PM, Jan Beulich wrote: >>>>> On 09.11.16 at 10:42, <rcojocaru@bitdefender.com> wrote: >>> Added support for a new event type, VM_EVENT_REASON_INTERRUPT, >>> which is now fired in a one-shot manner when enabled via the new >>> VM_EVENT_FLAG_GET_NEXT_INTERRUPT vm_event response flag. >>> The patch also fixes the behaviour of the xc_hvm_inject_trap() >>> hypercall, which would lead to non-architectural interrupts >>> overwriting pending (specifically reinjected) architectural ones. >> Looks quite okay, just some more or less mechanical comments. >> >>> --- a/xen/arch/x86/hvm/hvm.c >>> +++ b/xen/arch/x86/hvm/hvm.c >>> @@ -532,11 +532,23 @@ void hvm_do_resume(struct vcpu *v) >>> } >>> } >>> >>> - /* Inject pending hw/sw trap */ >>> - if ( v->arch.hvm_vcpu.inject_trap.vector != -1 ) >>> - { >>> + /* Inject pending hw/sw trap if there are no other pending interrupts. */ >>> + if ( v->arch.hvm_vcpu.inject_trap.vector != -1 && !hvm_event_pending(v) ) >>> hvm_inject_trap(&v->arch.hvm_vcpu.inject_trap); >>> - v->arch.hvm_vcpu.inject_trap.vector = -1; >>> + >>> + v->arch.hvm_vcpu.inject_trap.vector = -1; >> I don't see why you pull this out of the if() body. > That is intended, and covered by the "the patch also fixes the behaviour > of the xc_hvm_inject_trap() hypercall, which would lead to > non-architectural interrupts overwriting pending (specifically > reinjected) architectural ones" part of the patch description. > > If we couldn't inject the trap because there was a pending event (i.e. > the second if() condition, then not setting > v->arch.hvm_vcpu.inject_trap.vector to -1 would lead to the trap being > kept for injection at the first opportunity - and that could be when the > context has changed and we shouldn't inject it anymore. So > v->arch.hvm_vcpu.inject_trap.vector is therefore reset either way. > >>> + if ( unlikely(v->arch.vm_event) && >>> + v->arch.vm_event->monitor_next_interrupt ) >> Hard tab. > I'll fix it. > >>> --- a/xen/arch/x86/hvm/monitor.c >>> +++ b/xen/arch/x86/hvm/monitor.c >>> @@ -150,6 +150,21 @@ int hvm_monitor_cpuid(unsigned long insn_length, unsigned int leaf, >>> return monitor_traps(curr, 1, &req); >>> } >>> >>> +void hvm_monitor_interrupt(unsigned int vector, unsigned int type, >>> + unsigned int err, uint64_t cr2) >>> +{ >>> + struct vcpu *curr = current; >> Pointless local variable (used just once). > I'll remove it. > >>> --- a/xen/arch/x86/hvm/svm/svm.c >>> +++ b/xen/arch/x86/hvm/svm/svm.c >>> @@ -2220,6 +2220,21 @@ static void svm_invlpg(struct vcpu *v, unsigned long vaddr) >>> svm_asid_g_invlpg(v, vaddr); >>> } >>> >>> +static bool svm_get_pending_event(struct vcpu *v, struct hvm_trap *info) >>> +{ >>> + struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; >>> + >>> + if ( vmcb->eventinj.fields.v ) >>> + return false; >>> + >>> + info->vector = vmcb->eventinj.fields.vector; >>> + info->type = vmcb->eventinj.fields.type; >>> + info->error_code = vmcb->eventinj.fields.errorcode; >>> + info->cr2 = v->arch.hvm_vcpu.guest_cr[2]; >> I'd prefer for this last part to be put into generic code (i.e. the >> wrapper). > You mean setting CR2, which is common, right? I'll move it to the wrapper. > >>> --- a/xen/include/asm-arm/vm_event.h >>> +++ b/xen/include/asm-arm/vm_event.h >>> @@ -52,4 +52,10 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp) >>> /* Not supported on ARM. */ >>> } >>> >>> +static inline >>> +void vm_event_monitor_next_interrupt(struct vcpu *v) >>> +{ >>> + /* Not supported on ARM. */ >>> +} >> That's unfortunate. If it can't be implemented, shouldn't the caller at >> least be advised of this being unavailable? Wasn't there even some >> mechanism to report capabilities? > Yes, I forgot to update the capabilities list. Good point, I'll see > about that as well. > >>> --- a/xen/include/asm-x86/hvm/hvm.h >>> +++ b/xen/include/asm-x86/hvm/hvm.h >>> @@ -237,6 +237,8 @@ struct hvm_function_table { >>> /* Architecture function to setup TSC scaling ratio */ >>> void (*setup)(struct vcpu *v); >>> } tsc_scaling; >>> + >>> + bool (*get_pending_event)(struct vcpu *v, struct hvm_trap *info); >>> }; >> Stylistically I think this would better go a little earlier. > I'll move it after event_pending. > >>> --- a/xen/include/asm-x86/vm_event.h >>> +++ b/xen/include/asm-x86/vm_event.h >>> @@ -32,6 +32,7 @@ struct arch_vm_event { >>> struct vm_event_emul_insn_data insn; >>> } emul; >>> struct monitor_write_data write_data; >>> + bool monitor_next_interrupt; >>> }; >> I think there's a 32-bit padding hole before write_data, so the new >> field would better go earlier (perhaps even right after flags). > I'll move it there. > >>> @@ -139,6 +144,8 @@ >>> * These kinds of events will be filtered out in future versions. >>> */ >>> #define VM_EVENT_REASON_PRIVILEGED_CALL 11 >>> +/* Result of toolstack-requested (non-architectural) trap injection. */ >>> +#define VM_EVENT_REASON_INTERRUPT 12 >> Considering the event reports all kinds of interruptions, I don't think >> the comment is appropriate. > True, I'll update it. > >>> @@ -259,6 +266,13 @@ struct vm_event_cpuid { >>> uint32_t _pad; >>> }; >>> >>> +struct vm_event_interrupt { >>> + uint32_t vector; >>> + uint32_t type; >>> + uint32_t error_code; >>> + uint64_t cr2; >>> +}; >> This being x86-specific, I think it should be named or union-ized >> accordingly. > Right, I'll rename it. You area also exposing X86_EVENTTYPE_* in the hypervisor ABI. This is probably fine as it is an ABI inherited from VT-x/SVM (and by some miracle, are actually compatible), but you do need to move the constants into the public API as well. ~Andrew
On 11/09/2016 01:17 PM, Jan Beulich wrote: >> @@ -259,6 +266,13 @@ struct vm_event_cpuid { >> > uint32_t _pad; >> > }; >> > >> > +struct vm_event_interrupt { >> > + uint32_t vector; >> > + uint32_t type; >> > + uint32_t error_code; >> > + uint64_t cr2; >> > +}; > This being x86-specific, I think it should be named or union-ized > accordingly. I think I should also pad this to be a multiple of 64 bits as the others are (with a uint32_t _pad; member). Thanks, Razvan
>>> On 09.11.16 at 12:32, <rcojocaru@bitdefender.com> wrote: > On 11/09/2016 01:17 PM, Jan Beulich wrote: >>>>> On 09.11.16 at 10:42, <rcojocaru@bitdefender.com> wrote: >>> --- a/xen/arch/x86/hvm/hvm.c >>> +++ b/xen/arch/x86/hvm/hvm.c >>> @@ -532,11 +532,23 @@ void hvm_do_resume(struct vcpu *v) >>> } >>> } >>> >>> - /* Inject pending hw/sw trap */ >>> - if ( v->arch.hvm_vcpu.inject_trap.vector != -1 ) >>> - { >>> + /* Inject pending hw/sw trap if there are no other pending interrupts. */ >>> + if ( v->arch.hvm_vcpu.inject_trap.vector != -1 && !hvm_event_pending(v) ) >>> hvm_inject_trap(&v->arch.hvm_vcpu.inject_trap); >>> - v->arch.hvm_vcpu.inject_trap.vector = -1; >>> + >>> + v->arch.hvm_vcpu.inject_trap.vector = -1; >> >> I don't see why you pull this out of the if() body. > > That is intended, and covered by the "the patch also fixes the behaviour > of the xc_hvm_inject_trap() hypercall, which would lead to > non-architectural interrupts overwriting pending (specifically > reinjected) architectural ones" part of the patch description. > > If we couldn't inject the trap because there was a pending event (i.e. > the second if() condition, then not setting > v->arch.hvm_vcpu.inject_trap.vector to -1 would lead to the trap being > kept for injection at the first opportunity - and that could be when the > context has changed and we shouldn't inject it anymore. So > v->arch.hvm_vcpu.inject_trap.vector is therefore reset either way. Ah, that's because you extend the condition. How about you leave the condition as is, and only make the actual call conditonal upon hvm_event_pending()'s return value? That's also make the patch better readable. Jan
On 11/09/2016 04:04 PM, Jan Beulich wrote: >>>> On 09.11.16 at 12:32, <rcojocaru@bitdefender.com> wrote: >> On 11/09/2016 01:17 PM, Jan Beulich wrote: >>>>>> On 09.11.16 at 10:42, <rcojocaru@bitdefender.com> wrote: >>>> --- a/xen/arch/x86/hvm/hvm.c >>>> +++ b/xen/arch/x86/hvm/hvm.c >>>> @@ -532,11 +532,23 @@ void hvm_do_resume(struct vcpu *v) >>>> } >>>> } >>>> >>>> - /* Inject pending hw/sw trap */ >>>> - if ( v->arch.hvm_vcpu.inject_trap.vector != -1 ) >>>> - { >>>> + /* Inject pending hw/sw trap if there are no other pending interrupts. */ >>>> + if ( v->arch.hvm_vcpu.inject_trap.vector != -1 && !hvm_event_pending(v) ) >>>> hvm_inject_trap(&v->arch.hvm_vcpu.inject_trap); >>>> - v->arch.hvm_vcpu.inject_trap.vector = -1; >>>> + >>>> + v->arch.hvm_vcpu.inject_trap.vector = -1; >>> >>> I don't see why you pull this out of the if() body. >> >> That is intended, and covered by the "the patch also fixes the behaviour >> of the xc_hvm_inject_trap() hypercall, which would lead to >> non-architectural interrupts overwriting pending (specifically >> reinjected) architectural ones" part of the patch description. >> >> If we couldn't inject the trap because there was a pending event (i.e. >> the second if() condition, then not setting >> v->arch.hvm_vcpu.inject_trap.vector to -1 would lead to the trap being >> kept for injection at the first opportunity - and that could be when the >> context has changed and we shouldn't inject it anymore. So >> v->arch.hvm_vcpu.inject_trap.vector is therefore reset either way. > > Ah, that's because you extend the condition. How about you leave > the condition as is, and only make the actual call conditonal > upon hvm_event_pending()'s return value? That's also make the > patch better readable. Of course, no problem. Thanks, Razvan
>>> On 09.11.16 at 12:49, <andrew.cooper3@citrix.com> wrote: > On 09/11/16 11:32, Razvan Cojocaru wrote: >> On 11/09/2016 01:17 PM, Jan Beulich wrote: >>>>>> On 09.11.16 at 10:42, <rcojocaru@bitdefender.com> wrote: >>>> @@ -259,6 +266,13 @@ struct vm_event_cpuid { >>>> uint32_t _pad; >>>> }; >>>> >>>> +struct vm_event_interrupt { >>>> + uint32_t vector; >>>> + uint32_t type; >>>> + uint32_t error_code; >>>> + uint64_t cr2; >>>> +}; >>> This being x86-specific, I think it should be named or union-ized >>> accordingly. >> Right, I'll rename it. > > You area also exposing X86_EVENTTYPE_* in the hypervisor ABI. > > This is probably fine as it is an ABI inherited from VT-x/SVM (and by > some miracle, are actually compatible), but you do need to move the > constants into the public API as well. I was about to make that comment too, but then checked: These constants are already exposed (via the inject trap hypercall). While moving them into the public header might be a good idea in general, I don't think that belongs into this patch. Jan
On 11/09/2016 01:17 PM, Jan Beulich wrote: >>>> On 09.11.16 at 10:42, <rcojocaru@bitdefender.com> wrote: >> +static bool svm_get_pending_event(struct vcpu *v, struct hvm_trap *info) >> +{ >> + struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; >> + >> + if ( vmcb->eventinj.fields.v ) >> + return false; >> + >> + info->vector = vmcb->eventinj.fields.vector; >> + info->type = vmcb->eventinj.fields.type; >> + info->error_code = vmcb->eventinj.fields.errorcode; >> + info->cr2 = v->arch.hvm_vcpu.guest_cr[2]; > > I'd prefer for this last part to be put into generic code (i.e. the > wrapper). Actually, doing this: static inline bool hvm_get_pending_event(struct vcpu *v, struct hvm_trap *info) { info->cr2 = v->arch.hvm_vcpu.guest_cr[2]; return hvm_funcs.get_pending_event(v, info); } leads to "error: dereferencing pointer to incomplete type" about v->, so to do this an additional #include will be necessary. Is that acceptable? Thanks, Razvan
>>> On 09.11.16 at 15:28, <rcojocaru@bitdefender.com> wrote: > On 11/09/2016 01:17 PM, Jan Beulich wrote: >>>>> On 09.11.16 at 10:42, <rcojocaru@bitdefender.com> wrote: >>> +static bool svm_get_pending_event(struct vcpu *v, struct hvm_trap *info) >>> +{ >>> + struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; >>> + >>> + if ( vmcb->eventinj.fields.v ) >>> + return false; >>> + >>> + info->vector = vmcb->eventinj.fields.vector; >>> + info->type = vmcb->eventinj.fields.type; >>> + info->error_code = vmcb->eventinj.fields.errorcode; >>> + info->cr2 = v->arch.hvm_vcpu.guest_cr[2]; >> >> I'd prefer for this last part to be put into generic code (i.e. the >> wrapper). > > Actually, doing this: > > static inline bool hvm_get_pending_event(struct vcpu *v, struct hvm_trap *info) > { > info->cr2 = v->arch.hvm_vcpu.guest_cr[2]; > return hvm_funcs.get_pending_event(v, info); > } > > leads to "error: dereferencing pointer to incomplete type" about v->, so > to do this an additional #include will be necessary. Is that acceptable? Better make it an out-of-line function in hvm.c. Jan
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 704fd64..06c426c 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -532,11 +532,23 @@ void hvm_do_resume(struct vcpu *v) } } - /* Inject pending hw/sw trap */ - if ( v->arch.hvm_vcpu.inject_trap.vector != -1 ) - { + /* Inject pending hw/sw trap if there are no other pending interrupts. */ + if ( v->arch.hvm_vcpu.inject_trap.vector != -1 && !hvm_event_pending(v) ) hvm_inject_trap(&v->arch.hvm_vcpu.inject_trap); - v->arch.hvm_vcpu.inject_trap.vector = -1; + + v->arch.hvm_vcpu.inject_trap.vector = -1; + + if ( unlikely(v->arch.vm_event) && + v->arch.vm_event->monitor_next_interrupt ) + { + struct hvm_trap info; + + if ( hvm_get_pending_event(v, &info) ) + { + hvm_monitor_interrupt(info.vector, info.type, info.error_code, + info.cr2); + v->arch.vm_event->monitor_next_interrupt = false; + } } } diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c index 401a8c6..00b2c3b 100644 --- a/xen/arch/x86/hvm/monitor.c +++ b/xen/arch/x86/hvm/monitor.c @@ -150,6 +150,21 @@ int hvm_monitor_cpuid(unsigned long insn_length, unsigned int leaf, return monitor_traps(curr, 1, &req); } +void hvm_monitor_interrupt(unsigned int vector, unsigned int type, + unsigned int err, uint64_t cr2) +{ + struct vcpu *curr = current; + vm_event_request_t req = { + .reason = VM_EVENT_REASON_INTERRUPT, + .u.interrupt.vector = vector, + .u.interrupt.type = type, + .u.interrupt.error_code = err, + .u.interrupt.cr2 = cr2, + }; + + monitor_traps(curr, 1, &req); +} + /* * Local variables: * mode: C diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index 16427f6..bba0399 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -2220,6 +2220,21 @@ static void svm_invlpg(struct vcpu *v, unsigned long vaddr) svm_asid_g_invlpg(v, vaddr); } +static bool svm_get_pending_event(struct vcpu *v, struct hvm_trap *info) +{ + struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; + + if ( vmcb->eventinj.fields.v ) + return false; + + info->vector = vmcb->eventinj.fields.vector; + info->type = vmcb->eventinj.fields.type; + info->error_code = vmcb->eventinj.fields.errorcode; + info->cr2 = v->arch.hvm_vcpu.guest_cr[2]; + + return true; +} + static struct hvm_function_table __initdata svm_function_table = { .name = "SVM", .cpu_up_prepare = svm_cpu_up_prepare, @@ -2272,6 +2287,7 @@ static struct hvm_function_table __initdata svm_function_table = { .tsc_scaling = { .max_ratio = ~TSC_RATIO_RSVD_BITS, }, + .get_pending_event = svm_get_pending_event, }; void svm_vmexit_handler(struct cpu_user_regs *regs) diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 9a8f694..6f64033 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -2134,6 +2134,26 @@ static int vmx_set_mode(struct vcpu *v, int mode) return 0; } +static bool vmx_get_pending_event(struct vcpu *v, struct hvm_trap *info) +{ + unsigned long intr_info, error_code; + + vmx_vmcs_enter(v); + __vmread(VM_ENTRY_INTR_INFO, &intr_info); + __vmread(VM_ENTRY_EXCEPTION_ERROR_CODE, &error_code); + vmx_vmcs_exit(v); + + if ( !(intr_info & INTR_INFO_VALID_MASK) ) + return false; + + info->vector = MASK_EXTR(intr_info, INTR_INFO_VECTOR_MASK); + info->type = MASK_EXTR(intr_info, INTR_INFO_INTR_TYPE_MASK); + info->error_code = error_code; + info->cr2 = v->arch.hvm_vcpu.guest_cr[2]; + + return true; +} + static struct hvm_function_table __initdata vmx_function_table = { .name = "VMX", .cpu_up_prepare = vmx_cpu_up_prepare, @@ -2203,6 +2223,7 @@ static struct hvm_function_table __initdata vmx_function_table = { .max_ratio = VMX_TSC_MULTIPLIER_MAX, .setup = vmx_setup_tsc_scaling, }, + .get_pending_event = vmx_get_pending_event, }; /* Handle VT-d posted-interrupt when VCPU is blocked. */ diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c index 1e88d67..a153ec5 100644 --- a/xen/arch/x86/vm_event.c +++ b/xen/arch/x86/vm_event.c @@ -134,6 +134,12 @@ void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp) v->arch.user_regs.eip = rsp->data.regs.x86.rip; } +void vm_event_monitor_next_interrupt(struct vcpu *v) +{ + ASSERT(v->arch.vm_event); + v->arch.vm_event->monitor_next_interrupt = true; +} + void vm_event_fill_regs(vm_event_request_t *req) { const struct cpu_user_regs *regs = guest_cpu_user_regs(); diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c index 907ab40..c947706 100644 --- a/xen/common/vm_event.c +++ b/xen/common/vm_event.c @@ -433,6 +433,9 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved) if ( rsp.flags & VM_EVENT_FLAG_SET_REGISTERS ) vm_event_set_registers(v, &rsp); + if ( rsp.flags & VM_EVENT_FLAG_GET_NEXT_INTERRUPT ) + vm_event_monitor_next_interrupt(v); + if ( rsp.flags & VM_EVENT_FLAG_VCPU_PAUSED ) vm_event_vcpu_unpause(v); } diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h index 66f2474..ab9c8cb 100644 --- a/xen/include/asm-arm/vm_event.h +++ b/xen/include/asm-arm/vm_event.h @@ -52,4 +52,10 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp) /* Not supported on ARM. */ } +static inline +void vm_event_monitor_next_interrupt(struct vcpu *v) +{ + /* Not supported on ARM. */ +} + #endif /* __ASM_ARM_VM_EVENT_H__ */ diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h index 7e7462e..e6d91ea 100644 --- a/xen/include/asm-x86/hvm/hvm.h +++ b/xen/include/asm-x86/hvm/hvm.h @@ -237,6 +237,8 @@ struct hvm_function_table { /* Architecture function to setup TSC scaling ratio */ void (*setup)(struct vcpu *v); } tsc_scaling; + + bool (*get_pending_event)(struct vcpu *v, struct hvm_trap *info); }; extern struct hvm_function_table hvm_funcs; @@ -428,6 +430,11 @@ static inline int hvm_event_pending(struct vcpu *v) return hvm_funcs.event_pending(v); } +static inline bool hvm_get_pending_event(struct vcpu *v, struct hvm_trap *info) +{ + return hvm_funcs.get_pending_event(v, info); +} + /* These bits in CR4 are owned by the host. */ #define HVM_CR4_HOST_MASK (mmu_cr4_features & \ (X86_CR4_VMXE | X86_CR4_PAE | X86_CR4_MCE)) diff --git a/xen/include/asm-x86/hvm/monitor.h b/xen/include/asm-x86/hvm/monitor.h index 82b85ec..85ca678 100644 --- a/xen/include/asm-x86/hvm/monitor.h +++ b/xen/include/asm-x86/hvm/monitor.h @@ -42,6 +42,8 @@ int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type, unsigned long trap_type, unsigned long insn_length); int hvm_monitor_cpuid(unsigned long insn_length, unsigned int leaf, unsigned int subleaf); +void hvm_monitor_interrupt(unsigned int vector, unsigned int type, + unsigned int err, uint64_t cr2); #endif /* __ASM_X86_HVM_MONITOR_H__ */ diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h index ca73f99..b5cd872 100644 --- a/xen/include/asm-x86/vm_event.h +++ b/xen/include/asm-x86/vm_event.h @@ -32,6 +32,7 @@ struct arch_vm_event { struct vm_event_emul_insn_data insn; } emul; struct monitor_write_data write_data; + bool monitor_next_interrupt; }; int vm_event_init_domain(struct domain *d); diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h index c28be5a..5c8c31e 100644 --- a/xen/include/public/vm_event.h +++ b/xen/include/public/vm_event.h @@ -105,6 +105,11 @@ * if any of those flags are set, only those will be honored). */ #define VM_EVENT_FLAG_SET_EMUL_INSN_DATA (1 << 9) +/* + * Have a one-shot VM_EVENT_REASON_INTERRUPT event sent for the first + * interrupt occuring after resuming the VCPU. + */ +#define VM_EVENT_FLAG_GET_NEXT_INTERRUPT (1 << 10) /* * Reasons for the vm event request @@ -139,6 +144,8 @@ * These kinds of events will be filtered out in future versions. */ #define VM_EVENT_REASON_PRIVILEGED_CALL 11 +/* Result of toolstack-requested (non-architectural) trap injection. */ +#define VM_EVENT_REASON_INTERRUPT 12 /* Supported values for the vm_event_write_ctrlreg index. */ #define VM_EVENT_X86_CR0 0 @@ -259,6 +266,13 @@ struct vm_event_cpuid { uint32_t _pad; }; +struct vm_event_interrupt { + uint32_t vector; + uint32_t type; + uint32_t error_code; + uint64_t cr2; +}; + #define MEM_PAGING_DROP_PAGE (1 << 0) #define MEM_PAGING_EVICT_FAIL (1 << 1) @@ -302,6 +316,7 @@ typedef struct vm_event_st { struct vm_event_debug software_breakpoint; struct vm_event_debug debug_exception; struct vm_event_cpuid cpuid; + struct vm_event_interrupt interrupt; } u; union { diff --git a/xen/include/xen/vm_event.h b/xen/include/xen/vm_event.h index 4f088c8..2fb3951 100644 --- a/xen/include/xen/vm_event.h +++ b/xen/include/xen/vm_event.h @@ -78,6 +78,8 @@ void vm_event_vcpu_unpause(struct vcpu *v); void vm_event_fill_regs(vm_event_request_t *req); void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp); +void vm_event_monitor_next_interrupt(struct vcpu *v); + #endif /* __VM_EVENT_H__ */ /*
Added support for a new event type, VM_EVENT_REASON_INTERRUPT, which is now fired in a one-shot manner when enabled via the new VM_EVENT_FLAG_GET_NEXT_INTERRUPT vm_event response flag. The patch also fixes the behaviour of the xc_hvm_inject_trap() hypercall, which would lead to non-architectural interrupts overwriting pending (specifically reinjected) architectural ones. Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> --- xen/arch/x86/hvm/hvm.c | 20 ++++++++++++++++---- xen/arch/x86/hvm/monitor.c | 15 +++++++++++++++ xen/arch/x86/hvm/svm/svm.c | 16 ++++++++++++++++ xen/arch/x86/hvm/vmx/vmx.c | 21 +++++++++++++++++++++ xen/arch/x86/vm_event.c | 6 ++++++ xen/common/vm_event.c | 3 +++ xen/include/asm-arm/vm_event.h | 6 ++++++ xen/include/asm-x86/hvm/hvm.h | 7 +++++++ xen/include/asm-x86/hvm/monitor.h | 2 ++ xen/include/asm-x86/vm_event.h | 1 + xen/include/public/vm_event.h | 15 +++++++++++++++ xen/include/xen/vm_event.h | 2 ++ 12 files changed, 110 insertions(+), 4 deletions(-)