| Message ID | 20170419151128.87416-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 19.04.17 at 17:11, <roger.pau@citrix.com> wrote: > +int allocate_and_map_gsi_pirq(struct domain *d, int *index, int *pirq_p) > +{ > + int irq, pirq, ret; > + > + if ( *index < 0 || *index >= nr_irqs_gsi ) > + { > + dprintk(XENLOG_G_ERR, "dom%d: map invalid irq %d\n", d->domain_id, > + *index); > + return -EINVAL; > + } > + > + irq = domain_pirq_to_irq(current->domain, *index); > + if ( irq <= 0 ) > + { > + if ( is_hardware_domain(current->domain) ) > + irq = *index; > + else { Please adjust coding style issues like the brace placement here. > + pcidevs_lock(); > + /* Verify or get pirq. */ > + spin_lock(&d->event_lock); > + pirq = domain_irq_to_pirq(d, irq); > + > + if ( *pirq_p < 0 ) > + { > + if ( pirq ) > + { > + dprintk(XENLOG_G_ERR, "dom%d: %d:%d already mapped to %d\n", > + d->domain_id, *index, *pirq_p, pirq); > + if ( pirq < 0 ) > + { > + ret = -EBUSY; > + goto done; > + } > + } > + else > + { > + pirq = get_free_pirq(d, MAP_PIRQ_TYPE_GSI); > + if ( pirq < 0 ) > + { > + dprintk(XENLOG_G_ERR, "dom%d: no free pirq\n", d->domain_id); > + ret = pirq; > + goto done; > + } > + } > + } > + else > + { > + if ( pirq && pirq != *pirq_p ) > + { > + dprintk(XENLOG_G_ERR, "dom%d: pirq %d conflicts with irq %d\n", > + d->domain_id, *index, *pirq_p); > + ret = -EEXIST; > + goto done; > + } > + else > + pirq = *pirq_p; > + } > + > + ret = map_domain_pirq(d, pirq, irq, MAP_PIRQ_TYPE_GSI, NULL); > + if ( ret == 0 ) > + *pirq_p = pirq; > + > + done: > + spin_unlock(&d->event_lock); > + pcidevs_unlock(); All of the code above is being repeated in allocate_and_map_msi_pirq(), merely with the multi-MSI addition. This is too much code duplication for my taste. Additionally, with it being split like this it is then questionable what acquiring the PCI devices lock is good for here (I would think it is needed at most in the MSI case). > +int allocate_and_map_msi_pirq(struct domain *d, int *index, int *pirq_p, > + struct msi_info *msi) > +{ > + int irq, pirq, ret, type; > + > + irq = *index; > + if ( irq == -1 || msi->entry_nr > 1 ) > + irq = create_irq(NUMA_NO_NODE); This doesn't look to be an exact equivalent of the original code: Even with MAP_PIRQ_TYPE_MULTI_MSI entry_nr may be 1, and the original code calls create_irq() also in that case. If this is an intended change, the rationale should be provided in the commit message. But as you don't alter map_domain_pirq(), I doubt this is correct. > + if ( irq < nr_irqs_gsi || irq >= nr_irqs ) > + { > + dprintk(XENLOG_G_ERR, "dom%d: can't create irq for msi!\n", > + d->domain_id); > + ret = -EINVAL; > + goto free_irq; > + } > + > + msi->irq = irq; > + type = (msi->entry_nr > 1 && !msi->table_base) ? MAP_PIRQ_TYPE_MULTI_MSI > + : MAP_PIRQ_TYPE_MSI; Same here - you're not necessarily reconstructing the type passed into the hypercall. Jan
On Fri, May 12, 2017 at 06:56:01AM -0600, Jan Beulich wrote: > >>> On 19.04.17 at 17:11, <roger.pau@citrix.com> wrote: > > +int allocate_and_map_gsi_pirq(struct domain *d, int *index, int *pirq_p) > > +{ > > + int irq, pirq, ret; > > + > > + if ( *index < 0 || *index >= nr_irqs_gsi ) > > + { > > + dprintk(XENLOG_G_ERR, "dom%d: map invalid irq %d\n", d->domain_id, > > + *index); > > + return -EINVAL; > > + } > > + > > + irq = domain_pirq_to_irq(current->domain, *index); > > + if ( irq <= 0 ) > > + { > > + if ( is_hardware_domain(current->domain) ) > > + irq = *index; > > + else { > > Please adjust coding style issues like the brace placement here. Done. > > + pcidevs_lock(); > > + /* Verify or get pirq. */ > > + spin_lock(&d->event_lock); > > + pirq = domain_irq_to_pirq(d, irq); > > + > > + if ( *pirq_p < 0 ) > > + { > > + if ( pirq ) > > + { > > + dprintk(XENLOG_G_ERR, "dom%d: %d:%d already mapped to %d\n", > > + d->domain_id, *index, *pirq_p, pirq); > > + if ( pirq < 0 ) > > + { > > + ret = -EBUSY; > > + goto done; > > + } > > + } > > + else > > + { > > + pirq = get_free_pirq(d, MAP_PIRQ_TYPE_GSI); > > + if ( pirq < 0 ) > > + { > > + dprintk(XENLOG_G_ERR, "dom%d: no free pirq\n", d->domain_id); > > + ret = pirq; > > + goto done; > > + } > > + } > > + } > > + else > > + { > > + if ( pirq && pirq != *pirq_p ) > > + { > > + dprintk(XENLOG_G_ERR, "dom%d: pirq %d conflicts with irq %d\n", > > + d->domain_id, *index, *pirq_p); > > + ret = -EEXIST; > > + goto done; > > + } > > + else > > + pirq = *pirq_p; > > + } > > + > > + ret = map_domain_pirq(d, pirq, irq, MAP_PIRQ_TYPE_GSI, NULL); > > + if ( ret == 0 ) > > + *pirq_p = pirq; > > + > > + done: > > + spin_unlock(&d->event_lock); > > + pcidevs_unlock(); > > All of the code above is being repeated in allocate_and_map_msi_pirq(), > merely with the multi-MSI addition. This is too much code duplication for > my taste. I can try to factor this out into a separate helper that's used by both. > Additionally, with it being split like this it is then questionable > what acquiring the PCI devices lock is good for here (I would think it is > needed at most in the MSI case). Right, also I'm not sure why the PCI devices lock is acquired before calling into domain_irq_to_pirq, is that because of lock ordering rules with the domain event lock? > > +int allocate_and_map_msi_pirq(struct domain *d, int *index, int *pirq_p, > > + struct msi_info *msi) > > +{ > > + int irq, pirq, ret, type; > > + > > + irq = *index; > > + if ( irq == -1 || msi->entry_nr > 1 ) > > + irq = create_irq(NUMA_NO_NODE); > > This doesn't look to be an exact equivalent of the original code: Even > with MAP_PIRQ_TYPE_MULTI_MSI entry_nr may be 1, and the original > code calls create_irq() also in that case. If this is an intended change, > the rationale should be provided in the commit message. But as you > don't alter map_domain_pirq(), I doubt this is correct. My bad then, it's quite hard for me to figure out exactly what's the meaning/usage of those types, since they are not documented anywhere that I can find. physdev.h contains 3 different MSI related types: * MAP_PIRQ_TYPE_MSI_SEG maps into MAP_PIRQ_TYPE_MSI. * MAP_PIRQ_TYPE_MULTI_MSI is only available to map MSI interrupts (no MSI-X). * MAP_PIRQ_TYPE_MSI can map both MSI/MSI-X: - If table_base != 0 it's a MSI-X interrupt. - If table_base == 0 it's a single MSI interrupt AND entry_nr must be 1. Let me know if this is accurate. > > + if ( irq < nr_irqs_gsi || irq >= nr_irqs ) > > + { > > + dprintk(XENLOG_G_ERR, "dom%d: can't create irq for msi!\n", > > + d->domain_id); > > + ret = -EINVAL; > > + goto free_irq; > > + } > > + > > + msi->irq = irq; > > + type = (msi->entry_nr > 1 && !msi->table_base) ? MAP_PIRQ_TYPE_MULTI_MSI > > + : MAP_PIRQ_TYPE_MSI; > > Same here - you're not necessarily reconstructing the type passed > into the hypercall. Right, see my comment above. Thanks, Roger.
On Wed, Apr 19, 2017 at 04:11:26PM +0100, Roger Pau Monne wrote: [...] > + if ( *pirq_p < 0 ) > + { > + if ( pirq ) > + { > + dprintk(XENLOG_G_ERR, "dom%d: %d:%d already mapped to %d\n", > + d->domain_id, *index, *pirq_p, pirq); > + if ( pirq < 0 ) > + { > + ret = -EBUSY; > + goto done; > + } > + } > + else if ( msi->entry_nr > 1 && !msi->table_base ) > + { > + if ( msi->entry_nr <= 0 || msi->entry_nr > 32 ) > + ret = -EDOM; > + else if ( msi->entry_nr != 1 && !iommu_intremap ) > + ret = -EOPNOTSUPP; > + else > + { > + while ( msi->entry_nr & (msi->entry_nr - 1) ) > + msi->entry_nr += msi->entry_nr & -msi->entry_nr; > + pirq = get_free_pirqs(d, msi->entry_nr); > + ret = 0; > + if ( pirq < 0 ) > + { > + while ( (msi->entry_nr >>= 1) > 1 ) > + if ( get_free_pirqs(d, msi->entry_nr) > 0 ) > + break; Maybe I'm missing something, but I think the code above should be: if ( (pirq = get_free_pirqs(d, msi->entry_nr)) > 0 ) break; Or else the pirq returned by get_free_pirqs is not stored anywhere, and thus pirq will be -ENOSPC even when PIRQs have been allocated. Let me know if this is true or not, so that I can send a patch to fix the current code (which also has this issue). Roger.
>>> On 16.05.17 at 11:47, <roger.pau@citrix.com> wrote: > On Fri, May 12, 2017 at 06:56:01AM -0600, Jan Beulich wrote: >> >>> On 19.04.17 at 17:11, <roger.pau@citrix.com> wrote: >> > + pcidevs_lock(); >> > + /* Verify or get pirq. */ >> > + spin_lock(&d->event_lock); >> > + pirq = domain_irq_to_pirq(d, irq); >> > + >> > + if ( *pirq_p < 0 ) >> > + { >> > + if ( pirq ) >> > + { >> > + dprintk(XENLOG_G_ERR, "dom%d: %d:%d already mapped to %d\n", >> > + d->domain_id, *index, *pirq_p, pirq); >> > + if ( pirq < 0 ) >> > + { >> > + ret = -EBUSY; >> > + goto done; >> > + } >> > + } >> > + else >> > + { >> > + pirq = get_free_pirq(d, MAP_PIRQ_TYPE_GSI); >> > + if ( pirq < 0 ) >> > + { >> > + dprintk(XENLOG_G_ERR, "dom%d: no free pirq\n", d->domain_id); >> > + ret = pirq; >> > + goto done; >> > + } >> > + } >> > + } >> > + else >> > + { >> > + if ( pirq && pirq != *pirq_p ) >> > + { >> > + dprintk(XENLOG_G_ERR, "dom%d: pirq %d conflicts with irq %d\n", >> > + d->domain_id, *index, *pirq_p); >> > + ret = -EEXIST; >> > + goto done; >> > + } >> > + else >> > + pirq = *pirq_p; >> > + } >> > + >> > + ret = map_domain_pirq(d, pirq, irq, MAP_PIRQ_TYPE_GSI, NULL); >> > + if ( ret == 0 ) >> > + *pirq_p = pirq; >> > + >> > + done: >> > + spin_unlock(&d->event_lock); >> > + pcidevs_unlock(); >> >> All of the code above is being repeated in allocate_and_map_msi_pirq(), >> merely with the multi-MSI addition. This is too much code duplication for >> my taste. > > I can try to factor this out into a separate helper that's used by both. > >> Additionally, with it being split like this it is then questionable >> what acquiring the PCI devices lock is good for here (I would think it is >> needed at most in the MSI case). > > Right, also I'm not sure why the PCI devices lock is acquired before calling > into domain_irq_to_pirq, is that because of lock ordering rules with the domain > event lock? Yes. map_domain_pirq() in the MSI case requires both locks to be held. >> > +int allocate_and_map_msi_pirq(struct domain *d, int *index, int *pirq_p, >> > + struct msi_info *msi) >> > +{ >> > + int irq, pirq, ret, type; >> > + >> > + irq = *index; >> > + if ( irq == -1 || msi->entry_nr > 1 ) >> > + irq = create_irq(NUMA_NO_NODE); >> >> This doesn't look to be an exact equivalent of the original code: Even >> with MAP_PIRQ_TYPE_MULTI_MSI entry_nr may be 1, and the original >> code calls create_irq() also in that case. If this is an intended change, >> the rationale should be provided in the commit message. But as you >> don't alter map_domain_pirq(), I doubt this is correct. > > My bad then, it's quite hard for me to figure out exactly what's the > meaning/usage of those types, since they are not documented anywhere that I can > find. physdev.h contains 3 different MSI related types: > > * MAP_PIRQ_TYPE_MSI_SEG maps into MAP_PIRQ_TYPE_MSI. This was needed because of the extra segment information which wasn't part of the original MAP_PIRQ_TYPE_MSI. Otherwise the two are identical, so .. > * MAP_PIRQ_TYPE_MULTI_MSI is only available to map MSI interrupts (no MSI-X). > * MAP_PIRQ_TYPE_MSI can map both MSI/MSI-X: > - If table_base != 0 it's a MSI-X interrupt. > - If table_base == 0 it's a single MSI interrupt AND entry_nr must be 1. > > Let me know if this is accurate. ... almost - entry_nr when coming in as hypercall argument isn't required to be 1; instead physdev_map_pirq() sets it to 1 when table_base is zero. Jan
>>> On 16.05.17 at 13:52, <roger.pau@citrix.com> wrote: > On Wed, Apr 19, 2017 at 04:11:26PM +0100, Roger Pau Monne wrote: > [...] >> + if ( *pirq_p < 0 ) >> + { >> + if ( pirq ) >> + { >> + dprintk(XENLOG_G_ERR, "dom%d: %d:%d already mapped to %d\n", >> + d->domain_id, *index, *pirq_p, pirq); >> + if ( pirq < 0 ) >> + { >> + ret = -EBUSY; >> + goto done; >> + } >> + } >> + else if ( msi->entry_nr > 1 && !msi->table_base ) >> + { >> + if ( msi->entry_nr <= 0 || msi->entry_nr > 32 ) >> + ret = -EDOM; >> + else if ( msi->entry_nr != 1 && !iommu_intremap ) >> + ret = -EOPNOTSUPP; >> + else >> + { >> + while ( msi->entry_nr & (msi->entry_nr - 1) ) >> + msi->entry_nr += msi->entry_nr & -msi->entry_nr; >> + pirq = get_free_pirqs(d, msi->entry_nr); >> + ret = 0; >> + if ( pirq < 0 ) >> + { >> + while ( (msi->entry_nr >>= 1) > 1 ) >> + if ( get_free_pirqs(d, msi->entry_nr) > 0 ) >> + break; > > Maybe I'm missing something, but I think the code above should be: > > if ( (pirq = get_free_pirqs(d, msi->entry_nr)) > 0 ) > break; > > Or else the pirq returned by get_free_pirqs is not stored anywhere, and thus > pirq will be -ENOSPC even when PIRQs have been allocated. > > Let me know if this is true or not, so that I can send a patch to fix the > current code (which also has this issue). The code is correct. get_free_pirqs() doesn't alter the slots it finds free, and here we only want to know how many we could allocate at this point (given that we weren't able to allocate as many as were requested). Jan
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c index 676ba5216f..8f6a91994c 100644 --- a/xen/arch/x86/irq.c +++ b/xen/arch/x86/irq.c @@ -2537,3 +2537,178 @@ bool_t hvm_domain_use_pirq(const struct domain *d, const struct pirq *pirq) return is_hvm_domain(d) && pirq && pirq->arch.hvm.emuirq != IRQ_UNBOUND; } + +int allocate_and_map_gsi_pirq(struct domain *d, int *index, int *pirq_p) +{ + int irq, pirq, ret; + + if ( *index < 0 || *index >= nr_irqs_gsi ) + { + dprintk(XENLOG_G_ERR, "dom%d: map invalid irq %d\n", d->domain_id, + *index); + return -EINVAL; + } + + irq = domain_pirq_to_irq(current->domain, *index); + if ( irq <= 0 ) + { + if ( is_hardware_domain(current->domain) ) + irq = *index; + else { + dprintk(XENLOG_G_ERR, "dom%d: map pirq with incorrect irq!\n", + d->domain_id); + return -EINVAL; + } + } + + pcidevs_lock(); + /* Verify or get pirq. */ + spin_lock(&d->event_lock); + pirq = domain_irq_to_pirq(d, irq); + + if ( *pirq_p < 0 ) + { + if ( pirq ) + { + dprintk(XENLOG_G_ERR, "dom%d: %d:%d already mapped to %d\n", + d->domain_id, *index, *pirq_p, pirq); + if ( pirq < 0 ) + { + ret = -EBUSY; + goto done; + } + } + else + { + pirq = get_free_pirq(d, MAP_PIRQ_TYPE_GSI); + if ( pirq < 0 ) + { + dprintk(XENLOG_G_ERR, "dom%d: no free pirq\n", d->domain_id); + ret = pirq; + goto done; + } + } + } + else + { + if ( pirq && pirq != *pirq_p ) + { + dprintk(XENLOG_G_ERR, "dom%d: pirq %d conflicts with irq %d\n", + d->domain_id, *index, *pirq_p); + ret = -EEXIST; + goto done; + } + else + pirq = *pirq_p; + } + + ret = map_domain_pirq(d, pirq, irq, MAP_PIRQ_TYPE_GSI, NULL); + if ( ret == 0 ) + *pirq_p = pirq; + + done: + spin_unlock(&d->event_lock); + pcidevs_unlock(); + return ret; +} + +int allocate_and_map_msi_pirq(struct domain *d, int *index, int *pirq_p, + struct msi_info *msi) +{ + int irq, pirq, ret, type; + + irq = *index; + if ( irq == -1 || msi->entry_nr > 1 ) + irq = create_irq(NUMA_NO_NODE); + + if ( irq < nr_irqs_gsi || irq >= nr_irqs ) + { + dprintk(XENLOG_G_ERR, "dom%d: can't create irq for msi!\n", + d->domain_id); + ret = -EINVAL; + goto free_irq; + } + + msi->irq = irq; + type = (msi->entry_nr > 1 && !msi->table_base) ? MAP_PIRQ_TYPE_MULTI_MSI + : MAP_PIRQ_TYPE_MSI; + + pcidevs_lock(); + /* Verify or get pirq. */ + spin_lock(&d->event_lock); + pirq = domain_irq_to_pirq(d, irq); + if ( *pirq_p < 0 ) + { + if ( pirq ) + { + dprintk(XENLOG_G_ERR, "dom%d: %d:%d already mapped to %d\n", + d->domain_id, *index, *pirq_p, pirq); + if ( pirq < 0 ) + { + ret = -EBUSY; + goto done; + } + } + else if ( msi->entry_nr > 1 && !msi->table_base ) + { + if ( msi->entry_nr <= 0 || msi->entry_nr > 32 ) + ret = -EDOM; + else if ( msi->entry_nr != 1 && !iommu_intremap ) + ret = -EOPNOTSUPP; + else + { + while ( msi->entry_nr & (msi->entry_nr - 1) ) + msi->entry_nr += msi->entry_nr & -msi->entry_nr; + pirq = get_free_pirqs(d, msi->entry_nr); + ret = 0; + if ( pirq < 0 ) + { + while ( (msi->entry_nr >>= 1) > 1 ) + if ( get_free_pirqs(d, msi->entry_nr) > 0 ) + break; + dprintk(XENLOG_G_ERR, "dom%d: no block of %d free pirqs\n", + d->domain_id, msi->entry_nr << 1); + ret = pirq; + } + } + if ( ret < 0 ) + goto done; + } + else + { + pirq = get_free_pirq(d, type); + if ( pirq < 0 ) + { + dprintk(XENLOG_G_ERR, "dom%d: no free pirq\n", d->domain_id); + ret = pirq; + goto done; + } + } + } + else + { + if ( pirq && pirq != *pirq_p ) + { + dprintk(XENLOG_G_ERR, "dom%d: pirq %d conflicts with irq %d\n", + d->domain_id, *index, *pirq_p); + ret = -EEXIST; + goto done; + } + else + pirq = *pirq_p; + } + + ret = map_domain_pirq(d, pirq, irq, type, msi); + if ( ret == 0 ) + *pirq_p = pirq; + + done: + spin_unlock(&d->event_lock); + pcidevs_unlock(); + free_irq: + if ( ret != 0 && + ((msi->entry_nr > 1 && !msi->table_base) || *index == -1) ) + destroy_irq(irq); + return ret; +} + diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c index eec4a41231..b5eded4be9 100644 --- a/xen/arch/x86/physdev.c +++ b/xen/arch/x86/physdev.c @@ -92,8 +92,7 @@ int physdev_map_pirq(domid_t domid, int type, int *index, int *pirq_p, struct msi_info *msi) { struct domain *d = current->domain; - int pirq, irq, ret = 0; - void *map_data = NULL; + int ret; if ( domid == DOMID_SELF && is_hvm_domain(d) && has_pirq(d) ) { @@ -119,135 +118,22 @@ int physdev_map_pirq(domid_t domid, int type, int *index, int *pirq_p, switch ( type ) { case MAP_PIRQ_TYPE_GSI: - if ( *index < 0 || *index >= nr_irqs_gsi ) - { - dprintk(XENLOG_G_ERR, "dom%d: map invalid irq %d\n", - d->domain_id, *index); - ret = -EINVAL; - goto free_domain; - } - - irq = domain_pirq_to_irq(current->domain, *index); - if ( irq <= 0 ) - { - if ( is_hardware_domain(current->domain) ) - irq = *index; - else { - dprintk(XENLOG_G_ERR, "dom%d: map pirq with incorrect irq!\n", - d->domain_id); - ret = -EINVAL; - goto free_domain; - } - } + ret = allocate_and_map_gsi_pirq(d, index, pirq_p); break; - case MAP_PIRQ_TYPE_MSI: if ( !msi->table_base ) msi->entry_nr = 1; - irq = *index; - if ( irq == -1 ) + /* fallthrough */ case MAP_PIRQ_TYPE_MULTI_MSI: - irq = create_irq(NUMA_NO_NODE); - - if ( irq < nr_irqs_gsi || irq >= nr_irqs ) - { - dprintk(XENLOG_G_ERR, "dom%d: can't create irq for msi!\n", - d->domain_id); - ret = -EINVAL; - goto free_domain; - } - - msi->irq = irq; - map_data = msi; + ret = allocate_and_map_msi_pirq(d, index, pirq_p, msi); break; - default: dprintk(XENLOG_G_ERR, "dom%d: wrong map_pirq type %x\n", d->domain_id, type); ret = -EINVAL; - goto free_domain; - } - - pcidevs_lock(); - /* Verify or get pirq. */ - spin_lock(&d->event_lock); - pirq = domain_irq_to_pirq(d, irq); - if ( *pirq_p < 0 ) - { - if ( pirq ) - { - dprintk(XENLOG_G_ERR, "dom%d: %d:%d already mapped to %d\n", - d->domain_id, *index, *pirq_p, pirq); - if ( pirq < 0 ) - { - ret = -EBUSY; - goto done; - } - } - else if ( type == MAP_PIRQ_TYPE_MULTI_MSI ) - { - if ( msi->entry_nr <= 0 || msi->entry_nr > 32 ) - ret = -EDOM; - else if ( msi->entry_nr != 1 && !iommu_intremap ) - ret = -EOPNOTSUPP; - else - { - while ( msi->entry_nr & (msi->entry_nr - 1) ) - msi->entry_nr += msi->entry_nr & -msi->entry_nr; - pirq = get_free_pirqs(d, msi->entry_nr); - if ( pirq < 0 ) - { - while ( (msi->entry_nr >>= 1) > 1 ) - if ( get_free_pirqs(d, msi->entry_nr) > 0 ) - break; - dprintk(XENLOG_G_ERR, "dom%d: no block of %d free pirqs\n", - d->domain_id, msi->entry_nr << 1); - ret = pirq; - } - } - if ( ret < 0 ) - goto done; - } - else - { - pirq = get_free_pirq(d, type); - if ( pirq < 0 ) - { - dprintk(XENLOG_G_ERR, "dom%d: no free pirq\n", d->domain_id); - ret = pirq; - goto done; - } - } - } - else - { - if ( pirq && pirq != *pirq_p ) - { - dprintk(XENLOG_G_ERR, "dom%d: pirq %d conflicts with irq %d\n", - d->domain_id, *index, *pirq_p); - ret = -EEXIST; - goto done; - } - else - pirq = *pirq_p; + break; } - ret = map_domain_pirq(d, pirq, irq, type, map_data); - if ( ret == 0 ) - *pirq_p = pirq; - - done: - spin_unlock(&d->event_lock); - pcidevs_unlock(); - if ( ret != 0 ) - switch ( type ) - { - case MAP_PIRQ_TYPE_MSI: - if ( *index == -1 ) - case MAP_PIRQ_TYPE_MULTI_MSI: - destroy_irq(irq); - break; - } free_domain: rcu_unlock_domain(d); return ret; diff --git a/xen/include/asm-x86/irq.h b/xen/include/asm-x86/irq.h index ef625ebb13..2b1701e67b 100644 --- a/xen/include/asm-x86/irq.h +++ b/xen/include/asm-x86/irq.h @@ -200,4 +200,9 @@ bool_t cpu_has_pending_apic_eoi(void); static inline void arch_move_irqs(struct vcpu *v) { } +struct msi_info; +int allocate_and_map_gsi_pirq(struct domain *d, int *index, int *pirq_p); +int allocate_and_map_msi_pirq(struct domain *d, int *index, int *pirq_p, + struct msi_info *msi); + #endif /* _ASM_HW_IRQ_H */
Move the code to allocate and map a domain pirq (either GSI or MSI) into the x86 irq code base, so that it can be used outside of the physdev ops. This change shouldn't affect the functionality of the already existing physdev ops. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Jan Beulich <jbeulich@suse.com> Andrew Cooper <andrew.cooper3@citrix.com> --- Changes since v1: - New in this version. --- xen/arch/x86/irq.c | 175 ++++++++++++++++++++++++++++++++++++++++++++++ xen/arch/x86/physdev.c | 124 ++------------------------------ xen/include/asm-x86/irq.h | 5 ++ 3 files changed, 185 insertions(+), 119 deletions(-)