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

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

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?