From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v2 01/17] x86/hvm: simplify hvmemul_do_io() Date: Wed, 17 Jun 2015 15:47:45 +0100 Message-ID: <5581A4B102000078000863F4@mail.emea.novell.com> References: <1434037381-10917-1-git-send-email-paul.durrant@citrix.com> <1434037381-10917-2-git-send-email-paul.durrant@citrix.com> <558192CF02000078000862CD@mail.emea.novell.com> <9AAE0902D5BC7E449B7C8E4E778ABCD0259531E4@AMSPEX01CL01.citrite.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Z5Ed2-0001Ce-8I for xen-devel@lists.xenproject.org; Wed, 17 Jun 2015 14:47:48 +0000 In-Reply-To: <9AAE0902D5BC7E449B7C8E4E778ABCD0259531E4@AMSPEX01CL01.citrite.net> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Paul Durrant Cc: Andrew Cooper , "Keir(Xen.org)" , "xen-devel@lists.xenproject.org" List-Id: xen-devel@lists.xenproject.org >>> On 17.06.15 at 15:54, wrote: >> -----Original Message----- >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: 17 June 2015 14:31 >> >>> On 11.06.15 at 17:42, 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 and . indicates the >> > + * direction: IOREQ_READ means a read from to and >> > + * IOREQ_WRITE means a write from to . Each access has >> > + * width and up to * accesses will be performed. If >> > + * X86EMUL_OKAY is returned then will be updated with the >> number >> > + * of accesses actually performed. >> > + * >> > + * NOTE: If * is greater than 1, each access will use the >> > + * pointer; there is no implicit interation over a >> > + * block of memory starting at . >> > + */ >> > +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