On 05/03/17 12:51, Jan Beulich wrote:
>>>> On 03.05.17 at 11:10, <rcojoc...@bitdefender.com> wrote:
>> @@ -483,67 +483,7 @@ void hvm_do_resume(struct vcpu *v)
>> if ( !handle_hvm_io_completion(v) )
>> return;
>>
>> - if ( unlikely(v->arch.vm_event) )
>> - {
>> - struct monitor_write_data *w = &v->arch.vm_event->write_data;
>> -
>> - if ( unlikely(v->arch.vm_event->emulate_flags) )
>> - {
>> - enum emul_kind kind = EMUL_KIND_NORMAL;
>> -
>> - /*
>> - * Please observ the order here to match the flag descriptions
>> - * provided in public/vm_event.h
>> - */
>> - if ( v->arch.vm_event->emulate_flags &
>> - VM_EVENT_FLAG_SET_EMUL_READ_DATA )
>> - kind = EMUL_KIND_SET_CONTEXT_DATA;
>> - else if ( v->arch.vm_event->emulate_flags &
>> - VM_EVENT_FLAG_EMULATE_NOWRITE )
>> - kind = EMUL_KIND_NOWRITE;
>> - else if ( v->arch.vm_event->emulate_flags &
>> - VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
>> - kind = EMUL_KIND_SET_CONTEXT_INSN;
>> -
>> - hvm_emulate_one_vm_event(kind, TRAP_invalid_op,
>> - X86_EVENT_NO_EC);
>> -
>> - v->arch.vm_event->emulate_flags = 0;
>> - }
>> -
>> - if ( w->do_write.msr )
>> - {
>> - if ( hvm_msr_write_intercept(w->msr, w->value, 0) ==
>> - X86EMUL_EXCEPTION )
>> - hvm_inject_hw_exception(TRAP_gp_fault, 0);
>> -
>> - w->do_write.msr = 0;
>> - }
>> -
>> - if ( w->do_write.cr0 )
>> - {
>> - if ( hvm_set_cr0(w->cr0, 0) == X86EMUL_EXCEPTION )
>> - hvm_inject_hw_exception(TRAP_gp_fault, 0);
>> -
>> - w->do_write.cr0 = 0;
>> - }
>> -
>> - if ( w->do_write.cr4 )
>> - {
>> - if ( hvm_set_cr4(w->cr4, 0) == X86EMUL_EXCEPTION )
>> - hvm_inject_hw_exception(TRAP_gp_fault, 0);
>> -
>> - w->do_write.cr4 = 0;
>> - }
>> -
>> - if ( w->do_write.cr3 )
>> - {
>> - if ( hvm_set_cr3(w->cr3, 0) == X86EMUL_EXCEPTION )
>> - hvm_inject_hw_exception(TRAP_gp_fault, 0);
>> -
>> - w->do_write.cr3 = 0;
>> - }
>> - }
>> + hvm_vm_event_do_resume(v);
>
> As indicated before, I think we want to keep
>
> if ( unlikely(v->arch.vm_event) )
>
> here of in an inline wrapper, to avoid the actual function call in the
> common case.
Will do.
>> --- /dev/null
>> +++ b/xen/arch/x86/hvm/vm_event.c
>> @@ -0,0 +1,101 @@
>> +/*
>> + * arch/x86/hvm/vm_event.c
>> + *
>> + * HVM vm_event handling routines
>> + *
>> + * Copyright (c) 2017 Razvan Cojocaru (rcojoc...@bitdefender.com)
>
> I'm notoriously bad when it comes to copyrights, but you just
> moving code makes me wonder whether this is appropriate.
To be honest I quite agree with you, and in the beginning I just meant
to have no Copyright line in there at all - but I remembered a
discussion a while back where a patch was I believe rejected because it
lacked one. So I've just copied Tamas' file (vm_event.c) and only
changed the copyright line because I didn't really know what else to put
there.
I'm quite happy to remove it altogether. Will that do?
>> +void hvm_vm_event_do_resume(struct vcpu *v)
>> +{
>> + struct monitor_write_data *w;
>> +
>> + if ( likely(!v->arch.vm_event) )
>> + return;
>> +
>> + w = &v->arch.vm_event->write_data;
>> +
>> + if ( unlikely(v->arch.vm_event->emulate_flags) )
>> + {
>> + enum emul_kind kind = EMUL_KIND_NORMAL;
>> +
>> + /*
>> + * Please observe the order here to match the flag descriptions
>> + * provided in public/vm_event.h
>> + */
>> + if ( v->arch.vm_event->emulate_flags &
>> + VM_EVENT_FLAG_SET_EMUL_READ_DATA )
>> + kind = EMUL_KIND_SET_CONTEXT_DATA;
>> + else if ( v->arch.vm_event->emulate_flags &
>> + VM_EVENT_FLAG_EMULATE_NOWRITE )
>> + kind = EMUL_KIND_NOWRITE;
>> + else if ( v->arch.vm_event->emulate_flags &
>> + VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
>> + kind = EMUL_KIND_SET_CONTEXT_INSN;
>> +
>> + hvm_emulate_one_vm_event(kind, TRAP_invalid_op,
>> + X86_EVENT_NO_EC);
>> +
>> + v->arch.vm_event->emulate_flags = 0;
>> + }
>> +
>> + if ( w->do_write.cr0 )
>> + {
>> + if ( hvm_set_cr0(w->cr0, 0) == X86EMUL_EXCEPTION )
>> + hvm_inject_hw_exception(TRAP_gp_fault, 0);
>> +
>> + w->do_write.cr0 = 0;
>> + }
>> +
>> + if ( w->do_write.cr4 )
>> + {
>> + if ( hvm_set_cr4(w->cr4, 0) == X86EMUL_EXCEPTION )
>> + hvm_inject_hw_exception(TRAP_gp_fault, 0);
>> +
>> + w->do_write.cr4 = 0;
>> + }
>> +
>> + if ( w->do_write.cr3 )
>> + {
>> + if ( hvm_set_cr3(w->cr3, 0) == X86EMUL_EXCEPTION )
>> + hvm_inject_hw_exception(TRAP_gp_fault, 0);
>> +
>> + w->do_write.cr3 = 0;
>> + }
>> +
>> + if ( w->do_write.msr )
>> + {
>> + if ( hvm_msr_write_intercept(w->msr, w->value, 0) ==
>> + X86EMUL_EXCEPTION )
>> + hvm_inject_hw_exception(TRAP_gp_fault, 0);
>> +
>> + w->do_write.msr = 0;
>> + }
>
> I wonder whether all of these outer if()-s wouldn't better have
> unlikely() too.
It can't hurt, unless anyone objects I'll wrap them in unlikely()s.
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
