On 10/11/16 15:53, Razvan Cojocaru wrote: > On 11/10/2016 05:47 PM, Jan Beulich wrote: >>>>> On 10.11.16 at 09:35, <rcojoc...@bitdefender.com> wrote: >>> Changes since V1: >>> - Modified the if() in hvm_do_resume() for readability. >>> - Replaced hard tab with spaces. >>> - Removed a local variable used only once. >>> - Moved cr2 assignment to the common part of the code. >>> - Now listing the new event in the x86 vm_event capability list. >>> - Moved struct variables for readability. >> Hmm, looks like you've moved the field in the structure declaration, >> but not the two initializers (in SVM and VMX code). > I'll modify those as well. > >>> --- a/xen/arch/x86/hvm/hvm.c >>> +++ b/xen/arch/x86/hvm/hvm.c >>> @@ -535,9 +535,24 @@ void hvm_do_resume(struct vcpu *v) >>> /* Inject pending hw/sw trap */ >>> if ( v->arch.hvm_vcpu.inject_trap.vector != -1 ) >>> { >>> - hvm_inject_trap(&v->arch.hvm_vcpu.inject_trap); >>> + if ( !hvm_event_pending(v) ) >>> + hvm_inject_trap(&v->arch.hvm_vcpu.inject_trap); >>> + >>> 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; >>> + } >>> + } >>> } >>> >>> static int hvm_print_line( >>> @@ -6047,6 +6062,12 @@ void hvm_domain_soft_reset(struct domain *d) >>> hvm_destroy_all_ioreq_servers(d); >>> } >>> >>> +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); >>> +} >> Unless you expect more callers, I'm tempted to suggest to make this >> static for now (and move it up ahead of its only caller). > Will do. > >>> --- 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; >>> +} >> I think at this point we're determined to no longer permit ASSERT()s >> like this: Either use a simple if() or a BUG_ON(). Andrew, please >> correct me if I've misunderstood earlier discussions. > I'll change it to a simple if(). > >>> @@ -259,6 +266,14 @@ struct vm_event_cpuid { >>> uint32_t _pad; >>> }; >>> >>> +struct vm_event_interrupt_x86 { >>> + uint32_t vector; >>> + uint32_t type; >>> + uint32_t error_code; >>> + uint64_t cr2; >>> + uint32_t _pad; >>> +}; >> I'm pretty certain this is not what you want, as this make the layout >> vary between 32-bit (compat) and 64-bit (native) callers. > I'll remove the _pad.
You need to invert the pad and cr2, so cr2 starts at 16 bytes into the structure. Remember that uint64_t has 8 byte alignment in 64bit, but only 4 byte alignment in 32bit. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel