Re: [PATCH resend 2/3] x86: enable multi-vector MSI

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 16/07/13 12:48, Jan Beulich wrote:
>>>> On 16.07.13 at 13:36, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 16/07/13 11:14, Jan Beulich wrote:
>>> +int get_free_pirqs(struct domain *d, unsigned int nr)
>>> +{
>>> +    unsigned int i, found = 0;
>>> +
>>> +    ASSERT(spin_is_locked(&d->event_lock));
>>> +
>>> +    for ( i = d->nr_pirqs - 1; i >= nr_irqs_gsi; --i )
>>> +        if ( is_free_pirq(d, pirq_info(d, i)) )
>>> +        {
>>> +            pirq_get_info(d, i);
>>> +            if ( ++found == nr )
>>> +                return i;
>>> +        }
>>> +        else
>>> +            found = 0;
>>> +
>>> +    return -ENOSPC;
>>> +}
>>> +
>> Is there any reason why this loop is backwards? Unless I am mistaken,
>> guests can choose their own pirqs when binding them, reducing the
>> likelyhood that the top of the available space will be free.
> This just follows the behavior of get_free_pirq(). I'm not up to
> having the two behave differently.
>
>>> @@ -210,8 +237,15 @@ int physdev_map_pirq(domid_t domid, int 
>>>   done:
>>>      spin_unlock(&d->event_lock);
>>>      spin_unlock(&pcidevs_lock);
>>> -    if ( (ret != 0) && (type == MAP_PIRQ_TYPE_MSI) && (*index == -1) )
>>> -        destroy_irq(irq);
>>> +    if ( ret != 0 )
>>> +        switch ( type )
>>> +        {
>>> +        case MAP_PIRQ_TYPE_MSI:
>>> +            if ( *index == -1 )
>>> +        case MAP_PIRQ_TYPE_MULTI_MSI:
>>> +                destroy_irq(irq);
>>> +            break;
>>> +        }
>> Do we not need to create and destroy entry_nr irqs in this function, or
>> is a multi-vector-msi now considered as just as single irq ?
>>
>> I ask because this appears to lack the "repeating certain operations for
>> all involved IRQs" described in the comment.
> No, there's a single create_irq() in the function, and having a single
> destroy_irq() here matches this. The remaining ones (both!) are in
> map_domain_pirq().

Ok.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> Also, as a general remark, asking for changes in a series that was
> posted 2.5 months ago (and deferred just because of the 4.3
> release process) seems a little strange to me. I had to repost
> merely to collect ack-s, and didn't really expect further requests
> for adjustments as there was ample time before to do so.
>
> Jan
>

2.5 months ago, I was very busy with XSAs, hotfixes and a release of
XenServer, so I appologies for not reviewing back then.

~Andrew