Re: [PATCH 3/5] virtio: Don't expose legacy net features when VIRTIO_NET_NO_LEGACY defined.

public inbox for netdev@vger.kernel.org 
 help / color / mirror / Atom feed

From: Rusty Russell <rusty@rustcorp•com.au>
To: "Michael S. Tsirkin" <mst@redhat•com>
Cc: lkml <linux-kernel@vger•kernel.org>,
	"netdev\@vger.kernel.org" <netdev@vger•kernel.org>
Subject: Re: [PATCH 3/5] virtio: Don't expose legacy net features when VIRTIO_NET_NO_LEGACY defined.
Date: Mon, 16 Feb 2015 16:39:42 +1030	[thread overview]
Message-ID: <87egpqtmjd.fsf@rustcorp.com.au> (raw)
In-Reply-To: <20150216051435.GA14616@redhat.com>

"Michael S. Tsirkin" <mst@redhat•com> writes:
>>  	struct virtio_net_hdr hdr;
>>  	__virtio16 num_buffers;	/* Number of merged rx buffers */
>>  };
>> +#else /* ... VIRTIO_NET_NO_LEGACY */
>> +/*
>> + * This header comes first in the scatter-gather list.  If you don't
>> + * specify GSO or CSUM features, you can simply ignore the header.
>> + *
>> + * This is bitwise-equivalent to the legacy struct virtio_net_hdr_mrg_rxbuf.
>
> I would add "but with all fields squashed into the main structure".

Yep.

>> + */
>> +struct virtio_net_hdr_v1 {
>> +#define VIRTIO_NET_HDR_F_NEEDS_CSUM	1	/* Use csum_start, csum_offset */
>> +#define VIRTIO_NET_HDR_F_DATA_VALID	2	/* Csum is valid */
>> +	__u8 flags;
>> +#define VIRTIO_NET_HDR_GSO_NONE		0	/* Not a GSO frame */
>> +#define VIRTIO_NET_HDR_GSO_TCPV4	1	/* GSO frame, IPv4 TCP (TSO) */
>> +#define VIRTIO_NET_HDR_GSO_UDP		3	/* GSO frame, IPv4 UDP (UFO) */
>> +#define VIRTIO_NET_HDR_GSO_TCPV6	4	/* GSO frame, IPv6 TCP */
>> +#define VIRTIO_NET_HDR_GSO_ECN		0x80	/* TCP has ECN set */
>> +	__u8 gso_type;
>> +	__virtio16 hdr_len;	/* Ethernet + IP + tcp/udp hdrs */
>> +	__virtio16 gso_size;	/* Bytes to append to hdr_len per frame */
>> +	__virtio16 csum_start;	/* Position to start checksumming from */
>> +	__virtio16 csum_offset;	/* Offset after that to place checksum */
>> +	__virtio16 num_buffers;	/* Number of merged rx buffers */
>> +};
>> +#endif /* ...VIRTIO_NET_NO_LEGACY */
>
> I note that this way host code can't be structured like this:
>
>
> 	struct virtio_net_hdr_v1 modern;
> 	/* handle modern guests */
> 	....
>
> #ifndef VIRTIO_NET_NO_LEGACY
> 	struct virtio_net_hdr_mrg_rxbuf mrg;
> 	/* handle legacy guests */
> 	
> #endif
>
>
> Define virtio_net_hdr_v1 unconditionally?

Thanks, that's a good idea!  Here's the incremental:

virtio_net: unconditionally define struct virtio_net_hdr_v1.

This was introduced in commit ed9ecb0415b97b5f9f91f146e1977bb372c74c6d,
but only defined if !VIRTIO_NET_NO_LEGACY.  We should always define
it: easier for users to have conditional legacy code.

Suggested-by: "Michael S. Tsirkin" <mst@redhat•com>
Signed-off-by: Rusty Russell <rusty@rustcorp•com.au>

diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index 4a9b58113d6e..7bbee79ca293 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -74,39 +74,12 @@ struct virtio_net_config {
 	__u16 max_virtqueue_pairs;
 } __attribute__((packed));
 
-#ifndef VIRTIO_NET_NO_LEGACY
-/* This header comes first in the scatter-gather list.
- * For legacy virtio, if VIRTIO_F_ANY_LAYOUT is not negotiated, it must
- * be the first element of the scatter-gather list.  If you don't
- * specify GSO or CSUM features, you can simply ignore the header. */
-struct virtio_net_hdr {
-#define VIRTIO_NET_HDR_F_NEEDS_CSUM	1	// Use csum_start, csum_offset
-#define VIRTIO_NET_HDR_F_DATA_VALID	2	// Csum is valid
-	__u8 flags;
-#define VIRTIO_NET_HDR_GSO_NONE		0	// Not a GSO frame
-#define VIRTIO_NET_HDR_GSO_TCPV4	1	// GSO frame, IPv4 TCP (TSO)
-#define VIRTIO_NET_HDR_GSO_UDP		3	// GSO frame, IPv4 UDP (UFO)
-#define VIRTIO_NET_HDR_GSO_TCPV6	4	// GSO frame, IPv6 TCP
-#define VIRTIO_NET_HDR_GSO_ECN		0x80	// TCP has ECN set
-	__u8 gso_type;
-	__virtio16 hdr_len;		/* Ethernet + IP + tcp/udp hdrs */
-	__virtio16 gso_size;		/* Bytes to append to hdr_len per frame */
-	__virtio16 csum_start;	/* Position to start checksumming from */
-	__virtio16 csum_offset;	/* Offset after that to place checksum */
-};
-
-/* This is the version of the header to use when the MRG_RXBUF
- * feature has been negotiated. */
-struct virtio_net_hdr_mrg_rxbuf {
-	struct virtio_net_hdr hdr;
-	__virtio16 num_buffers;	/* Number of merged rx buffers */
-};
-#else /* ... VIRTIO_NET_NO_LEGACY */
 /*
  * This header comes first in the scatter-gather list.  If you don't
  * specify GSO or CSUM features, you can simply ignore the header.
  *
- * This is bitwise-equivalent to the legacy struct virtio_net_hdr_mrg_rxbuf.
+ * This is bitwise-equivalent to the legacy struct virtio_net_hdr_mrg_rxbuf,
+ * only flattened.
  */
 struct virtio_net_hdr_v1 {
 #define VIRTIO_NET_HDR_F_NEEDS_CSUM	1	/* Use csum_start, csum_offset */
@@ -124,6 +97,29 @@ struct virtio_net_hdr_v1 {
 	__virtio16 csum_offset;	/* Offset after that to place checksum */
 	__virtio16 num_buffers;	/* Number of merged rx buffers */
 };
+
+#ifndef VIRTIO_NET_NO_LEGACY
+/* This header comes first in the scatter-gather list.
+ * For legacy virtio, if VIRTIO_F_ANY_LAYOUT is not negotiated, it must
+ * be the first element of the scatter-gather list.  If you don't
+ * specify GSO or CSUM features, you can simply ignore the header. */
+struct virtio_net_hdr {
+	/* See VIRTIO_NET_HDR_F_* */
+	__u8 flags;
+	/* See VIRTIO_NET_HDR_GSO_* */
+	__u8 gso_type;
+	__virtio16 hdr_len;		/* Ethernet + IP + tcp/udp hdrs */
+	__virtio16 gso_size;		/* Bytes to append to hdr_len per frame */
+	__virtio16 csum_start;	/* Position to start checksumming from */
+	__virtio16 csum_offset;	/* Offset after that to place checksum */
+};
+
+/* This is the version of the header to use when the MRG_RXBUF
+ * feature has been negotiated. */
+struct virtio_net_hdr_mrg_rxbuf {
+	struct virtio_net_hdr hdr;
+	__virtio16 num_buffers;	/* Number of merged rx buffers */
+};
 #endif /* ...VIRTIO_NET_NO_LEGACY */
 
 /*

next prev parent reply	other threads:[~2015-02-16  6:09 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-02-06  5:06 [PATCH 0/5] virtio 1.0 cleanups and one fix Rusty Russell
2015-02-06  5:06 ` [PATCH 3/5] virtio: Don't expose legacy net features when VIRTIO_NET_NO_LEGACY defined Rusty Russell
2015-02-08 10:59   ` Michael S. Tsirkin
2015-02-08 23:50     ` Rusty Russell
2015-02-11  0:06       ` Rusty Russell
2015-02-16  5:14         ` Michael S. Tsirkin
2015-02-16  6:09           ` Rusty Russell [this message]
2015-02-14 20:43     ` Michael S. Tsirkin
2015-02-16  3:29       ` Rusty Russell
2015-02-08  9:52 ` [PATCH 0/5] virtio 1.0 cleanups and one fix Michael S. Tsirkin

find likely ancestor, descendant, or conflicting patches for this message:
( dfblob:4a9b58113d6 dfblob:7bbee79ca29 )
 OR (
bs:"Re: [PATCH 3/5] virtio: Don't expose legacy net features when VIRTIO_NET_NO_LEGACY defined." )
	(help)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87egpqtmjd.fsf@rustcorp.com.au \
    --to=rusty@rustcorp$(echo .)com.au \
    --cc=linux-kernel@vger$(echo .)kernel.org \
    --cc=mst@redhat$(echo .)com \
    --cc=netdev@vger$(echo .)kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

Be sure your reply has a Subject: header at the top and a blank line before the message body.

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox