From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Durrant Subject: Re: [PATCH v2 04/17] x86/hvm: unify dpci portio intercept with standard portio intercept Date: Wed, 17 Jun 2015 14:46:21 +0000 Message-ID: <9AAE0902D5BC7E449B7C8E4E778ABCD0259535FB@AMSPEX01CL01.citrite.net> References: <1434037381-10917-1-git-send-email-paul.durrant@citrix.com> <1434037381-10917-5-git-send-email-paul.durrant@citrix.com> <5581A21D02000078000863C1@mail.emea.novell.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 1Z5Ebg-0000oD-O2 for xen-devel@lists.xenproject.org; Wed, 17 Jun 2015 14:46:24 +0000 In-Reply-To: <5581A21D02000078000863C1@mail.emea.novell.com> Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Andrew Cooper , "Keir (Xen.org)" , "xen-devel@lists.xenproject.org" List-Id: xen-devel@lists.xenproject.org > -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 17 June 2015 15:37 > To: Paul Durrant > Cc: Andrew Cooper; xen-devel@lists.xenproject.org; Keir (Xen.org) > Subject: Re: [PATCH v2 04/17] x86/hvm: unify dpci portio intercept with > standard portio intercept > > >>> On 11.06.15 at 17:42, wrote: > > --- a/xen/arch/x86/hvm/io.c > > +++ b/xen/arch/x86/hvm/io.c > > @@ -208,185 +208,113 @@ void hvm_io_assist(ioreq_t *p) > > } > > } > > > > -static int dpci_ioport_read(uint32_t mport, ioreq_t *p) > > +static bool_t dpci_portio_accept(struct hvm_io_handler *io_handler, > > + struct vcpu *v, > > + uint64_t addr, > > + uint32_t size) > > { > > - struct hvm_vcpu_io *vio = ¤t->arch.hvm_vcpu.hvm_io; > > - int rc = X86EMUL_OKAY, i, step = p->df ? -p->size : p->size; > > - uint32_t data = 0; > > - > > - for ( i = 0; i < p->count; i++ ) > > - { > > - if ( vio->mmio_retrying ) > > - { > > - if ( vio->mmio_large_read_bytes != p->size ) > > - return X86EMUL_UNHANDLEABLE; > > - memcpy(&data, vio->mmio_large_read, p->size); > > - vio->mmio_large_read_bytes = 0; > > - vio->mmio_retrying = 0; > > - } > > - else switch ( p->size ) > > - { > > - case 1: > > - data = inb(mport); > > - break; > > - case 2: > > - data = inw(mport); > > - break; > > - case 4: > > - data = inl(mport); > > - break; > > - default: > > - BUG(); > > - } > > + struct hvm_iommu *hd = domain_hvm_iommu(v->domain); > > + struct hvm_vcpu_io *vio = &v->arch.hvm_vcpu.hvm_io; > > + struct g2m_ioport *g2m_ioport; > > + uint32_t start, end; > > + uint32_t gport = addr, mport; > > > > - if ( p->data_is_ptr ) > > - { > > - switch ( hvm_copy_to_guest_phys(p->data + step * i, > > - &data, p->size) ) > > - { > > - case HVMCOPY_okay: > > - break; > > - case HVMCOPY_gfn_paged_out: > > - case HVMCOPY_gfn_shared: > > - rc = X86EMUL_RETRY; > > - break; > > - case HVMCOPY_bad_gfn_to_mfn: > > - /* Drop the write as real hardware would. */ > > - continue; > > - case HVMCOPY_bad_gva_to_gfn: > > - ASSERT(0); > > - /* fall through */ > > - default: > > - rc = X86EMUL_UNHANDLEABLE; > > - break; > > - } > > - if ( rc != X86EMUL_OKAY) > > - break; > > - } > > - else > > - p->data = data; > > - } > > > > - if ( rc == X86EMUL_RETRY ) > > + list_for_each_entry( g2m_ioport, &hd->arch.g2m_ioport_list, list ) > > { > > - vio->mmio_retry = 1; > > - vio->mmio_large_read_bytes = p->size; > > - memcpy(vio->mmio_large_read, &data, p->size); > > + start = g2m_ioport->gport; > > + end = start + g2m_ioport->np; > > + if ( (gport >= start) && (gport < end) ) > > + goto found; > > } > > > > - if ( i != 0 ) > > + return 0; > > + > > + found: > > + mport = (gport - start) + g2m_ioport->mport; > > + > > + if ( !ioports_access_permitted(current->domain, mport, > > Isn't v == current here (which of course should be made obvious > again by naming it curr)? And then - is this check needed here at > all? I realize you just move it, but the permission check should be > (and is being) done in the XEN_DOMCTL_ioport_mapping handler. > Ok. I'll happily drop the check. > > +void register_dpci_portio_handler(struct domain *d) > > +{ > > + struct hvm_io_handler *handler = &d- > >arch.hvm_domain.dpci_handler; > > > > - switch ( p->dir ) > > - { > > - case IOREQ_READ: > > - rc = dpci_ioport_read(mport, p); > > - break; > > - case IOREQ_WRITE: > > - rc = dpci_ioport_write(mport, p); > > - break; > > - default: > > - gdprintk(XENLOG_ERR, "Error: couldn't handle p->dir = %d", p->dir); > > - rc = X86EMUL_UNHANDLEABLE; > > - } > > + handler->type = IOREQ_TYPE_PIO; > > + handler->ops = &dpci_portio_ops; > > > > - return rc; > > + hvm_register_io_handler(d, handler, 1); > > Again registering a pointer into struct domain, with invariant > initialization. > Agreed, but I don't see any other way of unifying this with the main process loop. Do you have an alternative suggestion? Paul > Jan