[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 1/5] virtio: define VIRTIO_PCI_CAP_PCI_CFG in header.

[Rusty Russell]

This provides backdoor access to the device MMIOs, and every device should
have one.  From the virtio 1.0 spec (CS03):

  4.1.4.7.1 Device Requirements: PCI configuration access capability

  The device MUST present at least one VIRTIO_PCI_CAP_PCI_CFG capability.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/uapi/linux/virtio_pci.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
index 3b7e4d2765fb..75301468359f 100644
--- a/include/uapi/linux/virtio_pci.h
+++ b/include/uapi/linux/virtio_pci.h
@@ -109,8 +109,10 @@
 #define VIRTIO_PCI_CAP_NOTIFY_CFG	2
 /* ISR access */
 #define VIRTIO_PCI_CAP_ISR_CFG		3
-/* Device specific confiuration */
+/* Device specific configuration */
 #define VIRTIO_PCI_CAP_DEVICE_CFG	4
+/* PCI configuration access */
+#define VIRTIO_PCI_CAP_PCI_CFG		5
 
 /* This is the PCI capability header: */
 struct virtio_pci_cap {
-- 
2.1.0

Re: [PATCH 1/5] virtio: define VIRTIO_PCI_CAP_PCI_CFG in header.

[Michael S. Tsirkin]

On Fri, Feb 06, 2015 at 03:36:52PM +1030, Rusty Russell wrote:
> This provides backdoor access to the device MMIOs, and every device should
> have one.  From the virtio 1.0 spec (CS03):
> 
>   4.1.4.7.1 Device Requirements: PCI configuration access capability
> 
>   The device MUST present at least one VIRTIO_PCI_CAP_PCI_CFG capability.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  include/uapi/linux/virtio_pci.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
> index 3b7e4d2765fb..75301468359f 100644
> --- a/include/uapi/linux/virtio_pci.h
> +++ b/include/uapi/linux/virtio_pci.h
> @@ -109,8 +109,10 @@
>  #define VIRTIO_PCI_CAP_NOTIFY_CFG	2
>  /* ISR access */
>  #define VIRTIO_PCI_CAP_ISR_CFG		3
> -/* Device specific confiuration */
> +/* Device specific configuration */
>  #define VIRTIO_PCI_CAP_DEVICE_CFG	4
> +/* PCI configuration access */
> +#define VIRTIO_PCI_CAP_PCI_CFG		5
>  
>  /* This is the PCI capability header: */
>  struct virtio_pci_cap {
> -- 
> 2.1.0

[PATCH 2/5] virtio: Don't expose legacy block features when VIRTIO_BLK_NO_LEGACY defined.

[Rusty Russell]

This allows modern implementations to ensure they don't use legacy
feature bits or SCSI commands (which are not used in v1.0 non-legacy).

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/uapi/linux/virtio_blk.h | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
index 247c8ba8544a..3c53eec4ae22 100644
--- a/include/uapi/linux/virtio_blk.h
+++ b/include/uapi/linux/virtio_blk.h
@@ -31,22 +31,25 @@
 #include <linux/virtio_types.h>
 
 /* Feature bits */
-#define VIRTIO_BLK_F_BARRIER	0	/* Does host support barriers? */
 #define VIRTIO_BLK_F_SIZE_MAX	1	/* Indicates maximum segment size */
 #define VIRTIO_BLK_F_SEG_MAX	2	/* Indicates maximum # of segments */
 #define VIRTIO_BLK_F_GEOMETRY	4	/* Legacy geometry available  */
 #define VIRTIO_BLK_F_RO		5	/* Disk is read-only */
 #define VIRTIO_BLK_F_BLK_SIZE	6	/* Block size of disk is available*/
-#define VIRTIO_BLK_F_SCSI	7	/* Supports scsi command passthru */
-#define VIRTIO_BLK_F_WCE	9	/* Writeback mode enabled after reset */
 #define VIRTIO_BLK_F_TOPOLOGY	10	/* Topology information is available */
-#define VIRTIO_BLK_F_CONFIG_WCE	11	/* Writeback mode available in config */
 #define VIRTIO_BLK_F_MQ		12	/* support more than one vq */
 
+/* Legacy feature bits */
+#ifndef VIRTIO_BLK_NO_LEGACY
+#define VIRTIO_BLK_F_BARRIER	0	/* Does host support barriers? */
+#define VIRTIO_BLK_F_SCSI	7	/* Supports scsi command passthru */
+#define VIRTIO_BLK_F_WCE	9	/* Writeback mode enabled after reset */
+#define VIRTIO_BLK_F_CONFIG_WCE	11	/* Writeback mode available in config */
 #ifndef __KERNEL__
 /* Old (deprecated) name for VIRTIO_BLK_F_WCE. */
 #define VIRTIO_BLK_F_FLUSH VIRTIO_BLK_F_WCE
 #endif
+#endif /* !VIRTIO_BLK_NO_LEGACY */
 
 #define VIRTIO_BLK_ID_BYTES	20	/* ID string length */
 
@@ -100,8 +103,10 @@ struct virtio_blk_config {
 #define VIRTIO_BLK_T_IN		0
 #define VIRTIO_BLK_T_OUT	1
 
+#ifndef VIRTIO_BLK_NO_LEGACY
 /* This bit says it's a scsi command, not an actual read or write. */
 #define VIRTIO_BLK_T_SCSI_CMD	2
+#endif /* VIRTIO_BLK_NO_LEGACY */
 
 /* Cache flush command */
 #define VIRTIO_BLK_T_FLUSH	4
@@ -109,8 +114,10 @@ struct virtio_blk_config {
 /* Get device ID command */
 #define VIRTIO_BLK_T_GET_ID    8
 
+#ifndef VIRTIO_BLK_NO_LEGACY
 /* Barrier before this op. */
 #define VIRTIO_BLK_T_BARRIER	0x80000000
+#endif /* !VIRTIO_BLK_NO_LEGACY */
 
 /* This is the first element of the read scatter-gather list. */
 struct virtio_blk_outhdr {
@@ -122,12 +129,14 @@ struct virtio_blk_outhdr {
 	__virtio64 sector;
 };
 
+#ifndef VIRTIO_BLK_NO_LEGACY
 struct virtio_scsi_inhdr {
 	__virtio32 errors;
 	__virtio32 data_len;
 	__virtio32 sense_len;
 	__virtio32 residual;
 };
+#endif /* !VIRTIO_BLK_NO_LEGACY */
 
 /* And this is the final byte of the write scatter-gather list. */
 #define VIRTIO_BLK_S_OK		0
-- 
2.1.0

Re: [PATCH 2/5] virtio: Don't expose legacy block features when VIRTIO_BLK_NO_LEGACY defined.

[Michael S. Tsirkin]

On Fri, Feb 06, 2015 at 03:36:53PM +1030, Rusty Russell wrote:
> This allows modern implementations to ensure they don't use legacy
> feature bits or SCSI commands (which are not used in v1.0 non-legacy).
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  include/uapi/linux/virtio_blk.h | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
> index 247c8ba8544a..3c53eec4ae22 100644
> --- a/include/uapi/linux/virtio_blk.h
> +++ b/include/uapi/linux/virtio_blk.h
> @@ -31,22 +31,25 @@
>  #include <linux/virtio_types.h>
>  
>  /* Feature bits */
> -#define VIRTIO_BLK_F_BARRIER	0	/* Does host support barriers? */
>  #define VIRTIO_BLK_F_SIZE_MAX	1	/* Indicates maximum segment size */
>  #define VIRTIO_BLK_F_SEG_MAX	2	/* Indicates maximum # of segments */
>  #define VIRTIO_BLK_F_GEOMETRY	4	/* Legacy geometry available  */
>  #define VIRTIO_BLK_F_RO		5	/* Disk is read-only */
>  #define VIRTIO_BLK_F_BLK_SIZE	6	/* Block size of disk is available*/
> -#define VIRTIO_BLK_F_SCSI	7	/* Supports scsi command passthru */
> -#define VIRTIO_BLK_F_WCE	9	/* Writeback mode enabled after reset */
>  #define VIRTIO_BLK_F_TOPOLOGY	10	/* Topology information is available */
> -#define VIRTIO_BLK_F_CONFIG_WCE	11	/* Writeback mode available in config */
>  #define VIRTIO_BLK_F_MQ		12	/* support more than one vq */
>  
> +/* Legacy feature bits */
> +#ifndef VIRTIO_BLK_NO_LEGACY
> +#define VIRTIO_BLK_F_BARRIER	0	/* Does host support barriers? */
> +#define VIRTIO_BLK_F_SCSI	7	/* Supports scsi command passthru */
> +#define VIRTIO_BLK_F_WCE	9	/* Writeback mode enabled after reset */
> +#define VIRTIO_BLK_F_CONFIG_WCE	11	/* Writeback mode available in config */
>  #ifndef __KERNEL__
>  /* Old (deprecated) name for VIRTIO_BLK_F_WCE. */
>  #define VIRTIO_BLK_F_FLUSH VIRTIO_BLK_F_WCE
>  #endif
> +#endif /* !VIRTIO_BLK_NO_LEGACY */
>  
>  #define VIRTIO_BLK_ID_BYTES	20	/* ID string length */
>  
> @@ -100,8 +103,10 @@ struct virtio_blk_config {
>  #define VIRTIO_BLK_T_IN		0
>  #define VIRTIO_BLK_T_OUT	1
>  
> +#ifndef VIRTIO_BLK_NO_LEGACY
>  /* This bit says it's a scsi command, not an actual read or write. */
>  #define VIRTIO_BLK_T_SCSI_CMD	2
> +#endif /* VIRTIO_BLK_NO_LEGACY */
>  
>  /* Cache flush command */
>  #define VIRTIO_BLK_T_FLUSH	4
> @@ -109,8 +114,10 @@ struct virtio_blk_config {
>  /* Get device ID command */
>  #define VIRTIO_BLK_T_GET_ID    8
>  
> +#ifndef VIRTIO_BLK_NO_LEGACY
>  /* Barrier before this op. */
>  #define VIRTIO_BLK_T_BARRIER	0x80000000
> +#endif /* !VIRTIO_BLK_NO_LEGACY */
>  
>  /* This is the first element of the read scatter-gather list. */
>  struct virtio_blk_outhdr {
> @@ -122,12 +129,14 @@ struct virtio_blk_outhdr {
>  	__virtio64 sector;
>  };
>  
> +#ifndef VIRTIO_BLK_NO_LEGACY
>  struct virtio_scsi_inhdr {
>  	__virtio32 errors;
>  	__virtio32 data_len;
>  	__virtio32 sense_len;
>  	__virtio32 residual;
>  };
> +#endif /* !VIRTIO_BLK_NO_LEGACY */
>  
>  /* And this is the final byte of the write scatter-gather list. */
>  #define VIRTIO_BLK_S_OK		0
> -- 
> 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 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]

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

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.

[PATCH 4/5] virtio: Don't expose legacy config features when VIRTIO_CONFIG_NO_LEGACY defined.

[Rusty Russell]

The VIRTIO_F_ANY_LAYOUT and VIRTIO_F_NOTIFY_ON_EMPTY features are pre-1.0
only.

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

diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
index a6d0cdeaacd4..c18264df9504 100644
--- a/include/uapi/linux/virtio_config.h
+++ b/include/uapi/linux/virtio_config.h
@@ -49,12 +49,14 @@
 #define VIRTIO_TRANSPORT_F_START	28
 #define VIRTIO_TRANSPORT_F_END		33
 
+#ifndef VIRTIO_CONFIG_NO_LEGACY
 /* Do we get callbacks when the ring is completely used, even if we've
  * suppressed them? */
 #define VIRTIO_F_NOTIFY_ON_EMPTY	24
 
 /* Can the device handle any descriptor layout? */
 #define VIRTIO_F_ANY_LAYOUT		27
+#endif /* VIRTIO_CONFIG_NO_LEGACY */
 
 /* v1.0 compliant. */
 #define VIRTIO_F_VERSION_1		32
-- 
2.1.0

Re: [PATCH 4/5] virtio: Don't expose legacy config features when VIRTIO_CONFIG_NO_LEGACY defined.

[Michael S. Tsirkin]

On Fri, Feb 06, 2015 at 03:36:55PM +1030, Rusty Russell wrote:
> The VIRTIO_F_ANY_LAYOUT and VIRTIO_F_NOTIFY_ON_EMPTY features are pre-1.0
> only.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  include/uapi/linux/virtio_config.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> index a6d0cdeaacd4..c18264df9504 100644
> --- a/include/uapi/linux/virtio_config.h
> +++ b/include/uapi/linux/virtio_config.h
> @@ -49,12 +49,14 @@
>  #define VIRTIO_TRANSPORT_F_START	28
>  #define VIRTIO_TRANSPORT_F_END		33
>  
> +#ifndef VIRTIO_CONFIG_NO_LEGACY
>  /* Do we get callbacks when the ring is completely used, even if we've
>   * suppressed them? */
>  #define VIRTIO_F_NOTIFY_ON_EMPTY	24
>  
>  /* Can the device handle any descriptor layout? */
>  #define VIRTIO_F_ANY_LAYOUT		27
> +#endif /* VIRTIO_CONFIG_NO_LEGACY */
>  
>  /* v1.0 compliant. */
>  #define VIRTIO_F_VERSION_1		32
> -- 
> 2.1.0

[PATCH 5/5] virtio: don't require a config space on the console device.

[Rusty Russell]

Strictly, it's only needed when we have features (size or multiport).

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/char/virtio_console.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 26afb56a8073..8f182b0015b5 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1986,10 +1986,14 @@ static int virtcons_probe(struct virtio_device *vdev)
 	bool multiport;
 	bool early = early_put_chars != NULL;
 
-	if (!vdev->config->get) {
-		dev_err(&vdev->dev, "%s failure: config access disabled\n",
-			__func__);
-		return -EINVAL;
+	/* We only need a config space if features are offered */
+	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE)
+	    || virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT)) {
+		if (!vdev->config->get) {
+			dev_err(&vdev->dev, "%s failure: config access disabled\n",
+				__func__);
+			return -EINVAL;
+		}
 	}
 
 	/* Ensure to read early_put_chars now */
-- 
2.1.0

Re: [PATCH 5/5] virtio: don't require a config space on the console device.

[Michael S. Tsirkin]

On Fri, Feb 06, 2015 at 03:36:56PM +1030, Rusty Russell wrote:
> Strictly, it's only needed when we have features (size or multiport).
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  drivers/char/virtio_console.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 26afb56a8073..8f182b0015b5 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1986,10 +1986,14 @@ static int virtcons_probe(struct virtio_device *vdev)
>  	bool multiport;
>  	bool early = early_put_chars != NULL;
>  
> -	if (!vdev->config->get) {
> -		dev_err(&vdev->dev, "%s failure: config access disabled\n",
> -			__func__);
> -		return -EINVAL;
> +	/* We only need a config space if features are offered */
> +	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE)
> +	    || virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT)) {
> +		if (!vdev->config->get) {

I dislike nested ifs. How about
		if (!vdev->config->get && (
		    (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE)
		    || virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT))

?


> +			dev_err(&vdev->dev, "%s failure: config access disabled\n",
> +				__func__);
> +			return -EINVAL;
> +		}
>  	}
>  
>  	/* Ensure to read early_put_chars now */
> -- 
> 2.1.0

Re: [PATCH 5/5] virtio: don't require a config space on the console device.

[Rusty Russell]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Fri, Feb 06, 2015 at 03:36:56PM +1030, Rusty Russell wrote:
>> Strictly, it's only needed when we have features (size or multiport).
>> 
>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>> ---
>>  drivers/char/virtio_console.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
>> index 26afb56a8073..8f182b0015b5 100644
>> --- a/drivers/char/virtio_console.c
>> +++ b/drivers/char/virtio_console.c
>> @@ -1986,10 +1986,14 @@ static int virtcons_probe(struct virtio_device *vdev)
>>  	bool multiport;
>>  	bool early = early_put_chars != NULL;
>>  
>> -	if (!vdev->config->get) {
>> -		dev_err(&vdev->dev, "%s failure: config access disabled\n",
>> -			__func__);
>> -		return -EINVAL;
>> +	/* We only need a config space if features are offered */
>> +	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE)
>> +	    || virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT)) {
>> +		if (!vdev->config->get) {
>
> I dislike nested ifs. How about
> 		if (!vdev->config->get && (
> 		    (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE)
> 		    || virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT))

OK, fixed.

Thanks,
Rusty.

virtio: don't require a config space on the console device.

Strictly, it's only needed when we have features (size or multiport).

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 26afb56a8073..fae2dbbf5745 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1986,7 +1986,10 @@ static int virtcons_probe(struct virtio_device *vdev)
 	bool multiport;
 	bool early = early_put_chars != NULL;
 
-	if (!vdev->config->get) {
+	/* We only need a config space if features are offered */
+	if (!vdev->config->get &&
+	    (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE)
+	     || virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT))) {
 		dev_err(&vdev->dev, "%s failure: config access disabled\n",
 			__func__);
 		return -EINVAL;

Re: [PATCH 5/5] virtio: don't require a config space on the console device.

[Amit Shah]

On (Mon) 09 Feb 2015 [10:26:29], Rusty Russell wrote:
> virtio: don't require a config space on the console device.
> 
> Strictly, it's only needed when we have features (size or multiport).
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Reviewed-by: Amit Shah <amit.shah@redhat.com>

> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 26afb56a8073..fae2dbbf5745 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1986,7 +1986,10 @@ static int virtcons_probe(struct virtio_device *vdev)
>  	bool multiport;
>  	bool early = early_put_chars != NULL;
>  
> -	if (!vdev->config->get) {
> +	/* We only need a config space if features are offered */
> +	if (!vdev->config->get &&
> +	    (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE)
> +	     || virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT))) {
>  		dev_err(&vdev->dev, "%s failure: config access disabled\n",
>  			__func__);
>  		return -EINVAL;

		Amit

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