VIRTIO_NET_F_HASH_REPORT allows to report hash values calculated on the
host. When VHOST_NET_F_VIRTIO_NET_HDR is employed, it will report no
hash values (i.e., the hash_report member is always set to
VIRTIO_NET_HASH_REPORT_NONE). Otherwise, the values reported by the
underlying socket will be reported.
VIRTIO_NET_F_HASH_REPORT requires VIRTIO_F_VERSION_1.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Tested-by: Lei Yang <leiyang@redhat.com>
---
drivers/vhost/net.c | 49 +++++++++++++++++++++++++++++--------------------
1 file changed, 29 insertions(+), 20 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index b9b9e9d40951856d881d77ac74331d914473cd56..16b241b44f89820a42c302f3586ea6bb5e0d4289 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -73,6 +73,7 @@ enum {
VHOST_NET_FEATURES = VHOST_FEATURES |
(1ULL << VHOST_NET_F_VIRTIO_NET_HDR) |
(1ULL << VIRTIO_NET_F_MRG_RXBUF) |
+ (1ULL << VIRTIO_NET_F_HASH_REPORT) |
(1ULL << VIRTIO_F_ACCESS_PLATFORM) |
(1ULL << VIRTIO_F_RING_RESET)
};
@@ -1097,9 +1098,11 @@ static void handle_rx(struct vhost_net *net)
.msg_controllen = 0,
.msg_flags = MSG_DONTWAIT,
};
- struct virtio_net_hdr hdr = {
- .flags = 0,
- .gso_type = VIRTIO_NET_HDR_GSO_NONE
+ struct virtio_net_hdr_v1_hash hdr = {
+ .hdr = {
+ .flags = 0,
+ .gso_type = VIRTIO_NET_HDR_GSO_NONE
+ }
};
size_t total_len = 0;
int err, mergeable;
@@ -1110,7 +1113,6 @@ static void handle_rx(struct vhost_net *net)
bool set_num_buffers;
struct socket *sock;
struct iov_iter fixup;
- __virtio16 num_buffers;
int recv_pkts = 0;
mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_RX);
@@ -1191,30 +1193,30 @@ static void handle_rx(struct vhost_net *net)
vhost_discard_vq_desc(vq, headcount);
continue;
}
+ hdr.hdr.num_buffers = cpu_to_vhost16(vq, headcount);
/* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
if (unlikely(vhost_hlen)) {
- if (copy_to_iter(&hdr, sizeof(hdr),
- &fixup) != sizeof(hdr)) {
+ if (copy_to_iter(&hdr, vhost_hlen,
+ &fixup) != vhost_hlen) {
vq_err(vq, "Unable to write vnet_hdr "
"at addr %p\n", vq->iov->iov_base);
goto out;
}
- } else {
+ } else if (likely(set_num_buffers)) {
/* Header came from socket; we'll need to patch
* ->num_buffers over if VIRTIO_NET_F_MRG_RXBUF
*/
- iov_iter_advance(&fixup, sizeof(hdr));
+ iov_iter_advance(&fixup, offsetof(struct virtio_net_hdr_v1, num_buffers));
+
+ if (copy_to_iter(&hdr.hdr.num_buffers, sizeof(hdr.hdr.num_buffers),
+ &fixup) != sizeof(hdr.hdr.num_buffers)) {
+ vq_err(vq, "Failed num_buffers write");
+ vhost_discard_vq_desc(vq, headcount);
+ goto out;
+ }
}
/* TODO: Should check and handle checksum. */
- num_buffers = cpu_to_vhost16(vq, headcount);
- if (likely(set_num_buffers) &&
- copy_to_iter(&num_buffers, sizeof num_buffers,
- &fixup) != sizeof num_buffers) {
- vq_err(vq, "Failed num_buffers write");
- vhost_discard_vq_desc(vq, headcount);
- goto out;
- }
nvq->done_idx += headcount;
if (nvq->done_idx > VHOST_NET_BATCH)
vhost_net_signal_used(nvq);
@@ -1607,10 +1609,13 @@ static int vhost_net_set_features(struct vhost_net *n, u64 features)
size_t vhost_hlen, sock_hlen, hdr_len;
int i;
- hdr_len = (features & ((1ULL << VIRTIO_NET_F_MRG_RXBUF) |
- (1ULL << VIRTIO_F_VERSION_1))) ?
- sizeof(struct virtio_net_hdr_mrg_rxbuf) :
- sizeof(struct virtio_net_hdr);
+ if (features & (1ULL << VIRTIO_NET_F_HASH_REPORT))
+ hdr_len = sizeof(struct virtio_net_hdr_v1_hash);
+ else if (features & ((1ULL << VIRTIO_NET_F_MRG_RXBUF) |
+ (1ULL << VIRTIO_F_VERSION_1)))
+ hdr_len = sizeof(struct virtio_net_hdr_mrg_rxbuf);
+ else
+ hdr_len = sizeof(struct virtio_net_hdr);
if (features & (1 << VHOST_NET_F_VIRTIO_NET_HDR)) {
/* vhost provides vnet_hdr */
vhost_hlen = hdr_len;
@@ -1691,6 +1696,10 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
return -EFAULT;
if (features & ~VHOST_NET_FEATURES)
return -EOPNOTSUPP;
+ if ((features & ((1ULL << VIRTIO_F_VERSION_1) |
+ (1ULL << VIRTIO_NET_F_HASH_REPORT))) ==
+ (1ULL << VIRTIO_NET_F_HASH_REPORT))
+ return -EINVAL;
return vhost_net_set_features(n, features);
case VHOST_GET_BACKEND_FEATURES:
features = VHOST_NET_BACKEND_FEATURES;
--
2.48.1On Fri, Mar 7, 2025 at 7:02 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> VIRTIO_NET_F_HASH_REPORT allows to report hash values calculated on the
> host. When VHOST_NET_F_VIRTIO_NET_HDR is employed, it will report no
> hash values (i.e., the hash_report member is always set to
> VIRTIO_NET_HASH_REPORT_NONE). Otherwise, the values reported by the
> underlying socket will be reported.
>
> VIRTIO_NET_F_HASH_REPORT requires VIRTIO_F_VERSION_1.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Tested-by: Lei Yang <leiyang@redhat.com>
> ---
> drivers/vhost/net.c | 49 +++++++++++++++++++++++++++++--------------------
> 1 file changed, 29 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index b9b9e9d40951856d881d77ac74331d914473cd56..16b241b44f89820a42c302f3586ea6bb5e0d4289 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -73,6 +73,7 @@ enum {
> VHOST_NET_FEATURES = VHOST_FEATURES |
> (1ULL << VHOST_NET_F_VIRTIO_NET_HDR) |
> (1ULL << VIRTIO_NET_F_MRG_RXBUF) |
> + (1ULL << VIRTIO_NET_F_HASH_REPORT) |
> (1ULL << VIRTIO_F_ACCESS_PLATFORM) |
> (1ULL << VIRTIO_F_RING_RESET)
> };
> @@ -1097,9 +1098,11 @@ static void handle_rx(struct vhost_net *net)
> .msg_controllen = 0,
> .msg_flags = MSG_DONTWAIT,
> };
> - struct virtio_net_hdr hdr = {
> - .flags = 0,
> - .gso_type = VIRTIO_NET_HDR_GSO_NONE
> + struct virtio_net_hdr_v1_hash hdr = {
> + .hdr = {
> + .flags = 0,
> + .gso_type = VIRTIO_NET_HDR_GSO_NONE
> + }
> };
> size_t total_len = 0;
> int err, mergeable;
> @@ -1110,7 +1113,6 @@ static void handle_rx(struct vhost_net *net)
> bool set_num_buffers;
> struct socket *sock;
> struct iov_iter fixup;
> - __virtio16 num_buffers;
> int recv_pkts = 0;
>
> mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_RX);
> @@ -1191,30 +1193,30 @@ static void handle_rx(struct vhost_net *net)
> vhost_discard_vq_desc(vq, headcount);
> continue;
> }
> + hdr.hdr.num_buffers = cpu_to_vhost16(vq, headcount);
> /* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
> if (unlikely(vhost_hlen)) {
> - if (copy_to_iter(&hdr, sizeof(hdr),
> - &fixup) != sizeof(hdr)) {
> + if (copy_to_iter(&hdr, vhost_hlen,
> + &fixup) != vhost_hlen) {
> vq_err(vq, "Unable to write vnet_hdr "
> "at addr %p\n", vq->iov->iov_base);
> goto out;
Is this an "issue" specific to RSS/HASH? If it's not, we need a separate patch.
Honestly, I'm not sure if it's too late to fix this.
Others look fine.
Thanks
On 2025/03/10 13:43, Jason Wang wrote:
> On Fri, Mar 7, 2025 at 7:02 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> VIRTIO_NET_F_HASH_REPORT allows to report hash values calculated on the
>> host. When VHOST_NET_F_VIRTIO_NET_HDR is employed, it will report no
>> hash values (i.e., the hash_report member is always set to
>> VIRTIO_NET_HASH_REPORT_NONE). Otherwise, the values reported by the
>> underlying socket will be reported.
>>
>> VIRTIO_NET_F_HASH_REPORT requires VIRTIO_F_VERSION_1.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> Tested-by: Lei Yang <leiyang@redhat.com>
>> ---
>> drivers/vhost/net.c | 49 +++++++++++++++++++++++++++++--------------------
>> 1 file changed, 29 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>> index b9b9e9d40951856d881d77ac74331d914473cd56..16b241b44f89820a42c302f3586ea6bb5e0d4289 100644
>> --- a/drivers/vhost/net.c
>> +++ b/drivers/vhost/net.c
>> @@ -73,6 +73,7 @@ enum {
>> VHOST_NET_FEATURES = VHOST_FEATURES |
>> (1ULL << VHOST_NET_F_VIRTIO_NET_HDR) |
>> (1ULL << VIRTIO_NET_F_MRG_RXBUF) |
>> + (1ULL << VIRTIO_NET_F_HASH_REPORT) |
>> (1ULL << VIRTIO_F_ACCESS_PLATFORM) |
>> (1ULL << VIRTIO_F_RING_RESET)
>> };
>> @@ -1097,9 +1098,11 @@ static void handle_rx(struct vhost_net *net)
>> .msg_controllen = 0,
>> .msg_flags = MSG_DONTWAIT,
>> };
>> - struct virtio_net_hdr hdr = {
>> - .flags = 0,
>> - .gso_type = VIRTIO_NET_HDR_GSO_NONE
>> + struct virtio_net_hdr_v1_hash hdr = {
>> + .hdr = {
>> + .flags = 0,
>> + .gso_type = VIRTIO_NET_HDR_GSO_NONE
>> + }
>> };
>> size_t total_len = 0;
>> int err, mergeable;
>> @@ -1110,7 +1113,6 @@ static void handle_rx(struct vhost_net *net)
>> bool set_num_buffers;
>> struct socket *sock;
>> struct iov_iter fixup;
>> - __virtio16 num_buffers;
>> int recv_pkts = 0;
>>
>> mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_RX);
>> @@ -1191,30 +1193,30 @@ static void handle_rx(struct vhost_net *net)
>> vhost_discard_vq_desc(vq, headcount);
>> continue;
>> }
>> + hdr.hdr.num_buffers = cpu_to_vhost16(vq, headcount);
>> /* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
>> if (unlikely(vhost_hlen)) {
>> - if (copy_to_iter(&hdr, sizeof(hdr),
>> - &fixup) != sizeof(hdr)) {
>> + if (copy_to_iter(&hdr, vhost_hlen,
>> + &fixup) != vhost_hlen) {
>> vq_err(vq, "Unable to write vnet_hdr "
>> "at addr %p\n", vq->iov->iov_base);
>> goto out;
>
> Is this an "issue" specific to RSS/HASH? If it's not, we need a separate patch.
>
> Honestly, I'm not sure if it's too late to fix this.
There is nothing wrong with the current implementation. The current
implementation fills the header with zero except num_buffers, which it
fills some real value. This functionality is working fine with
VIRTIO_NET_F_MRG_RXBUF and VIRTIO_F_VERSION_1, which change the header size.
Now I'm adding VIRTIO_NET_F_HASH_REPORT and it adds the hash_report
field, which also needs to be initialized with zero, so I'm making sure
vhost_net will also initialize it.
Regards,
Akihiko Odaki
>
> Others look fine.
>
> Thanks
>
On Mon, Mar 10, 2025 at 3:04 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2025/03/10 13:43, Jason Wang wrote:
> > On Fri, Mar 7, 2025 at 7:02 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> VIRTIO_NET_F_HASH_REPORT allows to report hash values calculated on the
> >> host. When VHOST_NET_F_VIRTIO_NET_HDR is employed, it will report no
> >> hash values (i.e., the hash_report member is always set to
> >> VIRTIO_NET_HASH_REPORT_NONE). Otherwise, the values reported by the
> >> underlying socket will be reported.
> >>
> >> VIRTIO_NET_F_HASH_REPORT requires VIRTIO_F_VERSION_1.
> >>
> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >> Tested-by: Lei Yang <leiyang@redhat.com>
> >> ---
> >> drivers/vhost/net.c | 49 +++++++++++++++++++++++++++++--------------------
> >> 1 file changed, 29 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> >> index b9b9e9d40951856d881d77ac74331d914473cd56..16b241b44f89820a42c302f3586ea6bb5e0d4289 100644
> >> --- a/drivers/vhost/net.c
> >> +++ b/drivers/vhost/net.c
> >> @@ -73,6 +73,7 @@ enum {
> >> VHOST_NET_FEATURES = VHOST_FEATURES |
> >> (1ULL << VHOST_NET_F_VIRTIO_NET_HDR) |
> >> (1ULL << VIRTIO_NET_F_MRG_RXBUF) |
> >> + (1ULL << VIRTIO_NET_F_HASH_REPORT) |
> >> (1ULL << VIRTIO_F_ACCESS_PLATFORM) |
> >> (1ULL << VIRTIO_F_RING_RESET)
> >> };
> >> @@ -1097,9 +1098,11 @@ static void handle_rx(struct vhost_net *net)
> >> .msg_controllen = 0,
> >> .msg_flags = MSG_DONTWAIT,
> >> };
> >> - struct virtio_net_hdr hdr = {
> >> - .flags = 0,
> >> - .gso_type = VIRTIO_NET_HDR_GSO_NONE
> >> + struct virtio_net_hdr_v1_hash hdr = {
> >> + .hdr = {
> >> + .flags = 0,
> >> + .gso_type = VIRTIO_NET_HDR_GSO_NONE
> >> + }
> >> };
> >> size_t total_len = 0;
> >> int err, mergeable;
> >> @@ -1110,7 +1113,6 @@ static void handle_rx(struct vhost_net *net)
> >> bool set_num_buffers;
> >> struct socket *sock;
> >> struct iov_iter fixup;
> >> - __virtio16 num_buffers;
> >> int recv_pkts = 0;
> >>
> >> mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_RX);
> >> @@ -1191,30 +1193,30 @@ static void handle_rx(struct vhost_net *net)
> >> vhost_discard_vq_desc(vq, headcount);
> >> continue;
> >> }
> >> + hdr.hdr.num_buffers = cpu_to_vhost16(vq, headcount);
> >> /* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
> >> if (unlikely(vhost_hlen)) {
> >> - if (copy_to_iter(&hdr, sizeof(hdr),
> >> - &fixup) != sizeof(hdr)) {
> >> + if (copy_to_iter(&hdr, vhost_hlen,
> >> + &fixup) != vhost_hlen) {
> >> vq_err(vq, "Unable to write vnet_hdr "
> >> "at addr %p\n", vq->iov->iov_base);
> >> goto out;
> >
> > Is this an "issue" specific to RSS/HASH? If it's not, we need a separate patch.
> >
> > Honestly, I'm not sure if it's too late to fix this.
>
> There is nothing wrong with the current implementation.
Note that I meant the vhost_hlen part, and the current code is tricky.
The comment said:
"""
/* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
"""
So it tries to only offer virtio_net_hdr even if vhost_hlen is the set
to mrg_rxbuf len.
And this patch changes this behaviour.
Thanks
> The current
> implementation fills the header with zero except num_buffers, which it
> fills some real value. This functionality is working fine with
> VIRTIO_NET_F_MRG_RXBUF and VIRTIO_F_VERSION_1, which change the header size.
>
> Now I'm adding VIRTIO_NET_F_HASH_REPORT and it adds the hash_report
> field, which also needs to be initialized with zero, so I'm making sure
> vhost_net will also initialize it.
>
> Regards,
> Akihiko Odaki
>
> >
> > Others look fine.
> >
> > Thanks
> >
>
On 2025/03/11 9:42, Jason Wang wrote:
> On Mon, Mar 10, 2025 at 3:04 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2025/03/10 13:43, Jason Wang wrote:
>>> On Fri, Mar 7, 2025 at 7:02 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>
>>>> VIRTIO_NET_F_HASH_REPORT allows to report hash values calculated on the
>>>> host. When VHOST_NET_F_VIRTIO_NET_HDR is employed, it will report no
>>>> hash values (i.e., the hash_report member is always set to
>>>> VIRTIO_NET_HASH_REPORT_NONE). Otherwise, the values reported by the
>>>> underlying socket will be reported.
>>>>
>>>> VIRTIO_NET_F_HASH_REPORT requires VIRTIO_F_VERSION_1.
>>>>
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>> Tested-by: Lei Yang <leiyang@redhat.com>
>>>> ---
>>>> drivers/vhost/net.c | 49 +++++++++++++++++++++++++++++--------------------
>>>> 1 file changed, 29 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>>>> index b9b9e9d40951856d881d77ac74331d914473cd56..16b241b44f89820a42c302f3586ea6bb5e0d4289 100644
>>>> --- a/drivers/vhost/net.c
>>>> +++ b/drivers/vhost/net.c
>>>> @@ -73,6 +73,7 @@ enum {
>>>> VHOST_NET_FEATURES = VHOST_FEATURES |
>>>> (1ULL << VHOST_NET_F_VIRTIO_NET_HDR) |
>>>> (1ULL << VIRTIO_NET_F_MRG_RXBUF) |
>>>> + (1ULL << VIRTIO_NET_F_HASH_REPORT) |
>>>> (1ULL << VIRTIO_F_ACCESS_PLATFORM) |
>>>> (1ULL << VIRTIO_F_RING_RESET)
>>>> };
>>>> @@ -1097,9 +1098,11 @@ static void handle_rx(struct vhost_net *net)
>>>> .msg_controllen = 0,
>>>> .msg_flags = MSG_DONTWAIT,
>>>> };
>>>> - struct virtio_net_hdr hdr = {
>>>> - .flags = 0,
>>>> - .gso_type = VIRTIO_NET_HDR_GSO_NONE
>>>> + struct virtio_net_hdr_v1_hash hdr = {
>>>> + .hdr = {
>>>> + .flags = 0,
>>>> + .gso_type = VIRTIO_NET_HDR_GSO_NONE
>>>> + }
>>>> };
>>>> size_t total_len = 0;
>>>> int err, mergeable;
>>>> @@ -1110,7 +1113,6 @@ static void handle_rx(struct vhost_net *net)
>>>> bool set_num_buffers;
>>>> struct socket *sock;
>>>> struct iov_iter fixup;
>>>> - __virtio16 num_buffers;
>>>> int recv_pkts = 0;
>>>>
>>>> mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_RX);
>>>> @@ -1191,30 +1193,30 @@ static void handle_rx(struct vhost_net *net)
>>>> vhost_discard_vq_desc(vq, headcount);
>>>> continue;
>>>> }
>>>> + hdr.hdr.num_buffers = cpu_to_vhost16(vq, headcount);
>>>> /* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
>>>> if (unlikely(vhost_hlen)) {
>>>> - if (copy_to_iter(&hdr, sizeof(hdr),
>>>> - &fixup) != sizeof(hdr)) {
>>>> + if (copy_to_iter(&hdr, vhost_hlen,
>>>> + &fixup) != vhost_hlen) {
>>>> vq_err(vq, "Unable to write vnet_hdr "
>>>> "at addr %p\n", vq->iov->iov_base);
>>>> goto out;
>>>
>>> Is this an "issue" specific to RSS/HASH? If it's not, we need a separate patch.
>>>
>>> Honestly, I'm not sure if it's too late to fix this.
>>
>> There is nothing wrong with the current implementation.
>
> Note that I meant the vhost_hlen part, and the current code is tricky.
>
> The comment said:
>
> """
> /* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
> """
>
> So it tries to only offer virtio_net_hdr even if vhost_hlen is the set
> to mrg_rxbuf len.
>
> And this patch changes this behaviour.
mrg_rxbuf only adds the num_buffers field, which is always set for
mrg_rxbuf.
The num_buffers was not set for VIRTIO_F_VERSION_1 in the past, but this
was also fixed with commit a3b9c053d82a ("vhost/net: Set num_buffers for
virtio 1.0")
So there is no behavioral change for existing features with this patch.
Regards,
Akihiko Odaki
>
> Thanks
>
>> The current
>> implementation fills the header with zero except num_buffers, which it
>> fills some real value. This functionality is working fine with
>> VIRTIO_NET_F_MRG_RXBUF and VIRTIO_F_VERSION_1, which change the header size.
>>
>> Now I'm adding VIRTIO_NET_F_HASH_REPORT and it adds the hash_report
>> field, which also needs to be initialized with zero, so I'm making sure
>> vhost_net will also initialize it.
>>
>> Regards,
>> Akihiko Odaki
>>
>>>
>>> Others look fine.
>>>
>>> Thanks
>>>
>>
>
On Tue, Mar 11, 2025 at 2:24 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2025/03/11 9:42, Jason Wang wrote:
> > On Mon, Mar 10, 2025 at 3:04 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> On 2025/03/10 13:43, Jason Wang wrote:
> >>> On Fri, Mar 7, 2025 at 7:02 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>>>
> >>>> VIRTIO_NET_F_HASH_REPORT allows to report hash values calculated on the
> >>>> host. When VHOST_NET_F_VIRTIO_NET_HDR is employed, it will report no
> >>>> hash values (i.e., the hash_report member is always set to
> >>>> VIRTIO_NET_HASH_REPORT_NONE). Otherwise, the values reported by the
> >>>> underlying socket will be reported.
> >>>>
> >>>> VIRTIO_NET_F_HASH_REPORT requires VIRTIO_F_VERSION_1.
> >>>>
> >>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >>>> Tested-by: Lei Yang <leiyang@redhat.com>
> >>>> ---
> >>>> drivers/vhost/net.c | 49 +++++++++++++++++++++++++++++--------------------
> >>>> 1 file changed, 29 insertions(+), 20 deletions(-)
> >>>>
> >>>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> >>>> index b9b9e9d40951856d881d77ac74331d914473cd56..16b241b44f89820a42c302f3586ea6bb5e0d4289 100644
> >>>> --- a/drivers/vhost/net.c
> >>>> +++ b/drivers/vhost/net.c
> >>>> @@ -73,6 +73,7 @@ enum {
> >>>> VHOST_NET_FEATURES = VHOST_FEATURES |
> >>>> (1ULL << VHOST_NET_F_VIRTIO_NET_HDR) |
> >>>> (1ULL << VIRTIO_NET_F_MRG_RXBUF) |
> >>>> + (1ULL << VIRTIO_NET_F_HASH_REPORT) |
> >>>> (1ULL << VIRTIO_F_ACCESS_PLATFORM) |
> >>>> (1ULL << VIRTIO_F_RING_RESET)
> >>>> };
> >>>> @@ -1097,9 +1098,11 @@ static void handle_rx(struct vhost_net *net)
> >>>> .msg_controllen = 0,
> >>>> .msg_flags = MSG_DONTWAIT,
> >>>> };
> >>>> - struct virtio_net_hdr hdr = {
> >>>> - .flags = 0,
> >>>> - .gso_type = VIRTIO_NET_HDR_GSO_NONE
> >>>> + struct virtio_net_hdr_v1_hash hdr = {
> >>>> + .hdr = {
> >>>> + .flags = 0,
> >>>> + .gso_type = VIRTIO_NET_HDR_GSO_NONE
> >>>> + }
> >>>> };
> >>>> size_t total_len = 0;
> >>>> int err, mergeable;
> >>>> @@ -1110,7 +1113,6 @@ static void handle_rx(struct vhost_net *net)
> >>>> bool set_num_buffers;
> >>>> struct socket *sock;
> >>>> struct iov_iter fixup;
> >>>> - __virtio16 num_buffers;
> >>>> int recv_pkts = 0;
> >>>>
> >>>> mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_RX);
> >>>> @@ -1191,30 +1193,30 @@ static void handle_rx(struct vhost_net *net)
> >>>> vhost_discard_vq_desc(vq, headcount);
> >>>> continue;
> >>>> }
> >>>> + hdr.hdr.num_buffers = cpu_to_vhost16(vq, headcount);
> >>>> /* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
> >>>> if (unlikely(vhost_hlen)) {
> >>>> - if (copy_to_iter(&hdr, sizeof(hdr),
> >>>> - &fixup) != sizeof(hdr)) {
> >>>> + if (copy_to_iter(&hdr, vhost_hlen,
> >>>> + &fixup) != vhost_hlen) {
> >>>> vq_err(vq, "Unable to write vnet_hdr "
> >>>> "at addr %p\n", vq->iov->iov_base);
> >>>> goto out;
> >>>
> >>> Is this an "issue" specific to RSS/HASH? If it's not, we need a separate patch.
> >>>
> >>> Honestly, I'm not sure if it's too late to fix this.
> >>
> >> There is nothing wrong with the current implementation.
> >
> > Note that I meant the vhost_hlen part, and the current code is tricky.
> >
> > The comment said:
> >
> > """
> > /* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
> > """
> >
> > So it tries to only offer virtio_net_hdr even if vhost_hlen is the set
> > to mrg_rxbuf len.
> >
> > And this patch changes this behaviour.
>
> mrg_rxbuf only adds the num_buffers field, which is always set for
> mrg_rxbuf.
>
> The num_buffers was not set for VIRTIO_F_VERSION_1 in the past, but this
> was also fixed with commit a3b9c053d82a ("vhost/net: Set num_buffers for
> virtio 1.0")
>
> So there is no behavioral change for existing features with this patch.
I meant this part.
>>>> + if (copy_to_iter(&hdr, vhost_hlen,
>>>> + &fixup) != vhost_hlen) {
We should copy only sizeof(hdr) instead of vhost_hlen.
Anything I miss?
Thanks
>
> Regards,
> Akihiko Odaki
>
> >
> > Thanks
> >
> >> The current
> >> implementation fills the header with zero except num_buffers, which it
> >> fills some real value. This functionality is working fine with
> >> VIRTIO_NET_F_MRG_RXBUF and VIRTIO_F_VERSION_1, which change the header size.
> >>
> >> Now I'm adding VIRTIO_NET_F_HASH_REPORT and it adds the hash_report
> >> field, which also needs to be initialized with zero, so I'm making sure
> >> vhost_net will also initialize it.
> >>
> >> Regards,
> >> Akihiko Odaki
> >>
> >>>
> >>> Others look fine.
> >>>
> >>> Thanks
> >>>
> >>
> >
>
On 2025/03/12 12:36, Jason Wang wrote:
> On Tue, Mar 11, 2025 at 2:24 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2025/03/11 9:42, Jason Wang wrote:
>>> On Mon, Mar 10, 2025 at 3:04 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>
>>>> On 2025/03/10 13:43, Jason Wang wrote:
>>>>> On Fri, Mar 7, 2025 at 7:02 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>>>
>>>>>> VIRTIO_NET_F_HASH_REPORT allows to report hash values calculated on the
>>>>>> host. When VHOST_NET_F_VIRTIO_NET_HDR is employed, it will report no
>>>>>> hash values (i.e., the hash_report member is always set to
>>>>>> VIRTIO_NET_HASH_REPORT_NONE). Otherwise, the values reported by the
>>>>>> underlying socket will be reported.
>>>>>>
>>>>>> VIRTIO_NET_F_HASH_REPORT requires VIRTIO_F_VERSION_1.
>>>>>>
>>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>>>> Tested-by: Lei Yang <leiyang@redhat.com>
>>>>>> ---
>>>>>> drivers/vhost/net.c | 49 +++++++++++++++++++++++++++++--------------------
>>>>>> 1 file changed, 29 insertions(+), 20 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>>>>>> index b9b9e9d40951856d881d77ac74331d914473cd56..16b241b44f89820a42c302f3586ea6bb5e0d4289 100644
>>>>>> --- a/drivers/vhost/net.c
>>>>>> +++ b/drivers/vhost/net.c
>>>>>> @@ -73,6 +73,7 @@ enum {
>>>>>> VHOST_NET_FEATURES = VHOST_FEATURES |
>>>>>> (1ULL << VHOST_NET_F_VIRTIO_NET_HDR) |
>>>>>> (1ULL << VIRTIO_NET_F_MRG_RXBUF) |
>>>>>> + (1ULL << VIRTIO_NET_F_HASH_REPORT) |
>>>>>> (1ULL << VIRTIO_F_ACCESS_PLATFORM) |
>>>>>> (1ULL << VIRTIO_F_RING_RESET)
>>>>>> };
>>>>>> @@ -1097,9 +1098,11 @@ static void handle_rx(struct vhost_net *net)
>>>>>> .msg_controllen = 0,
>>>>>> .msg_flags = MSG_DONTWAIT,
>>>>>> };
>>>>>> - struct virtio_net_hdr hdr = {
>>>>>> - .flags = 0,
>>>>>> - .gso_type = VIRTIO_NET_HDR_GSO_NONE
>>>>>> + struct virtio_net_hdr_v1_hash hdr = {
>>>>>> + .hdr = {
>>>>>> + .flags = 0,
>>>>>> + .gso_type = VIRTIO_NET_HDR_GSO_NONE
>>>>>> + }
>>>>>> };
>>>>>> size_t total_len = 0;
>>>>>> int err, mergeable;
>>>>>> @@ -1110,7 +1113,6 @@ static void handle_rx(struct vhost_net *net)
>>>>>> bool set_num_buffers;
>>>>>> struct socket *sock;
>>>>>> struct iov_iter fixup;
>>>>>> - __virtio16 num_buffers;
>>>>>> int recv_pkts = 0;
>>>>>>
>>>>>> mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_RX);
>>>>>> @@ -1191,30 +1193,30 @@ static void handle_rx(struct vhost_net *net)
>>>>>> vhost_discard_vq_desc(vq, headcount);
>>>>>> continue;
>>>>>> }
>>>>>> + hdr.hdr.num_buffers = cpu_to_vhost16(vq, headcount);
>>>>>> /* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
>>>>>> if (unlikely(vhost_hlen)) {
>>>>>> - if (copy_to_iter(&hdr, sizeof(hdr),
>>>>>> - &fixup) != sizeof(hdr)) {
>>>>>> + if (copy_to_iter(&hdr, vhost_hlen,
>>>>>> + &fixup) != vhost_hlen) {
>>>>>> vq_err(vq, "Unable to write vnet_hdr "
>>>>>> "at addr %p\n", vq->iov->iov_base);
>>>>>> goto out;
>>>>>
>>>>> Is this an "issue" specific to RSS/HASH? If it's not, we need a separate patch.
>>>>>
>>>>> Honestly, I'm not sure if it's too late to fix this.
>>>>
>>>> There is nothing wrong with the current implementation.
>>>
>>> Note that I meant the vhost_hlen part, and the current code is tricky.
>>>
>>> The comment said:
>>>
>>> """
>>> /* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
>>> """
>>>
>>> So it tries to only offer virtio_net_hdr even if vhost_hlen is the set
>>> to mrg_rxbuf len.
>>>
>>> And this patch changes this behaviour.
>>
>> mrg_rxbuf only adds the num_buffers field, which is always set for
>> mrg_rxbuf.
>>
>> The num_buffers was not set for VIRTIO_F_VERSION_1 in the past, but this
>> was also fixed with commit a3b9c053d82a ("vhost/net: Set num_buffers for
>> virtio 1.0")
>>
>> So there is no behavioral change for existing features with this patch.
>
> I meant this part.
>
>>>>> + if (copy_to_iter(&hdr, vhost_hlen,
>>>>> + &fixup) != vhost_hlen) {
>
> We should copy only sizeof(hdr) instead of vhost_hlen.> > Anything I miss?
sizeof(hdr) will be greater than vhost_hlen when neither
VIRTIO_NET_F_MRG_RXBUF or VIRTIO_F_VERSION_1 is negotiated.
Regards,
Akihiko Odaki
>
> Thanks
>
>>
>> Regards,
>> Akihiko Odaki
>>
>>>
>>> Thanks
>>>
>>>> The current
>>>> implementation fills the header with zero except num_buffers, which it
>>>> fills some real value. This functionality is working fine with
>>>> VIRTIO_NET_F_MRG_RXBUF and VIRTIO_F_VERSION_1, which change the header size.
>>>>
>>>> Now I'm adding VIRTIO_NET_F_HASH_REPORT and it adds the hash_report
>>>> field, which also needs to be initialized with zero, so I'm making sure
>>>> vhost_net will also initialize it.
>>>>
>>>> Regards,
>>>> Akihiko Odaki
>>>>
>>>>>
>>>>> Others look fine.
>>>>>
>>>>> Thanks
>>>>>
>>>>
>>>
>>
>
On Wed, Mar 12, 2025 at 1:59 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2025/03/12 12:36, Jason Wang wrote:
> > On Tue, Mar 11, 2025 at 2:24 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> On 2025/03/11 9:42, Jason Wang wrote:
> >>> On Mon, Mar 10, 2025 at 3:04 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>>>
> >>>> On 2025/03/10 13:43, Jason Wang wrote:
> >>>>> On Fri, Mar 7, 2025 at 7:02 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>>>>>
> >>>>>> VIRTIO_NET_F_HASH_REPORT allows to report hash values calculated on the
> >>>>>> host. When VHOST_NET_F_VIRTIO_NET_HDR is employed, it will report no
> >>>>>> hash values (i.e., the hash_report member is always set to
> >>>>>> VIRTIO_NET_HASH_REPORT_NONE). Otherwise, the values reported by the
> >>>>>> underlying socket will be reported.
> >>>>>>
> >>>>>> VIRTIO_NET_F_HASH_REPORT requires VIRTIO_F_VERSION_1.
> >>>>>>
> >>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >>>>>> Tested-by: Lei Yang <leiyang@redhat.com>
> >>>>>> ---
> >>>>>> drivers/vhost/net.c | 49 +++++++++++++++++++++++++++++--------------------
> >>>>>> 1 file changed, 29 insertions(+), 20 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> >>>>>> index b9b9e9d40951856d881d77ac74331d914473cd56..16b241b44f89820a42c302f3586ea6bb5e0d4289 100644
> >>>>>> --- a/drivers/vhost/net.c
> >>>>>> +++ b/drivers/vhost/net.c
> >>>>>> @@ -73,6 +73,7 @@ enum {
> >>>>>> VHOST_NET_FEATURES = VHOST_FEATURES |
> >>>>>> (1ULL << VHOST_NET_F_VIRTIO_NET_HDR) |
> >>>>>> (1ULL << VIRTIO_NET_F_MRG_RXBUF) |
> >>>>>> + (1ULL << VIRTIO_NET_F_HASH_REPORT) |
> >>>>>> (1ULL << VIRTIO_F_ACCESS_PLATFORM) |
> >>>>>> (1ULL << VIRTIO_F_RING_RESET)
> >>>>>> };
> >>>>>> @@ -1097,9 +1098,11 @@ static void handle_rx(struct vhost_net *net)
> >>>>>> .msg_controllen = 0,
> >>>>>> .msg_flags = MSG_DONTWAIT,
> >>>>>> };
> >>>>>> - struct virtio_net_hdr hdr = {
> >>>>>> - .flags = 0,
> >>>>>> - .gso_type = VIRTIO_NET_HDR_GSO_NONE
> >>>>>> + struct virtio_net_hdr_v1_hash hdr = {
> >>>>>> + .hdr = {
> >>>>>> + .flags = 0,
> >>>>>> + .gso_type = VIRTIO_NET_HDR_GSO_NONE
> >>>>>> + }
> >>>>>> };
> >>>>>> size_t total_len = 0;
> >>>>>> int err, mergeable;
> >>>>>> @@ -1110,7 +1113,6 @@ static void handle_rx(struct vhost_net *net)
> >>>>>> bool set_num_buffers;
> >>>>>> struct socket *sock;
> >>>>>> struct iov_iter fixup;
> >>>>>> - __virtio16 num_buffers;
> >>>>>> int recv_pkts = 0;
> >>>>>>
> >>>>>> mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_RX);
> >>>>>> @@ -1191,30 +1193,30 @@ static void handle_rx(struct vhost_net *net)
> >>>>>> vhost_discard_vq_desc(vq, headcount);
> >>>>>> continue;
> >>>>>> }
> >>>>>> + hdr.hdr.num_buffers = cpu_to_vhost16(vq, headcount);
> >>>>>> /* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
> >>>>>> if (unlikely(vhost_hlen)) {
> >>>>>> - if (copy_to_iter(&hdr, sizeof(hdr),
> >>>>>> - &fixup) != sizeof(hdr)) {
> >>>>>> + if (copy_to_iter(&hdr, vhost_hlen,
> >>>>>> + &fixup) != vhost_hlen) {
> >>>>>> vq_err(vq, "Unable to write vnet_hdr "
> >>>>>> "at addr %p\n", vq->iov->iov_base);
> >>>>>> goto out;
> >>>>>
> >>>>> Is this an "issue" specific to RSS/HASH? If it's not, we need a separate patch.
> >>>>>
> >>>>> Honestly, I'm not sure if it's too late to fix this.
> >>>>
> >>>> There is nothing wrong with the current implementation.
> >>>
> >>> Note that I meant the vhost_hlen part, and the current code is tricky.
> >>>
> >>> The comment said:
> >>>
> >>> """
> >>> /* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
> >>> """
> >>>
> >>> So it tries to only offer virtio_net_hdr even if vhost_hlen is the set
> >>> to mrg_rxbuf len.
> >>>
> >>> And this patch changes this behaviour.
> >>
> >> mrg_rxbuf only adds the num_buffers field, which is always set for
> >> mrg_rxbuf.
> >>
> >> The num_buffers was not set for VIRTIO_F_VERSION_1 in the past, but this
> >> was also fixed with commit a3b9c053d82a ("vhost/net: Set num_buffers for
> >> virtio 1.0")
> >>
> >> So there is no behavioral change for existing features with this patch.
> >
> > I meant this part.
> >
> >>>>> + if (copy_to_iter(&hdr, vhost_hlen,
> >>>>> + &fixup) != vhost_hlen) {
> >
> > We should copy only sizeof(hdr) instead of vhost_hlen.> > Anything I miss?
>
> sizeof(hdr) will be greater than vhost_hlen when neither
> VIRTIO_NET_F_MRG_RXBUF or VIRTIO_F_VERSION_1 is negotiated.
Just to make sure we are on the same page I meant we should only
advance sizeof(struct virtio_net_hdr) here to make sure we can set the
num_buffers correctly.
But this is not what the code did here.
Thanks
>
> Regards,
> Akihiko Odaki
>
> >
> > Thanks
> >
> >>
> >> Regards,
> >> Akihiko Odaki
> >>
> >>>
> >>> Thanks
> >>>
> >>>> The current
> >>>> implementation fills the header with zero except num_buffers, which it
> >>>> fills some real value. This functionality is working fine with
> >>>> VIRTIO_NET_F_MRG_RXBUF and VIRTIO_F_VERSION_1, which change the header size.
> >>>>
> >>>> Now I'm adding VIRTIO_NET_F_HASH_REPORT and it adds the hash_report
> >>>> field, which also needs to be initialized with zero, so I'm making sure
> >>>> vhost_net will also initialize it.
> >>>>
> >>>> Regards,
> >>>> Akihiko Odaki
> >>>>
> >>>>>
> >>>>> Others look fine.
> >>>>>
> >>>>> Thanks
> >>>>>
> >>>>
> >>>
> >>
> >
>
On 2025/03/17 10:15, Jason Wang wrote:
> On Wed, Mar 12, 2025 at 1:59 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2025/03/12 12:36, Jason Wang wrote:
>>> On Tue, Mar 11, 2025 at 2:24 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>
>>>> On 2025/03/11 9:42, Jason Wang wrote:
>>>>> On Mon, Mar 10, 2025 at 3:04 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>>>
>>>>>> On 2025/03/10 13:43, Jason Wang wrote:
>>>>>>> On Fri, Mar 7, 2025 at 7:02 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>>>>>
>>>>>>>> VIRTIO_NET_F_HASH_REPORT allows to report hash values calculated on the
>>>>>>>> host. When VHOST_NET_F_VIRTIO_NET_HDR is employed, it will report no
>>>>>>>> hash values (i.e., the hash_report member is always set to
>>>>>>>> VIRTIO_NET_HASH_REPORT_NONE). Otherwise, the values reported by the
>>>>>>>> underlying socket will be reported.
>>>>>>>>
>>>>>>>> VIRTIO_NET_F_HASH_REPORT requires VIRTIO_F_VERSION_1.
>>>>>>>>
>>>>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>>>>>> Tested-by: Lei Yang <leiyang@redhat.com>
>>>>>>>> ---
>>>>>>>> drivers/vhost/net.c | 49 +++++++++++++++++++++++++++++--------------------
>>>>>>>> 1 file changed, 29 insertions(+), 20 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>>>>>>>> index b9b9e9d40951856d881d77ac74331d914473cd56..16b241b44f89820a42c302f3586ea6bb5e0d4289 100644
>>>>>>>> --- a/drivers/vhost/net.c
>>>>>>>> +++ b/drivers/vhost/net.c
>>>>>>>> @@ -73,6 +73,7 @@ enum {
>>>>>>>> VHOST_NET_FEATURES = VHOST_FEATURES |
>>>>>>>> (1ULL << VHOST_NET_F_VIRTIO_NET_HDR) |
>>>>>>>> (1ULL << VIRTIO_NET_F_MRG_RXBUF) |
>>>>>>>> + (1ULL << VIRTIO_NET_F_HASH_REPORT) |
>>>>>>>> (1ULL << VIRTIO_F_ACCESS_PLATFORM) |
>>>>>>>> (1ULL << VIRTIO_F_RING_RESET)
>>>>>>>> };
>>>>>>>> @@ -1097,9 +1098,11 @@ static void handle_rx(struct vhost_net *net)
>>>>>>>> .msg_controllen = 0,
>>>>>>>> .msg_flags = MSG_DONTWAIT,
>>>>>>>> };
>>>>>>>> - struct virtio_net_hdr hdr = {
>>>>>>>> - .flags = 0,
>>>>>>>> - .gso_type = VIRTIO_NET_HDR_GSO_NONE
>>>>>>>> + struct virtio_net_hdr_v1_hash hdr = {
>>>>>>>> + .hdr = {
>>>>>>>> + .flags = 0,
>>>>>>>> + .gso_type = VIRTIO_NET_HDR_GSO_NONE
>>>>>>>> + }
>>>>>>>> };
>>>>>>>> size_t total_len = 0;
>>>>>>>> int err, mergeable;
>>>>>>>> @@ -1110,7 +1113,6 @@ static void handle_rx(struct vhost_net *net)
>>>>>>>> bool set_num_buffers;
>>>>>>>> struct socket *sock;
>>>>>>>> struct iov_iter fixup;
>>>>>>>> - __virtio16 num_buffers;
>>>>>>>> int recv_pkts = 0;
>>>>>>>>
>>>>>>>> mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_RX);
>>>>>>>> @@ -1191,30 +1193,30 @@ static void handle_rx(struct vhost_net *net)
>>>>>>>> vhost_discard_vq_desc(vq, headcount);
>>>>>>>> continue;
>>>>>>>> }
>>>>>>>> + hdr.hdr.num_buffers = cpu_to_vhost16(vq, headcount);
>>>>>>>> /* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
>>>>>>>> if (unlikely(vhost_hlen)) {
>>>>>>>> - if (copy_to_iter(&hdr, sizeof(hdr),
>>>>>>>> - &fixup) != sizeof(hdr)) {
>>>>>>>> + if (copy_to_iter(&hdr, vhost_hlen,
>>>>>>>> + &fixup) != vhost_hlen) {
>>>>>>>> vq_err(vq, "Unable to write vnet_hdr "
>>>>>>>> "at addr %p\n", vq->iov->iov_base);
>>>>>>>> goto out;
>>>>>>>
>>>>>>> Is this an "issue" specific to RSS/HASH? If it's not, we need a separate patch.
>>>>>>>
>>>>>>> Honestly, I'm not sure if it's too late to fix this.
>>>>>>
>>>>>> There is nothing wrong with the current implementation.
>>>>>
>>>>> Note that I meant the vhost_hlen part, and the current code is tricky.
>>>>>
>>>>> The comment said:
>>>>>
>>>>> """
>>>>> /* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
>>>>> """
>>>>>
>>>>> So it tries to only offer virtio_net_hdr even if vhost_hlen is the set
>>>>> to mrg_rxbuf len.
>>>>>
>>>>> And this patch changes this behaviour.
>>>>
>>>> mrg_rxbuf only adds the num_buffers field, which is always set for
>>>> mrg_rxbuf.
>>>>
>>>> The num_buffers was not set for VIRTIO_F_VERSION_1 in the past, but this
>>>> was also fixed with commit a3b9c053d82a ("vhost/net: Set num_buffers for
>>>> virtio 1.0")
>>>>
>>>> So there is no behavioral change for existing features with this patch.
>>>
>>> I meant this part.
>>>
>>>>>>> + if (copy_to_iter(&hdr, vhost_hlen,
>>>>>>> + &fixup) != vhost_hlen) {
>>>
>>> We should copy only sizeof(hdr) instead of vhost_hlen.> > Anything I miss?
>>
>> sizeof(hdr) will be greater than vhost_hlen when neither
>> VIRTIO_NET_F_MRG_RXBUF or VIRTIO_F_VERSION_1 is negotiated.
>
> Just to make sure we are on the same page I meant we should only
> advance sizeof(struct virtio_net_hdr) here to make sure we can set the
> num_buffers correctly.
>
> But this is not what the code did here.
This code wrote num_buffers in hdr instead of setting it later.
However, with v10, I changed it again to fill the whole header with zero
and to set num_buffers later. The end result is identical either way;
num_buffers has the correct value and the other fields are filled with zero.
Regards,
Akihiko Odaki
>
> Thanks
>
>>
>> Regards,
>> Akihiko Odaki
>>
>>>
>>> Thanks
>>>
>>>>
>>>> Regards,
>>>> Akihiko Odaki
>>>>
>>>>>
>>>>> Thanks
>>>>>
>>>>>> The current
>>>>>> implementation fills the header with zero except num_buffers, which it
>>>>>> fills some real value. This functionality is working fine with
>>>>>> VIRTIO_NET_F_MRG_RXBUF and VIRTIO_F_VERSION_1, which change the header size.
>>>>>>
>>>>>> Now I'm adding VIRTIO_NET_F_HASH_REPORT and it adds the hash_report
>>>>>> field, which also needs to be initialized with zero, so I'm making sure
>>>>>> vhost_net will also initialize it.
>>>>>>
>>>>>> Regards,
>>>>>> Akihiko Odaki
>>>>>>
>>>>>>>
>>>>>>> Others look fine.
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
© 2016 - 2026 Red Hat, Inc.