Re: [virtio-dev] Re: [PATCH] virtio_net: support split header

[Cornelia Huck <cohuck@redhat.com>]

On Fri, Aug 05 2022, Heng Qi <hengqi@linux.alibaba.com> wrote:

> å 2022/8/4 äå9:50, Cornelia Huck åé:
>> On Thu, Aug 04 2022, Heng Qi<hengqi@linux.alibaba.com>  wrote:
>>
>>> å 2022/8/4 äå2:27, Jason Wang åé:
>>>> On Mon, Aug 1, 2022 at 2:59 PM Heng Qi<hengqi@linux.alibaba.com>   wrote:
>>>>> @@ -3820,9 +3826,13 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>>>>>    driver MUST NOT use the \field{csum_start} and \field{csum_offset}.
>>>>>
>>>>>    If one of the VIRTIO_NET_F_GUEST_TSO4, TSO6, UFO, USO4 or USO6 options have
>>>>> -been negotiated, the driver MAY use \field{hdr_len} only as a hint about the
>>>>> +been negotiated and the VIRTIO_NET_HDR_F_SPLIT_HEADER bit in \field{flags}
>>>>> +is not set, the driver MAY use \field{hdr_len} only as a hint about the
>>>>>    transport header size.
>>>>> -The driver MUST NOT rely on \field{hdr_len} to be correct.
>>>>> +
>>>>> +If the VIRTIO_NET_HDR_F_SPLIT_HEADER bit in \field{flags} is not set, the driver
>>>>> +MUST NOT rely on \field{hdr_len} to be correct.
>>>> I think we should keep the above description as-is. For whatever case,
>>>> the driver must not trust the metadata set by the device and must
>>>> perform necessary sanity tests on them.
>>>
>>> My idea is to keep the current description as it is,
>>> but to emphasize in the next version:
>>> "If the VIRTIO_NET_HDR_F_SPLIT_HEADER bit in \field{flags} is set,
>>> the driver MAY treat the \field{hdr_len} as the length of the
>>> protocol header inside the first descriptor."
>>
>> Just to be clear, you suggest using
>>
>> "If one of the VIRTIO_NET_F_GUEST_TSO4, TSO6, UFO, USO4 or USO6 options have
>> been negotiated, the driver MAY use \field{hdr_len} only as a hint about the
>> transport header size.
>>
>> The driver MUST NOT rely on \field{hdr_len} to be correct.
>>
>> If the VIRTIO_NET_HDR_F_SPLIT_HEADER bit in \field{flags} is set,
>> the driver MAY treat the \field{hdr_len} as the length of the
>> protocol header inside the first descriptor."
>
> Yes. I will use the above description to make it clearer in the next version.
>
>
>>
>> (Maybe "...the driver MAY use \field{hdr_len} as a hint about the length
>> of the protocol header..."? It's still not reliable, right?)
>>
> \field{hdr_len} is unreliable when VIRTIO_NET_F_SPLIT_HEADER is not negotiated.
>
>
> If VIRTIO_NET_F_SPLIT_HEADER is negotiated, "split header" MAY perform the split
> from the IP layer, so the protocol header and the transport header are different.
>
> so I think the "...the driver MAY use \field{hdr_len} only as a hint about the
> transport header size..." paragraph can be left as-is.

Ok, then let's keep it like that.