Re: [Xen-devel] [PATCH 4.1] x86: fix emuirq regression from XSA-21 fix (was: Re: [PATCH] xen: reuse the same pirq allocated when driver load first time)

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 27 Jun 2013, Jan Beulich wrote:
> >>> On 25.06.13 at 13:03, Stefano Stabellini 
> >>> <stefano.stabellini@xxxxxxxxxxxxx>
> wrote:
> > On Tue, 25 Jun 2013, Stefano Stabellini wrote:
> >> On Tue, 25 Jun 2013, Jan Beulich wrote:
> >> > >>> On 25.06.13 at 07:33, DuanZhenzhong <zhenzhong.duan@xxxxxxxxxx> 
> >> > >>> wrote:
> >> > > Stefano Stabellini wrote:
> >> > >> On Mon, 24 Jun 2013, Zhenzhong Duan wrote:
> >> > >>> Could you have a look if there is something wrong in xen side of 
> >> > >>> clearing 
> >> > >>> the mapping?
> >> > >>
> >> > >> What I am saying is that the error you are getting:
> >> > >>
> >> > >> pt_msix_disable: Unbind msix with pirq 67, gvec 0
> >> > >> pt_msix_disable: Unmap msix with pirq 67
> >> > >> pt_msix_disable: Error: Unmapping of MSI-X failed. [00:04.0]
> >> > >>
> >> > >> cannot be caused by domain_pirq_to_emuirq(d, pirq) returning
> >> > >> IRQ_UNBOUND.
> >> > >> So, why are you getting this error? What is failing?
> >> > >> I am ready to believe the problem is in Xen but Without understanding
> >> > >> why you are getting the error it's hard to find a solution.
> >> > >>   
> >> > > I found the reason, you are looking at xen-unstable, I was working 
> >> > > with 
> >> > > 4.1.30-OVM, it has patch of CVE-2012-4536 / XSA-21.
> >> > > That patch set ret to -EINVAL initially. After remove that line, unmap 
> >> > > succeed.
> >> > 
> >> > Removing that line certainly isn't right. The proper fix is the one
> >> > below/attached.
> >> > 
> >> > Jan
> >> > 
> >> > ****************************************************
> >> > x86: fix emuirq regression from XSA-21 fix
> >> > 
> >> > The XSA-21 patch broke the assumption of "ret" being zero prior to the
> >> > IRQ_UNBOUND check.
> >> > 
> >> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >> > 
> >> > --- a/xen/arch/x86/physdev.c
> >> > +++ b/xen/arch/x86/physdev.c
> >> > @@ -243,6 +243,8 @@ static int physdev_unmap_pirq(struct phy
> >> >          spin_lock(&d->event_lock);
> >> >          if ( domain_pirq_to_emuirq(d, unmap->pirq) != IRQ_UNBOUND )
> >> >              ret = unmap_domain_pirq_emuirq(d, unmap->pirq);
> >> > +        else
> >> > +            ret = 0;
> >> >          spin_unlock(&d->event_lock);
> >> >          if ( unmap->domid == DOMID_SELF || ret )
> >> >              goto free_domain;
> >> 
> >> 
> >> This is unnecessary.
> >> ret is 0 regardless because of the previous:
> >> 
> >> ret = xsm_unmap_domain_pirq(XSM_TARGET, d);
> >>
> > 
> > Ops, I read now the patch for XSA-21.
> > The change above is fine (and makes the code more readable anyway).
> 
> So is this some odd way of saying Reviewed-by?
 
Yes :)

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel