Re: [XEN PATCH v11 4/8] x86/physdev: Return pirq that irq was already mapped to

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2024/7/4 20:47, Jan Beulich wrote:
> On 30.06.2024 14:33, Jiqian Chen wrote:
>> allocate_pirq is to allocate a pirq for a irq, and it supports to
>> allocate a free pirq(pirq parameter is <0) or a specific pirq (pirq
>> parameter is > 0).
>>
>> For current code, it has four usecases.
>>
>> First, pirq>0 and current_pirq>0, (current_pirq means if irq already
>> has a mapped pirq), if pirq==current_pirq means the irq already has
>> mapped to the pirq expected by the caller, it successes, if
>> pirq!=current_pirq means the pirq expected by the caller has been
>> mapped into other irq, it fails.
>>
>> Second, pirq>0 and current_pirq<0, it means pirq expected by the
>> caller has not been allocated to any irqs, so it can be allocated to
>> caller, it successes.
>>
>> Third, pirq<0 and current_pirq<0, it means caller want to allocate a
>> free pirq for irq and irq has no mapped pirq, it successes.
>>
>> Fourth, pirq<0 and current_pirq>0, it means caller want to allocate
>> a free pirq for irq but irq has a mapped pirq, then it returns the
>> negative pirq, so it fails.
>>
>> The problem is in Fourth, since the irq has a mapped pirq(current_pirq),
>> and the caller doesn't want to allocate a specified pirq to the irq, so
>> the current_pirq should be returned directly in this case, indicating
>> that the allocation is successful. That can help caller to success when
>> caller just want to allocate a free pirq but doesn't know if the irq
>> already has a mapped pirq or not.
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>> Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> 
> I think the change is correct, and actually fixes a regression. You want
> 
> Fixes: 0762e2502f1f ("x86/physdev: factor out the code to allocate and map a 
> pirq")
> 
> which would also have helped reviewing quite a bit. And it likely would
> also have helped you write a description which is easier to follow.
> Enumerating all the cases isn't really needed here; what is needed is
> an explanation of what went wrong in that re-factoring.
> 
>> --- a/xen/arch/x86/irq.c
>> +++ b/xen/arch/x86/irq.c
>> @@ -2897,6 +2897,8 @@ static int allocate_pirq(struct domain *d, int index, 
>> int pirq, int irq,
>>                      d->domain_id, index, pirq, current_pirq);
>>              if ( current_pirq < 0 )
>>                  return -EBUSY;
>> +            else
>> +                return current_pirq;
> 
> Please can this be simply
> 
>             pirq = current_pirq;
> 
> without any "else", and then taking the normal return path. That again is
> (imo) closer to what was there before.
> 
> I would further suggest that you split this fix out of this series and
> re-submit soon with a for-4.19 tag and with Oleksii Cc-ed, so that this
> can be considered for inclusion in 4.19.
Thanks, will split and send today.

> 
> Jan

-- 
Best regards,
Jiqian Chen.