From mboxrd@z Thu Jan  1 00:00:00 1970
From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Subject: Re: [PATCH 2/2] x86/pci: Remove unnecessary check in VF
 value computation
Date: Tue, 18 Feb 2014 11:56:50 -0500
Message-ID: <530390D2.6020000@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>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta5.messagelabs.com ([195.245.231.135])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <boris.ostrovsky@oracle.com>) id 1WFnx5-0006v2-PC
	for xen-devel@lists.xenproject.org; Tue, 18 Feb 2014 16:55:23 +0000
In-Reply-To: <53039B61020000780011D68B@nat28.tlf.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
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 11:41 AM, Jan Beulich wrote:
>>>> On 18.02.14 at 16:49, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>> On 02/18/2014 05:16 AM, Jan Beulich wrote:
>>>>>> On 13.02.14 at 10:48, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>>>>> On 12.02.14 at 22:05, Boris Ostrovsky <boris.ostrovsky@oracle.com> 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 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?

-boris