[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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.



Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.