Re: [Xen-devel] [PATCH v2 1/3] x86/physdev: factor out the code to allocate and map a pirq

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.05.17 at 11:47, <roger.pau@xxxxxxxxxx> wrote:
> On Fri, May 12, 2017 at 06:56:01AM -0600, Jan Beulich wrote:
>> >>> On 19.04.17 at 17:11, <roger.pau@xxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel