From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v2 03/17] x86/hvm: unify internal portio and mmio intercepts Date: Wed, 17 Jun 2015 15:22:35 +0100 Message-ID: <55819ECB0200007800086386@mail.emea.novell.com> References: <1434037381-10917-1-git-send-email-paul.durrant@citrix.com> <1434037381-10917-4-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.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Z5EEf-0003wG-Pm for xen-devel@lists.xenproject.org; Wed, 17 Jun 2015 14:22:38 +0000 In-Reply-To: <1434037381-10917-4-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: > - fail2: > + fail6: > rtc_deinit(d); > stdvga_deinit(d); > vioapic_deinit(d); > - fail1: > + fail5: > if ( is_hardware_domain(d) ) > xfree(d->arch.hvm_domain.io_bitmap); > - xfree(d->arch.hvm_domain.io_handler); > + fail4: > + xfree(d->arch.hvm_domain.portio_handler); > + fail3: > + xfree(d->arch.hvm_domain.mmio_handler); > + fail2: > xfree(d->arch.hvm_domain.params); > + fail1: > fail0: We certainly don't want two consecutive labels. And with struct domain starting out zero-initialized you don't need these many earlier labels either (xfree(NULL) is perfectly fine). > @@ -86,31 +175,40 @@ static int hvm_mmio_access(struct vcpu *v, > } > else > { > - rc = read(v, p->addr + step * i, p->size, &data); > + addr = (p->type == IOREQ_TYPE_COPY) ? > + p->addr + step * i : > + p->addr; > + rc = ops->read(handler, v, addr, p->size, &data); Why this IOREQ_TYPE_COPY dependency that wasn't there before? > if ( rc != X86EMUL_OKAY ) > break; > } > - switch ( hvm_copy_to_guest_phys(p->data + step * i, > - &data, p->size) ) > + > + if ( p->data_is_ptr ) > { > - 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; > + 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); ASSERT_UNREACHABLE() please if you already touch such code. > @@ -163,240 +270,182 @@ static int hvm_mmio_access(struct vcpu *v, > return rc; > } > > -bool_t hvm_mmio_internal(paddr_t gpa) > -{ > - struct vcpu *curr = current; > - unsigned int i; > - > - for ( i = 0; i < HVM_MMIO_HANDLER_NR; ++i ) > - if ( hvm_mmio_handlers[i]->check(curr, gpa) ) > - return 1; > +static DEFINE_RCU_READ_LOCK(intercept_rcu_lock); > > - return 0; > -} > - > -int hvm_mmio_intercept(ioreq_t *p) > +struct hvm_io_handler *hvm_find_io_handler(struct vcpu *v, I suppose v == current here, i.e. it should be named curr. And I guess the function should return a pointer to const. > + ioreq_t *p) > { > - struct vcpu *v = current; > - int i; > + struct domain *d = v->domain; > + struct hvm_io_handler *handler; > > - for ( i = 0; i < HVM_MMIO_HANDLER_NR; i++ ) > - { > - hvm_mmio_check_t check = > - hvm_mmio_handlers[i]->check; > + rcu_read_lock(&intercept_rcu_lock); > > - if ( check(v, p->addr) ) > - { > - if ( unlikely(p->count > 1) && > - !check(v, unlikely(p->df) > - ? p->addr - (p->count - 1L) * p->size > - : p->addr + (p->count - 1L) * p->size) ) > - p->count = 1; > - > - return hvm_mmio_access( > - v, p, > - hvm_mmio_handlers[i]->read, > - hvm_mmio_handlers[i]->write); > - } > - } > - > - return X86EMUL_UNHANDLEABLE; > -} > - > -static int process_portio_intercept(portio_action_t action, ioreq_t *p) > -{ > - 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; > - > - if ( !p->data_is_ptr ) > + list_for_each_entry( handler, > + &d->arch.hvm_domain.io_handler.list, > + list_entry ) > { > - if ( p->dir == IOREQ_READ ) > - { > - 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 > - rc = action(IOREQ_READ, p->addr, p->size, &data); > - p->data = data; > - } > - else > - { > - data = p->data; > - rc = action(IOREQ_WRITE, p->addr, p->size, &data); > - } > - return rc; > - } > + const struct hvm_io_ops *ops = handler->ops; > + uint64_t start, end; > > - if ( p->dir == IOREQ_READ ) > - { > - for ( i = 0; i < p->count; i++ ) > + if ( handler->type != p->type ) > + continue; > + > + switch ( handler->type ) > { > - if ( vio->mmio_retrying ) > + case IOREQ_TYPE_PIO: > + start = p->addr; > + end = p->addr + p->size; > + break; > + case IOREQ_TYPE_COPY: > + if ( p->df ) > { > - 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; > + start = (p->addr - (p->count - 1) * p->size); You're re-introducing a problem here that I fixed not so long ago: The way it's coded here the multiplication is a 32-bit unsigned one, but we need it to be a 64-bit signed one. I.e. you need 1L here (and elsewhere) instead of just 1. > + end = p->addr + p->size; > } > else > { > - rc = action(IOREQ_READ, p->addr, p->size, &data); > - if ( rc != X86EMUL_OKAY ) > - break; > - } > - 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; > + start = p->addr; > + end = p->addr + p->count * p->size; Same here - one of the multiplicands needs to be extended to long. > +done: Labels indented by at least one blank please. > +void register_mmio_handler(struct domain *d, const struct hvm_mmio_ops *ops) > +{ > + struct hvm_mmio_handler *mmio_handler; > + unsigned int i; > + > + for ( i = 0; i < NR_MMIO_HANDLERS; i++ ) > { > - if ( type != handler->hdl_list[i].type ) > - continue; > - addr = handler->hdl_list[i].addr; > - size = handler->hdl_list[i].size; > - if ( (p->addr >= addr) && > - ((p->addr + p->size) <= (addr + size)) ) > - { > - if ( type == HVM_PORTIO ) > - return process_portio_intercept( > - handler->hdl_list[i].action.portio, p); > + mmio_handler = &d->arch.hvm_domain.mmio_handler[i]; > > - if ( unlikely(p->count > 1) && > - (unlikely(p->df) > - ? p->addr - (p->count - 1L) * p->size < addr > - : p->addr + p->count * 1L * p->size - 1 >= addr + size) ) > - p->count = 1; > + if ( mmio_handler->io_handler.ops == NULL ) > + break; > + } > > - return handler->hdl_list[i].action.mmio(p); > - } > + BUG_ON(i == NR_MMIO_HANDLERS); > + > + mmio_handler->io_handler.type = IOREQ_TYPE_COPY; > + mmio_handler->io_handler.ops = &mmio_ops; > + > + hvm_register_io_handler(d, &mmio_handler->io_handler, 0); So you're building a linked list of elements you could go through as an array? And the ->io_handler initialization is the same for all array elements. This doesn't look like clear simplification to me. > + mmio_handler->ops = ops; And why is this being done after registration? And is _all_ of this really usefully per-domain anyway? > -void relocate_io_handler( > - struct domain *d, unsigned long old_addr, unsigned long new_addr, > - unsigned long size, int type) > +bool_t hvm_mmio_internal(paddr_t gpa) > { > - struct hvm_io_handler *handler = d->arch.hvm_domain.io_handler; > - int i; > - > - for ( i = 0; i < handler->num_slot; i++ ) > - if ( (handler->hdl_list[i].addr == old_addr) && > - (handler->hdl_list[i].size == size) && > - (handler->hdl_list[i].type == type) ) > - handler->hdl_list[i].addr = new_addr; > + ioreq_t p = { > + .type = IOREQ_TYPE_COPY, > + .addr = gpa > + }; > + > + return (hvm_find_io_handler(current, &p) != NULL); Pointless parentheses. > -typedef int (*portio_action_t)( > - int dir, uint32_t port, uint32_t bytes, uint32_t *val); > -typedef int (*mmio_action_t)(ioreq_t *); > -struct io_handler { > - int type; > - unsigned long addr; > - unsigned long size; > - union { > - portio_action_t portio; > - mmio_action_t mmio; > - void *ptr; > - } action; > -}; > - > -struct hvm_io_handler { > - int num_slot; > - struct io_handler hdl_list[MAX_IO_HANDLER]; > -}; > - > struct hvm_mmio_ops { > hvm_mmio_check_t check; > hvm_mmio_read_t read; > hvm_mmio_write_t write; > }; > > -extern const struct hvm_mmio_ops hpet_mmio_ops; > -extern const struct hvm_mmio_ops vlapic_mmio_ops; > -extern const struct hvm_mmio_ops vioapic_mmio_ops; > -extern const struct hvm_mmio_ops msixtbl_mmio_ops; > -extern const struct hvm_mmio_ops iommu_mmio_ops; As a result they should all become static in their source files. > +#define NR_MMIO_HANDLERS 5 > > -#define HVM_MMIO_HANDLER_NR 5 > +struct hvm_mmio_handler { > + const struct hvm_mmio_ops *ops; > + struct hvm_io_handler io_handler; > +}; > > -int hvm_io_intercept(ioreq_t *p, int type); > -void register_io_handler( > - struct domain *d, unsigned long addr, unsigned long size, > - void *action, int type); > -void relocate_io_handler( > - struct domain *d, unsigned long old_addr, unsigned long new_addr, > - unsigned long size, int type); > +typedef int (*portio_action_t)( > + int dir, uint32_t port, uint32_t bytes, uint32_t *val); > + > +#define NR_PORTIO_HANDLERS 16 > + > +struct hvm_portio_handler { > + uint64_t start, end; Why uint64_t? (Note that this is simply the place I spotted this first at - the problem appears to exist in all of the port I/O handling.) Jan