| Message ID | 1373548466-14804-1-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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?
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
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
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(+)