Re: [Xen-devel] [PATCH V2 1/3] xen/vm_access: Support for memory-content hiding

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.06.15 at 11:03, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -578,6 +578,28 @@ static int hvmemul_read(
>          container_of(ctxt, struct hvm_emulate_ctxt, ctxt));
>  }
>  
> +static int hvmemul_read_set_context(
> +    enum x86_segment seg,
> +    unsigned long offset,
> +    void *p_data,
> +    unsigned int bytes,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    struct vcpu *curr = current;
> +    unsigned int len;
> +
> +    if ( !curr->arch.vm_event.emul_read_data )
> +        return X86EMUL_UNHANDLEABLE;
> +
> +    len = min_t(unsigned int,
> +        bytes, curr->arch.vm_event.emul_read_data->size);
> +
> +    if ( len )
> +        memcpy(p_data, curr->arch.vm_event.emul_read_data->data, len);

And the rest of the destination simply remains unmodified /
uninitialized?

> @@ -891,14 +934,37 @@ static int hvmemul_rep_outs(
>                            !!(ctxt->regs->eflags & X86_EFLAGS_DF), NULL);
>  }
>  
> -static int hvmemul_rep_movs(
> +static int hvmemul_rep_outs_set_context(
> +    enum x86_segment src_seg,
> +    unsigned long src_offset,
> +    uint16_t dst_port,
> +    unsigned int bytes_per_rep,
> +    unsigned long *reps,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    struct vcpu *curr = current;
> +    unsigned int safe_bytes;
> +
> +    if ( !curr->arch.vm_event.emul_read_data )
> +        return X86EMUL_UNHANDLEABLE;
> +
> +    safe_bytes = min_t(unsigned int, bytes_per_rep,
> +        curr->arch.vm_event.emul_read_data->size);
> +
> +    return hvmemul_do_pio(dst_port, reps, safe_bytes, 0, IOREQ_WRITE,
> +                          !!(ctxt->regs->eflags & X86_EFLAGS_DF),
> +                          curr->arch.vm_event.emul_read_data->data);

This isn't consistent with e.g. rep_movs below - you shouldn't
reduce the width of the operation.

Also - did I overlook where *reps gets forced to 1? If it's being
done elsewhere, perhaps worth an ASSERT()?

> @@ -981,7 +1047,19 @@ static int hvmemul_rep_movs(
>       */
>      rc = hvm_copy_from_guest_phys(buf, sgpa, bytes);
>      if ( rc == HVMCOPY_okay )
> +    {
> +        struct vcpu *curr = current;
> +
> +        if ( unlikely(set_context) && curr->arch.vm_event.emul_read_data )
> +        {
> +            unsigned long safe_bytes = min_t(unsigned long, bytes,
> +                curr->arch.vm_event.emul_read_data->size);

The variable doesn't need to be unsigned long.

> @@ -1000,13 +1078,40 @@ static int hvmemul_rep_movs(
>      return X86EMUL_OKAY;
>  }
>  
> -static int hvmemul_rep_stos(
> +static int hvmemul_rep_movs(
> +   enum x86_segment src_seg,
> +   unsigned long src_offset,
> +   enum x86_segment dst_seg,
> +   unsigned long dst_offset,
> +   unsigned int bytes_per_rep,
> +   unsigned long *reps,
> +   struct x86_emulate_ctxt *ctxt)
> +{
> +    return _hvmemul_rep_movs(src_seg, src_offset, dst_seg, dst_offset,
> +                             bytes_per_rep, reps, ctxt, 0);
> +}
> +
> +static int hvmemul_rep_movs_set_context(
> +   enum x86_segment src_seg,
> +   unsigned long src_offset,
> +   enum x86_segment dst_seg,
> +   unsigned long dst_offset,
> +   unsigned int bytes_per_rep,
> +   unsigned long *reps,
> +   struct x86_emulate_ctxt *ctxt)
> +{
> +    return _hvmemul_rep_movs(src_seg, src_offset, dst_seg, dst_offset,
> +                             bytes_per_rep, reps, ctxt, 1);
> +}

Perhaps putting a flag hvmemul_ctxt would be better?

> @@ -1408,6 +1569,32 @@ static const struct x86_emulate_ops 
> hvm_emulate_ops_no_write = {
>      .invlpg        = hvmemul_invlpg
>  };
>  
> +static const struct x86_emulate_ops hvm_emulate_ops_set_context = {
> +    .read          = hvmemul_read_set_context,
> +    .insn_fetch    = hvmemul_insn_fetch,
> +    .write         = hvmemul_write,
> +    .cmpxchg       = hvmemul_cmpxchg_set_context,
> +    .rep_ins       = hvmemul_rep_ins,
> +    .rep_outs      = hvmemul_rep_outs_set_context,
> +    .rep_movs      = hvmemul_rep_movs_set_context,
> +    .rep_stos      = hvmemul_rep_stos_set_context,

If you don't override .write, why would you override .rep_stos?

> @@ -1528,18 +1715,31 @@ int hvm_emulate_one_no_write(
>      return _hvm_emulate_one(hvmemul_ctxt, &hvm_emulate_ops_no_write);
>  }
>  
> -void hvm_mem_access_emulate_one(bool_t nowrite, unsigned int trapnr,
> +int hvm_emulate_one_set_context(

static?

> +void hvm_mem_access_emulate_one(enum emul_kind kind, unsigned int trapnr,
>      unsigned int errcode)
>  {
>      struct hvm_emulate_ctxt ctx = {{ 0 }};
> -    int rc;
> +    int rc = X86EMUL_UNHANDLEABLE;
>  
>      hvm_emulate_prepare(&ctx, guest_cpu_user_regs());
>  
> -    if ( nowrite )
> -        rc = hvm_emulate_one_no_write(&ctx);
> -    else
> +    switch ( kind ) {
> +    case EMUL_KIND_NORMAL:
>          rc = hvm_emulate_one(&ctx);

Perhaps this should be the default case? If not, the initialization
of rc would better be put in the default case instead of at the top
of the function.

> @@ -63,6 +64,21 @@ static int vm_event_enable(
>      vm_event_ring_lock_init(ved);
>      vm_event_ring_lock(ved);
>  
> +    for_each_vcpu( d, v )
> +    {
> +        if ( v->arch.vm_event.emul_read_data )
> +            break;

continue?

Jan

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