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@amd.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
 xen/arch/x86/irq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 9a611c79e024..5ccca1646eb1 100644
--- 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;
         }
         else if ( type == MAP_PIRQ_TYPE_MULTI_MSI )
         {
-- 
2.34.1

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@amd.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>

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.

Jan

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@amd.com>
>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> 
> 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.