From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Zhang, Yang Z" Subject: Re: [PATCH v2 3/3] pt-irq fixes and improvements Date: Tue, 10 Jun 2014 00:32:30 +0000 Message-ID: References: <538DEF42020000780001767A@mail.emea.novell.com> <538DF12A020000780001769C@mail.emea.novell.com> <538DE0C3.5010203@citrix.com> <538EF3C00200007800017A1C@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WuA4K-0005RF-UM for xen-devel@lists.xenproject.org; Tue, 10 Jun 2014 00:37:41 +0000 In-Reply-To: <538EF3C00200007800017A1C@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 , xen-devel Cc: "Tian, Kevin" , Keir Fraser , Andrew Cooper , Ian Jackson , Ian Campbell , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org Jan Beulich wrote on 2014-06-04: > 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 Acked-by: Yang Zhang > --- > v2: A few extra coding style corrections according to Andrew's > suggestions. > --- a/tools/libxc/xc_domain.c > +++ b/tools/libxc/xc_domain.c > @@ -1702,16 +1702,21 @@ 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; > + bind->u.pci.device = device; > 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; > + } > + > rc = do_domctl(xch, &domctl); > return rc; > } > @@ -1737,11 +1742,22 @@ 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; > + } > + > rc = do_domctl(xch, &domctl); > return rc; > } > --- a/xen/arch/x86/physdev.c > +++ b/xen/arch/x86/physdev.c > @@ -37,9 +37,8 @@ static int physdev_hvm_map_pirq( > switch ( type ) > { > case MAP_PIRQ_TYPE_GSI: { > - struct hvm_irq_dpci *hvm_irq_dpci; > - struct hvm_girq_dpci_mapping *girq; > - uint32_t machine_gsi = 0; > + const struct hvm_irq_dpci *hvm_irq_dpci; > + unsigned int machine_gsi = 0; > > if ( *index < 0 || *index >= NR_HVM_IRQS ) { @@ -52,6 +51,8 @@ > static int physdev_hvm_map_pirq( hvm_irq_dpci = > domain_get_irq_dpci(d); if ( hvm_irq_dpci ) { > + const struct hvm_girq_dpci_mapping *girq; > + > BUILD_BUG_ON(ARRAY_SIZE(hvm_irq_dpci->girq) < NR_HVM_IRQS); > list_for_each_entry ( girq, > &hvm_irq_dpci->girq[*index], > --- a/xen/drivers/passthrough/io.c > +++ b/xen/drivers/passthrough/io.c > @@ -51,11 +51,8 @@ static int pt_irq_guest_eoi(struct domai static > void pt_irq_time_out(void *data) { > struct hvm_pirq_dpci *irq_map = data; > - unsigned int guest_gsi; > - struct hvm_irq_dpci *dpci = NULL; > - struct dev_intx_gsi_link *digl; > - struct hvm_girq_dpci_mapping *girq; > - uint32_t device, intx; > + const struct hvm_irq_dpci *dpci; > + const struct dev_intx_gsi_link *digl; > > spin_lock(&irq_map->dom->event_lock); > @@ -63,16 +60,16 @@ static void pt_irq_time_out(void *data) > ASSERT(dpci); > list_for_each_entry ( digl, &irq_map->digl_list, list ) > { > - guest_gsi = digl->gsi; > + unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx); > + const struct hvm_girq_dpci_mapping *girq; > + > list_for_each_entry ( girq, &dpci->girq[guest_gsi], list ) > { > struct pirq *pirq = pirq_info(irq_map->dom, > girq->machine_gsi); > > pirq_dpci(pirq)->flags |= HVM_IRQ_DPCI_EOI_LATCH; > } > - device = digl->device; > - intx = digl->intx; > - hvm_pci_intx_deassert(irq_map->dom, device, intx); > + hvm_pci_intx_deassert(irq_map->dom, digl->device, > + digl->intx); > } > > pt_pirq_iterate(irq_map->dom, pt_irq_guest_eoi, NULL); @@ -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 ) @@ -113,6 +106,8 @@ int > pt_irq_create_bind( hvm_irq_dpci = domain_get_irq_dpci(d); if ( > hvm_irq_dpci == NULL ) { > + unsigned int i; > + > hvm_irq_dpci = xzalloc(struct hvm_irq_dpci); if ( hvm_irq_dpci > == NULL ) { @@ -122,7 +117,7 @@ int pt_irq_create_bind( > softirq_tasklet_init( > &hvm_irq_dpci->dirq_tasklet, > hvm_dirq_assist, (unsigned long)d); > - for ( int i = 0; i < NR_HVM_IRQS; i++ ) > + for ( i = 0; i < NR_HVM_IRQS; i++ ) > INIT_LIST_HEAD(&hvm_irq_dpci->girq[i]); > d->arch.hvm_domain.irq.dpci = hvm_irq_dpci; @@ -136,7 +131,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; @@ -169,15 +166,16 > @@ int pt_irq_create_bind( { > uint32_t mask = HVM_IRQ_DPCI_MACH_MSI | > HVM_IRQ_DPCI_GUEST_MSI; > > - if ( (pirq_dpci->flags & mask) != mask) > + if ( (pirq_dpci->flags & mask) != mask ) > { > - spin_unlock(&d->event_lock); > - return -EBUSY; > + spin_unlock(&d->event_lock); > + return -EBUSY; > } > - /* if pirq is already mapped as vmsi, update the guest > data/addr */ > + /* If pirq is already mapped as vmsi, update guest > + data/addr. */ > if ( pirq_dpci->gmsi.gvec != pt_irq_bind->u.msi.gvec || > - pirq_dpci->gmsi.gflags != pt_irq_bind->u.msi.gflags) { > + pirq_dpci->gmsi.gflags != pt_irq_bind->u.msi.gflags ) > + { > /* Directly clear pending EOIs before enabling new MSI > info. */ pirq_guest_eoi(info); @@ -185,7 +183,7 @@ > int pt_irq_create_bind( > pirq_dpci->gmsi.gflags = pt_irq_bind->u.msi.gflags; > } > } > - /* Caculate dest_vcpu_id for MSI-type pirq migration */ > + /* Calculate dest_vcpu_id for MSI-type pirq migration. */ > dest = pirq_dpci->gmsi.gflags & VMSI_DEST_ID_MASK; dest_mode = > !!(pirq_dpci->gmsi.gflags & VMSI_DM_MASK); dest_vcpu_id = > hvm_girq_dest_2_vcpu_id(d, dest, dest_mode); @@ -193,36 +191,37 > @@ int pt_irq_create_bind( spin_unlock(&d->event_lock); if ( > dest_vcpu_id >= 0 ) > hvm_migrate_pirqs(d->vcpu[dest_vcpu_id]); > + break; > } > - else > + > + case PT_IRQ_TYPE_PCI: > + case PT_IRQ_TYPE_MSI_TRANSLATE: > { > - device = pt_irq_bind->u.pci.device; > - intx = pt_irq_bind->u.pci.intx; > - guest_gsi = hvm_pci_intx_gsi(device, intx); > - link = hvm_pci_intx_link(device, intx); > - hvm_irq_dpci->link_cnt[link]++; > + unsigned int bus = pt_irq_bind->u.pci.bus; > + unsigned int device = pt_irq_bind->u.pci.device; > + unsigned int intx = pt_irq_bind->u.pci.intx; > + unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx); > + unsigned int link = hvm_pci_intx_link(device, intx); > + struct dev_intx_gsi_link *digl = xmalloc(struct dev_intx_gsi_link); > + struct hvm_girq_dpci_mapping *girq = > + xmalloc(struct hvm_girq_dpci_mapping); > > - digl = xmalloc(struct dev_intx_gsi_link); > - if ( !digl ) > + if ( !digl || !girq ) > { > spin_unlock(&d->event_lock); > - return -ENOMEM; > - } > - > - girq = xmalloc(struct hvm_girq_dpci_mapping); > - if ( !girq ) > - { > + xfree(girq); > xfree(digl); - spin_unlock(&d->event_lock); > return -ENOMEM; > } > + hvm_irq_dpci->link_cnt[link]++; > + > + digl->bus = bus; > digl->device = device; > digl->intx = intx; > - digl->gsi = guest_gsi; > - digl->link = link; > list_add_tail(&digl->list, &pirq_dpci->digl_list); > + girq->bus = bus; > girq->device = device; > girq->intx = intx; > girq->machine_gsi = pirq; > @@ -261,12 +260,12 @@ int pt_irq_create_bind( > kill_timer(&pirq_dpci->timer); > pirq_dpci->dom = NULL; list_del(&girq->list); - > xfree(girq); list_del(&digl->list); > hvm_irq_dpci->link_cnt[link]--; pirq_dpci->flags = 0; > pirq_cleanup_check(info, d); > spin_unlock(&d->event_lock); + > xfree(girq); xfree(digl); return rc; > } > @@ -276,33 +275,51 @@ int pt_irq_create_bind( > > if ( iommu_verbose ) > dprintk(XENLOG_G_INFO, > - "d%d: bind: m_gsi=%u g_gsi=%u device=%u intx=%u\n", > - d->domain_id, pirq, guest_gsi, device, intx); + > "d%d: bind: m_gsi=%u g_gsi=%u dev=%02x.%02x.%u > intx=%u\n", + d->domain_id, pirq, guest_gsi, bus, + > PCI_SLOT(device), PCI_FUNC(device), intx); + > break; > } > + > + default: > + spin_unlock(&d->event_lock); > + return -EOPNOTSUPP; > + } > + > return 0; > } > > int pt_irq_destroy_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; > - uint32_t machine_gsi, guest_gsi; > - uint32_t device, intx, link; > + unsigned int machine_gsi = pt_irq_bind->machine_irq; > + unsigned int bus = pt_irq_bind->u.pci.bus; > + unsigned int device = pt_irq_bind->u.pci.device; > + unsigned int intx = pt_irq_bind->u.pci.intx; > + unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx); > + unsigned int link = hvm_pci_intx_link(device, intx); > struct dev_intx_gsi_link *digl, *tmp; > struct hvm_girq_dpci_mapping *girq; > struct pirq *pirq; > - machine_gsi = pt_irq_bind->machine_irq; > - device = pt_irq_bind->u.pci.device; > - intx = pt_irq_bind->u.pci.intx; > - guest_gsi = hvm_pci_intx_gsi(device, intx); > - link = hvm_pci_intx_link(device, intx); > + switch ( pt_irq_bind->irq_type ) > + { > + case PT_IRQ_TYPE_PCI: > + case PT_IRQ_TYPE_MSI_TRANSLATE: > + break; > + case PT_IRQ_TYPE_MSI: > + return 0; > + default: > + return -EOPNOTSUPP; > + } > > if ( iommu_verbose ) > dprintk(XENLOG_G_INFO, > - "d%d: unbind: m_gsi=%u g_gsi=%u device=%u intx=%u\n", - > d->domain_id, machine_gsi, guest_gsi, device, intx); + > "d%d: unbind: m_gsi=%u g_gsi=%u dev=%02x:%02x.%u > intx=%u\n", > + d->domain_id, machine_gsi, guest_gsi, bus, + > PCI_SLOT(device), PCI_FUNC(device), intx); > > spin_lock(&d->event_lock); > @@ -314,18 +331,28 @@ int pt_irq_destroy_bind( > return -EINVAL; > } > - hvm_irq_dpci->link_cnt[link]--; > - > list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list ) > { > - if ( girq->machine_gsi == machine_gsi ) > + if ( girq->bus == bus && > + girq->device == device && > + girq->intx == intx && > + girq->machine_gsi == machine_gsi ) > { > - list_del(&girq->list); > - xfree(girq); > - break; > + list_del(&girq->list); > + xfree(girq); > + girq = NULL; > + break; > } > } > + if ( girq ) > + { > + spin_unlock(&d->event_lock); > + return -EINVAL; > + } > + > + hvm_irq_dpci->link_cnt[link]--; > + > pirq = pirq_info(d, machine_gsi); > pirq_dpci = pirq_dpci(pirq); > @@ -334,10 +361,9 @@ int pt_irq_destroy_bind( > { > list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list ) > { > - if ( digl->device == device && > - digl->intx == intx && > - digl->link == link && > - digl->gsi == guest_gsi ) > + if ( digl->bus == bus && > + digl->device == device && > + digl->intx == intx ) > { > list_del(&digl->list); > xfree(digl); > @@ -359,8 +385,9 @@ int pt_irq_destroy_bind( > > if ( iommu_verbose ) > dprintk(XENLOG_G_INFO, > - "d%d unmap: m_irq=%u device=%u intx=%u\n", > - d->domain_id, machine_gsi, device, intx); > + "d%d unmap: m_irq=%u dev=%02x:%02x.%u intx=%u\n", > + d->domain_id, machine_gsi, bus, > + PCI_SLOT(device), PCI_FUNC(device), intx); > > return 0; > } > @@ -481,11 +508,10 @@ static void hvm_pci_msi_assert( static int > _hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci, > void *arg) { > - uint32_t device, intx; > - struct dev_intx_gsi_link *digl; > - > if ( test_and_clear_bool(pirq_dpci->masked) ) > { > + const struct dev_intx_gsi_link *digl; > + > if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI ) > { > hvm_pci_msi_assert(d, pirq_dpci); @@ -496,12 +522,10 @@ > static int _hvm_dirq_assist(struct domai > { > struct pirq *info = dpci_pirq(pirq_dpci); > - device = digl->device; > - intx = digl->intx; > if ( hvm_domain_use_pirq(d, info) ) > send_guest_pirq(d, info); > else > - hvm_pci_intx_assert(d, device, intx); > + hvm_pci_intx_assert(d, digl->device, digl->intx); > pirq_dpci->pending++; > > if ( pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE ) @@ > -537,16 +561,13 @@ static void hvm_dirq_assist(unsigned lon } > > static void __hvm_dpci_eoi(struct domain *d, > - struct hvm_girq_dpci_mapping *girq, > - union vioapic_redir_entry *ent) > + const struct hvm_girq_dpci_mapping *girq, > + const union vioapic_redir_entry *ent) > { > - uint32_t device, intx; > struct pirq *pirq; > struct hvm_pirq_dpci *pirq_dpci; > - device = girq->device; > - intx = girq->intx; > - hvm_pci_intx_deassert(d, device, intx); > + hvm_pci_intx_deassert(d, girq->device, girq->intx); > > pirq = pirq_info(d, girq->machine_gsi); > pirq_dpci = pirq_dpci(pirq); > @@ -556,8 +577,8 @@ static void __hvm_dpci_eoi(struct domain > * since interrupt is still not EOIed > */ > if ( --pirq_dpci->pending || > - ( ent && ent->fields.mask ) || > - ! pt_irq_need_timer(pirq_dpci->flags) ) > + (ent && ent->fields.mask) || > + !pt_irq_need_timer(pirq_dpci->flags) ) > return; > stop_timer(&pirq_dpci->timer); > @@ -565,10 +586,10 @@ static void __hvm_dpci_eoi(struct domain } > > void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi, > - union vioapic_redir_entry *ent) > + const union vioapic_redir_entry *ent) > { > - struct hvm_irq_dpci *hvm_irq_dpci; > - struct hvm_girq_dpci_mapping *girq; > + const struct hvm_irq_dpci *hvm_irq_dpci; > + const struct hvm_girq_dpci_mapping *girq; > > if ( !iommu_enabled ) > return; > --- a/xen/drivers/passthrough/vtd/x86/vtd.c > +++ b/xen/drivers/passthrough/vtd/x86/vtd.c > @@ -69,11 +69,13 @@ static int _hvm_dpci_isairq_eoi(struct d { > struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq; > unsigned int isairq = (long)arg; > - struct dev_intx_gsi_link *digl, *tmp; > + const struct dev_intx_gsi_link *digl; > > - list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list ) > + list_for_each_entry ( digl, &pirq_dpci->digl_list, list ) > { > - if ( hvm_irq->pci_link.route[digl->link] == isairq ) > + unsigned int link = hvm_pci_intx_link(digl->device, > + digl->intx); > + > + if ( hvm_irq->pci_link.route[link] == isairq ) > { > hvm_pci_intx_deassert(d, digl->device, digl->intx); > if ( --pirq_dpci->pending == 0 ) > --- a/xen/include/asm-x86/hvm/io.h > +++ b/xen/include/asm-x86/hvm/io.h > @@ -123,7 +123,7 @@ int handle_pio(uint16_t port, unsigned i void > hvm_interrupt_post(struct vcpu *v, int vector, int type); void > hvm_io_assist(ioreq_t *p); void hvm_dpci_eoi(struct domain *d, > unsigned int guest_irq, > - union vioapic_redir_entry *ent); > + const union vioapic_redir_entry *ent); > void msix_write_completion(struct vcpu *); > > struct hvm_hw_stdvga { > --- a/xen/include/xen/hvm/irq.h > +++ b/xen/include/xen/hvm/irq.h > @@ -30,10 +30,9 @@ > > struct dev_intx_gsi_link { > struct list_head list; + uint8_t bus; uint8_t device; uint8_t > intx; > - uint8_t gsi; > - uint8_t link; > }; > > #define _HVM_IRQ_DPCI_MACH_PCI_SHIFT 0 > @@ -69,6 +68,7 @@ struct hvm_gmsi_info { > > struct hvm_girq_dpci_mapping { > struct list_head list; + uint8_t bus; uint8_t device; uint8_t > intx; uint8_t machine_gsi; Best regards, Yang