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

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Wed, Sep 14, 2016 at 9:55 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> 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.

Well, I thought it was obvious that the built-in emulator in Xen is
what this patch is referring to. Otherwise none of this makes sense.

>
>> Note: This patch only has been compile-tested.
>
> This certainly needs to change before this can be committed.

I know, it's in process. That's why I put this note here to not get
too hasty about merging it.

>> @@ -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?

That's what I did before by turning this into if-else's and you
requested to go back to a switch. With a switch though I'll rather
make the cases explicit as a fall-through just makes it harder to read
for no real benefit.

>
>> +    default:
>> +        return;
>
> Why don't you retain the previous default handling?

The default label is unused and this makes it more apparent when
reading the code.

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

Ack.

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

True, they can be put into a union there as well.

>
>> @@ -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?

Yes, see the definition in include/asm-x86/hvm/emulate.h:

struct hvm_emulate_ctxt {
    struct x86_emulate_ctxt ctxt;

    /* Cache of 16 bytes of instruction. */
    uint8_t insn_buf[16];

Tamas

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