|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 03/17] x86/hvm: unify internal portio and mmio intercepts
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@xxxxxxxx] > Sent: 17 June 2015 15:23 > To: Paul Durrant > Cc: Andrew Cooper; xen-devel@xxxxxxxxxxxxxxxxxxxx; Keir (Xen.org) > Subject: Re: [PATCH v2 03/17] x86/hvm: unify internal portio and mmio > intercepts > > >>> On 11.06.15 at 17:42, <paul.durrant@xxxxxxxxxx> 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). > Ok. I'll see if I can condense some of them. > > @@ -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? > Because the unified function is used for both port and memory mapped I/O and it's only correct to increment in the addr value in the mmio case (i.e. when ioreq type is 'copy'). > > 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. > Ok. I'll change them as I see them. > > @@ -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. > Ok. > > + 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. Ok. That's clearly a bit subtle though, since I missed it. Would it be more obvious to have something like: start = p->addr - (uint64_t)p->size * (p->count - 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. > Ok. > > +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. > In the process loop, because it works on both mmio and portio, I use a generic io_hander struct that I can then use container_of() to get back at the outer portio or mmio struct when I need to. I agree it looks complex, but it's a relatively small price to remove the almost totally duplicated code if we have separate process loops for mmio and portio. > > + mmio_handler->ops = ops; > > And why is this being done after registration? Possibly it doesn't. > > And is _all_ of this really usefully per-domain anyway? > Maybe the portio handlers could go global too but it seems more flexible to make things per-domain. > > -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. Ok. > > > -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. > Ok. > > +#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.) Because the common process loop and other functions deal in 64-bit vals so that they don't have to care whether they are port or mmio address values. They are cast down to 32-bits before the action is invoked. Paul > > Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |