There is a bug when setting the RSS options in virtio_net that can break
the whole machine, getting the kernel into an infinite loop.
Running the following command in any QEMU virtual machine with virtionet
will reproduce this problem:
# ethtool -X eth0 hfunc toeplitz
This is how the problem happens:
1) ethtool_set_rxfh() calls virtnet_set_rxfh()
2) virtnet_set_rxfh() calls virtnet_commit_rss_command()
3) virtnet_commit_rss_command() populates 4 entries for the rss
scatter-gather
4) Since the command above does not have a key, then the last
scatter-gatter entry will be zeroed, since rss_key_size == 0.
sg_buf_size = vi->rss_key_size;
5) This buffer is passed to qemu, but qemu is not happy with a buffer
with zero length, and do the following in virtqueue_map_desc() (QEMU
function):
if (!sz) {
virtio_error(vdev, "virtio: zero sized buffers are not allowed");
6) virtio_error() (also QEMU function) set the device as broken
vdev->broken = true;
7) Qemu bails out, and do not repond this crazy kernel.
8) The kernel is waiting for the response to come back (function
virtnet_send_command())
9) The kernel is waiting doing the following :
while (!virtqueue_get_buf(vi->cvq, &tmp) &&
!virtqueue_is_broken(vi->cvq))
cpu_relax();
10) None of the following functions above is true, thus, the kernel
loops here forever. Keeping in mind that virtqueue_is_broken() does
not look at the qemu `vdev->broken`, so, it never realizes that the
vitio is broken at QEMU side.
Fix it by not sending the key scatter-gatter key if it is not set.
Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.")
Signed-off-by: Breno Leitao <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/net/virtio_net.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index d7ce4a1011ea..5a7700b103f8 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3041,11 +3041,16 @@ static int virtnet_set_ringparam(struct net_device *dev,
static bool virtnet_commit_rss_command(struct virtnet_info *vi)
{
struct net_device *dev = vi->dev;
+ int has_key = vi->rss_key_size;
struct scatterlist sgs[4];
unsigned int sg_buf_size;
+ int nents = 3;
+
+ if (has_key)
+ nents += 1;
/* prepare sgs */
- sg_init_table(sgs, 4);
+ sg_init_table(sgs, nents);
sg_buf_size = offsetof(struct virtio_net_ctrl_rss, indirection_table);
sg_set_buf(&sgs[0], &vi->ctrl->rss, sg_buf_size);
@@ -3057,8 +3062,13 @@ static bool virtnet_commit_rss_command(struct virtnet_info *vi)
- offsetof(struct virtio_net_ctrl_rss, max_tx_vq);
sg_set_buf(&sgs[2], &vi->ctrl->rss.max_tx_vq, sg_buf_size);
- sg_buf_size = vi->rss_key_size;
- sg_set_buf(&sgs[3], vi->ctrl->rss.key, sg_buf_size);
+ if (has_key) {
+ /* Only populate if key is available, otherwise
+ * populating a buffer with zero size breaks virtio
+ */
+ sg_buf_size = vi->rss_key_size;
+ sg_set_buf(&sgs[3], vi->ctrl->rss.key, sg_buf_size);
+ }
if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
vi->has_rss ? VIRTIO_NET_CTRL_MQ_RSS_CONFIG
--
2.43.0
On Thu, 21 Mar 2024 09:54:30 -0700, Breno Leitao <[email protected]> wrote:
> There is a bug when setting the RSS options in virtio_net that can break
> the whole machine, getting the kernel into an infinite loop.
>
> Running the following command in any QEMU virtual machine with virtionet
> will reproduce this problem:
>
> # ethtool -X eth0 hfunc toeplitz
>
> This is how the problem happens:
>
> 1) ethtool_set_rxfh() calls virtnet_set_rxfh()
>
> 2) virtnet_set_rxfh() calls virtnet_commit_rss_command()
>
> 3) virtnet_commit_rss_command() populates 4 entries for the rss
> scatter-gather
>
> 4) Since the command above does not have a key, then the last
> scatter-gatter entry will be zeroed, since rss_key_size == 0.
> sg_buf_size = vi->rss_key_size;
if (vi->has_rss || vi->has_rss_hash_report) {
vi->rss_indir_table_size =
virtio_cread16(vdev, offsetof(struct virtio_net_config,
rss_max_indirection_table_length));
vi->rss_key_size =
virtio_cread8(vdev, offsetof(struct virtio_net_config, rss_max_key_size));
vi->rss_hash_types_supported =
virtio_cread32(vdev, offsetof(struct virtio_net_config, supported_hash_types));
vi->rss_hash_types_supported &=
~(VIRTIO_NET_RSS_HASH_TYPE_IP_EX |
VIRTIO_NET_RSS_HASH_TYPE_TCP_EX |
VIRTIO_NET_RSS_HASH_TYPE_UDP_EX);
dev->hw_features |= NETIF_F_RXHASH;
}
vi->rss_key_size is initiated here, I wonder if there is something wrong?
Thanks.
>
> 5) This buffer is passed to qemu, but qemu is not happy with a buffer
> with zero length, and do the following in virtqueue_map_desc() (QEMU
> function):
>
> if (!sz) {
> virtio_error(vdev, "virtio: zero sized buffers are not allowed");
>
> 6) virtio_error() (also QEMU function) set the device as broken
>
> vdev->broken = true;
>
> 7) Qemu bails out, and do not repond this crazy kernel.
>
> 8) The kernel is waiting for the response to come back (function
> virtnet_send_command())
>
> 9) The kernel is waiting doing the following :
>
> while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> !virtqueue_is_broken(vi->cvq))
> cpu_relax();
>
> 10) None of the following functions above is true, thus, the kernel
> loops here forever. Keeping in mind that virtqueue_is_broken() does
> not look at the qemu `vdev->broken`, so, it never realizes that the
> vitio is broken at QEMU side.
>
> Fix it by not sending the key scatter-gatter key if it is not set.
>
> Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.")
> Signed-off-by: Breno Leitao <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/net/virtio_net.c | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index d7ce4a1011ea..5a7700b103f8 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3041,11 +3041,16 @@ static int virtnet_set_ringparam(struct net_device *dev,
> static bool virtnet_commit_rss_command(struct virtnet_info *vi)
> {
> struct net_device *dev = vi->dev;
> + int has_key = vi->rss_key_size;
> struct scatterlist sgs[4];
> unsigned int sg_buf_size;
> + int nents = 3;
> +
> + if (has_key)
> + nents += 1;
>
> /* prepare sgs */
> - sg_init_table(sgs, 4);
> + sg_init_table(sgs, nents);
>
> sg_buf_size = offsetof(struct virtio_net_ctrl_rss, indirection_table);
> sg_set_buf(&sgs[0], &vi->ctrl->rss, sg_buf_size);
> @@ -3057,8 +3062,13 @@ static bool virtnet_commit_rss_command(struct virtnet_info *vi)
> - offsetof(struct virtio_net_ctrl_rss, max_tx_vq);
> sg_set_buf(&sgs[2], &vi->ctrl->rss.max_tx_vq, sg_buf_size);
>
> - sg_buf_size = vi->rss_key_size;
> - sg_set_buf(&sgs[3], vi->ctrl->rss.key, sg_buf_size);
> + if (has_key) {
> + /* Only populate if key is available, otherwise
> + * populating a buffer with zero size breaks virtio
> + */
> + sg_buf_size = vi->rss_key_size;
> + sg_set_buf(&sgs[3], vi->ctrl->rss.key, sg_buf_size);
> + }
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
> vi->has_rss ? VIRTIO_NET_CTRL_MQ_RSS_CONFIG
> --
> 2.43.0
>
Hello Xuan,
On Fri, Mar 22, 2024 at 10:00:22AM +0800, Xuan Zhuo wrote:
> On Thu, 21 Mar 2024 09:54:30 -0700, Breno Leitao <[email protected]> wrote:
> > 4) Since the command above does not have a key, then the last
> > scatter-gatter entry will be zeroed, since rss_key_size == 0.
> > sg_buf_size = vi->rss_key_size;
>
>
>
> if (vi->has_rss || vi->has_rss_hash_report) {
> vi->rss_indir_table_size =
> virtio_cread16(vdev, offsetof(struct virtio_net_config,
> rss_max_indirection_table_length));
> vi->rss_key_size =
> virtio_cread8(vdev, offsetof(struct virtio_net_config, rss_max_key_size));
>
> vi->rss_hash_types_supported =
> virtio_cread32(vdev, offsetof(struct virtio_net_config, supported_hash_types));
> vi->rss_hash_types_supported &=
> ~(VIRTIO_NET_RSS_HASH_TYPE_IP_EX |
> VIRTIO_NET_RSS_HASH_TYPE_TCP_EX |
> VIRTIO_NET_RSS_HASH_TYPE_UDP_EX);
>
> dev->hw_features |= NETIF_F_RXHASH;
> }
>
>
> vi->rss_key_size is initiated here, I wonder if there is something wrong?
Not really, the code above is never executed (in my machines). This is
because `vi->has_rss` and `vi->has_rss_hash_report` are both unset.
Looking further, vdev does not have the VIRTIO_NET_F_RSS and
VIRTIO_NET_F_HASH_REPORT features.
Also, when I run `ethtool -x`, I got:
# ethtool -x eth0
RX flow hash indirection table for eth0 with 1 RX ring(s):
Operation not supported
RSS hash key:
Operation not supported
RSS hash function:
toeplitz: on
xor: off
crc32: off
On Fri, 22 Mar 2024 03:21:21 -0700, Breno Leitao <[email protected]> wrote:
> Hello Xuan,
>
> On Fri, Mar 22, 2024 at 10:00:22AM +0800, Xuan Zhuo wrote:
> > On Thu, 21 Mar 2024 09:54:30 -0700, Breno Leitao <[email protected]> wrote:
>
> > > 4) Since the command above does not have a key, then the last
> > > scatter-gatter entry will be zeroed, since rss_key_size == 0.
> > > sg_buf_size = vi->rss_key_size;
> >
> >
> >
> > if (vi->has_rss || vi->has_rss_hash_report) {
> > vi->rss_indir_table_size =
> > virtio_cread16(vdev, offsetof(struct virtio_net_config,
> > rss_max_indirection_table_length));
> > vi->rss_key_size =
> > virtio_cread8(vdev, offsetof(struct virtio_net_config, rss_max_key_size));
> >
> > vi->rss_hash_types_supported =
> > virtio_cread32(vdev, offsetof(struct virtio_net_config, supported_hash_types));
> > vi->rss_hash_types_supported &=
> > ~(VIRTIO_NET_RSS_HASH_TYPE_IP_EX |
> > VIRTIO_NET_RSS_HASH_TYPE_TCP_EX |
> > VIRTIO_NET_RSS_HASH_TYPE_UDP_EX);
> >
> > dev->hw_features |= NETIF_F_RXHASH;
> > }
> >
> >
> > vi->rss_key_size is initiated here, I wonder if there is something wrong?
>
> Not really, the code above is never executed (in my machines). This is
> because `vi->has_rss` and `vi->has_rss_hash_report` are both unset.
>
> Looking further, vdev does not have the VIRTIO_NET_F_RSS and
> VIRTIO_NET_F_HASH_REPORT features.
>
> Also, when I run `ethtool -x`, I got:
>
> # ethtool -x eth0
> RX flow hash indirection table for eth0 with 1 RX ring(s):
> Operation not supported
> RSS hash key:
> Operation not supported
> RSS hash function:
> toeplitz: on
> xor: off
> crc32: off
The spec saies:
Note that if the device offers VIRTIO_NET_F_HASH_REPORT, even if it
supports only one pair of virtqueues, it MUST support at least one of
commands of VIRTIO_NET_CTRL_MQ class to configure reported hash
parameters:
If the device offers VIRTIO_NET_F_RSS, it MUST support
VIRTIO_NET_CTRL_MQ_RSS_CONFIG command per 5.1.6.5.7.1.
Otherwise the device MUST support VIRTIO_NET_CTRL_MQ_HASH_CONFIG command
per 5.1.6.5.6.4.
So if we have not anyone of `vi->has_rss` and `vi->has_rss_hash_report`,
we should return from virtnet_set_rxfh directly.
Thanks.
Hello Xuan,
On Mon, Mar 25, 2024 at 01:57:53PM +0800, Xuan Zhuo wrote:
> On Fri, 22 Mar 2024 03:21:21 -0700, Breno Leitao <[email protected]> wrote:
> > Hello Xuan,
> >
> > On Fri, Mar 22, 2024 at 10:00:22AM +0800, Xuan Zhuo wrote:
> > > On Thu, 21 Mar 2024 09:54:30 -0700, Breno Leitao <[email protected]> wrote:
> >
> > > > 4) Since the command above does not have a key, then the last
> > > > scatter-gatter entry will be zeroed, since rss_key_size == 0.
> > > > sg_buf_size = vi->rss_key_size;
> > >
> > >
> > >
> > > if (vi->has_rss || vi->has_rss_hash_report) {
> > > vi->rss_indir_table_size =
> > > virtio_cread16(vdev, offsetof(struct virtio_net_config,
> > > rss_max_indirection_table_length));
> > > vi->rss_key_size =
> > > virtio_cread8(vdev, offsetof(struct virtio_net_config, rss_max_key_size));
> > >
> > > vi->rss_hash_types_supported =
> > > virtio_cread32(vdev, offsetof(struct virtio_net_config, supported_hash_types));
> > > vi->rss_hash_types_supported &=
> > > ~(VIRTIO_NET_RSS_HASH_TYPE_IP_EX |
> > > VIRTIO_NET_RSS_HASH_TYPE_TCP_EX |
> > > VIRTIO_NET_RSS_HASH_TYPE_UDP_EX);
> > >
> > > dev->hw_features |= NETIF_F_RXHASH;
> > > }
> > >
> > >
> > > vi->rss_key_size is initiated here, I wonder if there is something wrong?
> >
> > Not really, the code above is never executed (in my machines). This is
> > because `vi->has_rss` and `vi->has_rss_hash_report` are both unset.
> >
> > Looking further, vdev does not have the VIRTIO_NET_F_RSS and
> > VIRTIO_NET_F_HASH_REPORT features.
> >
> > Also, when I run `ethtool -x`, I got:
> >
> > # ethtool -x eth0
> > RX flow hash indirection table for eth0 with 1 RX ring(s):
> > Operation not supported
> > RSS hash key:
> > Operation not supported
> > RSS hash function:
> > toeplitz: on
> > xor: off
> > crc32: off
>
>
> The spec saies:
> Note that if the device offers VIRTIO_NET_F_HASH_REPORT, even if it
> supports only one pair of virtqueues, it MUST support at least one of
> commands of VIRTIO_NET_CTRL_MQ class to configure reported hash
> parameters:
>
> If the device offers VIRTIO_NET_F_RSS, it MUST support
> VIRTIO_NET_CTRL_MQ_RSS_CONFIG command per 5.1.6.5.7.1.
>
> Otherwise the device MUST support VIRTIO_NET_CTRL_MQ_HASH_CONFIG command
> per 5.1.6.5.6.4.
>
>
> So if we have not anyone of `vi->has_rss` and `vi->has_rss_hash_report`,
> we should return from virtnet_set_rxfh directly.
Makes sense. Although it is not clear to me how vi->has_rss_hash_report
is related here, but, I am convinced that we shouldn't do any RSS
operation if the device doesn't have the RSS feature, i.e, vi->has_rss
is false.
That said, I am thinking about something like this. How does it sound?
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 5a7700b103f8..8c1ad7361cf2 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3780,6 +3780,9 @@ static int virtnet_set_rxfh(struct net_device *dev,
struct virtnet_info *vi = netdev_priv(dev);
int i;
+ if (!vi->has_rss)
+ return -EOPNOTSUPP;
+
if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE &&
rxfh->hfunc != ETH_RSS_HASH_TOP)
return -EOPNOTSUPP;
Thanks!
在 2024/3/25 下午7:35, Xuan Zhuo 写道:
> On Mon, 25 Mar 2024 04:26:08 -0700, Breno Leitao <[email protected]> wrote:
>> Hello Xuan,
>>
>> On Mon, Mar 25, 2024 at 01:57:53PM +0800, Xuan Zhuo wrote:
>>> On Fri, 22 Mar 2024 03:21:21 -0700, Breno Leitao <[email protected]> wrote:
>>>> Hello Xuan,
>>>>
>>>> On Fri, Mar 22, 2024 at 10:00:22AM +0800, Xuan Zhuo wrote:
>>>>> On Thu, 21 Mar 2024 09:54:30 -0700, Breno Leitao <[email protected]> wrote:
>>>>>> 4) Since the command above does not have a key, then the last
>>>>>> scatter-gatter entry will be zeroed, since rss_key_size == 0.
>>>>>> sg_buf_size = vi->rss_key_size;
>>>>>
>>>>>
>>>>> if (vi->has_rss || vi->has_rss_hash_report) {
>>>>> vi->rss_indir_table_size =
>>>>> virtio_cread16(vdev, offsetof(struct virtio_net_config,
>>>>> rss_max_indirection_table_length));
>>>>> vi->rss_key_size =
>>>>> virtio_cread8(vdev, offsetof(struct virtio_net_config, rss_max_key_size));
>>>>>
>>>>> vi->rss_hash_types_supported =
>>>>> virtio_cread32(vdev, offsetof(struct virtio_net_config, supported_hash_types));
>>>>> vi->rss_hash_types_supported &=
>>>>> ~(VIRTIO_NET_RSS_HASH_TYPE_IP_EX |
>>>>> VIRTIO_NET_RSS_HASH_TYPE_TCP_EX |
>>>>> VIRTIO_NET_RSS_HASH_TYPE_UDP_EX);
>>>>>
>>>>> dev->hw_features |= NETIF_F_RXHASH;
>>>>> }
>>>>>
>>>>>
>>>>> vi->rss_key_size is initiated here, I wonder if there is something wrong?
>>>> Not really, the code above is never executed (in my machines). This is
>>>> because `vi->has_rss` and `vi->has_rss_hash_report` are both unset.
>>>>
>>>> Looking further, vdev does not have the VIRTIO_NET_F_RSS and
>>>> VIRTIO_NET_F_HASH_REPORT features.
>>>>
>>>> Also, when I run `ethtool -x`, I got:
>>>>
>>>> # ethtool -x eth0
>>>> RX flow hash indirection table for eth0 with 1 RX ring(s):
>>>> Operation not supported
>>>> RSS hash key:
>>>> Operation not supported
>>>> RSS hash function:
>>>> toeplitz: on
>>>> xor: off
>>>> crc32: off
>>>
>>> The spec saies:
>>> Note that if the device offers VIRTIO_NET_F_HASH_REPORT, even if it
>>> supports only one pair of virtqueues, it MUST support at least one of
>>> commands of VIRTIO_NET_CTRL_MQ class to configure reported hash
>>> parameters:
>>>
>>> If the device offers VIRTIO_NET_F_RSS, it MUST support
>>> VIRTIO_NET_CTRL_MQ_RSS_CONFIG command per 5.1.6.5.7.1.
>>>
>>> Otherwise the device MUST support VIRTIO_NET_CTRL_MQ_HASH_CONFIG command
>>> per 5.1.6.5.6.4.
>>>
>>>
>>> So if we have not anyone of `vi->has_rss` and `vi->has_rss_hash_report`,
>>> we should return from virtnet_set_rxfh directly.
>> Makes sense. Although it is not clear to me how vi->has_rss_hash_report
>> is related here, but, I am convinced that we shouldn't do any RSS
>> operation if the device doesn't have the RSS feature, i.e, vi->has_rss
>> is false.
>>
>> That said, I am thinking about something like this. How does it sound?
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 5a7700b103f8..8c1ad7361cf2 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -3780,6 +3780,9 @@ static int virtnet_set_rxfh(struct net_device *dev,
>> struct virtnet_info *vi = netdev_priv(dev);
>> int i;
>>
>> + if (!vi->has_rss)
>> + return -EOPNOTSUPP;
>> +
> Should we check has_rss_hash_report?
Hi, Breno.
You can refer to the following modification. It is worth noting
that \field{rss_max_indirection_table_length} should only be
accessed if VIRTIO_NET_F_RSS is negotiated, which I have
modified below:
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 727c874..fb4c438 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3836,10 +3836,16 @@ static int virtnet_set_rxfh(struct net_device *dev,
struct virtnet_info *vi = netdev_priv(dev);
int i;
+ if (!vi->has_rss && !vi->has_rss_hash_report)
+ return -EOPNOTSUPP;
+
if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE &&
rxfh->hfunc != ETH_RSS_HASH_TOP)
return -EOPNOTSUPP;
+ if (rxfh->indir && !vi->has_rss)
+ return -EINVAL;
+
if (rxfh->indir) {
for (i = 0; i < vi->rss_indir_table_size; ++i)
vi->ctrl->rss.indirection_table[i] =
rxfh->indir[i];
@@ -4757,13 +4763,14 @@ static int virtnet_probe(struct virtio_device *vdev)
if (virtio_has_feature(vdev, VIRTIO_NET_F_HASH_REPORT))
vi->has_rss_hash_report = true;
- if (virtio_has_feature(vdev, VIRTIO_NET_F_RSS))
+ if (virtio_has_feature(vdev, VIRTIO_NET_F_RSS)) {
vi->has_rss = true;
-
- if (vi->has_rss || vi->has_rss_hash_report) {
vi->rss_indir_table_size =
virtio_cread16(vdev, offsetof(struct
virtio_net_config,
rss_max_indirection_table_length));
+ }
+
+ if (vi->has_rss || vi->has_rss_hash_report) {
vi->rss_key_size =
virtio_cread8(vdev, offsetof(struct
virtio_net_config, rss_max_key_size));
Regards,
Heng
>
> @Heng Qi
>
> Could you help us?
>
> Thanks.
>
>
>> if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE &&
>> rxfh->hfunc != ETH_RSS_HASH_TOP)
>> return -EOPNOTSUPP;
>>
>> Thanks!
On Mon, 25 Mar 2024 04:26:08 -0700, Breno Leitao <[email protected]> wrote:
> Hello Xuan,
>
> On Mon, Mar 25, 2024 at 01:57:53PM +0800, Xuan Zhuo wrote:
> > On Fri, 22 Mar 2024 03:21:21 -0700, Breno Leitao <[email protected]> wrote:
> > > Hello Xuan,
> > >
> > > On Fri, Mar 22, 2024 at 10:00:22AM +0800, Xuan Zhuo wrote:
> > > > On Thu, 21 Mar 2024 09:54:30 -0700, Breno Leitao <[email protected]> wrote:
> > >
> > > > > 4) Since the command above does not have a key, then the last
> > > > > scatter-gatter entry will be zeroed, since rss_key_size == 0.
> > > > > sg_buf_size = vi->rss_key_size;
> > > >
> > > >
> > > >
> > > > if (vi->has_rss || vi->has_rss_hash_report) {
> > > > vi->rss_indir_table_size =
> > > > virtio_cread16(vdev, offsetof(struct virtio_net_config,
> > > > rss_max_indirection_table_length));
> > > > vi->rss_key_size =
> > > > virtio_cread8(vdev, offsetof(struct virtio_net_config, rss_max_key_size));
> > > >
> > > > vi->rss_hash_types_supported =
> > > > virtio_cread32(vdev, offsetof(struct virtio_net_config, supported_hash_types));
> > > > vi->rss_hash_types_supported &=
> > > > ~(VIRTIO_NET_RSS_HASH_TYPE_IP_EX |
> > > > VIRTIO_NET_RSS_HASH_TYPE_TCP_EX |
> > > > VIRTIO_NET_RSS_HASH_TYPE_UDP_EX);
> > > >
> > > > dev->hw_features |= NETIF_F_RXHASH;
> > > > }
> > > >
> > > >
> > > > vi->rss_key_size is initiated here, I wonder if there is something wrong?
> > >
> > > Not really, the code above is never executed (in my machines). This is
> > > because `vi->has_rss` and `vi->has_rss_hash_report` are both unset.
> > >
> > > Looking further, vdev does not have the VIRTIO_NET_F_RSS and
> > > VIRTIO_NET_F_HASH_REPORT features.
> > >
> > > Also, when I run `ethtool -x`, I got:
> > >
> > > # ethtool -x eth0
> > > RX flow hash indirection table for eth0 with 1 RX ring(s):
> > > Operation not supported
> > > RSS hash key:
> > > Operation not supported
> > > RSS hash function:
> > > toeplitz: on
> > > xor: off
> > > crc32: off
> >
> >
> > The spec saies:
> > Note that if the device offers VIRTIO_NET_F_HASH_REPORT, even if it
> > supports only one pair of virtqueues, it MUST support at least one of
> > commands of VIRTIO_NET_CTRL_MQ class to configure reported hash
> > parameters:
> >
> > If the device offers VIRTIO_NET_F_RSS, it MUST support
> > VIRTIO_NET_CTRL_MQ_RSS_CONFIG command per 5.1.6.5.7.1.
> >
> > Otherwise the device MUST support VIRTIO_NET_CTRL_MQ_HASH_CONFIG command
> > per 5.1.6.5.6.4.
> >
> >
> > So if we have not anyone of `vi->has_rss` and `vi->has_rss_hash_report`,
> > we should return from virtnet_set_rxfh directly.
>
> Makes sense. Although it is not clear to me how vi->has_rss_hash_report
> is related here, but, I am convinced that we shouldn't do any RSS
> operation if the device doesn't have the RSS feature, i.e, vi->has_rss
> is false.
>
> That said, I am thinking about something like this. How does it sound?
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 5a7700b103f8..8c1ad7361cf2 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3780,6 +3780,9 @@ static int virtnet_set_rxfh(struct net_device *dev,
> struct virtnet_info *vi = netdev_priv(dev);
> int i;
>
> + if (!vi->has_rss)
> + return -EOPNOTSUPP;
> +
Should we check has_rss_hash_report?
@Heng Qi
Could you help us?
Thanks.
> if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE &&
> rxfh->hfunc != ETH_RSS_HASH_TOP)
> return -EOPNOTSUPP;
>
> Thanks!