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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan