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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> --- /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.

> +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.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel