From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 3/3] pt-irq fixes and improvements Date: Tue, 3 Jun 2014 15:50:43 +0100 Message-ID: <538DE0C3.5010203@citrix.com> References: <538DEF42020000780001767A@mail.emea.novell.com> <538DF12A020000780001769C@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Wrq37-0005JZ-4B for xen-devel@lists.xenproject.org; Tue, 03 Jun 2014 14:50:49 +0000 In-Reply-To: <538DF12A020000780001769C@mail.emea.novell.com> 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: Kevin Tian , Keir Fraser , Ian Jackson , Ian Campbell , Stefano Stabellini , Yang Z Zhang , xen-devel List-Id: xen-devel@lists.xenproject.org On 03/06/14 15:00, Jan Beulich wrote: > Tools side: > - don't silently ignore unrecognized PT_IRQ_TYPE_* values > - respect that the interface type contains a union, making the code at > once no longer depend on the hypervisor ignoring the bus field of the > PCI portion of the interface structure) > > Hypervisor side: > - don't ignore the PCI bus number passed in > - don't store values (gsi, link) calculated from other stored values > - avoid calling xfree() with a spin lock held where easily possible > - have pt_irq_destroy_bind() respect the passed in type > - scope reduction and constification of various variables > - use switch instead of if/else-if chains > - formatting > > Signed-off-by: Jan Beulich Reviewed-by: Andrew Cooper with a few suggestions for further cleanup... > --- a/tools/libxc/xc_domain.c > +++ b/tools/libxc/xc_domain.c > @@ -1702,15 +1702,20 @@ int xc_domain_bind_pt_irq( > bind->hvm_domid = domid; > bind->irq_type = irq_type; > bind->machine_irq = machine_irq; > - if ( irq_type == PT_IRQ_TYPE_PCI || > - irq_type == PT_IRQ_TYPE_MSI_TRANSLATE ) > + switch ( irq_type ) > { > + case PT_IRQ_TYPE_PCI: > + case PT_IRQ_TYPE_MSI_TRANSLATE: > bind->u.pci.bus = bus; > bind->u.pci.device = device; Given all this other cleanup, can you nuke the trailing whitespace here... > bind->u.pci.intx = intx; > - } > - else if ( irq_type == PT_IRQ_TYPE_ISA ) > + case PT_IRQ_TYPE_ISA: > bind->u.isa.isa_irq = isa_irq; > + break; > + default: > + errno = EINVAL; > + return -1; > + } > ... and here. > rc = do_domctl(xch, &domctl); > return rc; > @@ -1737,10 +1742,21 @@ int xc_domain_unbind_pt_irq( > bind->hvm_domid = domid; > bind->irq_type = irq_type; > bind->machine_irq = machine_irq; > - bind->u.pci.bus = bus; > - bind->u.pci.device = device; > - bind->u.pci.intx = intx; > - bind->u.isa.isa_irq = isa_irq; > + switch ( irq_type ) > + { > + case PT_IRQ_TYPE_PCI: > + case PT_IRQ_TYPE_MSI_TRANSLATE: > + bind->u.pci.bus = bus; > + bind->u.pci.device = device; > + bind->u.pci.intx = intx; > + break; > + case PT_IRQ_TYPE_ISA: > + bind->u.isa.isa_irq = isa_irq; > + break; > + default: > + errno = EINVAL; > + return -1; > + } > And here in this function. > @@ -96,13 +93,9 @@ void free_hvm_irq_dpci(struct hvm_irq_dp > int pt_irq_create_bind( > struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind) > { > - struct hvm_irq_dpci *hvm_irq_dpci = NULL; > + struct hvm_irq_dpci *hvm_irq_dpci; > struct hvm_pirq_dpci *pirq_dpci; > struct pirq *info; > - uint32_t guest_gsi; > - uint32_t device, intx, link; > - struct dev_intx_gsi_link *digl; > - struct hvm_girq_dpci_mapping *girq; > int rc, pirq = pt_irq_bind->machine_irq; > > if ( pirq < 0 || pirq >= d->nr_pirqs ) Given all the cleanup, there is a further piece which could be done between these two hunks. There is an int i (declared in a for loop initialiser, despite our fairly consistent C89 style), which should be unsigned. > @@ -136,7 +129,9 @@ int pt_irq_create_bind( > } > pirq_dpci = pirq_dpci(info); > > - if ( pt_irq_bind->irq_type == PT_IRQ_TYPE_MSI ) > + switch ( pt_irq_bind->irq_type ) > + { > + case PT_IRQ_TYPE_MSI: > { > uint8_t dest, dest_mode; > int dest_vcpu_id; >