| Message ID | 20151109174021.3075.94401.stgit@bahia.huguette.org |
|---|---|
| State | New |
| Headers | show |
On Mon, 09 Nov 2015 18:41:33 +0100 Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > When running a fully emulated device in cross-endian conditions, including > a virtio 1.0 device offered to a big endian guest, we need to fix the vnet > headers. This is currently handled by the virtio_net_hdr_swap() function > in the core virtio-net code but it should actually be handled by the net > backend. > > With this patch, virtio-net now tries to configure the backend to do the > endian fixing when the device starts. If the backend cannot support the > requested endiannes, we have to fall back on virtio_net_hdr_swap(): this > is recorded in the needs_vnet_hdr_swap flag. > > The current vhost-net code also tries to configure net backends. This will > be no more needed and will be addressed in a subsequent patch. > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > --- > v2: - introduced virtio_net_needs_hdr_swap() to consolidate the logic in one > place > --- > hw/net/virtio-net.c | 43 ++++++++++++++++++++++++++++++++++++++-- > include/hw/virtio/virtio-net.h | 1 + > 2 files changed, 42 insertions(+), 2 deletions(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index a877614e3e7a..a10f7a0b4d28 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -152,6 +152,33 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) > } > } > > +static int peer_has_vnet_hdr(VirtIONet *n); Hm, you do not seem to use this? > + > +static void virtio_net_vnet_status(VirtIONet *n, uint8_t status) > +{ > + VirtIODevice *vdev = VIRTIO_DEVICE(n); > + NetClientState *peer = qemu_get_queue(n->nic)->peer; > + > + if (virtio_net_started(n, status)) { > + int r; > + > + if (virtio_is_big_endian(vdev)) { > + r = qemu_set_vnet_be(peer, true); > + } else { > + r = qemu_set_vnet_le(peer, true); > + } > + > + n->needs_vnet_hdr_swap = !!r; > + } else if (virtio_net_started(n, vdev->status) && > + !virtio_net_started(n, status)) { > + if (virtio_is_big_endian(vdev)) { > + qemu_set_vnet_be(peer, false); > + } else { > + qemu_set_vnet_le(peer, false); > + } > + } > +} > + > static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status) > { > VirtIONet *n = VIRTIO_NET(vdev); > @@ -159,6 +186,7 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status) > int i; > uint8_t queue_status; > > + virtio_net_vnet_status(n, status); > virtio_net_vhost_status(n, status); > > for (i = 0; i < n->max_queues; i++) { > @@ -949,6 +977,14 @@ static void work_around_broken_dhclient(struct virtio_net_hdr *hdr, > } > } > > +static bool virtio_net_needs_hdr_swap(VirtIONet *n) > +{ > + /* virtio_needs_swap() is constant for fixed endian targets: call it > + * first to filter them out without penalty. What do you mean with 'constant' here? It still needs to retrieve the feature bit from the device, no? > + */ > + return virtio_needs_swap(VIRTIO_DEVICE(n)) && n->needs_vnet_hdr_swap; > +} > + > static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt, > const void *buf, size_t size) > { > @@ -957,7 +993,10 @@ static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt, > void *wbuf = (void *)buf; > work_around_broken_dhclient(wbuf, wbuf + n->host_hdr_len, > size - n->host_hdr_len); > - virtio_net_hdr_swap(VIRTIO_DEVICE(n), wbuf); > + > + if (virtio_net_needs_hdr_swap(n)) { > + virtio_net_hdr_swap(VIRTIO_DEVICE(n), wbuf); > + } > iov_from_buf(iov, iov_cnt, 0, buf, sizeof(struct virtio_net_hdr)); > } else { > struct virtio_net_hdr hdr = { > @@ -1167,7 +1206,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q) > error_report("virtio-net header incorrect"); > exit(1); > } > - if (virtio_needs_swap(vdev)) { > + if (virtio_net_needs_hdr_swap(n)) { > virtio_net_hdr_swap(vdev, (void *) &mhdr); > sg2[0].iov_base = &mhdr; > sg2[0].iov_len = n->guest_hdr_len; > diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h > index f3cc25feca2b..27bc868fbc7d 100644 > --- a/include/hw/virtio/virtio-net.h > +++ b/include/hw/virtio/virtio-net.h > @@ -94,6 +94,7 @@ typedef struct VirtIONet { > uint64_t curr_guest_offloads; > QEMUTimer *announce_timer; > int announce_counter; > + bool needs_vnet_hdr_swap; > } VirtIONet; > > void virtio_net_set_netclient_name(VirtIONet *n, const char *name, > >
On Thu, 12 Nov 2015 18:52:55 +0100 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Mon, 09 Nov 2015 18:41:33 +0100 > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > When running a fully emulated device in cross-endian conditions, including > > a virtio 1.0 device offered to a big endian guest, we need to fix the vnet > > headers. This is currently handled by the virtio_net_hdr_swap() function > > in the core virtio-net code but it should actually be handled by the net > > backend. > > > > With this patch, virtio-net now tries to configure the backend to do the > > endian fixing when the device starts. If the backend cannot support the > > requested endiannes, we have to fall back on virtio_net_hdr_swap(): this > > is recorded in the needs_vnet_hdr_swap flag. > > > > The current vhost-net code also tries to configure net backends. This will > > be no more needed and will be addressed in a subsequent patch. > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > --- > > v2: - introduced virtio_net_needs_hdr_swap() to consolidate the logic in one > > place > > --- > > hw/net/virtio-net.c | 43 ++++++++++++++++++++++++++++++++++++++-- > > include/hw/virtio/virtio-net.h | 1 + > > 2 files changed, 42 insertions(+), 2 deletions(-) > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > > index a877614e3e7a..a10f7a0b4d28 100644 > > --- a/hw/net/virtio-net.c > > +++ b/hw/net/virtio-net.c > > @@ -152,6 +152,33 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) > > } > > } > > > > +static int peer_has_vnet_hdr(VirtIONet *n); > > Hm, you do not seem to use this? > Oops I needed that in a earlier version of this patch and forgot to remove it... > > + > > +static void virtio_net_vnet_status(VirtIONet *n, uint8_t status) > > +{ > > + VirtIODevice *vdev = VIRTIO_DEVICE(n); > > + NetClientState *peer = qemu_get_queue(n->nic)->peer; > > + > > + if (virtio_net_started(n, status)) { > > + int r; > > + > > + if (virtio_is_big_endian(vdev)) { > > + r = qemu_set_vnet_be(peer, true); > > + } else { > > + r = qemu_set_vnet_le(peer, true); > > + } > > + > > + n->needs_vnet_hdr_swap = !!r; > > + } else if (virtio_net_started(n, vdev->status) && > > + !virtio_net_started(n, status)) { > > + if (virtio_is_big_endian(vdev)) { > > + qemu_set_vnet_be(peer, false); > > + } else { > > + qemu_set_vnet_le(peer, false); > > + } > > + } > > +} > > + > > static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status) > > { > > VirtIONet *n = VIRTIO_NET(vdev); > > @@ -159,6 +186,7 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status) > > int i; > > uint8_t queue_status; > > > > + virtio_net_vnet_status(n, status); > > virtio_net_vhost_status(n, status); > > > > for (i = 0; i < n->max_queues; i++) { > > @@ -949,6 +977,14 @@ static void work_around_broken_dhclient(struct virtio_net_hdr *hdr, > > } > > } > > > > +static bool virtio_net_needs_hdr_swap(VirtIONet *n) > > +{ > > + /* virtio_needs_swap() is constant for fixed endian targets: call it > > + * first to filter them out without penalty. > > What do you mean with 'constant' here? It still needs to retrieve the > feature bit from the device, no? > Yes the comment is inaccurate... With the "virtio: cross-endian helpers fixes" series, virtio_needs_swap() indeed resolves to { return 0; } for fixed little endian targets but we still need to check the feature bit when the target is big endian. If I drop the call to virtio_needs_swap(), all targets will have the same penalty. If I keep it, fixed little endian targets have no penalty but fixed big endian targets get an extra check and bi-endian targets get two extra checks... Not sure what to decide... > > + */ > > + return virtio_needs_swap(VIRTIO_DEVICE(n)) && n->needs_vnet_hdr_swap; > > +} > > + > > static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt, > > const void *buf, size_t size) > > { > > @@ -957,7 +993,10 @@ static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt, > > void *wbuf = (void *)buf; > > work_around_broken_dhclient(wbuf, wbuf + n->host_hdr_len, > > size - n->host_hdr_len); > > - virtio_net_hdr_swap(VIRTIO_DEVICE(n), wbuf); > > + > > + if (virtio_net_needs_hdr_swap(n)) { > > + virtio_net_hdr_swap(VIRTIO_DEVICE(n), wbuf); > > + } > > iov_from_buf(iov, iov_cnt, 0, buf, sizeof(struct virtio_net_hdr)); > > } else { > > struct virtio_net_hdr hdr = { > > @@ -1167,7 +1206,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q) > > error_report("virtio-net header incorrect"); > > exit(1); > > } > > - if (virtio_needs_swap(vdev)) { > > + if (virtio_net_needs_hdr_swap(n)) { > > virtio_net_hdr_swap(vdev, (void *) &mhdr); > > sg2[0].iov_base = &mhdr; > > sg2[0].iov_len = n->guest_hdr_len; > > diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h > > index f3cc25feca2b..27bc868fbc7d 100644 > > --- a/include/hw/virtio/virtio-net.h > > +++ b/include/hw/virtio/virtio-net.h > > @@ -94,6 +94,7 @@ typedef struct VirtIONet { > > uint64_t curr_guest_offloads; > > QEMUTimer *announce_timer; > > int announce_counter; > > + bool needs_vnet_hdr_swap; > > } VirtIONet; > > > > void virtio_net_set_netclient_name(VirtIONet *n, const char *name, > > > > >
On Fri, 13 Nov 2015 09:26:26 +0100 Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > On Thu, 12 Nov 2015 18:52:55 +0100 > Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > > On Mon, 09 Nov 2015 18:41:33 +0100 > > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > +static bool virtio_net_needs_hdr_swap(VirtIONet *n) > > > +{ > > > + /* virtio_needs_swap() is constant for fixed endian targets: call it > > > + * first to filter them out without penalty. > > > > What do you mean with 'constant' here? It still needs to retrieve the > > feature bit from the device, no? > > > > Yes the comment is inaccurate... With the "virtio: cross-endian helpers fixes" > series, virtio_needs_swap() indeed resolves to { return 0; } for fixed little > endian targets but we still need to check the feature bit when the target is big > endian. > > If I drop the call to virtio_needs_swap(), all targets will have the same > penalty. If I keep it, fixed little endian targets have no penalty but fixed > big endian targets get an extra check and bi-endian targets get two extra checks... > > Not sure what to decide... The impact probably isn't too large, but improving one configuration via penalizing others feels wrong. Just check the bool value to make the decision?
On Fri, 13 Nov 2015 15:46:06 +0100 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Fri, 13 Nov 2015 09:26:26 +0100 > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > On Thu, 12 Nov 2015 18:52:55 +0100 > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > > > > On Mon, 09 Nov 2015 18:41:33 +0100 > > > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > > > +static bool virtio_net_needs_hdr_swap(VirtIONet *n) > > > > +{ > > > > + /* virtio_needs_swap() is constant for fixed endian targets: call it > > > > + * first to filter them out without penalty. > > > > > > What do you mean with 'constant' here? It still needs to retrieve the > > > feature bit from the device, no? > > > > > > > Yes the comment is inaccurate... With the "virtio: cross-endian helpers fixes" > > series, virtio_needs_swap() indeed resolves to { return 0; } for fixed little > > endian targets but we still need to check the feature bit when the target is big > > endian. > > > > If I drop the call to virtio_needs_swap(), all targets will have the same > > penalty. If I keep it, fixed little endian targets have no penalty but fixed > > big endian targets get an extra check and bi-endian targets get two extra checks... > > > > Not sure what to decide... > > The impact probably isn't too large, but improving one configuration > via penalizing others feels wrong. > > Just check the bool value to make the decision? Sounds fair. I'll drop the call to virtio_needs_swap() then. Thanks. -- Greg
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index a877614e3e7a..a10f7a0b4d28 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -152,6 +152,33 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) } } +static int peer_has_vnet_hdr(VirtIONet *n); + +static void virtio_net_vnet_status(VirtIONet *n, uint8_t status) +{ + VirtIODevice *vdev = VIRTIO_DEVICE(n); + NetClientState *peer = qemu_get_queue(n->nic)->peer; + + if (virtio_net_started(n, status)) { + int r; + + if (virtio_is_big_endian(vdev)) { + r = qemu_set_vnet_be(peer, true); + } else { + r = qemu_set_vnet_le(peer, true); + } + + n->needs_vnet_hdr_swap = !!r; + } else if (virtio_net_started(n, vdev->status) && + !virtio_net_started(n, status)) { + if (virtio_is_big_endian(vdev)) { + qemu_set_vnet_be(peer, false); + } else { + qemu_set_vnet_le(peer, false); + } + } +} + static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status) { VirtIONet *n = VIRTIO_NET(vdev); @@ -159,6 +186,7 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status) int i; uint8_t queue_status; + virtio_net_vnet_status(n, status); virtio_net_vhost_status(n, status); for (i = 0; i < n->max_queues; i++) { @@ -949,6 +977,14 @@ static void work_around_broken_dhclient(struct virtio_net_hdr *hdr, } } +static bool virtio_net_needs_hdr_swap(VirtIONet *n) +{ + /* virtio_needs_swap() is constant for fixed endian targets: call it + * first to filter them out without penalty. + */ + return virtio_needs_swap(VIRTIO_DEVICE(n)) && n->needs_vnet_hdr_swap; +} + static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt, const void *buf, size_t size) { @@ -957,7 +993,10 @@ static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt, void *wbuf = (void *)buf; work_around_broken_dhclient(wbuf, wbuf + n->host_hdr_len, size - n->host_hdr_len); - virtio_net_hdr_swap(VIRTIO_DEVICE(n), wbuf); + + if (virtio_net_needs_hdr_swap(n)) { + virtio_net_hdr_swap(VIRTIO_DEVICE(n), wbuf); + } iov_from_buf(iov, iov_cnt, 0, buf, sizeof(struct virtio_net_hdr)); } else { struct virtio_net_hdr hdr = { @@ -1167,7 +1206,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q) error_report("virtio-net header incorrect"); exit(1); } - if (virtio_needs_swap(vdev)) { + if (virtio_net_needs_hdr_swap(n)) { virtio_net_hdr_swap(vdev, (void *) &mhdr); sg2[0].iov_base = &mhdr; sg2[0].iov_len = n->guest_hdr_len; diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h index f3cc25feca2b..27bc868fbc7d 100644 --- a/include/hw/virtio/virtio-net.h +++ b/include/hw/virtio/virtio-net.h @@ -94,6 +94,7 @@ typedef struct VirtIONet { uint64_t curr_guest_offloads; QEMUTimer *announce_timer; int announce_counter; + bool needs_vnet_hdr_swap; } VirtIONet; void virtio_net_set_netclient_name(VirtIONet *n, const char *name,
When running a fully emulated device in cross-endian conditions, including a virtio 1.0 device offered to a big endian guest, we need to fix the vnet headers. This is currently handled by the virtio_net_hdr_swap() function in the core virtio-net code but it should actually be handled by the net backend. With this patch, virtio-net now tries to configure the backend to do the endian fixing when the device starts. If the backend cannot support the requested endiannes, we have to fall back on virtio_net_hdr_swap(): this is recorded in the needs_vnet_hdr_swap flag. The current vhost-net code also tries to configure net backends. This will be no more needed and will be addressed in a subsequent patch. Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> --- v2: - introduced virtio_net_needs_hdr_swap() to consolidate the logic in one place --- hw/net/virtio-net.c | 43 ++++++++++++++++++++++++++++++++++++++-- include/hw/virtio/virtio-net.h | 1 + 2 files changed, 42 insertions(+), 2 deletions(-)