Re: [Xen-devel] [PATCH] xen/pirq: fix error path cleanup when binding MSIs

[Shah, Amit]

On Mi, 2018-02-28 at 09:19 +, Roger Pau Monne wrote:
> Current cleanup in the error path of xen_bind_pirq_msi_to_irq is
> wrong. First of all there's an off-by-one in the cleanup loop, which
> can lead to unbinding wrong IRQs.
> 
> Secondly IRQs not bound won't be freed, thus leaking IRQ numbers.
> 
> Note that there's no need to differentiate between bound and unbound
> IRQs when freeing them, __unbind_from_irq will deal with both of them
> correctly.
> 
> Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups")
> Reported-by: Hooman Mirhadi 
> Signed-off-by: Roger Pau Monné 
> ---
> Cc: Boris Ostrovsky 
> Cc: Juergen Gross 
> Cc: Amit Shah 
> CC: sta...@vger.kernel.org
> Cc: xen-devel@lists.xenproject.org

The CC to stable got lost on commit, so this didn't actually make
it to the stable queue.  Can you please get it queued?

Thanks,
Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/pirq: fix error path cleanup when binding MSIs

[Shah, Amit]

On Mi, 2018-02-28 at 09:19 +, Roger Pau Monne wrote:
> Current cleanup in the error path of xen_bind_pirq_msi_to_irq is
> wrong. First of all there's an off-by-one in the cleanup loop, which
> can lead to unbinding wrong IRQs.
> 
> Secondly IRQs not bound won't be freed, thus leaking IRQ numbers.
> 
> Note that there's no need to differentiate between bound and unbound
> IRQs when freeing them, __unbind_from_irq will deal with both of them
> correctly.
> 
> Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups")
> Reported-by: Hooman Mirhadi 
> Signed-off-by: Roger Pau Monné 
> ---
> Cc: Boris Ostrovsky 
> Cc: Juergen Gross 
> Cc: Amit Shah 
> CC: sta...@vger.kernel.org
> Cc: xen-devel@lists.xenproject.org
> ---
>  drivers/xen/events/events_base.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/events/events_base.c
> b/drivers/xen/events/events_base.c
> index b241bfa529ce..159faf1269fb 100644
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -763,8 +763,8 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev,
> struct msi_desc *msidesc,
>   mutex_unlock(_mapping_update_lock);
>   return irq;
>  error_irq:
> - for (; i >= 0; i--)
> - __unbind_from_irq(irq + i);
> + while (nvec--)
> + __unbind_from_irq(irq + nvec);
>   mutex_unlock(_mapping_update_lock);
>   return ret;
>  }

Reviewed-by: Amit Shah 


Amit

Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] xen: events: free irqs in error condition

[Shah, Amit]

On Mi, 2018-02-28 at 08:16 +, Roger Pau Monné wrote:
> On Tue, Feb 27, 2018 at 05:32:53PM +0000, Shah, Amit wrote:
> > 
> > 
> > On Di, 2018-02-27 at 17:07 +, Roger Pau Monné wrote:
> > > 
> > > On Tue, Feb 27, 2018 at 03:55:58PM +, Amit Shah wrote:
> > > > 
> > > > 
> > > > In case of errors in irq setup for MSI, free up the allocated
> > > > irqs.
> > > > 
> > > > Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message
> > > > groups")
> > > > Reported-by: Hooman Mirhadi <mirha...@amazon.com>
> > > > CC: <sta...@vger.kernel.org>
> > > > CC: Roger Pau Monné <roger@citrix.com>
> > > > CC: Boris Ostrovsky <boris.ostrov...@oracle.com>
> > > > CC: Eduardo Valentin <edu...@amazon.com>
> > > > CC: Juergen Gross <jgr...@suse.com>
> > > > CC: Thomas Gleixner <t...@linutronix.de>
> > > > CC: "K. Y. Srinivasan" <k...@microsoft.com>
> > > > CC: Liu Shuo <shuo.a@intel.com>
> > > > CC: Anoob Soman <anoob.so...@citrix.com>
> > > > Signed-off-by: Amit Shah <a...@amazon.com>
> > > > ---
> > > >  drivers/xen/events/events_base.c | 5 -
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/xen/events/events_base.c
> > > > b/drivers/xen/events/events_base.c
> > > > index c86d10e..a299586 100644
> > > > --- a/drivers/xen/events/events_base.c
> > > > +++ b/drivers/xen/events/events_base.c
> > > > @@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct
> > > > pci_dev
> > > > *dev, struct msi_desc *msidesc,
> > > >  
> > > >     ret = irq_set_msi_desc(irq, msidesc);
> > > >     if (ret < 0)
> > > > -   goto error_irq;
> > > > +   goto error_desc;
> > > >  out:
> > > >     mutex_unlock(_mapping_update_lock);
> > > >     return irq;
> > > >  error_irq:
> > > > +   while (--nvec >= i)
> > > > +   xen_free_irq(irq + nvec);
> > > > +error_desc:
> > > >     while (i > 0) {
> > > >     i--;
> > > >     __unbind_from_irq(irq + i);
> > > It seems pointless to introduce another label and another loop to
> > > fix
> > > something that can be fixed with a single label and a single
> > > loop,
> > > this just makes the code more complex for no reason.
> > I disagree, just because there are two different cleanups to be
> > made
> > for two different issues; it's not as if the if.. and else
> > conditions
> > are going to be interleaved.
> Oh, I don't mind so much whether it ends up being two patches or a
> single one, but IMHO the code should end up looking similar to what I
> proposed, I would like to avoid having two loops and two labels.
> 
> Could you rework the series so that the end result uses a single loop
> (and label)?

That was the part I didn't like much, so it would be better if the
patch came from you :)

Amit

Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] xen: events: free irqs in error condition

[Shah, Amit]

On Di, 2018-02-27 at 17:07 +, Roger Pau Monné wrote:
> On Tue, Feb 27, 2018 at 03:55:58PM +, Amit Shah wrote:
> > 
> > In case of errors in irq setup for MSI, free up the allocated irqs.
> > 
> > Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups")
> > Reported-by: Hooman Mirhadi 
> > CC: 
> > CC: Roger Pau Monné 
> > CC: Boris Ostrovsky 
> > CC: Eduardo Valentin 
> > CC: Juergen Gross 
> > CC: Thomas Gleixner 
> > CC: "K. Y. Srinivasan" 
> > CC: Liu Shuo 
> > CC: Anoob Soman 
> > Signed-off-by: Amit Shah 
> > ---
> >  drivers/xen/events/events_base.c | 5 -
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/xen/events/events_base.c
> > b/drivers/xen/events/events_base.c
> > index c86d10e..a299586 100644
> > --- a/drivers/xen/events/events_base.c
> > +++ b/drivers/xen/events/events_base.c
> > @@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev
> > *dev, struct msi_desc *msidesc,
> >  
> >     ret = irq_set_msi_desc(irq, msidesc);
> >     if (ret < 0)
> > -   goto error_irq;
> > +   goto error_desc;
> >  out:
> >     mutex_unlock(_mapping_update_lock);
> >     return irq;
> >  error_irq:
> > +   while (--nvec >= i)
> > +   xen_free_irq(irq + nvec);
> > +error_desc:
> >     while (i > 0) {
> >     i--;
> >     __unbind_from_irq(irq + i);
> It seems pointless to introduce another label and another loop to fix
> something that can be fixed with a single label and a single loop,
> this just makes the code more complex for no reason.

I disagree, just because there are two different cleanups to be made
for two different issues; it's not as if the if.. and else conditions
are going to be interleaved.

Anyway it's a matter of taste.

Since you've already proposed the patch, would you mind baking a proper
one and posting it?

Thanks!


> IMHO the way to solve this issue is:
> 
> while (nvec--) {
>   if (nvec >= i)
>   xen_free_irq(irq + nvec);
>   else
>   __unbind_from_irq(irq + nvec);
> }

Amit

Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] xen: events: free irqs in error condition

[Shah, Amit]

On Di, 2018-02-27 at 08:14 +, Roger Pau Monné wrote:
> On Mon, Feb 26, 2018 at 06:57:03PM +0000, Shah, Amit wrote:
> > 
> > 
> > On Mo, 2018-02-26 at 18:14 +, Roger Pau Monné wrote:
> > > 
> > > On Mon, Feb 26, 2018 at 05:36:35PM +, Amit Shah wrote:
> > > > 
> > > > 
> > > > In case of errors in irq setup for MSI, free up the allocated
> > > > irqs.
> > > > 
> > > > Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message
> > > > groups")
> > > > Reported-by: Hooman Mirhadi <mirha...@amazon.com>
> > > > CC: <sta...@vger.kernel.org>
> > > > CC: Roger Pau Monné <roger@citrix.com>
> > > > CC: David Vrabel <david.vra...@citrix.com>
> > > > CC: Boris Ostrovsky <boris.ostrov...@oracle.com>
> > > > CC: Eduardo Valentin <edu...@amazon.com>
> > > > CC: Juergen Gross <jgr...@suse.com>
> > > > CC: Thomas Gleixner <t...@linutronix.de>
> > > > CC: "K. Y. Srinivasan" <k...@microsoft.com>
> > > > CC: Liu Shuo <shuo.a@intel.com>
> > > > CC: Anoob Soman <anoob.so...@citrix.com>
> > > > Signed-off-by: Amit Shah <a...@amazon.com>
> > > > ---
> > > >  drivers/xen/events/events_base.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/drivers/xen/events/events_base.c
> > > > b/drivers/xen/events/events_base.c
> > > > index b6b8b29..96aa575 100644
> > > > --- a/drivers/xen/events/events_base.c
> > > > +++ b/drivers/xen/events/events_base.c
> > > > @@ -758,6 +758,7 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev
> > > > *dev, struct msi_desc *msidesc,
> > > >  error_irq:
> > > >     for (; i >= 0; i--)
> > > >     __unbind_from_irq(irq + i);
> > > > +   xen_free_irq(irq);
> > > Hm, xen_free_irq calls irq_free_desc, which is
> > > irq_free_descs(irq,
> > > 1),
> > Er...  right.
> > 
> > > 
> > > I think you will have to introduce a new free function:
> > > 
> > > xen_free_irqs(unsigned irq, unsigned int nr)
> > > 
> > > That calls irq_free_descs(irq, nr)
> > Actually, xen_free_irq() is already done in __unbind_from_irq(), so
> > this patch is actually wrong and not needed.
> You still need to free unbound IRQs, AFAICT you could fix the issue
> with a single patch, like:
> 
> while (nvec--) {
>   if (nvec >= i)
>   xen_free_irq(irq + i);
>   else
>   __unbind_from_irq(irq + i);
> }

Agreed.

However, since these are two different things, I'd still like to
separate out into two patches, and two paths so it's easier to see
what's being done.

Sending v2 in a bit.

    Amit


Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] xen: events: free irqs in error condition

[Shah, Amit]

On Mo, 2018-02-26 at 18:14 +, Roger Pau Monné wrote:
> On Mon, Feb 26, 2018 at 05:36:35PM +, Amit Shah wrote:
> > 
> > In case of errors in irq setup for MSI, free up the allocated irqs.
> > 
> > Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups")
> > Reported-by: Hooman Mirhadi 
> > CC: 
> > CC: Roger Pau Monné 
> > CC: David Vrabel 
> > CC: Boris Ostrovsky 
> > CC: Eduardo Valentin 
> > CC: Juergen Gross 
> > CC: Thomas Gleixner 
> > CC: "K. Y. Srinivasan" 
> > CC: Liu Shuo 
> > CC: Anoob Soman 
> > Signed-off-by: Amit Shah 
> > ---
> >  drivers/xen/events/events_base.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/xen/events/events_base.c
> > b/drivers/xen/events/events_base.c
> > index b6b8b29..96aa575 100644
> > --- a/drivers/xen/events/events_base.c
> > +++ b/drivers/xen/events/events_base.c
> > @@ -758,6 +758,7 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev
> > *dev, struct msi_desc *msidesc,
> >  error_irq:
> >     for (; i >= 0; i--)
> >     __unbind_from_irq(irq + i);
> > +   xen_free_irq(irq);
> Hm, xen_free_irq calls irq_free_desc, which is irq_free_descs(irq,
> 1),

Er...  right.

> I think you will have to introduce a new free function:
> 
> xen_free_irqs(unsigned irq, unsigned int nr)
> 
> That calls irq_free_descs(irq, nr)

Actually, xen_free_irq() is already done in __unbind_from_irq(), so
this patch is actually wrong and not needed.


Amit
Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] Propagate microcode update errors

[Shah, Amit]

On Fr, 2018-02-16 at 13:19 +, Peter Lawthers wrote:
> From: Uwe Dannowski 
> 
> Errors on updating the microcode in the processor were silently
> dropped when invoked via the microcode_update hypercall. Also, the
> log
> message was misleading.
> 
> Signed-off-by: Uwe Dannowski 
> Reviewed-by: Stefan Nuernberger 
> Reviewed-by: Martin Pohlack 
> CC: David Woodhouse 
> CC: Amit Shah 
> CC: Jan Beulich 
> CC: Andrew Cooper 

Reviewed-by: Amit Shah 

-- 

Amit
Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel