| Message ID | 20240630123344.20623-5-Jiqian.Chen@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support device passthrough when dom0 is PVH on Xen | expand |
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
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 ) {