[PATCH 0/2] A couple of SR-IOV-related patches

[PATCH 0/2] A couple of SR-IOV-related patches

[Boris Ostrovsky]

The first patch fixes a bug in calculating offset to the Virtual Function's
memory space. It may be worth taking it to 4.4.

The second patch is removal of what seems to be a redundant check in computing
VF number.

Boris Ostrovsky (2):
  x86/pci: Store VF's memory space displacement in a 64-bit value
  x86/pci: Remove unnecessary check in VF value computation

 xen/arch/x86/msi.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

-- 
1.8.1.4

[PATCH 1/2] x86/pci: Store VF's memory space displacement in a 64-bit value

[Boris Ostrovsky]

VF's memory space offset can be greater than 4GB and therefore needs
to be stored in a 64-bit variable.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/x86/msi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 284042e..1aaceeb 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -610,7 +610,8 @@ static int msi_capability_init(struct pci_dev *dev,
 static u64 read_pci_mem_bar(u16 seg, u8 bus, u8 slot, u8 func, u8 bir, int vf)
 {
     u8 limit;
-    u32 addr, base = PCI_BASE_ADDRESS_0, disp = 0;
+    u32 addr, base = PCI_BASE_ADDRESS_0;
+    u64 disp = 0;
 
     if ( vf >= 0 )
     {
-- 
1.8.1.4

[PATCH 2/2] x86/pci: Remove unnecessary check in VF value computation

[Boris Ostrovsky]

This test is already performed a couple of lines above.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/x86/msi.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 1aaceeb..27e47c3 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -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;
-        }
         if ( vf >= num_vf )
             return 0;
         BUILD_BUG_ON(ARRAY_SIZE(pdev->vf_rlen) != PCI_SRIOV_NUM_BARS);
-- 
1.8.1.4

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

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1421 bytes --]

>>> 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:

> @@ -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 )
         {




[-- Attachment #2: x86-MSI-VF-divide-by-zero.patch --]
[-- Type: text/plain, Size: 640 bytes --]

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 )
         {

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

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

[Jan Beulich]

>>> 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?

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 )
>          {

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

[Boris Ostrovsky]

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 )
>>           {
>
>

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

[Jan Beulich]

>>> 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).

Jan

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

[Boris Ostrovsky]

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

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

[Jan Beulich]

>>> On 18.02.14 at 17:56, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
> 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 mean the old code looks wrong or the new one?

> 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.

Jan

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

[Boris Ostrovsky]

On 02/18/2014 12:04 PM, Jan Beulich wrote:
>>>> On 18.02.14 at 17:56, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>> 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 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