* [PATCH] x86/irq: simplify loop in unmap_domain_pirq
@ 2021-02-10 9:22 Roger Pau Monne
2021-02-19 14:39 ` Jan Beulich
0 siblings, 1 reply; 3+ messages in thread
From: Roger Pau Monne @ 2021-02-10 9:22 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Jan Beulich, Andrew Cooper, Wei Liu
The for loop in unmap_domain_pirq is unnecessary complicated, with
several places where the index is incremented, and also different
exit conditions spread between the loop body.
Simplify it by looping over each possible PIRQ using the for loop
syntax, and remove all possible in-loop exit points.
No functional change intended.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/x86/irq.c | 60 +++++++++++++---------------------------------
1 file changed, 16 insertions(+), 44 deletions(-)
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 032fe82167..856714b5bf 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2305,7 +2305,6 @@ done:
/* The pirq should have been unbound before this call. */
int unmap_domain_pirq(struct domain *d, int pirq)
{
- unsigned long flags;
struct irq_desc *desc;
int irq, ret = 0, rc;
unsigned int i, nr = 1;
@@ -2356,11 +2355,23 @@ int unmap_domain_pirq(struct domain *d, int pirq)
if ( msi_desc != NULL )
pci_disable_msi(msi_desc);
- spin_lock_irqsave(&desc->lock, flags);
-
- for ( i = 0; ; )
+ for ( i = 0; i < nr; i++, info = pirq_info(d, pirq + i) )
{
+ unsigned long flags;
+
+ if ( !info || info->arch.irq <= 0 )
+ {
+ printk(XENLOG_G_ERR "dom%d: MSI pirq %d not mapped\n",
+ d->domain_id, pirq + i);
+ continue;
+ }
+ irq = info->arch.irq;
+ desc = irq_to_desc(irq);
+
+ spin_lock_irqsave(&desc->lock, flags);
+
BUG_ON(irq != domain_pirq_to_irq(d, pirq + i));
+ BUG_ON(desc->msi_desc != msi_desc + i);
if ( !forced_unbind )
clear_domain_irq_pirq(d, irq, info);
@@ -2378,45 +2389,6 @@ int unmap_domain_pirq(struct domain *d, int pirq)
desc->msi_desc = NULL;
}
- if ( ++i == nr )
- break;
-
- spin_unlock_irqrestore(&desc->lock, flags);
-
- if ( !forced_unbind )
- cleanup_domain_irq_pirq(d, irq, info);
-
- rc = irq_deny_access(d, irq);
- if ( rc )
- {
- printk(XENLOG_G_ERR
- "dom%d: could not deny access to IRQ%d (pirq %d)\n",
- d->domain_id, irq, pirq + i);
- ret = rc;
- }
-
- do {
- info = pirq_info(d, pirq + i);
- if ( info && (irq = info->arch.irq) > 0 )
- break;
- printk(XENLOG_G_ERR "dom%d: MSI pirq %d not mapped\n",
- d->domain_id, pirq + i);
- } while ( ++i < nr );
-
- if ( i == nr )
- {
- desc = NULL;
- break;
- }
-
- desc = irq_to_desc(irq);
- BUG_ON(desc->msi_desc != msi_desc + i);
-
- spin_lock_irqsave(&desc->lock, flags);
- }
-
- if ( desc )
- {
spin_unlock_irqrestore(&desc->lock, flags);
if ( !forced_unbind )
@@ -2427,7 +2399,7 @@ int unmap_domain_pirq(struct domain *d, int pirq)
{
printk(XENLOG_G_ERR
"dom%d: could not deny access to IRQ%d (pirq %d)\n",
- d->domain_id, irq, pirq + nr - 1);
+ d->domain_id, irq, pirq + i);
ret = rc;
}
}
--
2.29.2
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] x86/irq: simplify loop in unmap_domain_pirq
2021-02-10 9:22 [PATCH] x86/irq: simplify loop in unmap_domain_pirq Roger Pau Monne
@ 2021-02-19 14:39 ` Jan Beulich
2021-02-22 14:56 ` Roger Pau Monné
0 siblings, 1 reply; 3+ messages in thread
From: Jan Beulich @ 2021-02-19 14:39 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, Wei Liu, xen-devel
On 10.02.2021 10:22, Roger Pau Monne wrote:
> The for loop in unmap_domain_pirq is unnecessary complicated, with
> several places where the index is incremented, and also different
> exit conditions spread between the loop body.
>
> Simplify it by looping over each possible PIRQ using the for loop
> syntax, and remove all possible in-loop exit points.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Quite a bit better indeed. Just one nit below (can be taken care of
while committing, once the tree will re-open).
> @@ -2356,11 +2355,23 @@ int unmap_domain_pirq(struct domain *d, int pirq)
> if ( msi_desc != NULL )
> pci_disable_msi(msi_desc);
>
> - spin_lock_irqsave(&desc->lock, flags);
> -
> - for ( i = 0; ; )
> + for ( i = 0; i < nr; i++, info = pirq_info(d, pirq + i) )
> {
> + unsigned long flags;
> +
> + if ( !info || info->arch.irq <= 0 )
> + {
> + printk(XENLOG_G_ERR "dom%d: MSI pirq %d not mapped\n",
> + d->domain_id, pirq + i);
%pd please as you touch/move this anyway.
> @@ -2378,45 +2389,6 @@ int unmap_domain_pirq(struct domain *d, int pirq)
> desc->msi_desc = NULL;
> }
>
> - if ( ++i == nr )
> - break;
> -
> - spin_unlock_irqrestore(&desc->lock, flags);
> -
> - if ( !forced_unbind )
> - cleanup_domain_irq_pirq(d, irq, info);
> -
> - rc = irq_deny_access(d, irq);
> - if ( rc )
> - {
> - printk(XENLOG_G_ERR
> - "dom%d: could not deny access to IRQ%d (pirq %d)\n",
> - d->domain_id, irq, pirq + i);
Looks like the pirq number logged here also was off by one, which
the re-arrangement takes care of.
Jan
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] x86/irq: simplify loop in unmap_domain_pirq
2021-02-19 14:39 ` Jan Beulich
@ 2021-02-22 14:56 ` Roger Pau Monné
0 siblings, 0 replies; 3+ messages in thread
From: Roger Pau Monné @ 2021-02-22 14:56 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Wei Liu, xen-devel
On Fri, Feb 19, 2021 at 03:39:14PM +0100, Jan Beulich wrote:
> On 10.02.2021 10:22, Roger Pau Monne wrote:
> > The for loop in unmap_domain_pirq is unnecessary complicated, with
> > several places where the index is incremented, and also different
> > exit conditions spread between the loop body.
> >
> > Simplify it by looping over each possible PIRQ using the for loop
> > syntax, and remove all possible in-loop exit points.
> >
> > No functional change intended.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Quite a bit better indeed. Just one nit below (can be taken care of
> while committing, once the tree will re-open).
Sure, if you want to queue this already please fix the format
string.
> > @@ -2356,11 +2355,23 @@ int unmap_domain_pirq(struct domain *d, int pirq)
> > if ( msi_desc != NULL )
> > pci_disable_msi(msi_desc);
> >
> > - spin_lock_irqsave(&desc->lock, flags);
> > -
> > - for ( i = 0; ; )
> > + for ( i = 0; i < nr; i++, info = pirq_info(d, pirq + i) )
> > {
> > + unsigned long flags;
> > +
> > + if ( !info || info->arch.irq <= 0 )
> > + {
> > + printk(XENLOG_G_ERR "dom%d: MSI pirq %d not mapped\n",
> > + d->domain_id, pirq + i);
>
> %pd please as you touch/move this anyway.
>
> > @@ -2378,45 +2389,6 @@ int unmap_domain_pirq(struct domain *d, int pirq)
> > desc->msi_desc = NULL;
> > }
> >
> > - if ( ++i == nr )
> > - break;
> > -
> > - spin_unlock_irqrestore(&desc->lock, flags);
> > -
> > - if ( !forced_unbind )
> > - cleanup_domain_irq_pirq(d, irq, info);
> > -
> > - rc = irq_deny_access(d, irq);
> > - if ( rc )
> > - {
> > - printk(XENLOG_G_ERR
> > - "dom%d: could not deny access to IRQ%d (pirq %d)\n",
> > - d->domain_id, irq, pirq + i);
>
> Looks like the pirq number logged here also was off by one, which
> the re-arrangement takes care of.
Indeed. I don't think it's worth fixing this now.
Thanks, Roger.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-02-22 14:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-10 9:22 [PATCH] x86/irq: simplify loop in unmap_domain_pirq Roger Pau Monne
2021-02-19 14:39 ` Jan Beulich
2021-02-22 14:56 ` Roger Pau Monné
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).