From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH 2/2] x86/pci: Remove unnecessary check in VF value computation Date: Tue, 18 Feb 2014 12:10:16 -0500 Message-ID: <530393F8.2040409@oracle.com> References: <1392239147-1547-1-git-send-email-boris.ostrovsky@oracle.com> <1392239147-1547-3-git-send-email-boris.ostrovsky@oracle.com> <52FCA2E0020000780011BF52@nat28.tlf.novell.com> <53034122020000780011D2CC@nat28.tlf.novell.com> <530380ED.4050708@oracle.com> <53039B61020000780011D68B@nat28.tlf.novell.com> <530390D2.6020000@oracle.com> <5303A090020000780011D6C6@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WFoA0-0007eT-VB for xen-devel@lists.xenproject.org; Tue, 18 Feb 2014 17:08:45 +0000 In-Reply-To: <5303A090020000780011D6C6@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: george.dunlap@eu.citrix.com, xen-devel@lists.xenproject.org, keir@xen.org List-Id: xen-devel@lists.xenproject.org On 02/18/2014 12:04 PM, Jan Beulich wrote: >>>> On 18.02.14 at 17:56, Boris Ostrovsky wrote: >> On 02/18/2014 11:41 AM, Jan Beulich wrote: >>>>>> On 18.02.14 at 16:49, Boris Ostrovsky wrote: >>>> On 02/18/2014 05:16 AM, Jan Beulich wrote: >>>>>>>> On 13.02.14 at 10:48, "Jan Beulich" wrote: >>>>>>>>> On 12.02.14 at 22:05, Boris Ostrovsky 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