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

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

On Mon, Sep 19, 2016 at 2:19 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> 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().

Yea.. I don't know what I was thinking =)

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

Ack.

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

The header just points out that setting all these flags at the same
time won't have a "combined" effect - evident from the if-else
treatment above. There is really no point to do an error, the error
would never reach the user. Setting response flags through vm_event
doesn't have an error-path back.

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

Yea, it's just a residue (the patch was first made when this header
was still included).

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

Ack.

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

Thanks!
Tamas

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