From: "Michael S. Tsirkin" <mst@redhat.com>
To: Dmitry Fleytman <dmitry@daynix.com>
Cc: Yan Vugenfirer <yan@daynix.com>,
Dmitry Fleytman <dfleytma@redhat.com>,
virtualization <virtualization@lists.linux-foundation.org>
Subject: Re: [PATCH 1/2 V2] virtio-net: dynamic network offloads configuration
Date: Tue, 2 Apr 2013 13:53:32 +0300 [thread overview]
Message-ID: <20130402105332.GA21572@redhat.com> (raw)
In-Reply-To: <CAGHCxhfcTrmh2eqn=EAhi+GnK0qWwE3+OGVq4LdCaU6=2FQc9A@mail.gmail.com>
On Tue, Apr 02, 2013 at 09:56:51AM +0300, Dmitry Fleytman wrote:
>
>
>
> On Sun, Mar 31, 2013 at 10:53 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sat, Mar 30, 2013 at 09:47:58PM +0300, Dmitry Fleytman wrote:
> > Michael, thanks for review,
> > I'll rename command as you suggested, indeed it will be better.
> >
> > Not sure I understand a concept of compat macros.
> > Is there documentation available on this topic?
> >
> > Dmitry
>
> No, but it's simple. It's just lots of arrays of options we apply only
> one of them depending on -M. See e.g. pc_machine_init.
>
>
>
> Do you mean something like this in PC_COMPAT_1_4:
>
> diff --git a/hw/pc.h b/hw/pc.h
> index 8e1dd4c..7ca4698 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -221,6 +221,10 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
> .property = "vectors",\
> /* DEV_NVECTORS_UNSPECIFIED as a uint32_t string */\
> .value = stringify(0xFFFFFFFF),\
> + },{ \
> + .driver = "virtio-net-pci", \
> + .property = "ctrl_guest_offloads", \
> + .value = "off", \
> },{\
> .driver = "e1000",\
> .property = "romfile",\
>
Exactly.
>
> >
> > On Thu, Mar 28, 2013 at 6:39 PM, Michael S. Tsirkin <mst@redhat.com>
> wrote:
> >
> > On Thu, Mar 28, 2013 at 12:00:59PM +0200, Dmitry Fleytman wrote:
> > > From: Dmitry Fleytman <dfleytma@redhat.com>
> > >
> > > Virtio-net driver currently negotiates network offloads
> > > on startup via features mechanism and have no ability to
> > > change offloads state later.
> > > This patch introduced a new control command that allows
> > > to configure device network offloads state dynamically.
> > > The patch also introduces a new feature flag
> > > VIRTIO_NET_F_CTRL_GUEST_OFFLD.
> > >
> > > Signed-off-by: Dmitry Fleytman <dfleytma@redhat.com>
> > > ---
> > > áhw/virtio-net.c | 111
> > ++++++++++++++++++++++++++++++++++++++++++++++++--------
> > > áhw/virtio-net.h | á19 ++++++++++
> > > á2 files changed, 115 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > > index 5917740..a016823 100644
> > > --- a/hw/virtio-net.c
> > > +++ b/hw/virtio-net.c
> > > @@ -360,6 +360,48 @@ static uint32_t virtio_net_bad_features
> (VirtIODevice
> > *vdev)
> > > á á áreturn features;
> > > á}
> > >
> > > +static void virtio_net_apply_offloads(VirtIONet *n)
> > > +{
> > > + á átap_set_offload(qemu_get_subqueue(n->nic, 0)->peer,
> > > + á á á á á á á á á á!!(n->curr_offloads &
> > VIRTIO_NET_OFFLOAD_GUEST_CSUM),
> > > + á á á á á á á á á á!!(n->curr_offloads &
> > VIRTIO_NET_OFFLOAD_GUEST_TSO4),
> > > + á á á á á á á á á á!!(n->curr_offloads &
> > VIRTIO_NET_OFFLOAD_GUEST_TSO6),
> > > + á á á á á á á á á á!!(n->curr_offloads &
> VIRTIO_NET_OFFLOAD_GUEST_ECN),
> > > + á á á á á á á á á á!!(n->curr_offloads &
> > VIRTIO_NET_OFFLOAD_GUEST_UFO));
> > > +}
> > > +
> > > +static uint32_t virtio_net_offloads_by_features(uint32_t features)
> > > +{
> > > + á áuint32_t offloads = 0;
> > > +
> > > + á áif ((1 << VIRTIO_NET_F_GUEST_CSUM) & features) {
> > > + á á á áoffloads |= VIRTIO_NET_OFFLOAD_GUEST_CSUM;
> > > + á á}
> > > +
> > > + á áif ((1 << VIRTIO_NET_F_GUEST_TSO4) & features) {
> > > + á á á áoffloads |= VIRTIO_NET_OFFLOAD_GUEST_TSO4;
> > > + á á}
> > > +
> > > + á áif ((1 << VIRTIO_NET_F_GUEST_TSO6) & features) {
> > > + á á á áoffloads |= VIRTIO_NET_OFFLOAD_GUEST_TSO6;
> > > + á á}
> > > +
> > > + á áif ((1 << VIRTIO_NET_F_GUEST_ECN) & features) {
> > > + á á á áoffloads |= VIRTIO_NET_OFFLOAD_GUEST_ECN;
> > > + á á}
> > > +
> > > + á áif ((1 << VIRTIO_NET_F_GUEST_UFO) & features) {
> > > + á á á áoffloads |= VIRTIO_NET_OFFLOAD_GUEST_UFO;
> > > + á á}
> > > +
> > > + á áreturn offloads;
> > > +}
> > > +
> > > +static inline uint32_t virtio_net_supported_offloads(VirtIONet *n)
> > > +{
> > > + á áreturn virtio_net_offloads_by_features(n->
> vdev.guest_features);
> > > +}
> > > +
> > > ástatic void virtio_net_set_features(VirtIODevice *vdev, uint32_t
> > features)
> > > á{
> > > á á áVirtIONet *n = to_virtio_net(vdev);
> > > @@ -371,12 +413,8 @@ static void virtio_net_set_features
> (VirtIODevice
> > *vdev, uint32_t features)
> > > á á ávirtio_net_set_mrg_rx_bufs(n, !!(features & (1 <<
> > VIRTIO_NET_F_MRG_RXBUF)));
> > >
> > > á á áif (n->has_vnet_hdr) {
> > > - á á á átap_set_offload(qemu_get_subqueue(n->nic, 0)->peer,
> > > - á á á á á á á á á á á á(features >> VIRTIO_NET_F_GUEST_CSUM) & 1,
> > > - á á á á á á á á á á á á(features >> VIRTIO_NET_F_GUEST_TSO4) & 1,
> > > - á á á á á á á á á á á á(features >> VIRTIO_NET_F_GUEST_TSO6) & 1,
> > > - á á á á á á á á á á á á(features >> VIRTIO_NET_F_GUEST_ECN) á& 1,
> > > - á á á á á á á á á á á á(features >> VIRTIO_NET_F_GUEST_UFO) á&
> 1);
> > > + á á á án->curr_offloads = virtio_net_offloads_by_features
> (features);
> > > + á á á ávirtio_net_apply_offloads(n);
> > > á á á}
> > >
> > > á á áfor (i = 0; ái < n->max_queues; i++) {
> > > @@ -422,6 +460,42 @@ static int virtio_net_handle_rx_mode(VirtIONet
> *n,
> > uint8_t cmd,
> > > á á áreturn VIRTIO_NET_OK;
> > > á}
> > >
> > > +static int virtio_net_handle_offloads(VirtIONet *n, uint8_t cmd,
> > > + á á á á á á á á á á á á á á á á á á struct iovec *iov, unsigned
> int
> > iov_cnt)
> > > +{
> > > + á áuint32_t offloads;
> > > + á ásize_t s;
> > > +
> > > + á áif (!((1 << VIRTIO_NET_F_CTRL_GUEST_OFFLD) & n->
> > vdev.guest_features)) {
> > > + á á á áreturn VIRTIO_NET_ERR;
> > > + á á}
> > > +
> > > + á ás = iov_to_buf(iov, iov_cnt, 0, &offloads, sizeof(offloads));
> > > + á áif (s != sizeof(offloads)) {
> > > + á á á áreturn VIRTIO_NET_ERR;
> > > + á á}
> > > +
> > > + á áif (cmd == VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET) {
> > > + á á á áuint32_t supported_offloads;
> > > +
> > > + á á á áif (!n->has_vnet_hdr) {
> > > + á á á á á áreturn VIRTIO_NET_ERR;
> > > + á á á á}
> > > +
> > > + á á á ásupported_offloads = virtio_net_supported_offloads(n);
> > > + á á á áif (offloads & ~supported_offloads) {
> > > + á á á á á áreturn VIRTIO_NET_ERR;
> > > + á á á á}
> > > +
> > > + á á á án->curr_offloads = offloads;
> > > + á á á ávirtio_net_apply_offloads(n);
> > > +
> > > + á á á áreturn VIRTIO_NET_OK;
> > > + á á} else {
> > > + á á á áreturn VIRTIO_NET_ERR;
> > > + á á}
> > > +}
> > > +
> > > ástatic int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> > > á á á á á á á á á á á á á á á á á struct iovec *iov, unsigned int
> > iov_cnt)
> > > á{
> > > @@ -591,6 +665,8 @@ static void virtio_net_handle_ctrl(VirtIODevice
> > *vdev, VirtQueue *vq)
> > > á á á á á á ástatus = virtio_net_handle_vlan_table(n, ctrl.cmd,
> iov,
> > iov_cnt);
> > > á á á á á} else if (ctrl.class == VIRTIO_NET_CTRL_MQ) {
> > > á á á á á á ástatus = virtio_net_handle_mq(n, ctrl.cmd, iov,
> iov_cnt);
> > > + á á á á} else if (ctrl.class == VIRTIO_NET_CTRL_GUEST_OFFLOADS) {
> > > + á á á á á ástatus = virtio_net_handle_offloads(n, ctrl.cmd, iov,
> > iov_cnt);
> > > á á á á á}
> > >
> > > á á á á ás = iov_from_buf(elem.in_sg, elem.in_num, 0, &status,
> sizeof
> > (status));
> > > @@ -1100,6 +1176,10 @@ static void virtio_net_save(QEMUFile *f,
> void
> > *opaque)
> > > á á á á á á áqemu_put_be32(f, n->vqs[i].tx_waiting);
> > > á á á á á}
> > > á á á}
> > > +
> > > + á áif ((1 << VIRTIO_NET_F_CTRL_GUEST_OFFLD) & n->
> vdev.guest_features) {
> > > + á á á áqemu_put_be32(f, n->curr_offloads);
> > > + á á}
> > > á}
> > >
> > > ástatic int virtio_net_load(QEMUFile *f, void *opaque, int
> version_id)
> > > @@ -1156,15 +1236,6 @@ static int virtio_net_load(QEMUFile *f, void
> > *opaque, int version_id)
> > > á á á á á á áerror_report("virtio-net: saved image requires
> vnet_hdr=
> > on");
> > > á á á á á á áreturn -1;
> > > á á á á á}
> > > -
> > > - á á á áif (n->has_vnet_hdr) {
> > > - á á á á á átap_set_offload(qemu_get_queue(n->nic)->peer,
> > > - á á á á á á á á á á(n->vdev.guest_features >>
> VIRTIO_NET_F_GUEST_CSUM)
> > & 1,
> > > - á á á á á á á á á á(n->vdev.guest_features >>
> VIRTIO_NET_F_GUEST_TSO4)
> > & 1,
> > > - á á á á á á á á á á(n->vdev.guest_features >>
> VIRTIO_NET_F_GUEST_TSO6)
> > & 1,
> > > - á á á á á á á á á á(n->vdev.guest_features >>
> VIRTIO_NET_F_GUEST_ECN) á
> > & 1,
> > > - á á á á á á á á á á(n->vdev.guest_features >>
> VIRTIO_NET_F_GUEST_UFO) á
> > & 1);
> > > - á á á á}
> > > á á á}
> > >
> > > á á áif (version_id >= 9) {
> > > @@ -1198,6 +1269,16 @@ static int virtio_net_load(QEMUFile *f, void
> > *opaque, int version_id)
> > > á á á á á}
> > > á á á}
> > >
> > > + á áif ((1 << VIRTIO_NET_F_CTRL_GUEST_OFFLD) & n->
> vdev.guest_features) {
> > > + á á á án->curr_offloads = qemu_get_be32(f);
> > > + á á} else {
> > > + á á á án->curr_offloads = virtio_net_supported_offloads(n);
> > > + á á}
> > > +
> > > + á áif (peer_has_vnet_hdr(n)) {
> > > + á á á ávirtio_net_apply_offloads(n);
> > > + á á}
> > > +
> > > á á ávirtio_net_set_queues(n);
> > >
> > > á á á/* Find the first multicast entry in the saved MAC filter */
> > > diff --git a/hw/virtio-net.h b/hw/virtio-net.h
> > > index 4d1a8cd..002d526 100644
> > > --- a/hw/virtio-net.h
> > > +++ b/hw/virtio-net.h
> > > @@ -27,6 +27,8 @@
> > > á/* The feature bitmap for virtio net */
> > > á#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_CTRL_GUEST_OFFLD 2 /* Control channel offload
> >
> > It's very long anyway, so let's make it the full
> > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS to match the command name.
> >
> > > + á á á á á á á á á á á á á á á á á á á á * configuration support *
> /
> > > á#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. */
> > > @@ -182,6 +184,7 @@ typedef struct VirtIONet {
> > > á á áuint16_t max_queues;
> > > á á áuint16_t curr_queues;
> > > á á ásize_t config_size;
> > > + á áuint32_t curr_offloads;
> > > á} VirtIONet;
> > >
> > > á#define VIRTIO_NET_CTRL_MAC á á1
> > > @@ -221,6 +224,21 @@ struct virtio_net_ctrl_mq {
> > > á #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN á á á á1
> > > á #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX á á á á0x8000
> > >
> > > +/*
> > > + * Control network offloads
> > > + *
> > > + * Dynamic offloads are available with the
> > > + * VIRTIO_NET_F_CTRL_GUEST_OFFLD feature bit.
> > > + */
> > > +#define VIRTIO_NET_CTRL_GUEST_OFFLOADS á 5
> > > + #define VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET á á á á0
> > > +
> > > +#define VIRTIO_NET_OFFLOAD_GUEST_CSUM á á á 1
> > > +#define VIRTIO_NET_OFFLOAD_GUEST_TSO4 á á á 2
> > > +#define VIRTIO_NET_OFFLOAD_GUEST_TSO6 á á á 4
> > > +#define VIRTIO_NET_OFFLOAD_GUEST_ECN á á á á8
> > > +#define VIRTIO_NET_OFFLOAD_GUEST_UFO á á á 16
> > > +
> > > á#define DEFINE_VIRTIO_NET_FEATURES(_state, _field) \
> > > á á á á áDEFINE_VIRTIO_COMMON_FEATURES(_state, _field), \
> > > á á á á áDEFINE_PROP_BIT("csum", _state, _field, VIRTIO_NET_F_CSUM,
> > true), \
> > > @@ -241,6 +259,7 @@ struct virtio_net_ctrl_mq {
> > > á á á á áDEFINE_PROP_BIT("ctrl_vlan", _state, _field,
> > VIRTIO_NET_F_CTRL_VLAN, true), \
> > > á á á á áDEFINE_PROP_BIT("ctrl_rx_extra", _state, _field,
> > VIRTIO_NET_F_CTRL_RX_EXTRA, true), \
> > > á á á á áDEFINE_PROP_BIT("ctrl_mac_addr", _state, _field,
> > VIRTIO_NET_F_CTRL_MAC_ADDR, true), \
> > > + á á á áDEFINE_PROP_BIT("ctrl_gst_offld", _state, _field,
> > VIRTIO_NET_F_CTRL_GUEST_OFFLD, true), \
> > > á á á á áDEFINE_PROP_BIT("mq", _state, _field, VIRTIO_NET_F_MQ,
> false)
> > >
> > > á#endif
> >
> >
> > Looks good. Better to replace ctrl_gst_offld by ctrl_guest_offloads.
> > Also need to patch hw/pc.h with compat macros so that
> > the feature is disabled for old machine types.
> >
> >
> > > --
> > > 1.8.1.4
> >
> >
>
>
next prev parent reply other threads:[~2013-04-02 10:53 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-03-28 10:00 [PATCH 0/2 V2] virtio-spec: dynamic network offloads configuration Dmitry Fleytman
2013-03-28 10:00 ` [PATCH 1/2 V2] virtio-net: " Dmitry Fleytman
2013-03-28 16:39 ` Michael S. Tsirkin
2013-03-30 18:47 ` Dmitry Fleytman
2013-03-31 7:53 ` Michael S. Tsirkin
2013-04-02 6:56 ` Dmitry Fleytman
2013-04-02 10:53 ` Michael S. Tsirkin [this message]
2013-03-28 10:01 ` [PATCH 2/2 V2] virtio-spec: " Dmitry Fleytman
2013-03-28 16:40 ` Michael S. Tsirkin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130402105332.GA21572@redhat.com \
--to=mst@redhat.com \
--cc=dfleytma@redhat.com \
--cc=dmitry@daynix.com \
--cc=virtualization@lists.linux-foundation.org \
--cc=yan@daynix.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.