Re: [Xen-devel] [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.09.16 at 18:51, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> @@ -1793,7 +1793,17 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt 
> *hvmemul_ctxt,
>          pfec |= PFEC_user_mode;
>  
>      hvmemul_ctxt->insn_buf_eip = regs->eip;
> -    if ( !vio->mmio_insn_bytes )
> +
> +    if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
> +    {
> +        BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) ==
> +                     sizeof(curr->arch.vm_event->emul.insn));

This should quite clearly be !=, and I think it builds only because you
use the wrong operand in the first sizeof().

> +        hvmemul_ctxt->insn_buf_bytes = 
> sizeof(curr->arch.vm_event->emul.insn);
> +        memcpy(hvmemul_ctxt->insn_buf, &curr->arch.vm_event->emul.insn,
> +               hvmemul_ctxt->insn_buf_bytes);

This memcpy()s between dissimilar types. Please omit the & and
properly add .data on the second argument (and this .data
addition should then also be mirrored in the BUILD_BUG_ON()).

> +    }
> +    else if ( !vio->mmio_insn_bytes )

And then - I'm sorry for not having thought of this before - I think
this would better not live here, or have an effect more explicitly
only when coming here through hvm_emulate_one_vm_event().
Since the former seems impractical, I think giving _hvm_emulate_one()
one or two extra parameters would be the most straightforward
approach.

> @@ -1944,11 +1954,11 @@ void hvm_mem_access_emulate_one(enum emul_kind kind, 
> unsigned int trapnr,
>      case EMUL_KIND_NOWRITE:
>          rc = hvm_emulate_one_no_write(&ctx);
>          break;
> -    case EMUL_KIND_SET_CONTEXT:
> -        ctx.set_context = 1;
> -        /* Intentional fall-through. */
>      default:
> +        ctx.set_context_data = (kind == EMUL_KIND_SET_CONTEXT_DATA);
> +        ctx.set_context_insn = (kind == EMUL_KIND_SET_CONTEXT_INSN);
>          rc = hvm_emulate_one(&ctx);
> +        break;
>      }

Thanks - this is a lot better readable now!

> @@ -1983,7 +1993,8 @@ void hvm_emulate_prepare(
>      hvmemul_ctxt->ctxt.force_writeback = 1;
>      hvmemul_ctxt->seg_reg_accessed = 0;
>      hvmemul_ctxt->seg_reg_dirty = 0;
> -    hvmemul_ctxt->set_context = 0;
> +    hvmemul_ctxt->set_context_data = 0;
> +    hvmemul_ctxt->set_context_insn = 0;

I think you can almost guess the comment here and on the declaration
of these fields: Please use false here and plain bool there.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -489,13 +489,16 @@ void hvm_do_resume(struct vcpu *v)
>  
>              if ( v->arch.vm_event->emulate_flags &
>                   VM_EVENT_FLAG_SET_EMUL_READ_DATA )
> -                kind = EMUL_KIND_SET_CONTEXT;
> +                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;

The header talking about incompatibilities between these flags -
wouldn't it be a good idea to actually -EINVAL such combinations?

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -57,6 +57,7 @@
>  #include <asm/altp2m.h>
>  #include <asm/event.h>
>  #include <asm/monitor.h>
> +#include <asm/vm_event.h>
>  #include <public/arch-x86/cpuid.h>

I have to admit that looking through the header changes you do I
can't figure why this adjustment is necessary.

> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -209,11 +209,18 @@ void vm_event_emulate_check(struct vcpu *v, 
> vm_event_response_t *rsp)
>          if ( p2m_mem_access_emulate_check(v, rsp) )
>          {
>              if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA )
> -                v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
> +                v->arch.vm_event->emul.read = rsp->data.emul.read;
>  
>              v->arch.vm_event->emulate_flags = rsp->flags;
>          }
>          break;
> +    case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
> +        if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
> +        {
> +            v->arch.vm_event->emul.insn = rsp->data.emul.insn;
> +            v->arch.vm_event->emulate_flags = rsp->flags;
> +        }
> +        break;
>      default:

Blank lines between non-fall-through case statements please (part
of me thinks I've said so before).

> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -398,7 +398,6 @@ void vm_event_resume(struct domain *d, struct 
> vm_event_domain *ved)
>           * In some cases the response type needs extra handling, so here
>           * we call the appropriate handlers.
>           */
> -
>          /* Check flags which apply only when the vCPU is paused */

Stray change.

Jan

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