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 10:49:01 -0500
Message-ID: <530380ED.4050708@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>
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.bemta4.messagelabs.com ([85.158.143.247])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <boris.ostrovsky@oracle.com>) id 1WFmtQ-0003cP-Q6
	for xen-devel@lists.xenproject.org; Tue, 18 Feb 2014 15:47:32 +0000
In-Reply-To: <53034122020000780011D2CC@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 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.

But as I said, either version is OK with me so you can add

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

-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@oracle.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- 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 )
>>           {
>
>