DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF reset
@ 2020-12-17 17:14 Simon Ellmann
  2020-12-18  2:34 ` Wang, Haiyue
  0 siblings, 1 reply; 8+ messages in thread
From: Simon Ellmann @ 2020-12-17 17:14 UTC (permalink / raw)
  To: Jeff Guo, Haiyue Wang; +Cc: dev, 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF reset
  2020-12-17 17:14 [dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF reset Simon Ellmann
@ 2020-12-18  2:34 ` Wang, Haiyue
  2020-12-23 10:17   ` Zhang, Qi Z
  2021-01-04 15:56   ` Ferruh Yigit
  0 siblings, 2 replies; 8+ messages in thread
From: Wang, Haiyue @ 2020-12-18  2:34 UTC (permalink / raw)
  To: Simon Ellmann, Guo, Jia; +Cc: dev

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF reset
  2020-12-18  2:34 ` Wang, Haiyue
@ 2020-12-23 10:17   ` Zhang, Qi Z
  2021-01-04 15:56   ` Ferruh Yigit
  1 sibling, 0 replies; 8+ messages in thread
From: Zhang, Qi Z @ 2020-12-23 10:17 UTC (permalink / raw)
  To: Wang, Haiyue, Simon Ellmann, Guo, Jia; +Cc: dev



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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF reset
  2020-12-18  2:34 ` Wang, Haiyue
  2020-12-23 10:17   ` Zhang, Qi Z
@ 2021-01-04 15:56   ` Ferruh Yigit
  2021-01-05  9:02     ` Simon Ellmann
  1 sibling, 1 reply; 8+ messages in thread
From: Ferruh Yigit @ 2021-01-04 15:56 UTC (permalink / raw)
  To: Wang, Haiyue, Simon Ellmann, Guo, Jia; +Cc: dev

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)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF reset
  2021-01-04 15:56   ` Ferruh Yigit
@ 2021-01-05  9:02     ` Simon Ellmann
  2021-01-05 10:29       ` Ferruh Yigit
  0 siblings, 1 reply; 8+ messages in thread
From: Simon Ellmann @ 2021-01-05  9:02 UTC (permalink / raw)
  To: Ferruh Yigit, Wang, Haiyue, Guo, Jia; +Cc: dev

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF reset
  2021-01-05  9:02     ` Simon Ellmann
@ 2021-01-05 10:29       ` Ferruh Yigit
  2021-01-05 12:52         ` Wang, Haiyue
  0 siblings, 1 reply; 8+ messages in thread
From: Ferruh Yigit @ 2021-01-05 10:29 UTC (permalink / raw)
  To: Simon Ellmann, Wang, Haiyue, Guo, Jia; +Cc: dev

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.



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF reset
  2021-01-05 10:29       ` Ferruh Yigit
@ 2021-01-05 12:52         ` Wang, Haiyue
  2021-01-05 14:10           ` Ferruh Yigit
  0 siblings, 1 reply; 8+ messages in thread
From: Wang, Haiyue @ 2021-01-05 12:52 UTC (permalink / raw)
  To: Yigit, Ferruh, Simon Ellmann, Guo, Jia; +Cc: dev

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF reset
  2021-01-05 12:52         ` Wang, Haiyue
@ 2021-01-05 14:10           ` Ferruh Yigit
  0 siblings, 0 replies; 8+ messages in thread
From: Ferruh Yigit @ 2021-01-05 14:10 UTC (permalink / raw)
  To: Wang, Haiyue, Simon Ellmann, Guo, Jia; +Cc: dev

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.


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-01-05 14:10 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-17 17:14 [dpdk-dev] [PATCH] net/ixgbe: clear registers of all queues on VF reset Simon Ellmann
2020-12-18  2:34 ` Wang, Haiyue
2020-12-23 10:17   ` Zhang, Qi Z
2021-01-04 15:56   ` Ferruh Yigit
2021-01-05  9:02     ` Simon Ellmann
2021-01-05 10:29       ` Ferruh Yigit
2021-01-05 12:52         ` Wang, Haiyue
2021-01-05 14:10           ` Ferruh Yigit

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git