From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v3 07/18] x86/hvm: unify internal portio and mmio intercepts Date: Wed, 24 Jun 2015 13:07:21 +0100 Message-ID: <558AB9990200007800088DC8@mail.emea.novell.com> References: <1435055997-30017-1-git-send-email-paul.durrant@citrix.com> <1435055997-30017-8-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 1Z7jSg-00043p-PF for xen-devel@lists.xenproject.org; Wed, 24 Jun 2015 12:07:26 +0000 In-Reply-To: <1435055997-30017-8-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 23.06.15 at 12:39, wrote: > The implementation of mmio and portio intercepts is unnecessarily different. > This leads to much code duplication. This patch unifies much of the > intercept handling, leaving only distinct handlers for stdvga mmio and dpci > portio. Subsequent patches will unify those handlers. > > Signed-off-by: Paul Durrant > Cc: Keir Fraser > Cc: Jan Beulich > Cc: Andrew Cooper > --- Please help reviewers of previous versions of your patches by summarizing changes in the current version here. > +static int hvm_portio_read(struct hvm_io_handler *handler, > + uint64_t addr, > + uint64_t size, > + uint64_t *data) > { > - struct vcpu *curr = current; > - unsigned int i; > + uint32_t val = 0; > + int rc; > > - for ( i = 0; i < HVM_MMIO_HANDLER_NR; ++i ) > - if ( hvm_mmio_handlers[i]->check(curr, gpa) ) > - return 1; > + BUG_ON(handler->type != IOREQ_TYPE_PIO); > > - return 0; > + rc = handler->u.portio.action(IOREQ_READ, addr, size, &val); > + if ( rc == X86EMUL_OKAY ) > + *data = val; I think there would be no harm doing this unconditionally, and it would eliminate the potential of the caller using uninitialized data. > @@ -284,29 +185,37 @@ static int process_portio_intercept(portio_action_t action, ioreq_t *p) > { > for ( i = 0; i < p->count; i++ ) > { > - data = 0; > - switch ( hvm_copy_from_guest_phys(&data, p->data + step * i, > - 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: > - data = ~0; > - break; > - case HVMCOPY_bad_gva_to_gfn: > - ASSERT(0); > - /* fall through */ > - default: > - rc = X86EMUL_UNHANDLEABLE; > - break; > + switch ( hvm_copy_from_guest_phys(&data, p->data + step * i, > + 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: > + data = ~0; > + break; > + case HVMCOPY_bad_gva_to_gfn: > + ASSERT_UNREACHABLE(); > + /* fall through */ > + default: > + rc = X86EMUL_UNHANDLEABLE; > + break; > + } > + if ( rc != X86EMUL_OKAY ) > + break; > } > - if ( rc != X86EMUL_OKAY ) > - break; > - rc = action(IOREQ_WRITE, p->addr, p->size, &data); > + else > + data = p->data; > + > + addr = (p->type == IOREQ_TYPE_COPY) ? > + p->addr + step * i : > + p->addr; Indentation. > @@ -324,78 +233,133 @@ static int process_portio_intercept(portio_action_t action, ioreq_t *p) > return rc; > } > > -/* > - * Check if the request is handled inside xen > - * return value: 0 --not handled; 1 --handled > - */ > -int hvm_io_intercept(ioreq_t *p, int type) > +static struct hvm_io_handler *hvm_find_io_handler(ioreq_t *p) > +{ > + struct vcpu *curr = current; > + struct domain *curr_d = curr->domain; "curr" is used only once (here) and hence pointless as a local variable. > + const struct hvm_io_ops *ops = > + (p->type == IOREQ_TYPE_COPY) ? > + &mmio_ops : > + &portio_ops; > + unsigned int i; > + > + for ( i = 0; i < curr_d->arch.hvm_domain.io_handler_count; i++ ) > + { > + struct hvm_io_handler *handler = > + &curr_d->arch.hvm_domain.io_handler[i]; > + uint64_t start, end, count = p->count, size = p->size; I'm not really happy with all these 64-bit local variables, but I guess they are the result of you not wanting to do things the way they're currently done everywhere... From a logical pov, start and end would better be paddr_t, count and size unsigned long. > + if ( handler->type != p->type ) > + continue; > + > + switch ( handler->type ) > + { > + case IOREQ_TYPE_PIO: > + start = p->addr; > + end = p->addr + size; > + break; > + case IOREQ_TYPE_COPY: > + if ( p->df ) > + { > + start = (p->addr - (count - 1) * size); > + end = p->addr + size; > + } > + else > + { > + start = p->addr; > + end = p->addr + count * size; > + } > + break; > + default: > + BUG(); > + } Doing this over and over again during each iteration looks kind of wasteful. > +static struct hvm_io_handler *hvm_next_io_handler(struct domain *d) > { > - struct hvm_io_handler *handler = d->arch.hvm_domain.io_handler; > - int num = handler->num_slot; > + unsigned int i = d->arch.hvm_domain.io_handler_count++; > > - BUG_ON(num >= MAX_IO_HANDLER); > + if (i == NR_IO_HANDLERS) Codingy style. > + domain_crash(d); return NULL; (or else you, despite having checked, return a pointer to an out of bounds slot to your caller) > +void relocate_portio_handler(struct domain *d, uint32_t old_addr, > + uint32_t new_addr, uint32_t size) > +{ > + ioreq_t p = { > + .type = IOREQ_TYPE_PIO, > + .addr = old_addr, > + .size = size > + }; > + struct hvm_io_handler *handler = hvm_find_io_handler(&p); This isn't quite the same as the original checking - there was an exact match of size required. > +struct hvm_io_handler { > + union { > + struct { > + const struct hvm_mmio_ops *ops; > + } mmio; > + struct { > + uint32_t start, end; > + portio_action_t action; > + } portio; > + } u; No need to name this union, this is not a public interface. Jan