| Message ID | 20160107113202.10897.33293.stgit@bahia.huguette.org |
|---|---|
| State | New |
| Headers | show |
On 07/01/2016 12:32, Greg Kurz 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, to be used in the TX and RX > paths. > > 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> > --- > hw/net/virtio-net.c | 33 +++++++++++++++++++++++++++++++-- > include/hw/virtio/virtio-net.h | 1 + > 2 files changed, 32 insertions(+), 2 deletions(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index a877614e3e7a..d4cc94ea5e55 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -152,6 +152,31 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) > } > } > > +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)) { Except if I miss something, "!virtio_net_started(n, status)" is always true in the case of "if (virtio_net_started(n, status)) { } else ...". > + 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 +184,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++) { > @@ -957,7 +983,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 (n->needs_vnet_hdr_swap) { > + 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 +1196,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q) > error_report("virtio-net header incorrect"); > exit(1); > } > - if (virtio_needs_swap(vdev)) { > + if (n->needs_vnet_hdr_swap) { > 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, 7 Jan 2016 17:22:34 +0100 Laurent Vivier <lvivier@redhat.com> wrote: > > > On 07/01/2016 12:32, Greg Kurz 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, to be used in the TX and RX > > paths. > > > > 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> > > --- > > hw/net/virtio-net.c | 33 +++++++++++++++++++++++++++++++-- > > include/hw/virtio/virtio-net.h | 1 + > > 2 files changed, 32 insertions(+), 2 deletions(-) > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > > index a877614e3e7a..d4cc94ea5e55 100644 > > --- a/hw/net/virtio-net.c > > +++ b/hw/net/virtio-net.c > > @@ -152,6 +152,31 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) > > } > > } > > > > +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)) { > > Except if I miss something, > > "!virtio_net_started(n, status)" is always true in the case of > "if (virtio_net_started(n, status)) { } else ...". > Of course... I'll fix it. > > + 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 +184,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++) { > > @@ -957,7 +983,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 (n->needs_vnet_hdr_swap) { > > + 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 +1196,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q) > > error_report("virtio-net header incorrect"); > > exit(1); > > } > > - if (virtio_needs_swap(vdev)) { > > + if (n->needs_vnet_hdr_swap) { > > 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 07/01/2016 12:32, Greg Kurz 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, to be used in the TX and RX > paths. > > 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> > --- > hw/net/virtio-net.c | 33 +++++++++++++++++++++++++++++++-- > include/hw/virtio/virtio-net.h | 1 + > 2 files changed, 32 insertions(+), 2 deletions(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index a877614e3e7a..d4cc94ea5e55 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -152,6 +152,31 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) > } > } > > +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); > + } > + } > +} Could you explain why check 'virtio_net_started(n, status)' and then 'virtio_net_started(n, vdev->status)' ? Why qemu_set_vnet_[bl]e() use "true" in the first case and "false" in the second case ? Why don't you store the result (r) in the second case ? > static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status) > { > VirtIONet *n = VIRTIO_NET(vdev); > @@ -159,6 +184,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++) { > @@ -957,7 +983,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 (n->needs_vnet_hdr_swap) { > + 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 +1196,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q) > error_report("virtio-net header incorrect"); > exit(1); > } > - if (virtio_needs_swap(vdev)) { > + if (n->needs_vnet_hdr_swap) { > 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, 7 Jan 2016 19:32:37 +0100 Laurent Vivier <lvivier@redhat.com> wrote: > Sorry for the late answer to this one, I got diverted :) > > On 07/01/2016 12:32, Greg Kurz 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, to be used in the TX and RX > > paths. > > > > 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> > > --- > > hw/net/virtio-net.c | 33 +++++++++++++++++++++++++++++++-- > > include/hw/virtio/virtio-net.h | 1 + > > 2 files changed, 32 insertions(+), 2 deletions(-) > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > > index a877614e3e7a..d4cc94ea5e55 100644 > > --- a/hw/net/virtio-net.c > > +++ b/hw/net/virtio-net.c > > @@ -152,6 +152,31 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) > > } > > } > > > > +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); > > + } > > + } > > +} > > Could you explain why check 'virtio_net_started(n, status)' and then > 'virtio_net_started(n, vdev->status)' ? > Before using the device, we need to inform the network backend about the endianness to use when parsing vnet headers. We do this when the driver activates the device (DRIVER_OK). This is the first check. After using the device, we need to reset the network backend to the default (guest native endianness), otherwise the guest may loose network connectivity if it is rebooted into a different endianness. We do this when the driver deactivates the device (no DRIVER_OK). The second check ensures the device was active before: if we don't check that, the 'else' branch would be executed each time the driver updates the status with something not containing DRIVER_OK... :-\ > Why qemu_set_vnet_[bl]e() use "true" in the first case and "false" in > the second case ? > "true" tells the backend to enforce the corresponding endianness. "false" tells the backed to reset to the default (guest native endianness). > Why don't you store the result (r) in the second case ? > Because @needs_vnet_hdr_swap is only being used when the device is active. Thank you for the time you spent on reviewing this series ! Bonne Annee ! -- Greg > > static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status) > > { > > VirtIONet *n = VIRTIO_NET(vdev); > > @@ -159,6 +184,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++) { > > @@ -957,7 +983,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 (n->needs_vnet_hdr_swap) { > > + 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 +1196,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q) > > error_report("virtio-net header incorrect"); > > exit(1); > > } > > - if (virtio_needs_swap(vdev)) { > > + if (n->needs_vnet_hdr_swap) { > > 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 08/01/2016 15:19, Greg Kurz wrote: > On Thu, 7 Jan 2016 19:32:37 +0100 > Laurent Vivier <lvivier@redhat.com> wrote: >> > > Sorry for the late answer to this one, I got diverted :) > >> >> On 07/01/2016 12:32, Greg Kurz 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, to be used in the TX and RX >>> paths. >>> >>> 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> >>> --- >>> hw/net/virtio-net.c | 33 +++++++++++++++++++++++++++++++-- >>> include/hw/virtio/virtio-net.h | 1 + >>> 2 files changed, 32 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >>> index a877614e3e7a..d4cc94ea5e55 100644 >>> --- a/hw/net/virtio-net.c >>> +++ b/hw/net/virtio-net.c >>> @@ -152,6 +152,31 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) >>> } >>> } >>> >>> +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); >>> + } >>> + } >>> +} >> >> Could you explain why check 'virtio_net_started(n, status)' and then >> 'virtio_net_started(n, vdev->status)' ? >> > > Before using the device, we need to inform the network backend about > the endianness to use when parsing vnet headers. We do this when the > driver activates the device (DRIVER_OK). This is the first check. > > After using the device, we need to reset the network backend to the > default (guest native endianness), otherwise the guest may loose network > connectivity if it is rebooted into a different endianness. We do this > when the driver deactivates the device (no DRIVER_OK). The second check > ensures the device was active before: if we don't check that, the 'else' > branch would be executed each time the driver updates the status with > something not containing DRIVER_OK... :-\ > >> Why qemu_set_vnet_[bl]e() use "true" in the first case and "false" in >> the second case ? >> > > "true" tells the backend to enforce the corresponding endianness. > "false" tells the backed to reset to the default (guest native endianness). > >> Why don't you store the result (r) in the second case ? >> > > Because @needs_vnet_hdr_swap is only being used when the device is active. > > Thank you for the time you spent on reviewing this series ! Thank you for the details, it's clear now. Perhaps this can be added in the commit log or in some comments ? > Bonne Annee ! Bonne Année ;) Laurent > -- > Greg > >>> static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status) >>> { >>> VirtIONet *n = VIRTIO_NET(vdev); >>> @@ -159,6 +184,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++) { >>> @@ -957,7 +983,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 (n->needs_vnet_hdr_swap) { >>> + 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 +1196,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q) >>> error_report("virtio-net header incorrect"); >>> exit(1); >>> } >>> - if (virtio_needs_swap(vdev)) { >>> + if (n->needs_vnet_hdr_swap) { >>> 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, 8 Jan 2016 16:25:18 +0100 Laurent Vivier <lvivier@redhat.com> wrote: > > > On 08/01/2016 15:19, Greg Kurz wrote: > > On Thu, 7 Jan 2016 19:32:37 +0100 > > Laurent Vivier <lvivier@redhat.com> wrote: > >> > > > > Sorry for the late answer to this one, I got diverted :) > > > >> > >> On 07/01/2016 12:32, Greg Kurz 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, to be used in the TX and RX > >>> paths. > >>> > >>> 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> > >>> --- > >>> hw/net/virtio-net.c | 33 +++++++++++++++++++++++++++++++-- > >>> include/hw/virtio/virtio-net.h | 1 + > >>> 2 files changed, 32 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > >>> index a877614e3e7a..d4cc94ea5e55 100644 > >>> --- a/hw/net/virtio-net.c > >>> +++ b/hw/net/virtio-net.c > >>> @@ -152,6 +152,31 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) > >>> } > >>> } > >>> > >>> +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); > >>> + } > >>> + } > >>> +} > >> > >> Could you explain why check 'virtio_net_started(n, status)' and then > >> 'virtio_net_started(n, vdev->status)' ? > >> > > > > Before using the device, we need to inform the network backend about > > the endianness to use when parsing vnet headers. We do this when the > > driver activates the device (DRIVER_OK). This is the first check. > > > > After using the device, we need to reset the network backend to the > > default (guest native endianness), otherwise the guest may loose network > > connectivity if it is rebooted into a different endianness. We do this > > when the driver deactivates the device (no DRIVER_OK). The second check > > ensures the device was active before: if we don't check that, the 'else' > > branch would be executed each time the driver updates the status with > > something not containing DRIVER_OK... :-\ > > > >> Why qemu_set_vnet_[bl]e() use "true" in the first case and "false" in > >> the second case ? > >> > > > > "true" tells the backend to enforce the corresponding endianness. > > "false" tells the backed to reset to the default (guest native endianness). > > > >> Why don't you store the result (r) in the second case ? > >> > > > > Because @needs_vnet_hdr_swap is only being used when the device is active. > > > > Thank you for the time you spent on reviewing this series ! > > Thank you for the details, it's clear now. > Perhaps this can be added in the commit log or in some comments ? > I realized when writing the mail that this is non-trivial indeed. I'm currently adding comments and updating the changelog :) > > Bonne Annee ! > > Bonne Année ;) > Laurent > > > -- > > Greg > > > >>> static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status) > >>> { > >>> VirtIONet *n = VIRTIO_NET(vdev); > >>> @@ -159,6 +184,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++) { > >>> @@ -957,7 +983,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 (n->needs_vnet_hdr_swap) { > >>> + 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 +1196,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q) > >>> error_report("virtio-net header incorrect"); > >>> exit(1); > >>> } > >>> - if (virtio_needs_swap(vdev)) { > >>> + if (n->needs_vnet_hdr_swap) { > >>> 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, > >>> > >>> > >> > > >
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index a877614e3e7a..d4cc94ea5e55 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -152,6 +152,31 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) } } +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 +184,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++) { @@ -957,7 +983,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 (n->needs_vnet_hdr_swap) { + 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 +1196,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q) error_report("virtio-net header incorrect"); exit(1); } - if (virtio_needs_swap(vdev)) { + if (n->needs_vnet_hdr_swap) { 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, to be used in the TX and RX paths. 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> --- hw/net/virtio-net.c | 33 +++++++++++++++++++++++++++++++-- include/hw/virtio/virtio-net.h | 1 + 2 files changed, 32 insertions(+), 2 deletions(-)