Virtualization - Feb 2023 - [patch net-next] net: virtio_net: implement exact header length guest feature

Mon, Feb 20, 2023 at 01:55:33PM CET, mst at redhat.com
wrote:
>On Mon, Feb 20, 2023 at 09:35:00AM +0100, Jiri Pirko wrote:
>> Fri, Feb 17, 2023 at 02:47:36PM CET, mst at redhat.com wrote:
>> >On Fri, Feb 17, 2023 at 01:53:55PM +0100, Jiri Pirko wrote:
>> >> Fri, Feb 17, 2023 at 01:22:01PM CET, mst at redhat.com wrote:
>> >> >On Fri, Feb 17, 2023 at 01:15:47PM +0100, Jiri Pirko
wrote:
>> >> >> From: Jiri Pirko <jiri at nvidia.com>
>> >> >> 
>> >> >> virtio_net_hdr_from_skb() fills up hdr_len to
skb_headlen(skb).
>> >> >> 
>> >> >> Virtio spec introduced a feature
VIRTIO_NET_F_GUEST_HDRLEN which when
>> >> >> set implicates that the driver provides the exact
size of the header.
>> >> >> 
>> >> >> The driver already complies to fill the correct
value. Introduce the
>> >> >> feature and advertise it.
>> >> >> 
>> >> >> Signed-off-by: Jiri Pirko <jiri at nvidia.com>
>> >> >
>> >> >Could you add a bit of motivation just for the record?
>> >> >Does this improve performance for some card? By how much?
>> >> >Expected to help some future card?
>> >> 
>> >> I can get that info, but isn't that rather something to be
appended to
>> >> the virtio-spec patch? I mean, the feature is there, this is
just
>> >> implementing it in one driver.
>> >
>> >It is more like using it in the driver.  It's not like we have
to use
>> >everything - it could be useful for e.g. dpdk but not linux.
>> >Implementing it in the Linux driver has support costs - for example
what
>> >if there's a bug and sometimes the length is incorrect?
>> >We'll be breaking things.
>> 
>> I understand. To my understanding this feature just fixes the original
>> ambiguity in the virtio spec.
>> 
>> Quoting the original virtio spec:
>> "hdr_len is a hint to the device as to how much of the header
needs to
>>  be kept to copy into each packet"
>> 
>> "a hint" might not be clear for the reader what does it mean,
if it is
>> "maybe like that" of "exactly like that". This
feature just makes it
>> crystal clear.
>> 
>> If you look at the tap implementation, it uses hdr_len to alloc
>> skb linear part. No hint, it counts with the provided value.
>> So if the driver is currently not precise, it breaks tap.
>
>Well that's only for gso though right?
Yep.

>And making it bigger than necessary works fine ...
Well yeah. But tap does not do that, does it? it uses hdr_len directly.

>
>> I will add this to the patch description and send v2.
>> 
>
>I feel this does not answer the question yet, or maybe I am being dense.
>My point was not about making hdr_len precise.  My point was that we are
>making a change here for no apparent reason. I am guessing you are not
>doing it for fun - so why? Is there a device with this feature bit
>you are aware of?
Afaik real hw which does emulation of virtio_net would benefit from
that, our hw including.

>
>
>
>> 
>> >
>> >The patch was submitted by Marvell but they never bothered with
>> >using it in Linux. I guess they are using it for something else?
>> >CC Vitaly who put this in.
>> >
>> >> 
>> >> >
>> >> >thanks!
>> >> >
>> >> >
>> >> >> ---
>> >> >>  drivers/net/virtio_net.c        | 6 ++++--
>> >> >>  include/uapi/linux/virtio_net.h | 1 +
>> >> >>  2 files changed, 5 insertions(+), 2 deletions(-)
>> >> >> 
>> >> >> diff --git a/drivers/net/virtio_net.c
b/drivers/net/virtio_net.c
>> >> >> index fb5e68ed3ec2..e85b03988733 100644
>> >> >> --- a/drivers/net/virtio_net.c
>> >> >> +++ b/drivers/net/virtio_net.c
>> >> >> @@ -62,7 +62,8 @@ static const unsigned long
guest_offloads[] = {
>> >> >>  	VIRTIO_NET_F_GUEST_UFO,
>> >> >>  	VIRTIO_NET_F_GUEST_CSUM,
>> >> >>  	VIRTIO_NET_F_GUEST_USO4,
>> >> >> -	VIRTIO_NET_F_GUEST_USO6
>> >> >> +	VIRTIO_NET_F_GUEST_USO6,
>> >> >> +	VIRTIO_NET_F_GUEST_HDRLEN
>> >> >>  };
>> >> >>  
>> >> >>  #define GUEST_OFFLOAD_GRO_HW_MASK ((1ULL <<
VIRTIO_NET_F_GUEST_TSO4) | \
>> >> >> @@ -4213,7 +4214,8 @@ static struct virtio_device_id
id_table[] = {
>> >> >>  	VIRTIO_NET_F_CTRL_MAC_ADDR, \
>> >> >>  	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS,
\
>> >> >>  	VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
>> >> >> -	VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT,
VIRTIO_NET_F_NOTF_COAL
>> >> >> +	VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT,
VIRTIO_NET_F_NOTF_COAL, \
>> >> >> +	VIRTIO_NET_F_GUEST_HDRLEN
>> >> >>  
>> >> >>  static unsigned int features[] = {
>> >> >>  	VIRTNET_FEATURES,
>> >> >> diff --git a/include/uapi/linux/virtio_net.h
b/include/uapi/linux/virtio_net.h
>> >> >> index b4062bed186a..12c1c9699935 100644
>> >> >> --- a/include/uapi/linux/virtio_net.h
>> >> >> +++ b/include/uapi/linux/virtio_net.h
>> >> >> @@ -61,6 +61,7 @@
>> >> >>  #define VIRTIO_NET_F_GUEST_USO6	55	/* Guest can
handle USOv6 in. */
>> >> >>  #define VIRTIO_NET_F_HOST_USO	56	/* Host can handle
USO in. */
>> >> >>  #define VIRTIO_NET_F_HASH_REPORT  57	/* Supports
hash report */
>> >> >> +#define VIRTIO_NET_F_GUEST_HDRLEN  59	/* Guest
provides the exact hdr_len value. */
>> >> >>  #define VIRTIO_NET_F_RSS	  60	/* Supports RSS RX
steering */
>> >> >>  #define VIRTIO_NET_F_RSC_EXT	  61	/* extended
coalescing info */
>> >> >>  #define VIRTIO_NET_F_STANDBY	  62	/* Act as standby
for another device
>> >> >> -- 
>> >> >> 2.39.0
>> >> >
>> >
>

On Mon, Feb 20, 2023 at 02:56:08PM +0100, Jiri Pirko
wrote:
> Mon, Feb 20, 2023 at 01:55:33PM CET, mst at redhat.com wrote:
> >On Mon, Feb 20, 2023 at 09:35:00AM +0100, Jiri Pirko wrote:
> >> Fri, Feb 17, 2023 at 02:47:36PM CET, mst at redhat.com wrote:
> >> >On Fri, Feb 17, 2023 at 01:53:55PM +0100, Jiri Pirko wrote:
> >> >> Fri, Feb 17, 2023 at 01:22:01PM CET, mst at redhat.com
wrote:
> >> >> >On Fri, Feb 17, 2023 at 01:15:47PM +0100, Jiri Pirko
wrote:
> >> >> >> From: Jiri Pirko <jiri at nvidia.com>
> >> >> >> 
> >> >> >> virtio_net_hdr_from_skb() fills up hdr_len to
skb_headlen(skb).
> >> >> >> 
> >> >> >> Virtio spec introduced a feature
VIRTIO_NET_F_GUEST_HDRLEN which when
> >> >> >> set implicates that the driver provides the
exact size of the header.
> >> >> >> 
> >> >> >> The driver already complies to fill the correct
value. Introduce the
> >> >> >> feature and advertise it.
> >> >> >> 
> >> >> >> Signed-off-by: Jiri Pirko <jiri at
nvidia.com>
> >> >> >
> >> >> >Could you add a bit of motivation just for the
record?
> >> >> >Does this improve performance for some card? By how
much?
> >> >> >Expected to help some future card?
> >> >> 
> >> >> I can get that info, but isn't that rather something
to be appended to
> >> >> the virtio-spec patch? I mean, the feature is there, this
is just
> >> >> implementing it in one driver.
> >> >
> >> >It is more like using it in the driver.  It's not like we
have to use
> >> >everything - it could be useful for e.g. dpdk but not linux.
> >> >Implementing it in the Linux driver has support costs - for
example what
> >> >if there's a bug and sometimes the length is incorrect?
> >> >We'll be breaking things.
> >> 
> >> I understand. To my understanding this feature just fixes the
original
> >> ambiguity in the virtio spec.
> >> 
> >> Quoting the original virtio spec:
> >> "hdr_len is a hint to the device as to how much of the header
needs to
> >>  be kept to copy into each packet"
> >> 
> >> "a hint" might not be clear for the reader what does it
mean, if it is
> >> "maybe like that" of "exactly like that". This
feature just makes it
> >> crystal clear.
> >> 
> >> If you look at the tap implementation, it uses hdr_len to alloc
> >> skb linear part. No hint, it counts with the provided value.
> >> So if the driver is currently not precise, it breaks tap.
> >
> >Well that's only for gso though right?
> 
> Yep.
> 
> 
> >And making it bigger than necessary works fine ...
> 
> Well yeah. But tap does not do that, does it? it uses hdr_len directly.
> 
I mean if hdr_len is bigger than necessary tap does work.

> >
> >> I will add this to the patch description and send v2.
> >> 
> >
> >I feel this does not answer the question yet, or maybe I am being
dense.
> >My point was not about making hdr_len precise.  My point was that we
are
> >making a change here for no apparent reason. I am guessing you are not
> >doing it for fun - so why? Is there a device with this feature bit
> >you are aware of?
> 
> Afaik real hw which does emulation of virtio_net would benefit from
> that, our hw including.
OK so do you have hardware which exposes this feature?
That is the bit I am missing. Maybe mention the make
in the commit log so
we know where to turn if we need to make changes here?
Or "under development" if it is not on the market yet.
> 
> >
> >
> >
> >> 
> >> >
> >> >The patch was submitted by Marvell but they never bothered
with
> >> >using it in Linux. I guess they are using it for something
else?
> >> >CC Vitaly who put this in.
> >> >
> >> >> 
> >> >> >
> >> >> >thanks!
> >> >> >
> >> >> >
> >> >> >> ---
> >> >> >>  drivers/net/virtio_net.c        | 6 ++++--
> >> >> >>  include/uapi/linux/virtio_net.h | 1 +
> >> >> >>  2 files changed, 5 insertions(+), 2
deletions(-)
> >> >> >> 
> >> >> >> diff --git a/drivers/net/virtio_net.c
b/drivers/net/virtio_net.c
> >> >> >> index fb5e68ed3ec2..e85b03988733 100644
> >> >> >> --- a/drivers/net/virtio_net.c
> >> >> >> +++ b/drivers/net/virtio_net.c
> >> >> >> @@ -62,7 +62,8 @@ static const unsigned long
guest_offloads[] = {
> >> >> >>  	VIRTIO_NET_F_GUEST_UFO,
> >> >> >>  	VIRTIO_NET_F_GUEST_CSUM,
> >> >> >>  	VIRTIO_NET_F_GUEST_USO4,
> >> >> >> -	VIRTIO_NET_F_GUEST_USO6
> >> >> >> +	VIRTIO_NET_F_GUEST_USO6,
> >> >> >> +	VIRTIO_NET_F_GUEST_HDRLEN
> >> >> >>  };
> >> >> >>  
> >> >> >>  #define GUEST_OFFLOAD_GRO_HW_MASK ((1ULL
<< VIRTIO_NET_F_GUEST_TSO4) | \
> >> >> >> @@ -4213,7 +4214,8 @@ static struct
virtio_device_id id_table[] = {
> >> >> >>  	VIRTIO_NET_F_CTRL_MAC_ADDR, \
> >> >> >>  	VIRTIO_NET_F_MTU,
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
> >> >> >>  	VIRTIO_NET_F_SPEED_DUPLEX,
VIRTIO_NET_F_STANDBY, \
> >> >> >> -	VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT,
VIRTIO_NET_F_NOTF_COAL
> >> >> >> +	VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT,
VIRTIO_NET_F_NOTF_COAL, \
> >> >> >> +	VIRTIO_NET_F_GUEST_HDRLEN
> >> >> >>  
> >> >> >>  static unsigned int features[] = {
> >> >> >>  	VIRTNET_FEATURES,
> >> >> >> diff --git a/include/uapi/linux/virtio_net.h
b/include/uapi/linux/virtio_net.h
> >> >> >> index b4062bed186a..12c1c9699935 100644
> >> >> >> --- a/include/uapi/linux/virtio_net.h
> >> >> >> +++ b/include/uapi/linux/virtio_net.h
> >> >> >> @@ -61,6 +61,7 @@
> >> >> >>  #define VIRTIO_NET_F_GUEST_USO6	55	/* Guest can
handle USOv6 in. */
> >> >> >>  #define VIRTIO_NET_F_HOST_USO	56	/* Host can
handle USO in. */
> >> >> >>  #define VIRTIO_NET_F_HASH_REPORT  57	/*
Supports hash report */
> >> >> >> +#define VIRTIO_NET_F_GUEST_HDRLEN  59	/* Guest
provides the exact hdr_len value. */
> >> >> >>  #define VIRTIO_NET_F_RSS	  60	/* Supports RSS
RX steering */
> >> >> >>  #define VIRTIO_NET_F_RSC_EXT	  61	/* extended
coalescing info */
> >> >> >>  #define VIRTIO_NET_F_STANDBY	  62	/* Act as
standby for another device
> >> >> >> -- 
> >> >> >> 2.39.0
> >> >> >
> >> >
> >

? 2023/2/20 21:56, Jiri Pirko ??:
> Mon, Feb 20, 2023 at 01:55:33PM CET, mst at redhat.com wrote:
>> On Mon, Feb 20, 2023 at 09:35:00AM +0100, Jiri Pirko wrote:
>>> Fri, Feb 17, 2023 at 02:47:36PM CET, mst at redhat.com wrote:
>>>> On Fri, Feb 17, 2023 at 01:53:55PM +0100, Jiri Pirko wrote:
>>>>> Fri, Feb 17, 2023 at 01:22:01PM CET, mst at redhat.com
wrote:
>>>>>> On Fri, Feb 17, 2023 at 01:15:47PM +0100, Jiri Pirko
wrote:
>>>>>>> From: Jiri Pirko <jiri at nvidia.com>
>>>>>>>
>>>>>>> virtio_net_hdr_from_skb() fills up hdr_len to
skb_headlen(skb).
>>>>>>>
>>>>>>> Virtio spec introduced a feature
VIRTIO_NET_F_GUEST_HDRLEN which when
>>>>>>> set implicates that the driver provides the exact
size of the header.
>>>>>>>
>>>>>>> The driver already complies to fill the correct
value. Introduce the
>>>>>>> feature and advertise it.
>>>>>>>
>>>>>>> Signed-off-by: Jiri Pirko <jiri at
nvidia.com>
>>>>>> Could you add a bit of motivation just for the record?
>>>>>> Does this improve performance for some card? By how
much?
>>>>>> Expected to help some future card?
>>>>> I can get that info, but isn't that rather something to
be appended to
>>>>> the virtio-spec patch? I mean, the feature is there, this
is just
>>>>> implementing it in one driver.
>>>> It is more like using it in the driver.  It's not like we
have to use
>>>> everything - it could be useful for e.g. dpdk but not linux.
>>>> Implementing it in the Linux driver has support costs - for
example what
>>>> if there's a bug and sometimes the length is incorrect?
>>>> We'll be breaking things.
>>> I understand. To my understanding this feature just fixes the
original
>>> ambiguity in the virtio spec.
>>>
>>> Quoting the original virtio spec:
>>> "hdr_len is a hint to the device as to how much of the header
needs to
>>>   be kept to copy into each packet"
>>>
>>> "a hint" might not be clear for the reader what does it
mean, if it is
>>> "maybe like that" of "exactly like that". This
feature just makes it
>>> crystal clear.
>>>
>>> If you look at the tap implementation, it uses hdr_len to alloc
>>> skb linear part. No hint, it counts with the provided value.
>>> So if the driver is currently not precise, it breaks tap.
>> Well that's only for gso though right?
> Yep.
>
>
>> And making it bigger than necessary works fine ...
> Well yeah. But tap does not do that, does it? it uses hdr_len directly.

tap_get_user() limit the head length:


static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
 ??????????????????????????? struct iov_iter *from, int noblock)
{
 ?? ?int good_linear = SKB_MAX_HEAD(TAP_RESERVE);
...

>
>
>>> I will add this to the patch description and send v2.
>>>
>> I feel this does not answer the question yet, or maybe I am being
dense.
>> My point was not about making hdr_len precise.  My point was that we
are
>> making a change here for no apparent reason. I am guessing you are not
>> doing it for fun - so why? Is there a device with this feature bit
>> you are aware of?
> Afaik real hw which does emulation of virtio_net would benefit from
> that, our hw including.

Note that driver can choose to no negotiate this feature, so malicious 
drivers can still try to use illegal value.

Thanks

>
>
>>
>>
>>>> The patch was submitted by Marvell but they never bothered with
>>>> using it in Linux. I guess they are using it for something
else?
>>>> CC Vitaly who put this in.
>>>>
>>>>>> thanks!
>>>>>>
>>>>>>
>>>>>>> ---
>>>>>>>   drivers/net/virtio_net.c        | 6 ++++--
>>>>>>>   include/uapi/linux/virtio_net.h | 1 +
>>>>>>>   2 files changed, 5 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/virtio_net.c
b/drivers/net/virtio_net.c
>>>>>>> index fb5e68ed3ec2..e85b03988733 100644
>>>>>>> --- a/drivers/net/virtio_net.c
>>>>>>> +++ b/drivers/net/virtio_net.c
>>>>>>> @@ -62,7 +62,8 @@ static const unsigned long
guest_offloads[] = {
>>>>>>>   	VIRTIO_NET_F_GUEST_UFO,
>>>>>>>   	VIRTIO_NET_F_GUEST_CSUM,
>>>>>>>   	VIRTIO_NET_F_GUEST_USO4,
>>>>>>> -	VIRTIO_NET_F_GUEST_USO6
>>>>>>> +	VIRTIO_NET_F_GUEST_USO6,
>>>>>>> +	VIRTIO_NET_F_GUEST_HDRLEN
>>>>>>>   };
>>>>>>>   
>>>>>>>   #define GUEST_OFFLOAD_GRO_HW_MASK ((1ULL <<
VIRTIO_NET_F_GUEST_TSO4) | \
>>>>>>> @@ -4213,7 +4214,8 @@ static struct
virtio_device_id id_table[] = {
>>>>>>>   	VIRTIO_NET_F_CTRL_MAC_ADDR, \
>>>>>>>   	VIRTIO_NET_F_MTU,
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>>>>>>>   	VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY,
\
>>>>>>> -	VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT,
VIRTIO_NET_F_NOTF_COAL
>>>>>>> +	VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT,
VIRTIO_NET_F_NOTF_COAL, \
>>>>>>> +	VIRTIO_NET_F_GUEST_HDRLEN
>>>>>>>   
>>>>>>>   static unsigned int features[] = {
>>>>>>>   	VIRTNET_FEATURES,
>>>>>>> diff --git a/include/uapi/linux/virtio_net.h
b/include/uapi/linux/virtio_net.h
>>>>>>> index b4062bed186a..12c1c9699935 100644
>>>>>>> --- a/include/uapi/linux/virtio_net.h
>>>>>>> +++ b/include/uapi/linux/virtio_net.h
>>>>>>> @@ -61,6 +61,7 @@
>>>>>>>   #define VIRTIO_NET_F_GUEST_USO6	55	/* Guest can
handle USOv6 in. */
>>>>>>>   #define VIRTIO_NET_F_HOST_USO	56	/* Host can
handle USO in. */
>>>>>>>   #define VIRTIO_NET_F_HASH_REPORT  57	/* Supports
hash report */
>>>>>>> +#define VIRTIO_NET_F_GUEST_HDRLEN  59	/* Guest
provides the exact hdr_len value. */
>>>>>>>   #define VIRTIO_NET_F_RSS	  60	/* Supports RSS RX
steering */
>>>>>>>   #define VIRTIO_NET_F_RSC_EXT	  61	/* extended
coalescing info */
>>>>>>>   #define VIRTIO_NET_F_STANDBY	  62	/* Act as
standby for another device
>>>>>>> -- 
>>>>>>> 2.39.0