Re: [Xen-devel] [PATCH v2 01/17] x86/hvm: simplify hvmemul_do_io()

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 17 June 2015 15:48
> To: Paul Durrant
> Cc: Andrew Cooper; xen-devel@xxxxxxxxxxxxxxxxxxxx; Keir (Xen.org)
> Subject: RE: [PATCH v2 01/17] x86/hvm: simplify hvmemul_do_io()
> 
> >>> On 17.06.15 at 15:54, <Paul.Durrant@xxxxxxxxxx> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 17 June 2015 14:31
> >> >>> On 11.06.15 at 17:42, <paul.durrant@xxxxxxxxxx> wrote:
> >> > @@ -190,7 +141,12 @@ static int hvmemul_do_io(
> >> >      p.count = *reps;
> >> >
> >> >      if ( dir == IOREQ_WRITE )
> >> > +    {
> >> > +        if ( !data_is_addr )
> >> > +            memcpy(&p.data, p_data, size);
> >> > +
> >> >          hvmtrace_io_assist(is_mmio, &p);
> >> > +    }
> >>
> >> Why would you need to do this _only_ here (i.e. either not at all,
> >> or not just for tracing purposes)? Or is this just to _restore_ the
> >> original value for tracing (in which case the question would be
> >> whether it indeed can get modified)?
> >
> > Not sure I follow. If hvmemul_do_io is  called with !data_is_addr then the
> > data arg will be a pointer to a buffer, rather than a gpa. So, the gpa that
> > was first copied into the ioreq (i.e. p) needs to be overwritten with the
> > actual data if it's a write.
> 
> Ah, it's being moved here from about 100 lines up (in the original
> code) - I simply forgot about that deletion by the time I got here.
> 
> >> > +static void hvmemul_release_page(struct page_info *page)
> >>
> >> inline?
> >>
> >
> > Probably, but I was hoping the compiler would make that decision.
> 
> Likely it will, but for functions as simple as this one we can help it.
> 
> >> > +/*
> >> > + * Perform I/O between <port> and <buffer>. <dir> indicates the
> >> > + * direction: IOREQ_READ means a read from <port> to <buffer> and
> >> > + * IOREQ_WRITE means a write from <buffer> to <port>. Each access
> has
> >> > + * width <size> and up to *<reps> accesses will be performed. If
> >> > + * X86EMUL_OKAY is returned then <reps> will be updated with the
> >> number
> >> > + * of accesses actually performed.
> >> > + *
> >> > + * NOTE: If *<reps> is greater than 1, each access will use the
> >> > + *       <buffer> pointer; there is no implicit interation over a
> >> > + *       block of memory starting at <buffer>.
> >> > + */
> >> > +int hvmemul_do_pio_buffer(uint16_t port,
> >> > +                          unsigned long *reps,
> >>
> >> Considering the comment - can't you instead drop the reps parameter
> >> here then?
> >>
> >
> > No. A rep stos does multiple port writes from the same buffer pointer.
> 
> A REP STOS doesn't do any port writes at all. REP OUTS does, but
> it accesses "a block of memory", which the comment specifically says
> doesn't happen here. I.e. I still think either the comment is wrong (or
> at least misleading) or the function could be simplified.
> 

Hmm. Good point. I'm sure I ran into a case where something was trying to multi 
rep port I/O from a buffer, but you're right... it makes no sense.

  Paul

> Jan


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