Re: [Xen-devel] [PATCH 2/2] x86/pci: Remove unnecessary check in VF value computation

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 02/18/2014 05:16 AM, Jan Beulich wrote:
> > > > On 13.02.14 at 10:48, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> > > > > On 12.02.14 at 22:05, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> > > This test is already performed a couple of lines above.
> > Except that it's the wrong code you remove:
> No opinion on this alternative at all?
Sorry Jan, I didn't realize you were waiting for me on this.

Yes, your version is fine although to be honest I don't see how the 
original patch had any issues with division by zero since we'd still be 
inside the 'if (stride)' clause.
But as I said, either version is OK with me so you can add

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>

-boris


> Jan
>
> > > @@ -639,11 +639,7 @@ static u64 read_pci_mem_bar(u16 seg, u8 bus, u8 slot, u8
> > func, u8 bir, int vf)
> > >           if ( vf < 0 || (vf && vf % stride) )
> > >               return 0;
> > >           if ( stride )
> > > -        {
> > > -            if ( vf % stride )
> > > -                return 0;
> > >               vf /= stride;
> > > -        }
> > Note how this second check carefully avoids a division by zero.
> >  From what I can tell I think that I simply forgot to remove the
> > right side of the earlier || after having converted it to the safer
> > variant inside the if(). Hence I think we instead want:
> >
> > x86/MSI: don't risk division by zero
> >
> > The check in question is redundant with the one in the immediately
> > following if(), where dividing by zero gets carefully avoided.
> >
> > Spotted-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >
> > --- a/xen/arch/x86/msi.c
> > +++ b/xen/arch/x86/msi.c
> > @@ -635,7 +635,7 @@ static u64 read_pci_mem_bar(u16 seg, u8
> >               return 0;
> >           base = pos + PCI_SRIOV_BAR;
> >           vf -= PCI_BDF(bus, slot, func) + offset;
> > -        if ( vf < 0 || (vf && vf % stride) )
> > +        if ( vf < 0 )
> >               return 0;
> >           if ( stride )
> >           {

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