Re: [Xen-devel] [PATCH V2 1/2] x86/vm_event: added hvm/vm_event.{h, c}

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 05/03/17 12:51, Jan Beulich wrote:
>>>> On 03.05.17 at 11:10, <rcojocaru@xxxxxxxxxxxxxxx> 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 (rcojocaru@xxxxxxxxxxxxxxx)
> 
> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel