Re: [virtio-comment] virtio spec change proposal

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Oct 02, 2018 at 09:43:25AM -0400, Yuri Benditovich wrote:
> Hello all,
> 
> We'd like to propose an extension of VIRTIO specification 1.0
> Please let us know if there are some objections.

I like the proposal. Some comments below, hope this helps with
preparing the spec patch.

> In case there is no objections we will prepare and send respective patch.
> 
> Feature: RSC (receive side coalescing) compatibility with Windows HCK (hardware
> compatibility kit) requiremenets.
> 
> Description:
> 
> 1. Current state:
> VIRTIO device supports RSC, i.e. able to coalesce TCP/UDP v4 and v6 fragments
> and build complete packet from them.
> This feature is controlled by VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
> VIRTIO_NET_F_GUEST_UFO feature bits.
> When it is enabled and received packet was built from fragments, the device
> indicates that by setting field 'gso_type' field to one of
> VIRTIO_NET_HDR_GSO_TCPV4, VIRTIO_NET_HDR_GSO_TCPV6 or VIRTIO_NET_HDR_GSO_UDP.
> Functionally this is good enough, but such an implementation is not compatible
> with Windows certification requirements, resulting failures in Windows
> certification tests.
> 
> 2. Expectations of Windows HCK testing software.
> Rules for coalescing are described in
> https://docs.microsoft.com/en-us/windows-hardware/drivers/network/
> rules-for-coalescing-tcp-ip-packets
> Guidelines for indication of coalesced segments
> https://docs.microsoft.com/en-us/windows-hardware/drivers/network/
> indicating-coalesced-segments
> These guidelines are applicable to the network driver, but the network driver
> shall be able to extract
> the required information from the received packet. The information required for
> the driver defined in
> https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/content/ndis/
> ns-ndis-_ndis_rsc_nbl_info
> and includes following per-packet information:
> - number of coalesced segments that were combined in the packet
> - number of duplicated ACKs that were encountered while received packet was
> combined
> 
> Current implementation of RSC does not provide such information in the packet.
> 
> 3. Existing workaround
> There is a workaround in the field for the problem described above.
> It uses feature bits 41 and 42 for control of TCP v4 and TCP v6 RSC and
> provides required information in the virtio header.
> For that it extends existing structure of virtio header from regular 12 bytes
> to 16 bytes in case one or both of these feature bits are acknowledged.
> The disadvantage of this solution is that such an extension of packet header
> may complicate further extensions of the specification.

So do we need to reserve bits 41 and 42 for now, as well?
Can be a separate patch.

> 4. Proposal of specification change.
> 
> a) Taking in account the workaround (3) declare feature bits 41 and 42 as
> reserved.
> 
> b) Define feature bits 43 for extended RSC information in coalesced packets
> (VIRTIO_NET_F_GUEST_RSC)

Maybe VIRTIO_NET_F_GUEST_RSC_INFO? This does not control whether to do RSC.
Or maybe VIRTIO_NET_F_GUEST_RSC_EXTENDED? This allows coalescing
of duplicate acks and variable sized packets ...


> c) Define additional flag bit 2 (value = 4) that the device can set in 'flags'
> field of existing virtio header with meaning of VIRTIO_NET_HDR_F_RSC_INFO. The
> device sets this bit only in coalesced packets and only if feature bits 43 was
> acknowledged.

And not valid for driver, right?

> If VIRTIO_NET_HDR_F_RSC_INFO is set in the packet, the device:
> 
> c.1) populates 16-bit value of 'number of coalesced segments' in 'csum_start'
> field of virtio header
> 
> c.2) populates 16-bits value of 'number of duplicated ACKs' in 'csum_offset'
> field of virtio header

Maybe its a good time to document behaviour when the bit
is not set:  
- duplicate acks not coalesced (number = 0)
- only full MTU segments coalesced (number = round_up(len / mtu))


> d) The device should populate proper checksum in coalesced packets
> (VIRTIO_NET_HDR_F_DATA_VALID should be set). If due to some reason the checksum
> is not populated, VIRTIO_NET_HDR_F_DATA_VALID must be not set. In case
> VIRTIO_NET_HDR_F_DATA_VALID is not set and VIRTIO_NET_HDR_F_RSC_INFO is set in
> the packet, the driver shall compute packet checksum without rely on checksum
> information in 'csum_start' and 'csum_offset'.

More specifically, please formulate exactly what should device do.

> e) new feature VIRTIO_NET_F_GUEST_RSC depends on VIRTIO_NET_F_CTRL_VQ,
> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS and (VIRTIO_NET_F_GUEST_TSO4 or
> VIRTIO_NET_F_GUEST_TSO6).
> 
> Thanks,
> Yuri Benditovich,
> Red Hat, Inc.
>