[dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF reset

[dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF reset

[Simon Ellmann]

ixgbe devices support up to 8 Rx and Tx queues per virtual function.
Currently, the registers of only seven queues are set to default when
resetting a VF.

Signed-off-by: Simon Ellmann <simon.ellmann@tum.de>
---
 drivers/net/ixgbe/base/ixgbe_vf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ixgbe/base/ixgbe_vf.c b/drivers/net/ixgbe/base/ixgbe_vf.c
index 91a5775eb..5e3ae1b51 100644
--- a/drivers/net/ixgbe/base/ixgbe_vf.c
+++ b/drivers/net/ixgbe/base/ixgbe_vf.c
@@ -84,7 +84,7 @@ static void ixgbe_virt_clr_reg(struct ixgbe_hw *hw)
 
 	IXGBE_WRITE_REG(hw, IXGBE_VFPSRTYPE, 0);
 
-	for (i = 0; i < 7; i++) {
+	for (i = 0; i < 8; i++) {
 		IXGBE_WRITE_REG(hw, IXGBE_VFRDH(i), 0);
 		IXGBE_WRITE_REG(hw, IXGBE_VFRDT(i), 0);
 		IXGBE_WRITE_REG(hw, IXGBE_VFRXDCTL(i), 0);
-- 
2.29.2

Re: [dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF reset

[Wang, Haiyue]

> -----Original Message-----
> From: Simon Ellmann <simon.ellmann@tum.de>
> Sent: Friday, December 18, 2020 01:15
> To: Guo, Jia <jia.guo@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev@dpdk.org; Simon Ellmann <simon.ellmann@tum.de>
> Subject: [PATCH] net/ixgbe: clear registers of all queues on VF reset
> 
> ixgbe devices support up to 8 Rx and Tx queues per virtual function.
> Currently, the registers of only seven queues are set to default when
> resetting a VF.
> 

Fixes: d17d0b7a2407 ("ixgbe/base: reset VF registers")
Cc: stable@dpdk.org

> Signed-off-by: Simon Ellmann <simon.ellmann@tum.de>
> ---
>  drivers/net/ixgbe/base/ixgbe_vf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Good catch, thanks!

Acked-by: Haiyue Wang <haiyue.wang@intel.com>

> --
> 2.29.2

Re: [dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF reset

[Zhang, Qi Z]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Wang, Haiyue
> Sent: Friday, December 18, 2020 10:35 AM
> To: Simon Ellmann <simon.ellmann@tum.de>; Guo, Jia <jia.guo@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF
> reset
> 
> > -----Original Message-----
> > From: Simon Ellmann <simon.ellmann@tum.de>
> > Sent: Friday, December 18, 2020 01:15
> > To: Guo, Jia <jia.guo@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>
> > Cc: dev@dpdk.org; Simon Ellmann <simon.ellmann@tum.de>
> > Subject: [PATCH] net/ixgbe: clear registers of all queues on VF reset
> >
> > ixgbe devices support up to 8 Rx and Tx queues per virtual function.
> > Currently, the registers of only seven queues are set to default when
> > resetting a VF.
> >
> 
> Fixes: d17d0b7a2407 ("ixgbe/base: reset VF registers")
> Cc: stable@dpdk.org
> 
> > Signed-off-by: Simon Ellmann <simon.ellmann@tum.de>
> > ---
> >  drivers/net/ixgbe/base/ixgbe_vf.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> 
> Good catch, thanks!
> 
> Acked-by: Haiyue Wang <haiyue.wang@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi
> 
> > --
> > 2.29.2

Re: [dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF reset

[Ferruh Yigit]

On 12/18/2020 2:34 AM, Wang, Haiyue wrote:
>> -----Original Message-----
>> From: Simon Ellmann <simon.ellmann@tum.de>
>> Sent: Friday, December 18, 2020 01:15
>> To: Guo, Jia <jia.guo@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>
>> Cc: dev@dpdk.org; Simon Ellmann <simon.ellmann@tum.de>
>> Subject: [PATCH] net/ixgbe: clear registers of all queues on VF reset
>>
>> ixgbe devices support up to 8 Rx and Tx queues per virtual function.
>> Currently, the registers of only seven queues are set to default when
>> resetting a VF.
>>
> 
> Fixes: d17d0b7a2407 ("ixgbe/base: reset VF registers")
> Cc: stable@dpdk.org
> 
>> Signed-off-by: Simon Ellmann <simon.ellmann@tum.de>
>> ---
>>   drivers/net/ixgbe/base/ixgbe_vf.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
> 
> Good catch, thanks!
> 
> Acked-by: Haiyue Wang <haiyue.wang@intel.com>
> 

This seems a very long lived defect, I am just suspicious if there was a reason 
to limit queue number to 7.


Simon,

How did you find the defect? And did you test/verify it with the update? 
(assuming you are using all 8 queues for the VF)

Re: [dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF reset

[Simon Ellmann]

On 1/4/21 4:56 PM, Ferruh Yigit wrote:

> On 12/18/2020 2:34 AM, Wang, Haiyue wrote:
>>> -----Original Message-----
>>> From: Simon Ellmann <simon.ellmann@tum.de>
>>> Sent: Friday, December 18, 2020 01:15
>>> To: Guo, Jia <jia.guo@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>
>>> Cc: dev@dpdk.org; Simon Ellmann <simon.ellmann@tum.de>
>>> Subject: [PATCH] net/ixgbe: clear registers of all queues on VF reset
>>>
>>> ixgbe devices support up to 8 Rx and Tx queues per virtual function.
>>> Currently, the registers of only seven queues are set to default when
>>> resetting a VF.
>>>
>>
>> Fixes: d17d0b7a2407 ("ixgbe/base: reset VF registers")
>> Cc: stable@dpdk.org
>>
>>> Signed-off-by: Simon Ellmann <simon.ellmann@tum.de>
>>> ---
>>>   drivers/net/ixgbe/base/ixgbe_vf.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>
>> Good catch, thanks!
>>
>> Acked-by: Haiyue Wang <haiyue.wang@intel.com>
>>
>
> This seems a very long lived defect, I am just suspicious if there was 
> a reason to limit queue number to 7.
>
>
> Simon,
>
> How did you find the defect? And did you test/verify it with the 
> update? (assuming you are using all 8 queues for the VF)

Hi Ferruh,

I was implementing ixgbevf for ixy.rs (https://ixy.rs/) by reading the 
code in DPDK and Linux. While doing that I noticed that DPDK was only 
resetting 7 queues which looked like a off-by-one error to me. I would 
have expected a comment if this behaviour was intentional. I haven't 
checked the update.

Simon

Re: [dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF reset

[Ferruh Yigit]

On 1/5/2021 9:02 AM, Simon Ellmann wrote:
> On 1/4/21 4:56 PM, Ferruh Yigit wrote:
> 
>> On 12/18/2020 2:34 AM, Wang, Haiyue wrote:
>>>> -----Original Message-----
>>>> From: Simon Ellmann <simon.ellmann@tum.de>
>>>> Sent: Friday, December 18, 2020 01:15
>>>> To: Guo, Jia <jia.guo@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>
>>>> Cc: dev@dpdk.org; Simon Ellmann <simon.ellmann@tum.de>
>>>> Subject: [PATCH] net/ixgbe: clear registers of all queues on VF reset
>>>>
>>>> ixgbe devices support up to 8 Rx and Tx queues per virtual function.
>>>> Currently, the registers of only seven queues are set to default when
>>>> resetting a VF.
>>>>
>>>
>>> Fixes: d17d0b7a2407 ("ixgbe/base: reset VF registers")
>>> Cc: stable@dpdk.org
>>>
>>>> Signed-off-by: Simon Ellmann <simon.ellmann@tum.de>
>>>> ---
>>>>   drivers/net/ixgbe/base/ixgbe_vf.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>
>>> Good catch, thanks!
>>>
>>> Acked-by: Haiyue Wang <haiyue.wang@intel.com>
>>>
>>
>> This seems a very long lived defect, I am just suspicious if there was a 
>> reason to limit queue number to 7.
>>
>>
>> Simon,
>>
>> How did you find the defect? And did you test/verify it with the update? 
>> (assuming you are using all 8 queues for the VF)
> 
> Hi Ferruh,
> 
> I was implementing ixgbevf for ixy.rs (https://ixy.rs/) by reading the code in 
> DPDK and Linux. While doing that I noticed that DPDK was only resetting 7 queues 
> which looked like a off-by-one error to me. I would have expected a comment if 
> this behaviour was intentional. I haven't checked the update.
> 

Most probably you are right, but when I tried to test this, the HW I have 
doesn't let me set more than 4 queues for the VF, so I was a little suspicious 
about hardcoded 8 value.

What do you think using 'hw->mac.max_rx_queues' instead? Which seems used a few 
other places to walk the queues?

Also can you please point the equivalent code in the Linux driver? As far as I 
can see it also uses 'adapter->num_rx_queues', but I wonder if is there any 
hardcoded value there.

Re: [dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF reset

[Wang, Haiyue]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Tuesday, January 5, 2021 18:30
> To: Simon Ellmann <simon.ellmann@tum.de>; Wang, Haiyue <haiyue.wang@intel.com>; Guo, Jia
> <jia.guo@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF reset
> 
> On 1/5/2021 9:02 AM, Simon Ellmann wrote:
> > On 1/4/21 4:56 PM, Ferruh Yigit wrote:
> >
> >> On 12/18/2020 2:34 AM, Wang, Haiyue wrote:
> >>>> -----Original Message-----
> >>>> From: Simon Ellmann <simon.ellmann@tum.de>
> >>>> Sent: Friday, December 18, 2020 01:15
> >>>> To: Guo, Jia <jia.guo@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>
> >>>> Cc: dev@dpdk.org; Simon Ellmann <simon.ellmann@tum.de>
> >>>> Subject: [PATCH] net/ixgbe: clear registers of all queues on VF reset
> >>>>
> >>>> ixgbe devices support up to 8 Rx and Tx queues per virtual function.
> >>>> Currently, the registers of only seven queues are set to default when
> >>>> resetting a VF.
> >>>>
> >>>
> >>> Fixes: d17d0b7a2407 ("ixgbe/base: reset VF registers")
> >>> Cc: stable@dpdk.org
> >>>
> >>>> Signed-off-by: Simon Ellmann <simon.ellmann@tum.de>
> >>>> ---
> >>>>   drivers/net/ixgbe/base/ixgbe_vf.c | 2 +-
> >>>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>
> >>> Good catch, thanks!
> >>>
> >>> Acked-by: Haiyue Wang <haiyue.wang@intel.com>
> >>>
> >>
> >> This seems a very long lived defect, I am just suspicious if there was a
> >> reason to limit queue number to 7.
> >>
> >>
> >> Simon,
> >>
> >> How did you find the defect? And did you test/verify it with the update?
> >> (assuming you are using all 8 queues for the VF)
> >
> > Hi Ferruh,
> >
> > I was implementing ixgbevf for ixy.rs (https://ixy.rs/) by reading the code in
> > DPDK and Linux. While doing that I noticed that DPDK was only resetting 7 queues
> > which looked like a off-by-one error to me. I would have expected a comment if
> > this behaviour was intentional. I haven't checked the update.
> >
> 
> Most probably you are right, but when I tried to test this, the HW I have
> doesn't let me set more than 4 queues for the VF, so I was a little suspicious
> about hardcoded 8 value.
> 
> What do you think using 'hw->mac.max_rx_queues' instead? Which seems used a few
> other places to walk the queues?
> 
> Also can you please point the equivalent code in the Linux driver? As far as I
> can see it also uses 'adapter->num_rx_queues', but I wonder if is there any
> hardcoded value there.
> 

I found the original detail log:

----
ixgbevf: Clear VF registers we are required to configure anyway after VFLR

This function resets just about any register that could "possibly leak"
any information from one VM instance to another.  The registers set it
is returning to initial values is the same as those we are required
to reconfigure after a VFLR anyway.  So this shouldn't affect any
driver that is doing what it is suppose to.

----

Also, the datasheet says it is '0 ... 7'

Receive DMA Registers,

0x01000 + 0x40*n, n=0...7 VFRDBAL ...

So it should be safe to uses '8' to reset the VF hardware BAR0 information.
The 'hw->mac.max_rx_queues' is about the software resource allocation in PF.

Re: [dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF reset

[Ferruh Yigit]

On 1/5/2021 12:52 PM, Wang, Haiyue wrote:
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Tuesday, January 5, 2021 18:30
>> To: Simon Ellmann <simon.ellmann@tum.de>; Wang, Haiyue <haiyue.wang@intel.com>; Guo, Jia
>> <jia.guo@intel.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF reset
>>
>> On 1/5/2021 9:02 AM, Simon Ellmann wrote:
>>> On 1/4/21 4:56 PM, Ferruh Yigit wrote:
>>>
>>>> On 12/18/2020 2:34 AM, Wang, Haiyue wrote:
>>>>>> -----Original Message-----
>>>>>> From: Simon Ellmann <simon.ellmann@tum.de>
>>>>>> Sent: Friday, December 18, 2020 01:15
>>>>>> To: Guo, Jia <jia.guo@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>
>>>>>> Cc: dev@dpdk.org; Simon Ellmann <simon.ellmann@tum.de>
>>>>>> Subject: [PATCH] net/ixgbe: clear registers of all queues on VF reset
>>>>>>
>>>>>> ixgbe devices support up to 8 Rx and Tx queues per virtual function.
>>>>>> Currently, the registers of only seven queues are set to default when
>>>>>> resetting a VF.
>>>>>>
>>>>>
>>>>> Fixes: d17d0b7a2407 ("ixgbe/base: reset VF registers")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>>> Signed-off-by: Simon Ellmann <simon.ellmann@tum.de>
>>>>>> ---
>>>>>>    drivers/net/ixgbe/base/ixgbe_vf.c | 2 +-
>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>
>>>>> Good catch, thanks!
>>>>>
>>>>> Acked-by: Haiyue Wang <haiyue.wang@intel.com>
>>>>>
>>>>
>>>> This seems a very long lived defect, I am just suspicious if there was a
>>>> reason to limit queue number to 7.
>>>>
>>>>
>>>> Simon,
>>>>
>>>> How did you find the defect? And did you test/verify it with the update?
>>>> (assuming you are using all 8 queues for the VF)
>>>
>>> Hi Ferruh,
>>>
>>> I was implementing ixgbevf for ixy.rs (https://ixy.rs/) by reading the code in
>>> DPDK and Linux. While doing that I noticed that DPDK was only resetting 7 queues
>>> which looked like a off-by-one error to me. I would have expected a comment if
>>> this behaviour was intentional. I haven't checked the update.
>>>
>>
>> Most probably you are right, but when I tried to test this, the HW I have
>> doesn't let me set more than 4 queues for the VF, so I was a little suspicious
>> about hardcoded 8 value.
>>
>> What do you think using 'hw->mac.max_rx_queues' instead? Which seems used a few
>> other places to walk the queues?
>>
>> Also can you please point the equivalent code in the Linux driver? As far as I
>> can see it also uses 'adapter->num_rx_queues', but I wonder if is there any
>> hardcoded value there.
>>
> 
> I found the original detail log:
> 
> ----
> ixgbevf: Clear VF registers we are required to configure anyway after VFLR
> 
> This function resets just about any register that could "possibly leak"
> any information from one VM instance to another.  The registers set it
> is returning to initial values is the same as those we are required
> to reconfigure after a VFLR anyway.  So this shouldn't affect any
> driver that is doing what it is suppose to.
> 
> ----
> 
> Also, the datasheet says it is '0 ... 7'
> 
> Receive DMA Registers,
> 
> 0x01000 + 0x40*n, n=0...7 VFRDBAL ...
> 
> So it should be safe to uses '8' to reset the VF hardware BAR0 information.
> The 'hw->mac.max_rx_queues' is about the software resource allocation in PF.
> 

Thanks Haiuye for the clarification, so I will proceed with the patch.