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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 02/18/2014 12:04 PM, Jan Beulich wrote:
> > > > On 18.02.14 at 17:56, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> > On 02/18/2014 11:41 AM, Jan Beulich wrote:
> > > > > > On 18.02.14 at 16:49, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> > > > 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.
> > > It's the very division that this patch removes:
> > >
> > > > > > --- 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 )
> > > > > >             {
> > > Which isn't inside the if(stride).
> > Yes, I see it now. I was staring at a wrong line.
> >
> > This actually now looks like a bug.
> You mean the old code looks wrong or the new one?

The old one. Your patch fixes it.


> > You do check above for '(num_vf > 1
> > && !stride) ' but presumably if things are really messed up num_vf can
> > be 1 but vf is 0. And then if stride is zero too then we are not doing
> > particularly well.
> >
> > So probably this should go into 4.4 as well?
> We've done with this unfixed quite fine so far, so it's generally
> okay to leave as is until 4.4.1 (read: not a regression). I
> personally wouldn't mind pushing it in, but only if other similar
> not too high priority bug fixes would also go in.
OK.

-boris

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