Re: [Xen-devel] [PATCH 2/2] x86/vm_event: Allow returning i-cache for emulation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.09.16 at 20:12, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> When emulating instructions the emulator maintains a small i-cache fetched
> from the guest memory. This patch extends the vm_event interface to allow
> returning this i-cache via the vm_event response instead.

I guess I'm sightly confused: Isn't the purpose to have the monitoring
app _write_ to the cache maintained in Xen? Or else, who is
"emulator" here? If that's the app, then I think subject and description
for hypervisor patches would better be written taking the perspective
of the hypervisor, especially when using potentially ambiguous terms.

> Note: This patch only has been compile-tested.

This certainly needs to change before this can be committed.

> @@ -1793,7 +1793,14 @@ 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 )
> +    {
> +        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);
> +    }
> +    else if ( !vio->mmio_insn_bytes )
>      {
>          hvmemul_ctxt->insn_buf_bytes =
>              hvm_get_insn_bytes(curr, hvmemul_ctxt->insn_buf) ?:



> @@ -1944,11 +1951,19 @@ 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:
> +    case EMUL_KIND_SET_CONTEXT_DATA:
> +        ctx.set_context_data = 1;
> +        rc = hvm_emulate_one(&ctx);
> +        break;
> +    case EMUL_KIND_SET_CONTEXT_INSN:
> +        ctx.set_context_insn = 1;
>          rc = hvm_emulate_one(&ctx);
> +        break;
> +    case EMUL_KIND_NORMAL:
> +        rc = hvm_emulate_one(&ctx);
> +        break;

One of the former two surely can be made fall through into the latter?

> +    default:
> +        return;

Why don't you retain the previous default handling?

> --- a/xen/include/asm-x86/hvm/emulate.h
> +++ b/xen/include/asm-x86/hvm/emulate.h
> @@ -34,20 +34,22 @@ struct hvm_emulate_ctxt {
>  
>      uint32_t intr_shadow;
>  
> -    bool_t set_context;
> +    bool_t set_context_data;
> +    bool_t set_context_insn;

As you have been told by others on patch 1 already - please take
the opportunity to switch to plain bool.

> --- a/xen/include/asm-x86/vm_event.h
> +++ b/xen/include/asm-x86/vm_event.h
> @@ -27,7 +27,8 @@
>   */
>  struct arch_vm_event {
>      uint32_t emulate_flags;
> -    struct vm_event_emul_read_data emul_read_data;
> +    struct vm_event_emul_read_data emul_read;
> +    struct vm_event_emul_insn_data emul_insn;

Why don't these get put in a union, when ...

> --- a/xen/include/public/vm_event.h
> +++ b/xen/include/public/vm_event.h
> @@ -97,6 +97,13 @@
>   * Requires the vCPU to be paused already (synchronous events only).
>   */
>  #define VM_EVENT_FLAG_SET_REGISTERS      (1 << 8)
> +/*
> + * Instruction cache is being sent back to the hypervisor in the event 
> response
> + * to be used by the emulator. This flag is only useful when combined with
> + * VM_EVENT_FLAG_EMULATE and is incompatible with also setting
> + * VM_EVENT_FLAG_EMULATE_NOWRITE or VM_EVENT_FLAG_SET_EMUL_READ_DATA.
> + */
> +#define VM_EVENT_FLAG_SET_EMUL_INSN_DATA (1 << 9)

... place these restrictions and use a union in in the public header?

> @@ -265,6 +272,10 @@ struct vm_event_emul_read_data {
>      uint8_t  data[sizeof(struct vm_event_regs_x86) - sizeof(uint32_t)];
>  };
>  
> +struct vm_event_emul_insn_data {
> +    uint8_t data[16]; /* Has to be completely filled */
> +};

Any reason for the 16 (rather than the architectural 15) here?

Jan


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