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 <ru...@rustcorp.com.au>
> > Signed-off-by: Michael S. Tsirkin <m...@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 <ler...@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?
