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 14:31:27 +0100 Message-ID: <558192CF02000078000862CD@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Z5DRD-0001CS-7v for xen-devel@lists.xenproject.org; Wed, 17 Jun 2015 13:31:31 +0000 In-Reply-To: <1434037381-10917-2-git-send-email-paul.durrant@citrix.com> 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 Fraser , xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org >>> On 11.06.15 at 17:42, wrote: > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -51,41 +51,23 @@ static void hvmtrace_io_assist(int is_mmio, ioreq_t *p) > } > > static int hvmemul_do_io( > - int is_mmio, paddr_t addr, unsigned long *reps, int size, > - paddr_t ram_gpa, int dir, int df, void *p_data) > + int is_mmio, paddr_t addr, unsigned long *reps, unsigned int size, > + uint8_t dir, bool_t df, bool_t data_is_addr, uint64_t data) is_mmio surely can become bool_t too? > { > struct vcpu *curr = current; > - struct hvm_vcpu_io *vio; > + struct hvm_vcpu_io *vio = &curr->arch.hvm_vcpu.hvm_io; > ioreq_t p = { > .type = is_mmio ? IOREQ_TYPE_COPY : IOREQ_TYPE_PIO, > .addr = addr, > .size = size, > .dir = dir, > .df = df, > - .data = ram_gpa, > - .data_is_ptr = (p_data == NULL), > + .data = data, > + .data_is_ptr = data_is_addr, /* ioreq_t field name is misleading */ > }; > - unsigned long ram_gfn = paddr_to_pfn(ram_gpa); > - p2m_type_t p2mt; > - struct page_info *ram_page; > + void *p_data = (void *)data; While presumably compiling fine, casting uint64_t to a pointer looks bogus without an intermediate cast to (unsigned) long or (u)intptr_t. > @@ -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)? > -int hvmemul_do_pio( > - unsigned long port, unsigned long *reps, int size, > - paddr_t ram_gpa, int dir, int df, void *p_data) > +int hvmemul_do_io_buffer( static? > + bool_t is_mmio, paddr_t addr, unsigned long *reps, unsigned int size, > + uint8_t dir, bool_t df, uint8_t *buffer) > { > - return hvmemul_do_io(0, port, reps, size, ram_gpa, dir, df, p_data); > + int rc; > + > + BUG_ON(buffer == NULL); > + > + rc = hvmemul_do_io(is_mmio, addr, reps, size, dir, df, 0, > + (uint64_t)buffer); Same remark regarding a cast between uint64_t and a pointer. > +static int hvmemul_acquire_page(unsigned long gmfn, struct page_info **page) > +{ > + struct vcpu *v = current; > + struct domain *d = v->domain; curr and currd please (albeit I don't see the need for the former as a local variable). > +static void hvmemul_release_page(struct page_info *page) inline? > +int hvmemul_do_io_addr( static? > + bool_t is_mmio, paddr_t addr, unsigned long *reps, > + unsigned int size, uint8_t dir, bool_t df, paddr_t ram_gpa) > +{ > + struct page_info *ram_page; > + int rc; > + > + rc = hvmemul_acquire_page(paddr_to_pfn(ram_gpa), &ram_page); > + if ( rc != X86EMUL_OKAY ) > + return rc; > + > + rc = hvmemul_do_io(is_mmio, addr, reps, size, dir, df, 1, > + (uint64_t)ram_gpa); Pointless cast. > +/* > + * 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? > @@ -1142,7 +1236,8 @@ static int hvmemul_read_io( > { > unsigned long reps = 1; > *val = 0; > - return hvmemul_do_pio(port, &reps, bytes, 0, IOREQ_READ, 0, val); > + return hvmemul_do_pio_buffer(port, &reps, bytes, IOREQ_READ, > + (uint8_t *)val); > } > > static int hvmemul_write_io( > @@ -1152,7 +1247,8 @@ static int hvmemul_write_io( > struct x86_emulate_ctxt *ctxt) > { > unsigned long reps = 1; > - return hvmemul_do_pio(port, &reps, bytes, 0, IOREQ_WRITE, 0, &val); > + return hvmemul_do_pio_buffer(port, &reps, bytes, IOREQ_WRITE, > + (uint8_t *)&val); To avoid such bogus casts, please have hvmemul_do_pio_buffer() take void * instead. Jan