[Qemu-devel] [PATCH] virtio-net: add feature bit for any header s/g

[Qemu-devel] [PATCH] virtio-net: add feature bit for any header s/g

[Michael S. Tsirkin]

Old qemu versions required that 1st s/g entry is the header.

Since QEMU 1.5, patchset titled "virtio-net: iovec handling cleanup"
removed this limitation but a feature bit is needed so guests know it's
safe to lay out header differently.

This patch applies on top and adds such a feature bit to QEMU.
It is set by default for virtio-net.
virtio net header inline with the data is beneficial
for latency and small packet bandwidth - guest driver
code utilizing this feature has been acked but missed 3.11
by a narrow margin, it's pending for 3.12.

This feature bit is cleared by default when compatibility with old
machine types is requested.

Other performance-sensitive devices (blk and scsi)
don't yet support arbitrary s/g layouts, so
we only set this bit for virtio-net for now.
There are plans to allow arbitrary layouts there, but
no code has been posted yet.

Cc: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/i386/pc.h           | 4 ++++
 include/hw/virtio/virtio-net.h | 1 +
 include/hw/virtio/virtio.h     | 2 ++
 3 files changed, 7 insertions(+)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 051f423..d8f91ad 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -264,6 +264,10 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
             .driver   = "Nehalem-" TYPE_X86_CPU,\
             .property = "level",\
             .value    = stringify(2),\
+        },{\
+            .driver   = "virtio-net-pci",\
+            .property = "any_layout",\
+            .value    = "off",\
         }
 
 #define PC_COMPAT_1_4 \
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index b315ac9..df60f16 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -243,6 +243,7 @@ struct virtio_net_ctrl_mq {
 
 #define DEFINE_VIRTIO_NET_FEATURES(_state, _field) \
         DEFINE_VIRTIO_COMMON_FEATURES(_state, _field), \
+        DEFINE_PROP_BIT("any_layout", _state, _field, VIRTIO_F_ANY_LAYOUT, true), \
         DEFINE_PROP_BIT("csum", _state, _field, VIRTIO_NET_F_CSUM, true), \
         DEFINE_PROP_BIT("guest_csum", _state, _field, VIRTIO_NET_F_GUEST_CSUM, true), \
         DEFINE_PROP_BIT("gso", _state, _field, VIRTIO_NET_F_GSO, true), \
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index a6c5c53..5d1d2be 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -43,6 +43,8 @@
 /* We notify when the ring is completely used, even if the guest is suppressing
  * callbacks */
 #define VIRTIO_F_NOTIFY_ON_EMPTY        24
+/* Can the device handle any descriptor layout? */
+#define VIRTIO_F_ANY_LAYOUT             27
 /* We support indirect buffer descriptors */
 #define VIRTIO_RING_F_INDIRECT_DESC     28
 /* The Guest publishes the used index for which it expects an interrupt
-- 
MST

Re: [Qemu-devel] [PATCH] virtio-net: add feature bit for any header s/g

[Laszlo Ersek]

On 07/11/13 15:15, Michael S. Tsirkin wrote:
> Old qemu versions required that 1st s/g entry is the header.
> 
> Since QEMU 1.5, patchset titled "virtio-net: iovec handling cleanup"
> removed this limitation but a feature bit is needed so guests know it's
> safe to lay out header differently.
> 
> This patch applies on top and adds such a feature bit to QEMU.
> It is set by default for virtio-net.
> virtio net header inline with the data is beneficial
> for latency and small packet bandwidth - guest driver
> code utilizing this feature has been acked but missed 3.11
> by a narrow margin, it's pending for 3.12.
> 
> This feature bit is cleared by default when compatibility with old
> machine types is requested.
> 
> Other performance-sensitive devices (blk and scsi)
> don't yet support arbitrary s/g layouts, so
> we only set this bit for virtio-net for now.
> There are plans to allow arbitrary layouts there, but
> no code has been posted yet.
> 
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/hw/i386/pc.h           | 4 ++++
>  include/hw/virtio/virtio-net.h | 1 +
>  include/hw/virtio/virtio.h     | 2 ++
>  3 files changed, 7 insertions(+)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 051f423..d8f91ad 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -264,6 +264,10 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
>              .driver   = "Nehalem-" TYPE_X86_CPU,\
>              .property = "level",\
>              .value    = stringify(2),\
> +        },{\
> +            .driver   = "virtio-net-pci",\
> +            .property = "any_layout",\
> +            .value    = "off",\
>          }
>  
>  #define PC_COMPAT_1_4 \
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index b315ac9..df60f16 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -243,6 +243,7 @@ struct virtio_net_ctrl_mq {
>  
>  #define DEFINE_VIRTIO_NET_FEATURES(_state, _field) \
>          DEFINE_VIRTIO_COMMON_FEATURES(_state, _field), \
> +        DEFINE_PROP_BIT("any_layout", _state, _field, VIRTIO_F_ANY_LAYOUT, true), \
>          DEFINE_PROP_BIT("csum", _state, _field, VIRTIO_NET_F_CSUM, true), \
>          DEFINE_PROP_BIT("guest_csum", _state, _field, VIRTIO_NET_F_GUEST_CSUM, true), \
>          DEFINE_PROP_BIT("gso", _state, _field, VIRTIO_NET_F_GSO, true), \
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index a6c5c53..5d1d2be 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -43,6 +43,8 @@
>  /* We notify when the ring is completely used, even if the guest is suppressing
>   * callbacks */
>  #define VIRTIO_F_NOTIFY_ON_EMPTY        24
> +/* Can the device handle any descriptor layout? */
> +#define VIRTIO_F_ANY_LAYOUT             27
>  /* We support indirect buffer descriptors */
>  #define VIRTIO_RING_F_INDIRECT_DESC     28
>  /* The Guest publishes the used index for which it expects an interrupt
> 

Take it FWIW,

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Just educate me: this adds a non-net-specific flag (ie.
VIRTIO_F_ANY_LAYOUT as opposed to "VIRTIO_NET_F_ANY_LAYOUT") to
virtio-net only. This seems to be the first such use:

  git grep DEFINE_PROP_BIT | grep VIRTIO_F_

reports nothing, while

  git grep DEFINE_PROP_BIT | grep VIRTIO_

reports a bunch of VIRTIO_NET_F_*, and one VIRTIO_SCSI_F_HOTPLUG.


Would DEFINE_VIRTIO_COMMON_FEATURES be a better macro to extend?
Granted, for example, the generic VIRTIO_F_NOTIFY_ON_EMPTY is absent
from that as well, but may that be because it should not be configured
externally?

The last paragraph of the commit message is not lost on me. The question
is whether to export the common flag universally, just ignore it in most
devices, vs. export it only where it's actually supported.

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH] virtio-net: add feature bit for any header s/g

[Michael S. Tsirkin]

On Thu, Jul 11, 2013 at 03:39:42PM +0200, Laszlo Ersek wrote:
> On 07/11/13 15:15, Michael S. Tsirkin wrote:
> > Old qemu versions required that 1st s/g entry is the header.
> > 
> > Since QEMU 1.5, patchset titled "virtio-net: iovec handling cleanup"
> > removed this limitation but a feature bit is needed so guests know it's
> > safe to lay out header differently.
> > 
> > This patch applies on top and adds such a feature bit to QEMU.
> > It is set by default for virtio-net.
> > virtio net header inline with the data is beneficial
> > for latency and small packet bandwidth - guest driver
> > code utilizing this feature has been acked but missed 3.11
> > by a narrow margin, it's pending for 3.12.
> > 
> > This feature bit is cleared by default when compatibility with old
> > machine types is requested.
> > 
> > Other performance-sensitive devices (blk and scsi)
> > don't yet support arbitrary s/g layouts, so
> > we only set this bit for virtio-net for now.
> > There are plans to allow arbitrary layouts there, but
> > no code has been posted yet.
> > 
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  include/hw/i386/pc.h           | 4 ++++
> >  include/hw/virtio/virtio-net.h | 1 +
> >  include/hw/virtio/virtio.h     | 2 ++
> >  3 files changed, 7 insertions(+)
> > 
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 051f423..d8f91ad 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -264,6 +264,10 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
> >              .driver   = "Nehalem-" TYPE_X86_CPU,\
> >              .property = "level",\
> >              .value    = stringify(2),\
> > +        },{\
> > +            .driver   = "virtio-net-pci",\
> > +            .property = "any_layout",\
> > +            .value    = "off",\
> >          }
> >  
> >  #define PC_COMPAT_1_4 \
> > diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> > index b315ac9..df60f16 100644
> > --- a/include/hw/virtio/virtio-net.h
> > +++ b/include/hw/virtio/virtio-net.h
> > @@ -243,6 +243,7 @@ struct virtio_net_ctrl_mq {
> >  
> >  #define DEFINE_VIRTIO_NET_FEATURES(_state, _field) \
> >          DEFINE_VIRTIO_COMMON_FEATURES(_state, _field), \
> > +        DEFINE_PROP_BIT("any_layout", _state, _field, VIRTIO_F_ANY_LAYOUT, true), \
> >          DEFINE_PROP_BIT("csum", _state, _field, VIRTIO_NET_F_CSUM, true), \
> >          DEFINE_PROP_BIT("guest_csum", _state, _field, VIRTIO_NET_F_GUEST_CSUM, true), \
> >          DEFINE_PROP_BIT("gso", _state, _field, VIRTIO_NET_F_GSO, true), \
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index a6c5c53..5d1d2be 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -43,6 +43,8 @@
> >  /* We notify when the ring is completely used, even if the guest is suppressing
> >   * callbacks */
> >  #define VIRTIO_F_NOTIFY_ON_EMPTY        24
> > +/* Can the device handle any descriptor layout? */
> > +#define VIRTIO_F_ANY_LAYOUT             27
> >  /* We support indirect buffer descriptors */
> >  #define VIRTIO_RING_F_INDIRECT_DESC     28
> >  /* The Guest publishes the used index for which it expects an interrupt
> > 
> 
> Take it FWIW,
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Just educate me: this adds a non-net-specific flag (ie.
> VIRTIO_F_ANY_LAYOUT as opposed to "VIRTIO_NET_F_ANY_LAYOUT") to
> virtio-net only. This seems to be the first such use:
> 
>   git grep DEFINE_PROP_BIT | grep VIRTIO_F_
> 
> reports nothing, while
> 
>   git grep DEFINE_PROP_BIT | grep VIRTIO_
> 
> reports a bunch of VIRTIO_NET_F_*, and one VIRTIO_SCSI_F_HOTPLUG.
> 
> 
> Would DEFINE_VIRTIO_COMMON_FEATURES be a better macro to extend?

No, this would add it for all devices, and only net actually
support any layout.

> Granted, for example, the generic VIRTIO_F_NOTIFY_ON_EMPTY is absent
> from that as well, but may that be because it should not be configured
> externally?
> 
> The last paragraph of the commit message is not lost on me. The question
> is whether to export the common flag universally, just ignore it in most
> devices, vs. export it only where it's actually supported.
> 
> Thanks,
> Laszlo

So we'll add a way for users to shoot themselves in the foot
by setting a flag incorrectly. Point being?

Re: [Qemu-devel] [PATCH] virtio-net: add feature bit for any header s/g

[Laszlo Ersek]

On 07/11/13 15:41, Michael S. Tsirkin wrote:

> So we'll add a way for users to shoot themselves in the foot
> by setting a flag incorrectly. Point being?

Point taken. The flag name being global / universal relates to the
concept, not support level. Exposing it in any device enables the user
to set the flag, which in turn enables the guest to negotiate it, and to
create descriptor chains that qemu doesn't recognize.

Thanks
Laszlo