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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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