From: Greg Kurz <hidden> Date: 2016-01-13 17:09:43
This series is a respin of the following patch:
http://patchwork.ozlabs.org/patch/565921/
Patch 1 is preliminary work: it gives better names to the helpers that are
involved in cross-endian support.
Patch 2 is actually a v2 of the original patch. All devices now call a
helper in the generic code, which DTRT according to vq->private_data, as
suggested by Michael.
---
Greg Kurz (2):
vhost: helpers to enable/disable vring endianness
vhost: disentangle vring endianness stuff from the core code
drivers/vhost/net.c | 3 +++
drivers/vhost/scsi.c | 3 +++
drivers/vhost/test.c | 2 ++
drivers/vhost/vhost.c | 40 ++++++++++++++++++++++++++++------------
drivers/vhost/vhost.h | 1 +
5 files changed, 37 insertions(+), 12 deletions(-)
From: Greg Kurz <hidden> Date: 2016-01-13 17:09:50
The default use case for vhost is when the host and the vring have the
same endianness (default native endianness). But there are cases where
they differ and vhost should byteswap when accessing the vring:
- the host is big endian and the vring comes from a virtio 1.0 device
which is always little endian
- the architecture is bi-endian and the vring comes from a legacy virtio
device with a different endianness than the endianness of the host (aka
legacy cross-endian)
These cases are handled by the vq->is_le and the optional vq->user_be,
with the following logic:
- if none of the fields is enabled, vhost access the vring without byteswap
- if the vring is virtio 1.0 and the host is big endian, vq->is_le is
enabled to enforce little endian access to the vring
- if the vring is legacy cross-endian, userspace enables vq->user_be
to inform vhost about the vring endianness. This endianness is then
enforced for vring accesses through vq->is_le again
The logic is unclear in the current code.
This patch introduces helpers with explicit enable and disable semantics,
for better clarity.
No behaviour change.
Signed-off-by: Greg Kurz <redacted>
---
drivers/vhost/vhost.c | 28 +++++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
On Wed, 13 Jan 2016 18:09:41 +0100
Greg Kurz [off-list ref] wrote:
The default use case for vhost is when the host and the vring have the
same endianness (default native endianness). But there are cases where
they differ and vhost should byteswap when accessing the vring:
- the host is big endian and the vring comes from a virtio 1.0 device
which is always little endian
- the architecture is bi-endian and the vring comes from a legacy virtio
device with a different endianness than the endianness of the host (aka
legacy cross-endian)
These cases are handled by the vq->is_le and the optional vq->user_be,
with the following logic:
- if none of the fields is enabled, vhost access the vring without byteswap
- if the vring is virtio 1.0 and the host is big endian, vq->is_le is
enabled to enforce little endian access to the vring
- if the vring is legacy cross-endian, userspace enables vq->user_be
to inform vhost about the vring endianness. This endianness is then
enforced for vring accesses through vq->is_le again
The logic is unclear in the current code.
This patch introduces helpers with explicit enable and disable semantics,
for better clarity.
No behaviour change.
Signed-off-by: Greg Kurz <redacted>
---
drivers/vhost/vhost.c | 28 +++++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-02-10 11:21:28
On Wed, Jan 13, 2016 at 06:09:41PM +0100, Greg Kurz wrote:
quoted hunk
The default use case for vhost is when the host and the vring have the
same endianness (default native endianness). But there are cases where
they differ and vhost should byteswap when accessing the vring:
- the host is big endian and the vring comes from a virtio 1.0 device
which is always little endian
- the architecture is bi-endian and the vring comes from a legacy virtio
device with a different endianness than the endianness of the host (aka
legacy cross-endian)
These cases are handled by the vq->is_le and the optional vq->user_be,
with the following logic:
- if none of the fields is enabled, vhost access the vring without byteswap
- if the vring is virtio 1.0 and the host is big endian, vq->is_le is
enabled to enforce little endian access to the vring
- if the vring is legacy cross-endian, userspace enables vq->user_be
to inform vhost about the vring endianness. This endianness is then
enforced for vring accesses through vq->is_le again
The logic is unclear in the current code.
This patch introduces helpers with explicit enable and disable semantics,
for better clarity.
No behaviour change.
Signed-off-by: Greg Kurz <redacted>
---
drivers/vhost/vhost.c | 28 +++++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
@@ -81,7 +86,7 @@ static long vhost_get_vring_endian(struct vhost_virtqueue *vq, u32 idx, return 0; }-static void vhost_init_is_le(struct vhost_virtqueue *vq)+static void vhost_enable_is_le(struct vhost_virtqueue *vq) { /* Note for legacy virtio: user_be is initialized at reset time * according to the host endianness. If userspace does not set an
From: Greg Kurz <hidden> Date: 2016-02-10 12:11:55
On Wed, 10 Feb 2016 13:21:22 +0200
"Michael S. Tsirkin" [off-list ref] wrote:
On Wed, Jan 13, 2016 at 06:09:41PM +0100, Greg Kurz wrote:
quoted
The default use case for vhost is when the host and the vring have the
same endianness (default native endianness). But there are cases where
they differ and vhost should byteswap when accessing the vring:
- the host is big endian and the vring comes from a virtio 1.0 device
which is always little endian
- the architecture is bi-endian and the vring comes from a legacy virtio
device with a different endianness than the endianness of the host (aka
legacy cross-endian)
These cases are handled by the vq->is_le and the optional vq->user_be,
with the following logic:
- if none of the fields is enabled, vhost access the vring without byteswap
- if the vring is virtio 1.0 and the host is big endian, vq->is_le is
enabled to enforce little endian access to the vring
- if the vring is legacy cross-endian, userspace enables vq->user_be
to inform vhost about the vring endianness. This endianness is then
enforced for vring accesses through vq->is_le again
The logic is unclear in the current code.
This patch introduces helpers with explicit enable and disable semantics,
for better clarity.
No behaviour change.
Signed-off-by: Greg Kurz <redacted>
---
drivers/vhost/vhost.c | 28 +++++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
Hmm this doesn't look like an improvement to me.
What does it mean to disable big endian? Make it little endian?
The default behavior for the device is native endian.
The SET_VRING_ENDIAN ioctl is used to make the device big endian
on little endian hosts, hence "enabling" cross-endian mode...
@@ -81,7 +86,7 @@ static long vhost_get_vring_endian(struct vhost_virtqueue *vq, u32 idx, return 0; }-static void vhost_init_is_le(struct vhost_virtqueue *vq)+static void vhost_enable_is_le(struct vhost_virtqueue *vq) { /* Note for legacy virtio: user_be is initialized at reset time * according to the host endianness. If userspace does not set an
Hmm this doesn't look like an improvement to me.
What does it mean to disable big endian? Make it little endian?
The default behavior for the device is native endian.
The SET_VRING_ENDIAN ioctl is used to make the device big endian
on little endian hosts, hence "enabling" cross-endian mode...
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-02-10 15:08:57
On Wed, Feb 10, 2016 at 01:11:34PM +0100, Greg Kurz wrote:
On Wed, 10 Feb 2016 13:21:22 +0200
"Michael S. Tsirkin" [off-list ref] wrote:
quoted
On Wed, Jan 13, 2016 at 06:09:41PM +0100, Greg Kurz wrote:
quoted
The default use case for vhost is when the host and the vring have the
same endianness (default native endianness). But there are cases where
they differ and vhost should byteswap when accessing the vring:
- the host is big endian and the vring comes from a virtio 1.0 device
which is always little endian
- the architecture is bi-endian and the vring comes from a legacy virtio
device with a different endianness than the endianness of the host (aka
legacy cross-endian)
These cases are handled by the vq->is_le and the optional vq->user_be,
with the following logic:
- if none of the fields is enabled, vhost access the vring without byteswap
- if the vring is virtio 1.0 and the host is big endian, vq->is_le is
enabled to enforce little endian access to the vring
- if the vring is legacy cross-endian, userspace enables vq->user_be
to inform vhost about the vring endianness. This endianness is then
enforced for vring accesses through vq->is_le again
The logic is unclear in the current code.
This patch introduces helpers with explicit enable and disable semantics,
for better clarity.
No behaviour change.
Signed-off-by: Greg Kurz <redacted>
---
drivers/vhost/vhost.c | 28 +++++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
Hmm this doesn't look like an improvement to me.
What does it mean to disable big endian? Make it little endian?
The default behavior for the device is native endian.
The SET_VRING_ENDIAN ioctl is used to make the device big endian
on little endian hosts, hence "enabling" cross-endian mode...
quoted
Existing reset seems to make sense.
... and we "disable" cross-endian mode on reset.
OK but that's enable cross-endian. Not enable be. We could have
something like vhost_disable_user_byte_swap though each time we try,
the result makes my head hurt: "swap" is a relative thing and hard to
keep track of.
@@ -81,7 +86,7 @@ static long vhost_get_vring_endian(struct vhost_virtqueue *vq, u32 idx, return 0; }-static void vhost_init_is_le(struct vhost_virtqueue *vq)+static void vhost_enable_is_le(struct vhost_virtqueue *vq) { /* Note for legacy virtio: user_be is initialized at reset time * according to the host endianness. If userspace does not set an
From: Greg Kurz <hidden> Date: 2016-02-10 15:28:29
On Wed, 10 Feb 2016 17:08:52 +0200
"Michael S. Tsirkin" [off-list ref] wrote:
On Wed, Feb 10, 2016 at 01:11:34PM +0100, Greg Kurz wrote:
quoted
On Wed, 10 Feb 2016 13:21:22 +0200
"Michael S. Tsirkin" [off-list ref] wrote:
quoted
On Wed, Jan 13, 2016 at 06:09:41PM +0100, Greg Kurz wrote:
quoted
The default use case for vhost is when the host and the vring have the
same endianness (default native endianness). But there are cases where
they differ and vhost should byteswap when accessing the vring:
- the host is big endian and the vring comes from a virtio 1.0 device
which is always little endian
- the architecture is bi-endian and the vring comes from a legacy virtio
device with a different endianness than the endianness of the host (aka
legacy cross-endian)
These cases are handled by the vq->is_le and the optional vq->user_be,
with the following logic:
- if none of the fields is enabled, vhost access the vring without byteswap
- if the vring is virtio 1.0 and the host is big endian, vq->is_le is
enabled to enforce little endian access to the vring
- if the vring is legacy cross-endian, userspace enables vq->user_be
to inform vhost about the vring endianness. This endianness is then
enforced for vring accesses through vq->is_le again
The logic is unclear in the current code.
This patch introduces helpers with explicit enable and disable semantics,
for better clarity.
No behaviour change.
Signed-off-by: Greg Kurz <redacted>
---
drivers/vhost/vhost.c | 28 +++++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
Hmm this doesn't look like an improvement to me.
What does it mean to disable big endian? Make it little endian?
The default behavior for the device is native endian.
The SET_VRING_ENDIAN ioctl is used to make the device big endian
on little endian hosts, hence "enabling" cross-endian mode...
quoted
Existing reset seems to make sense.
... and we "disable" cross-endian mode on reset.
OK but that's enable cross-endian. Not enable be. We could have
something like vhost_disable_user_byte_swap though each time we try,
the result makes my head hurt: "swap" is a relative thing and hard to
keep track of.
What about having something like below then ?
vhost_enable_cross_endian_big() to be called on little endian hosts with big endian devices
vhost_enable_cross_endian_little() to be called on big endian hosts with little endian devices
vhost_disable_cross_endian() to go back to the native endian default
@@ -81,7 +86,7 @@ static long vhost_get_vring_endian(struct vhost_virtqueue *vq, u32 idx, return 0; }-static void vhost_init_is_le(struct vhost_virtqueue *vq)+static void vhost_enable_is_le(struct vhost_virtqueue *vq) { /* Note for legacy virtio: user_be is initialized at reset time * according to the host endianness. If userspace does not set an
From: Greg Kurz <hidden> Date: 2016-01-13 17:09:55
The way vring endianness is being handled currently obfuscates
the code in vhost_init_used().
This patch tries to fix that by doing the following:
- move the the code that adjusts endianness to a dedicated helper
- export this helper so that backends explicitely call it
No behaviour change.
Signed-off-by: Greg Kurz <redacted>
---
drivers/vhost/net.c | 3 +++
drivers/vhost/scsi.c | 3 +++
drivers/vhost/test.c | 2 ++
drivers/vhost/vhost.c | 16 +++++++++++-----
drivers/vhost/vhost.h | 1 +
5 files changed, 20 insertions(+), 5 deletions(-)
On Wed, 13 Jan 2016 18:09:47 +0100
Greg Kurz [off-list ref] wrote:
The way vring endianness is being handled currently obfuscates
the code in vhost_init_used().
This patch tries to fix that by doing the following:
- move the the code that adjusts endianness to a dedicated helper
- export this helper so that backends explicitely call it
No behaviour change.
Signed-off-by: Greg Kurz <redacted>
---
drivers/vhost/net.c | 3 +++
drivers/vhost/scsi.c | 3 +++
drivers/vhost/test.c | 2 ++
drivers/vhost/vhost.c | 16 +++++++++++-----
drivers/vhost/vhost.h | 1 +
5 files changed, 20 insertions(+), 5 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-02-10 11:48:09
On Wed, Jan 13, 2016 at 06:09:47PM +0100, Greg Kurz wrote:
quoted hunk
The way vring endianness is being handled currently obfuscates
the code in vhost_init_used().
This patch tries to fix that by doing the following:
- move the the code that adjusts endianness to a dedicated helper
- export this helper so that backends explicitely call it
No behaviour change.
Signed-off-by: Greg Kurz <redacted>
---
drivers/vhost/net.c | 3 +++
drivers/vhost/scsi.c | 3 +++
drivers/vhost/test.c | 2 ++
drivers/vhost/vhost.c | 16 +++++++++++-----
drivers/vhost/vhost.h | 1 +
5 files changed, 20 insertions(+), 5 deletions(-)
I'd prefer "vhost_update_is_le" here. "endian" might also mean
"user_be". But see below pls.
quoted hunk
@@ -1166,12 +1175,9 @@ int vhost_init_used(struct vhost_virtqueue *vq) { __virtio16 last_used_idx; int r;- if (!vq->private_data) {- vhost_disable_is_le(vq);- return 0;- }- vhost_enable_is_le(vq);+ if (!vq->private_data)+ return 0; r = vhost_update_used_flags(vq); if (r)
Looking at how callers use this, maybe we should just rename init_used
to vhost_vq_init_access. The _used suffix was a hint that we
access the vq used ring. But maybe what callers care about is
that it must be called after access_ok.
From: Greg Kurz <hidden> Date: 2016-01-27 10:25:43
On Wed, 13 Jan 2016 18:09:34 +0100
Greg Kurz [off-list ref] wrote:
This series is a respin of the following patch:
http://patchwork.ozlabs.org/patch/565921/
Patch 1 is preliminary work: it gives better names to the helpers that are
involved in cross-endian support.
Patch 2 is actually a v2 of the original patch. All devices now call a
helper in the generic code, which DTRT according to vq->private_data, as
suggested by Michael.
Hi Michael,
This is just a friendly reminder for this series, which was
reviewed by Cornelia already.
Thanks.
--
Greg