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