Re: [Xen-devel] Ping: [PATCH] x86/HVM: fix hvmemul_rep_outs_set_context()

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 04 December 2017 10:13
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: Ping: [PATCH] x86/HVM: fix hvmemul_rep_outs_set_context()
> 
> >>> On 23.11.17 at 16:09, <JBeulich@xxxxxxxx> wrote:
> > There were two issues with this function: Its use of
> > hvmemul_do_pio_buffer() was wrong (the function deals only with
> > individual port accesses, not repeated ones, i.e. passing it
> > "*reps * bytes_per_rep" does not have the intended effect). And it
> > could have processed a larger set of operations in one go than was
> > probably intended (limited just by the size that xmalloc() can hand
> > back).
> >
> > By converting to proper use of hvmemul_do_pio_buffer(), no
> intermediate
> > buffer is needed at all. As a result a preemption check is being added.
> >
> > Also drop unused parameters from the function.
> >
> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Ping?

Apologies. In the midst of the discussion of the issue Andrew was hitting, I 
forgot about this one.

> 
> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -1348,28 +1348,41 @@ static int hvmemul_rep_ins(
> >  }
> >
> >  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)
> > +    unsigned long *reps)
> >  {
> > -    unsigned int bytes = *reps * bytes_per_rep;
> > -    char *buf;
> > -    int rc;
> > -
> > -    buf = xmalloc_array(char, bytes);
> > +    const struct arch_vm_event *ev = current->arch.vm_event;
> > +    const uint8_t *ptr;
> > +    unsigned int avail;
> > +    unsigned long done;
> > +    int rc = X86EMUL_OKAY;
> >
> > -    if ( buf == NULL )
> > +    ASSERT(bytes_per_rep <= 4);
> > +    if ( !ev )
> >          return X86EMUL_UNHANDLEABLE;
> >
> > -    rc = set_context_data(buf, bytes);
> > +    ptr = ev->emul.read.data;
> > +    avail = ev->emul.read.size;
> >
> > -    if ( rc == X86EMUL_OKAY )
> > -        rc = hvmemul_do_pio_buffer(dst_port, bytes, IOREQ_WRITE, buf);
> > +    for ( done = 0; done < *reps; ++done )
> > +    {
> > +        unsigned int size = min(bytes_per_rep, avail);
> > +        uint32_t data = 0;
> > +
> > +        if ( done && hypercall_preempt_check() )
> > +            break;
> > +
> > +        memcpy(&data, ptr, size);
> > +        avail -= size;
> > +        ptr += size;
> > +
> > +        rc = hvmemul_do_pio_buffer(dst_port, bytes_per_rep,
> IOREQ_WRITE,
> > &data);
> > +        if ( rc != X86EMUL_OKAY )
> > +            break;
> > +    }

This is a sub-optimal way to be handling this, but I don't see a lot of choice. 
It would, of course, be nicer to have the context buffer be mappable by QEMU 
then it could handle this as a single ioreq with count > 1 and indirect data 
but that would involve fixing the current problem of who owns the guest memory 
map so...

Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

> >
> > -    xfree(buf);
> > +    *reps = done;
> >
> >      return rc;
> >  }
> > @@ -1391,8 +1404,7 @@ static int hvmemul_rep_outs(
> >      int rc;
> >
> >      if ( unlikely(hvmemul_ctxt->set_context) )
> > -        return hvmemul_rep_outs_set_context(src_seg, src_offset, dst_port,
> > -                                            bytes_per_rep, reps, ctxt);
> > +        return hvmemul_rep_outs_set_context(dst_port, bytes_per_rep,
> reps);
> >
> >      rc = hvmemul_virtual_to_linear(
> >          src_seg, src_offset, bytes_per_rep, reps, hvm_access_read,
> >
> >
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@xxxxxxxxxxxxxxxxxxxx
> > https://lists.xenproject.org/mailman/listinfo/xen-devel
> 
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel