[PATCH 0/5] virtio 1.0 cleanups and one fix.

[PATCH 0/5] virtio 1.0 cleanups and one fix.

[Rusty Russell]

Hi all,

	Some minor fixes for my virtio-next tree.  Michael, does
QEMU implement the (compuslory!) VIRTIO_PCI_CAP_PCI_CFG field?  I'm
guessing not, since it wasn't defined in the Linux header :(

Rusty Russell (5):
  virtio: define VIRTIO_PCI_CAP_PCI_CFG in header.
  virtio: Don't expose legacy block features when VIRTIO_BLK_NO_LEGACY
    defined.
  virtio: Don't expose legacy net features when VIRTIO_NET_NO_LEGACY
    defined.
  virtio: Don't expose legacy config features when
    VIRTIO_CONFIG_NO_LEGACY defined.
  virtio: don't require a config space on the console device.

 drivers/char/virtio_console.c      | 12 ++++++++----
 include/uapi/linux/virtio_blk.h    | 17 +++++++++++++----
 include/uapi/linux/virtio_config.h |  2 ++
 include/uapi/linux/virtio_net.h    | 30 ++++++++++++++++++++++++++++--
 include/uapi/linux/virtio_pci.h    |  4 +++-
 5 files changed, 54 insertions(+), 11 deletions(-)

-- 
2.1.0

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

[Rusty Russell]

In particular, the virtio header always has the u16 num_buffers field.
We define a new 'struct virtio_net_modern_hdr' for this (rather than
simply calling it 'struct virtio_net_hdr', to avoid nasty type errors
if some parts of a project define VIRTIO_NET_NO_LEGACY and some don't.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/uapi/linux/virtio_net.h | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index b5f1677b291c..32754f3000e8 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -35,7 +35,6 @@
 #define VIRTIO_NET_F_CSUM	0	/* Host handles pkts w/ partial csum */
 #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
 #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
-#define VIRTIO_NET_F_GSO	6	/* Host handles pkts w/ any GSO type */
 #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
 #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
 #define VIRTIO_NET_F_GUEST_ECN	9	/* Guest can handle TSO[6] w/ ECN in. */
@@ -56,6 +55,10 @@
 					 * Steering */
 #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
 
+#ifndef VIRTIO_NET_NO_LEGACY
+#define VIRTIO_NET_F_GSO	6	/* Host handles pkts w/ any GSO type */
+#endif /* VIRTIO_NET_NO_LEGACY */
+
 #define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
 #define VIRTIO_NET_S_ANNOUNCE	2	/* Announcement is needed */
 
@@ -71,8 +74,9 @@ struct virtio_net_config {
 	__u16 max_virtqueue_pairs;
 } __attribute__((packed));
 
+#ifndef VIRTIO_NET_NO_LEGACY
 /* This header comes first in the scatter-gather list.
- * If VIRTIO_F_ANY_LAYOUT is not negotiated, it must
+ * 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 {
@@ -97,6 +101,28 @@ 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.
+ */
+struct virtio_net_modern_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 */
+	__virtio16 num_buffers;	/* Number of merged rx buffers */
+};
+#endif /* ...VIRTIO_NET_NO_LEGACY */
 
 /*
  * Control virtqueue data structures
-- 
2.1.0

Re: [PATCH 0/5] virtio 1.0 cleanups and one fix.

[Michael S. Tsirkin]

On Fri, Feb 06, 2015 at 03:36:51PM +1030, Rusty Russell wrote:
> Hi all,
> 
> 	Some minor fixes for my virtio-next tree.  Michael, does
> QEMU implement the (compuslory!) VIRTIO_PCI_CAP_PCI_CFG field?  I'm
> guessing not, since it wasn't defined in the Linux header :(

Not yet, thanks for the reminder.
BIOS is going to use that one so you can be sure
it'll be there when support is merged finally.

> Rusty Russell (5):
>   virtio: define VIRTIO_PCI_CAP_PCI_CFG in header.
>   virtio: Don't expose legacy block features when VIRTIO_BLK_NO_LEGACY
>     defined.
>   virtio: Don't expose legacy net features when VIRTIO_NET_NO_LEGACY
>     defined.
>   virtio: Don't expose legacy config features when
>     VIRTIO_CONFIG_NO_LEGACY defined.
>   virtio: don't require a config space on the console device.
> 
>  drivers/char/virtio_console.c      | 12 ++++++++----
>  include/uapi/linux/virtio_blk.h    | 17 +++++++++++++----
>  include/uapi/linux/virtio_config.h |  2 ++
>  include/uapi/linux/virtio_net.h    | 30 ++++++++++++++++++++++++++++--
>  include/uapi/linux/virtio_pci.h    |  4 +++-
>  5 files changed, 54 insertions(+), 11 deletions(-)
> 
> -- 
> 2.1.0

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

[Michael S. Tsirkin]

On Fri, Feb 06, 2015 at 03:36:54PM +1030, Rusty Russell wrote:
> In particular, the virtio header always has the u16 num_buffers field.
> We define a new 'struct virtio_net_modern_hdr' for this (rather than
> simply calling it 'struct virtio_net_hdr', to avoid nasty type errors
> if some parts of a project define VIRTIO_NET_NO_LEGACY and some don't.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  include/uapi/linux/virtio_net.h | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index b5f1677b291c..32754f3000e8 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -35,7 +35,6 @@
>  #define VIRTIO_NET_F_CSUM	0	/* Host handles pkts w/ partial csum */
>  #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
>  #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
> -#define VIRTIO_NET_F_GSO	6	/* Host handles pkts w/ any GSO type */
>  #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
>  #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
>  #define VIRTIO_NET_F_GUEST_ECN	9	/* Guest can handle TSO[6] w/ ECN in. */
> @@ -56,6 +55,10 @@
>  					 * Steering */
>  #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
>  
> +#ifndef VIRTIO_NET_NO_LEGACY
> +#define VIRTIO_NET_F_GSO	6	/* Host handles pkts w/ any GSO type */
> +#endif /* VIRTIO_NET_NO_LEGACY */
> +
>  #define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
>  #define VIRTIO_NET_S_ANNOUNCE	2	/* Announcement is needed */
>  
> @@ -71,8 +74,9 @@ struct virtio_net_config {
>  	__u16 max_virtqueue_pairs;
>  } __attribute__((packed));
>  
> +#ifndef VIRTIO_NET_NO_LEGACY
>  /* This header comes first in the scatter-gather list.
> - * If VIRTIO_F_ANY_LAYOUT is not negotiated, it must
> + * 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 {
> @@ -97,6 +101,28 @@ 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.
> + */
> +struct virtio_net_modern_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 */
> +	__virtio16 num_buffers;	/* Number of merged rx buffers */
> +};
> +#endif /* ...VIRTIO_NET_NO_LEGACY */

This kind of masks the fact that it's the same as
virtio_net_hdr_mrg_rxbuf. So it's forcing people to duplicate
code for transitional devices.

How about
struct virtio_net_modern_hdr {
	struct virtio_net_hdr_mrg_rxbuf hdr;
}


This will also make it look nicer when we start
adding stuff in the header, the main header
is separated in a struct by its own, so it's
easy to apply operations such as sizeof.


>  /*
>   * Control virtqueue data structures
> -- 
> 2.1.0

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

[Rusty Russell]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Fri, Feb 06, 2015 at 03:36:54PM +1030, Rusty Russell wrote:
>> In particular, the virtio header always has the u16 num_buffers field.
>> We define a new 'struct virtio_net_modern_hdr' for this (rather than
>> simply calling it 'struct virtio_net_hdr', to avoid nasty type errors
>> if some parts of a project define VIRTIO_NET_NO_LEGACY and some don't.
>> 
>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>> ---
>>  include/uapi/linux/virtio_net.h | 30 ++++++++++++++++++++++++++++--
>>  1 file changed, 28 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
>> index b5f1677b291c..32754f3000e8 100644
>> --- a/include/uapi/linux/virtio_net.h
>> +++ b/include/uapi/linux/virtio_net.h
>> @@ -35,7 +35,6 @@
>>  #define VIRTIO_NET_F_CSUM	0	/* Host handles pkts w/ partial csum */
>>  #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
>>  #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
>> -#define VIRTIO_NET_F_GSO	6	/* Host handles pkts w/ any GSO type */
>>  #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
>>  #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
>>  #define VIRTIO_NET_F_GUEST_ECN	9	/* Guest can handle TSO[6] w/ ECN in. */
>> @@ -56,6 +55,10 @@
>>  					 * Steering */
>>  #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
>>  
>> +#ifndef VIRTIO_NET_NO_LEGACY
>> +#define VIRTIO_NET_F_GSO	6	/* Host handles pkts w/ any GSO type */
>> +#endif /* VIRTIO_NET_NO_LEGACY */
>> +
>>  #define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
>>  #define VIRTIO_NET_S_ANNOUNCE	2	/* Announcement is needed */
>>  
>> @@ -71,8 +74,9 @@ struct virtio_net_config {
>>  	__u16 max_virtqueue_pairs;
>>  } __attribute__((packed));
>>  
>> +#ifndef VIRTIO_NET_NO_LEGACY
>>  /* This header comes first in the scatter-gather list.
>> - * If VIRTIO_F_ANY_LAYOUT is not negotiated, it must
>> + * 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 {
>> @@ -97,6 +101,28 @@ 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.
>> + */
>> +struct virtio_net_modern_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 */
>> +	__virtio16 num_buffers;	/* Number of merged rx buffers */
>> +};
>> +#endif /* ...VIRTIO_NET_NO_LEGACY */
>
> This kind of masks the fact that it's the same as
> virtio_net_hdr_mrg_rxbuf. So it's forcing people to duplicate
> code for transitional devices.

It's a hard problem to solve; someone suffers.  My preference is to make
non-legacy implementations as "clean" as possible, even if transitional
devices suffer (since their suffering should be shorter-lived).

Transitional devices can't define VIRTIO_NET_NO_LEGACY, so they'll
have to use virtio_net_hdr_mrg_rxbuf anyway.

> How about
> struct virtio_net_modern_hdr {
> 	struct virtio_net_hdr_mrg_rxbuf hdr;
> }

But modern code now has refer to a field like this:

        hdr.hdr.hdr.flags

> This will also make it look nicer when we start
> adding stuff in the header, the main header
> is separated in a struct by its own, so it's
> easy to apply operations such as sizeof.

net is the only header we've extended, and I'm not aware of any plans to
extend again.  If we did, we could wrap the header to make it a bit more
awkward to use, but easier for next time, but that won't help for the
time *after* that.

We might rename struct virtio_net_modern_hdr to struct virtio_net_hdr_v1
though (which makes future naming easier).

Cheers,
Rusty.

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

[Rusty Russell]

Rusty Russell <rusty@rustcorp.com.au> writes:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> On Fri, Feb 06, 2015 at 03:36:54PM +1030, Rusty Russell wrote:
>>> In particular, the virtio header always has the u16 num_buffers field.
>>> We define a new 'struct virtio_net_modern_hdr' for this (rather than
>>> simply calling it 'struct virtio_net_hdr', to avoid nasty type errors
>>> if some parts of a project define VIRTIO_NET_NO_LEGACY and some don't.
>>> 
>>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>>> ---
>>>  include/uapi/linux/virtio_net.h | 30 ++++++++++++++++++++++++++++--
>>>  1 file changed, 28 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
>>> index b5f1677b291c..32754f3000e8 100644
>>> --- a/include/uapi/linux/virtio_net.h
>>> +++ b/include/uapi/linux/virtio_net.h
>>> @@ -35,7 +35,6 @@
>>>  #define VIRTIO_NET_F_CSUM	0	/* Host handles pkts w/ partial csum */
>>>  #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
>>>  #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
>>> -#define VIRTIO_NET_F_GSO	6	/* Host handles pkts w/ any GSO type */
>>>  #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
>>>  #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
>>>  #define VIRTIO_NET_F_GUEST_ECN	9	/* Guest can handle TSO[6] w/ ECN in. */
>>> @@ -56,6 +55,10 @@
>>>  					 * Steering */
>>>  #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
>>>  
>>> +#ifndef VIRTIO_NET_NO_LEGACY
>>> +#define VIRTIO_NET_F_GSO	6	/* Host handles pkts w/ any GSO type */
>>> +#endif /* VIRTIO_NET_NO_LEGACY */
>>> +
>>>  #define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
>>>  #define VIRTIO_NET_S_ANNOUNCE	2	/* Announcement is needed */
>>>  
>>> @@ -71,8 +74,9 @@ struct virtio_net_config {
>>>  	__u16 max_virtqueue_pairs;
>>>  } __attribute__((packed));
>>>  
>>> +#ifndef VIRTIO_NET_NO_LEGACY
>>>  /* This header comes first in the scatter-gather list.
>>> - * If VIRTIO_F_ANY_LAYOUT is not negotiated, it must
>>> + * 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 {
>>> @@ -97,6 +101,28 @@ 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.
>>> + */
>>> +struct virtio_net_modern_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 */
>>> +	__virtio16 num_buffers;	/* Number of merged rx buffers */
>>> +};
>>> +#endif /* ...VIRTIO_NET_NO_LEGACY */
>>
>> This kind of masks the fact that it's the same as
>> virtio_net_hdr_mrg_rxbuf. So it's forcing people to duplicate
>> code for transitional devices.
>
> It's a hard problem to solve; someone suffers.  My preference is to make
> non-legacy implementations as "clean" as possible, even if transitional
> devices suffer (since their suffering should be shorter-lived).
>
> Transitional devices can't define VIRTIO_NET_NO_LEGACY, so they'll
> have to use virtio_net_hdr_mrg_rxbuf anyway.

Here's what I ended up with.  In effect, it's just the name change
and a comment that it's the same as the legacy virtio_net_hdr_mrg_rxbuf:


Subject: virtio: Don't expose legacy net features when VIRTIO_NET_NO_LEGACY defined.

In particular, the virtio header always has the u16 num_buffers field.
We define a new 'struct virtio_net_hdr_v1' for this (rather than
simply calling it 'struct virtio_net_hdr', to avoid nasty type errors
if some parts of a project define VIRTIO_NET_NO_LEGACY and some don't.

Transitional devices (which can't define VIRTIO_NET_NO_LEGACY) will
have to keep using struct virtio_net_hdr_mrg_rxbuf, which has the same
byte layout as struct virtio_net_hdr_v1.

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 b5f1677b291c..4a9b58113d6e 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -35,7 +35,6 @@
 #define VIRTIO_NET_F_CSUM	0	/* Host handles pkts w/ partial csum */
 #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
 #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
-#define VIRTIO_NET_F_GSO	6	/* Host handles pkts w/ any GSO type */
 #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
 #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
 #define VIRTIO_NET_F_GUEST_ECN	9	/* Guest can handle TSO[6] w/ ECN in. */
@@ -56,6 +55,10 @@
 					 * Steering */
 #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
 
+#ifndef VIRTIO_NET_NO_LEGACY
+#define VIRTIO_NET_F_GSO	6	/* Host handles pkts w/ any GSO type */
+#endif /* VIRTIO_NET_NO_LEGACY */
+
 #define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
 #define VIRTIO_NET_S_ANNOUNCE	2	/* Announcement is needed */
 
@@ -71,8 +74,9 @@ struct virtio_net_config {
 	__u16 max_virtqueue_pairs;
 } __attribute__((packed));
 
+#ifndef VIRTIO_NET_NO_LEGACY
 /* This header comes first in the scatter-gather list.
- * If VIRTIO_F_ANY_LAYOUT is not negotiated, it must
+ * 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 {
@@ -97,6 +101,30 @@ 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.
+ */
+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 */
 
 /*
  * Control virtqueue data structures

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

[Michael S. Tsirkin]

On Sun, Feb 08, 2015 at 11:59:08AM +0100, Michael S. Tsirkin wrote:
> On Fri, Feb 06, 2015 at 03:36:54PM +1030, Rusty Russell wrote:
> > In particular, the virtio header always has the u16 num_buffers field.
> > We define a new 'struct virtio_net_modern_hdr' for this (rather than
> > simply calling it 'struct virtio_net_hdr', to avoid nasty type errors
> > if some parts of a project define VIRTIO_NET_NO_LEGACY and some don't.
> > 
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > ---
> >  include/uapi/linux/virtio_net.h | 30 ++++++++++++++++++++++++++++--
> >  1 file changed, 28 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> > index b5f1677b291c..32754f3000e8 100644
> > --- a/include/uapi/linux/virtio_net.h
> > +++ b/include/uapi/linux/virtio_net.h
> > @@ -35,7 +35,6 @@
> >  #define VIRTIO_NET_F_CSUM	0	/* Host handles pkts w/ partial csum */
> >  #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
> >  #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
> > -#define VIRTIO_NET_F_GSO	6	/* Host handles pkts w/ any GSO type */
> >  #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
> >  #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
> >  #define VIRTIO_NET_F_GUEST_ECN	9	/* Guest can handle TSO[6] w/ ECN in. */
> > @@ -56,6 +55,10 @@
> >  					 * Steering */
> >  #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
> >  
> > +#ifndef VIRTIO_NET_NO_LEGACY
> > +#define VIRTIO_NET_F_GSO	6	/* Host handles pkts w/ any GSO type */
> > +#endif /* VIRTIO_NET_NO_LEGACY */
> > +
> >  #define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
> >  #define VIRTIO_NET_S_ANNOUNCE	2	/* Announcement is needed */
> >  
> > @@ -71,8 +74,9 @@ struct virtio_net_config {
> >  	__u16 max_virtqueue_pairs;
> >  } __attribute__((packed));
> >  
> > +#ifndef VIRTIO_NET_NO_LEGACY
> >  /* This header comes first in the scatter-gather list.
> > - * If VIRTIO_F_ANY_LAYOUT is not negotiated, it must
> > + * 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 {
> > @@ -97,6 +101,28 @@ 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.
> > + */
> > +struct virtio_net_modern_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 */
> > +	__virtio16 num_buffers;	/* Number of merged rx buffers */
> > +};
> > +#endif /* ...VIRTIO_NET_NO_LEGACY */
> 
> This kind of masks the fact that it's the same as
> virtio_net_hdr_mrg_rxbuf. So it's forcing people to duplicate
> code for transitional devices.
> 
> How about
> struct virtio_net_modern_hdr {
> 	struct virtio_net_hdr_mrg_rxbuf hdr;
> }
> 
> 
> This will also make it look nicer when we start
> adding stuff in the header, the main header
> is separated in a struct by its own, so it's
> easy to apply operations such as sizeof.

Ping.
Would you like a patch on top that does this?


> 
> >  /*
> >   * Control virtqueue data structures
> > -- 
> > 2.1.0

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

[Rusty Russell]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Sun, Feb 08, 2015 at 11:59:08AM +0100, Michael S. Tsirkin wrote:
>> On Fri, Feb 06, 2015 at 03:36:54PM +1030, Rusty Russell wrote:
>> > In particular, the virtio header always has the u16 num_buffers field.
>> > We define a new 'struct virtio_net_modern_hdr' for this (rather than
>> > simply calling it 'struct virtio_net_hdr', to avoid nasty type errors
>> > if some parts of a project define VIRTIO_NET_NO_LEGACY and some don't.
>> 
>> This kind of masks the fact that it's the same as
>> virtio_net_hdr_mrg_rxbuf. So it's forcing people to duplicate
>> code for transitional devices.
>> 
>> How about
>> struct virtio_net_modern_hdr {
>> 	struct virtio_net_hdr_mrg_rxbuf hdr;
>> }
>> 
>> 
>> This will also make it look nicer when we start
>> adding stuff in the header, the main header
>> is separated in a struct by its own, so it's
>> easy to apply operations such as sizeof.
>
> Ping.
> Would you like a patch on top that does this?

Hmm, I thought I replied...

Indeed, I did.  I disagreed, and simply renamed struct
virtio_net_modern_hdr to virtio_net_hdr_v1.

Cheers,
Rusty.

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

[Michael S. Tsirkin]

>  	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".

> + */
> +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?










>  /*
>   * Control virtqueue data structures

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

[Rusty Russell]

"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 */
 
 /*