Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support
On Tue, Apr 14, 2015 at 10:24:38AM +0200, Cornelia Huck wrote:
On Tue, 14 Apr 2015 10:42:56 +0930
Rusty Russell ru...@rustcorp.com.au wrote:
Michael S. Tsirkin m...@redhat.com writes:
On Wed, Apr 01, 2015 at 02:57:35PM +0200, Michael S. Tsirkin wrote:
Virtio 1.0 doesn't include a modern balloon device. At some point we'll
likely
define an incompatible interface with a different ID and different
semantics. But for now, it's not a big effort to support a transitional
balloon device: this has the advantage of supporting existing drivers,
transparently, as well as transports that don't allow mixing virtio 0 and
virtio 1 devices. And balloon is an easy device to test, so it's also
useful
for people to test virtio core handling of transitional devices.
The only interface issue is with the stats buffer, which has misaligned
fields. We could leave it as is, but this sets a bad precedent that
others might copy by mistake.
As we need to change stats code to do byteswaps for virtio 1.0, it seems
easy
to fix by prepending a 6 byte reserved field. I also had to change
config
structure field types from __le32 to __u32 to match other devices. This
means
we need a couple of __force tags for legacy path but that seems minor.
Rusty, what are your thoughts here?
How about merging this for 4.1?
I disagree with making any changes, other than allowing it to be used
with VIRTIO_F_VERSION_1.
However it doesn't seem to bother anyone else, so I've applied it for
4.1.
I'm still not really convinced about the stats change either, FWIW.
Still time to reconsider?
And should we perhaps wait with merging until
the spec change allowing version 1 has been accepted?
That's not how we did this historically: we require all parts
(spec,qemu,linux) to be available, but don't create specific order
between them. In particular, I'd strongly prefer not waiting until 4.2,
that would interfere with putting virtio 1 out to use in the field.
Since both Rusty and Cornelia are against virtio_balloon_stat_modern,
I accept this as the majority decision. And switching
over to __virtio tags found a bug, so I'm convinced now.
---
virtio_balloon: drop virtio_balloon_stat_modern
Looks like we are better off sticking with the misaligned stat struct,
to reduce the amount of virtio 1 specific code in balloon. So let's do
it.
Add a detailed comment to reduce the chance people copy this bad example.
This also fixes a bug on BE architectures: tag should use
cpu_to_le16, not cpu_to_le32.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
diff --git a/include/uapi/linux/virtio_balloon.h
b/include/uapi/linux/virtio_balloon.h
index f81b220..164e0c2 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -52,15 +52,31 @@ struct virtio_balloon_config {
#define VIRTIO_BALLOON_S_MEMTOT 5 /* Total amount of memory */
#define VIRTIO_BALLOON_S_NR 6
+/*
+ * Memory statistics structure.
+ * Driver fills an array of these structures and passes to device.
+ *
+ * NOTE: fields are laid out in a way that would make compiler add padding
+ * between and after fields, so we have to use compiler-specific attributes to
+ * pack it, to disable this padding. This also often causes compiler to
+ * generate suboptimal code.
+ *
+ * We maintain this for backwards compatibility, but don't follow this example.
+ *
+ * Do something like the below instead:
+ * struct virtio_balloon_stat {
+ * __virtio16 tag;
+ * __u8 reserved[6];
+ * __virtio64 val;
+ * };
+ *
+ * In other words, add explicit reserved fields to align field and
+ * structure boundaries at field size, avoiding compiler padding
+ * without the packed attribute.
+ */
struct virtio_balloon_stat {
- __u16 tag;
- __u64 val;
+ __virtio16 tag;
+ __virtio64 val;
} __attribute__((packed));
-struct virtio_balloon_stat_modern {
- __le16 tag;
- __u8 reserved[6];
- __le64 val;
-};
-
#endif /* _LINUX_VIRTIO_BALLOON_H */
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 0583720..9db546e 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -77,10 +77,7 @@ struct virtio_balloon {
/* Memory statistics */
int need_stats_update;
- union {
- struct virtio_balloon_stat_modern stats[VIRTIO_BALLOON_S_NR];
- struct virtio_balloon_stat legacy_stats[VIRTIO_BALLOON_S_NR];
- };
+ struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
/* To register callback in oom notifier call chain */
struct notifier_block nb;
@@ -93,10 +90,7 @@ static struct virtio_device_id id_table[] = {
static void stats_sg_init(struct virtio_balloon *vb, struct scatterlist *sg)
{
- if (virtio_has_feature(vb-vdev, VIRTIO_F_VERSION_1))
- sg_init_one(sg,
Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support
On Tue, 14 Apr 2015 10:42:56 +0930 Rusty Russell ru...@rustcorp.com.au wrote: Michael S. Tsirkin m...@redhat.com writes: On Wed, Apr 01, 2015 at 02:57:35PM +0200, Michael S. Tsirkin wrote: Virtio 1.0 doesn't include a modern balloon device. At some point we'll likely define an incompatible interface with a different ID and different semantics. But for now, it's not a big effort to support a transitional balloon device: this has the advantage of supporting existing drivers, transparently, as well as transports that don't allow mixing virtio 0 and virtio 1 devices. And balloon is an easy device to test, so it's also useful for people to test virtio core handling of transitional devices. The only interface issue is with the stats buffer, which has misaligned fields. We could leave it as is, but this sets a bad precedent that others might copy by mistake. As we need to change stats code to do byteswaps for virtio 1.0, it seems easy to fix by prepending a 6 byte reserved field. I also had to change config structure field types from __le32 to __u32 to match other devices. This means we need a couple of __force tags for legacy path but that seems minor. Rusty, what are your thoughts here? How about merging this for 4.1? I disagree with making any changes, other than allowing it to be used with VIRTIO_F_VERSION_1. However it doesn't seem to bother anyone else, so I've applied it for 4.1. I'm still not really convinced about the stats change either, FWIW. Still time to reconsider? And should we perhaps wait with merging until the spec change allowing version 1 has been accepted? ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support
On Tue, 14 Apr 2015 11:21:11 +0200
Michael S. Tsirkin m...@redhat.com wrote:
diff --git a/include/uapi/linux/virtio_balloon.h
b/include/uapi/linux/virtio_balloon.h
index f81b220..164e0c2 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -52,15 +52,31 @@ struct virtio_balloon_config {
#define VIRTIO_BALLOON_S_MEMTOT 5 /* Total amount of memory */
#define VIRTIO_BALLOON_S_NR 6
+/*
+ * Memory statistics structure.
+ * Driver fills an array of these structures and passes to device.
+ *
+ * NOTE: fields are laid out in a way that would make compiler add padding
+ * between and after fields, so we have to use compiler-specific attributes
to
+ * pack it, to disable this padding. This also often causes compiler to
+ * generate suboptimal code.
+ *
+ * We maintain this for backwards compatibility, but don't follow this
example.
s/this/the existing statistics structure/
+ *
+ * Do something like the below instead:
If you want to implement a similar structure, do...
Just that nobody gets the idea that they are supposed to implement new
balloon statistics ;)
+ * struct virtio_balloon_stat {
+ * __virtio16 tag;
+ * __u8 reserved[6];
+ * __virtio64 val;
+ * };
(...)
@@ -225,13 +219,8 @@ static inline void update_stat(struct virtio_balloon
*vb, int idx,
u16 tag, u64 val)
{
BUG_ON(idx = VIRTIO_BALLOON_S_NR);
- if (virtio_has_feature(vb-vdev, VIRTIO_F_VERSION_1)) {
- vb-stats[idx].tag = cpu_to_le32(tag);
- vb-stats[idx].val = cpu_to_le64(val);
- } else {
- vb-legacy_stats[idx].tag = tag;
- vb-legacy_stats[idx].val = val;
- }
+ vb-stats[idx].tag = cpu_to_virtio16(vb-vdev, tag);
Seems that nobody seemed to care much about statistics...
+ vb-stats[idx].val = cpu_to_virtio64(vb-vdev, val);
}
#define pages_to_bytes(x) ((u64)(x) PAGE_SHIFT)
With these changes merged in:
Acked-by: Cornelia Huck cornelia.h...@de.ibm.com
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support
On Tue, Apr 14, 2015 at 11:50:53AM +0200, Cornelia Huck wrote:
On Tue, 14 Apr 2015 11:21:11 +0200
Michael S. Tsirkin m...@redhat.com wrote:
diff --git a/include/uapi/linux/virtio_balloon.h
b/include/uapi/linux/virtio_balloon.h
index f81b220..164e0c2 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -52,15 +52,31 @@ struct virtio_balloon_config {
#define VIRTIO_BALLOON_S_MEMTOT 5 /* Total amount of memory */
#define VIRTIO_BALLOON_S_NR 6
+/*
+ * Memory statistics structure.
+ * Driver fills an array of these structures and passes to device.
+ *
+ * NOTE: fields are laid out in a way that would make compiler add padding
+ * between and after fields, so we have to use compiler-specific
attributes to
+ * pack it, to disable this padding. This also often causes compiler to
+ * generate suboptimal code.
+ *
+ * We maintain this for backwards compatibility, but don't follow this
example.
s/this/the existing statistics structure/
existing seems redundant. What else? non-existing?
+ *
+ * Do something like the below instead:
If you want to implement a similar structure, do...
Just that nobody gets the idea that they are supposed to implement new
balloon statistics ;)
+ * struct virtio_balloon_stat {
+ * __virtio16 tag;
+ * __u8 reserved[6];
+ * __virtio64 val;
+ * };
(...)
@@ -225,13 +219,8 @@ static inline void update_stat(struct virtio_balloon
*vb, int idx,
u16 tag, u64 val)
{
BUG_ON(idx = VIRTIO_BALLOON_S_NR);
- if (virtio_has_feature(vb-vdev, VIRTIO_F_VERSION_1)) {
- vb-stats[idx].tag = cpu_to_le32(tag);
- vb-stats[idx].val = cpu_to_le64(val);
- } else {
- vb-legacy_stats[idx].tag = tag;
- vb-legacy_stats[idx].val = val;
- }
+ vb-stats[idx].tag = cpu_to_virtio16(vb-vdev, tag);
Seems that nobody seemed to care much about statistics...
Or about BE guests ;)
+ vb-stats[idx].val = cpu_to_virtio64(vb-vdev, val);
}
#define pages_to_bytes(x) ((u64)(x) PAGE_SHIFT)
With these changes merged in:
Acked-by: Cornelia Huck cornelia.h...@de.ibm.com
OK, here's an updated incremental patch: only comment
changed.
diff --git a/include/uapi/linux/virtio_balloon.h
b/include/uapi/linux/virtio_balloon.h
index f81b220..984169a 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -52,15 +52,32 @@ struct virtio_balloon_config {
#define VIRTIO_BALLOON_S_MEMTOT 5 /* Total amount of memory */
#define VIRTIO_BALLOON_S_NR 6
+/*
+ * Memory statistics structure.
+ * Driver fills an array of these structures and passes to device.
+ *
+ * NOTE: fields are laid out in a way that would make compiler add padding
+ * between and after fields, so we have to use compiler-specific attributes to
+ * pack it, to disable this padding. This also often causes compiler to
+ * generate suboptimal code.
+ *
+ * We maintain this statistics structure format for backwards compatibility,
+ * but don't follow this example.
+ *
+ * If implementing a similar structure, do something like the below instead:
+ * struct virtio_balloon_stat {
+ * __virtio16 tag;
+ * __u8 reserved[6];
+ * __virtio64 val;
+ * };
+ *
+ * In other words, add explicit reserved fields to align field and
+ * structure boundaries at field size, avoiding compiler padding
+ * without the packed attribute.
+ */
struct virtio_balloon_stat {
- __u16 tag;
- __u64 val;
+ __virtio16 tag;
+ __virtio64 val;
} __attribute__((packed));
-struct virtio_balloon_stat_modern {
- __le16 tag;
- __u8 reserved[6];
- __le64 val;
-};
-
#endif /* _LINUX_VIRTIO_BALLOON_H */
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 0583720..9db546e 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -77,10 +77,7 @@ struct virtio_balloon {
/* Memory statistics */
int need_stats_update;
- union {
- struct virtio_balloon_stat_modern stats[VIRTIO_BALLOON_S_NR];
- struct virtio_balloon_stat legacy_stats[VIRTIO_BALLOON_S_NR];
- };
+ struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
/* To register callback in oom notifier call chain */
struct notifier_block nb;
@@ -93,10 +90,7 @@ static struct virtio_device_id id_table[] = {
static void stats_sg_init(struct virtio_balloon *vb, struct scatterlist *sg)
{
- if (virtio_has_feature(vb-vdev, VIRTIO_F_VERSION_1))
- sg_init_one(sg, vb-stats, sizeof(vb-stats));
- else
- sg_init_one(sg, vb-legacy_stats, sizeof(vb-legacy_stats));
+ sg_init_one(sg, vb-stats, sizeof(vb-stats));
}
static u32 page_to_balloon_pfn(struct page *page)
@@ -225,13 +219,8 @@
Re: [PATCH] virtio_balloon: drop virtio_balloon_stat_modern
On Tue, 14 Apr 2015 12:01:13 +0200 Michael S. Tsirkin m...@redhat.com wrote: Looks like we are better off sticking with the misaligned stat struct, to reduce the amount of virtio 1 specific code in balloon. So let's do it. Add a detailed comment to reduce the chance people copy this bad example. This also fixes a bug on BE architectures: tag should use cpu_to_le16, not cpu_to_le32. Acked-by: Cornelia Huck cornelia.h...@de.ibm.com Signed-off-by: Michael S. Tsirkin m...@redhat.com --- Just reposting so it's easier to apply. Feel free to squash into previous patch if you think it's neater. +1 for squashing from me. My A-b would also apply to the squashed patch. include/uapi/linux/virtio_balloon.h | 33 + drivers/virtio/virtio_balloon.c | 19 --- 2 files changed, 29 insertions(+), 23 deletions(-) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] virtio_balloon: drop virtio_balloon_stat_modern
Looks like we are better off sticking with the misaligned stat struct,
to reduce the amount of virtio 1 specific code in balloon. So let's do
it.
Add a detailed comment to reduce the chance people copy this bad example.
This also fixes a bug on BE architectures: tag should use
cpu_to_le16, not cpu_to_le32.
Acked-by: Cornelia Huck cornelia.h...@de.ibm.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
Just reposting so it's easier to apply.
Feel free to squash into previous patch if you think it's
neater.
include/uapi/linux/virtio_balloon.h | 33 +
drivers/virtio/virtio_balloon.c | 19 ---
2 files changed, 29 insertions(+), 23 deletions(-)
diff --git a/include/uapi/linux/virtio_balloon.h
b/include/uapi/linux/virtio_balloon.h
index f81b220..984169a 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -52,15 +52,32 @@ struct virtio_balloon_config {
#define VIRTIO_BALLOON_S_MEMTOT 5 /* Total amount of memory */
#define VIRTIO_BALLOON_S_NR 6
+/*
+ * Memory statistics structure.
+ * Driver fills an array of these structures and passes to device.
+ *
+ * NOTE: fields are laid out in a way that would make compiler add padding
+ * between and after fields, so we have to use compiler-specific attributes to
+ * pack it, to disable this padding. This also often causes compiler to
+ * generate suboptimal code.
+ *
+ * We maintain this statistics structure format for backwards compatibility,
+ * but don't follow this example.
+ *
+ * If implementing a similar structure, do something like the below instead:
+ * struct virtio_balloon_stat {
+ * __virtio16 tag;
+ * __u8 reserved[6];
+ * __virtio64 val;
+ * };
+ *
+ * In other words, add explicit reserved fields to align field and
+ * structure boundaries at field size, avoiding compiler padding
+ * without the packed attribute.
+ */
struct virtio_balloon_stat {
- __u16 tag;
- __u64 val;
+ __virtio16 tag;
+ __virtio64 val;
} __attribute__((packed));
-struct virtio_balloon_stat_modern {
- __le16 tag;
- __u8 reserved[6];
- __le64 val;
-};
-
#endif /* _LINUX_VIRTIO_BALLOON_H */
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 0583720..9db546e 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -77,10 +77,7 @@ struct virtio_balloon {
/* Memory statistics */
int need_stats_update;
- union {
- struct virtio_balloon_stat_modern stats[VIRTIO_BALLOON_S_NR];
- struct virtio_balloon_stat legacy_stats[VIRTIO_BALLOON_S_NR];
- };
+ struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
/* To register callback in oom notifier call chain */
struct notifier_block nb;
@@ -93,10 +90,7 @@ static struct virtio_device_id id_table[] = {
static void stats_sg_init(struct virtio_balloon *vb, struct scatterlist *sg)
{
- if (virtio_has_feature(vb-vdev, VIRTIO_F_VERSION_1))
- sg_init_one(sg, vb-stats, sizeof(vb-stats));
- else
- sg_init_one(sg, vb-legacy_stats, sizeof(vb-legacy_stats));
+ sg_init_one(sg, vb-stats, sizeof(vb-stats));
}
static u32 page_to_balloon_pfn(struct page *page)
@@ -225,13 +219,8 @@ static inline void update_stat(struct virtio_balloon *vb,
int idx,
u16 tag, u64 val)
{
BUG_ON(idx = VIRTIO_BALLOON_S_NR);
- if (virtio_has_feature(vb-vdev, VIRTIO_F_VERSION_1)) {
- vb-stats[idx].tag = cpu_to_le32(tag);
- vb-stats[idx].val = cpu_to_le64(val);
- } else {
- vb-legacy_stats[idx].tag = tag;
- vb-legacy_stats[idx].val = val;
- }
+ vb-stats[idx].tag = cpu_to_virtio16(vb-vdev, tag);
+ vb-stats[idx].val = cpu_to_virtio64(vb-vdev, val);
}
#define pages_to_bytes(x) ((u64)(x) PAGE_SHIFT)
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 7/8] vhost: feature to set the vring endianness
On Tue, 14 Apr 2015 16:20:23 +0200
Cornelia Huck cornelia.h...@de.ibm.com wrote:
On Fri, 10 Apr 2015 12:19:16 +0200
Greg Kurz gk...@linux.vnet.ibm.com wrote:
This patch brings cross-endian support to vhost when used to implement
legacy virtio devices. Since it is a relatively rare situation, the
feature availability is controlled by a kernel config option (not set
by default).
The vq-is_le boolean field is added to cache the endianness to be
used for ring accesses. It defaults to native endian, as expected
by legacy virtio devices. When the ring gets active, we force little
endian if the device is modern. When the ring is deactivated, we
revert to the native endian default.
If cross-endian was compiled in, a vq-user_be boolean field is added
so that userspace may request a specific endianness. This field is
used to override the default when activating the ring of a legacy
device. It has no effect on modern devices.
Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com
---
drivers/vhost/Kconfig | 10 ++
drivers/vhost/vhost.c | 76
+++-
drivers/vhost/vhost.h | 12 +--
include/uapi/linux/vhost.h |9 +
4 files changed, 103 insertions(+), 4 deletions(-)
+#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
+static long vhost_set_vring_big_endian(struct vhost_virtqueue *vq,
+ int __user *argp)
+{
+ struct vhost_vring_state s;
+
+ if (vq-private_data)
+ return -EBUSY;
+
+ if (copy_from_user(s, argp, sizeof(s)))
+ return -EFAULT;
+
+ if (s.num s.num != 1)
Checking for s.num 1 might be more obvious at a glance?
Sure since s.num is unsigned.
+ return -EINVAL;
+
+ vq-user_be = s.num;
+
+ return 0;
+}
+
(...)
+#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
+static void vhost_init_is_le(struct vhost_virtqueue *vq)
+{
+ vq-is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq-user_be;
So if the endianness is not explicitly set to BE, it will be forced to
LE (instead of native endian)? Won't that break userspace that does not
yet use the new interface when CONFIG_VHOST_SET_ENDIAN_LEGACY is set?
If userspace doesn't use the new interface, then vq-user_be will retain its
default value that was set in vhost_vq_reset(), i.e. :
vq-user_be = !virtio_legacy_is_little_endian();
This means default is native endian.
What about adding this comment ?
static void vhost_init_is_le(struct vhost_virtqueue *vq)
{
/* Note for legacy virtio: user_be is initialized in vhost_vq_reset()
* according to the host endianness. If userspace does not set an
* explicit endianness, the default behavior is native endian, as
* expected by legacy virtio.
*/
vq-is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq-user_be;
}
+}
+#else
+static void vhost_init_is_le(struct vhost_virtqueue *vq)
+{
+ if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
+ vq-is_le = true;
+}
+#endif
+
int vhost_init_used(struct vhost_virtqueue *vq)
{
__virtio16 last_used_idx;
int r;
- if (!vq-private_data)
+ if (!vq-private_data) {
+ vq-is_le = virtio_legacy_is_little_endian();
return 0;
+ }
+
+ vhost_init_is_le(vq);
r = vhost_update_used_flags(vq);
if (r)
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 7/8] vhost: feature to set the vring endianness
On Fri, 10 Apr 2015 12:19:16 +0200
Greg Kurz gk...@linux.vnet.ibm.com wrote:
This patch brings cross-endian support to vhost when used to implement
legacy virtio devices. Since it is a relatively rare situation, the
feature availability is controlled by a kernel config option (not set
by default).
The vq-is_le boolean field is added to cache the endianness to be
used for ring accesses. It defaults to native endian, as expected
by legacy virtio devices. When the ring gets active, we force little
endian if the device is modern. When the ring is deactivated, we
revert to the native endian default.
If cross-endian was compiled in, a vq-user_be boolean field is added
so that userspace may request a specific endianness. This field is
used to override the default when activating the ring of a legacy
device. It has no effect on modern devices.
Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com
---
drivers/vhost/Kconfig | 10 ++
drivers/vhost/vhost.c | 76
+++-
drivers/vhost/vhost.h | 12 +--
include/uapi/linux/vhost.h |9 +
4 files changed, 103 insertions(+), 4 deletions(-)
+#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
+static long vhost_set_vring_big_endian(struct vhost_virtqueue *vq,
+int __user *argp)
+{
+ struct vhost_vring_state s;
+
+ if (vq-private_data)
+ return -EBUSY;
+
+ if (copy_from_user(s, argp, sizeof(s)))
+ return -EFAULT;
+
+ if (s.num s.num != 1)
Checking for s.num 1 might be more obvious at a glance?
+ return -EINVAL;
+
+ vq-user_be = s.num;
+
+ return 0;
+}
+
(...)
+#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
+static void vhost_init_is_le(struct vhost_virtqueue *vq)
+{
+ vq-is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq-user_be;
So if the endianness is not explicitly set to BE, it will be forced to
LE (instead of native endian)? Won't that break userspace that does not
yet use the new interface when CONFIG_VHOST_SET_ENDIAN_LEGACY is set?
+}
+#else
+static void vhost_init_is_le(struct vhost_virtqueue *vq)
+{
+ if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
+ vq-is_le = true;
+}
+#endif
+
int vhost_init_used(struct vhost_virtqueue *vq)
{
__virtio16 last_used_idx;
int r;
- if (!vq-private_data)
+ if (!vq-private_data) {
+ vq-is_le = virtio_legacy_is_little_endian();
return 0;
+ }
+
+ vhost_init_is_le(vq);
r = vhost_update_used_flags(vq);
if (r)
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 7/8] vhost: feature to set the vring endianness
On Tue, 14 Apr 2015 17:13:52 +0200
Greg Kurz gk...@linux.vnet.ibm.com wrote:
On Tue, 14 Apr 2015 16:20:23 +0200
Cornelia Huck cornelia.h...@de.ibm.com wrote:
On Fri, 10 Apr 2015 12:19:16 +0200
Greg Kurz gk...@linux.vnet.ibm.com wrote:
+#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
+static void vhost_init_is_le(struct vhost_virtqueue *vq)
+{
+ vq-is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq-user_be;
So if the endianness is not explicitly set to BE, it will be forced to
LE (instead of native endian)? Won't that break userspace that does not
yet use the new interface when CONFIG_VHOST_SET_ENDIAN_LEGACY is set?
If userspace doesn't use the new interface, then vq-user_be will retain its
default value that was set in vhost_vq_reset(), i.e. :
vq-user_be = !virtio_legacy_is_little_endian();
This means default is native endian.
What about adding this comment ?
static void vhost_init_is_le(struct vhost_virtqueue *vq)
{
/* Note for legacy virtio: user_be is initialized in vhost_vq_reset()
* according to the host endianness. If userspace does not set an
* explicit endianness, the default behavior is native endian, as
* expected by legacy virtio.
*/
Good idea, as this is easy to miss.
vq-is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq-user_be;
}
+}
+#else
+static void vhost_init_is_le(struct vhost_virtqueue *vq)
+{
+ if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
+ vq-is_le = true;
+}
+#endif
+
int vhost_init_used(struct vhost_virtqueue *vq)
{
__virtio16 last_used_idx;
int r;
- if (!vq-private_data)
+ if (!vq-private_data) {
+ vq-is_le = virtio_legacy_is_little_endian();
return 0;
+ }
+
+ vhost_init_is_le(vq);
r = vhost_update_used_flags(vq);
if (r)
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support
Michael S. Tsirkin m...@redhat.com writes:
On Tue, Apr 14, 2015 at 11:50:53AM +0200, Cornelia Huck wrote:
On Tue, 14 Apr 2015 11:21:11 +0200
Michael S. Tsirkin m...@redhat.com wrote:
diff --git a/include/uapi/linux/virtio_balloon.h
b/include/uapi/linux/virtio_balloon.h
index f81b220..164e0c2 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -52,15 +52,31 @@ struct virtio_balloon_config {
#define VIRTIO_BALLOON_S_MEMTOT 5 /* Total amount of memory */
#define VIRTIO_BALLOON_S_NR 6
+/*
+ * Memory statistics structure.
+ * Driver fills an array of these structures and passes to device.
+ *
+ * NOTE: fields are laid out in a way that would make compiler add padding
+ * between and after fields, so we have to use compiler-specific
attributes to
+ * pack it, to disable this padding. This also often causes compiler to
+ * generate suboptimal code.
+ *
+ * We maintain this for backwards compatibility, but don't follow this
example.
s/this/the existing statistics structure/
existing seems redundant. What else? non-existing?
+ *
+ * Do something like the below instead:
If you want to implement a similar structure, do...
Just that nobody gets the idea that they are supposed to implement new
balloon statistics ;)
+ * struct virtio_balloon_stat {
+ * __virtio16 tag;
+ * __u8 reserved[6];
+ * __virtio64 val;
+ * };
(...)
@@ -225,13 +219,8 @@ static inline void update_stat(struct virtio_balloon
*vb, int idx,
u16 tag, u64 val)
{
BUG_ON(idx = VIRTIO_BALLOON_S_NR);
- if (virtio_has_feature(vb-vdev, VIRTIO_F_VERSION_1)) {
- vb-stats[idx].tag = cpu_to_le32(tag);
- vb-stats[idx].val = cpu_to_le64(val);
- } else {
- vb-legacy_stats[idx].tag = tag;
- vb-legacy_stats[idx].val = val;
- }
+ vb-stats[idx].tag = cpu_to_virtio16(vb-vdev, tag);
Seems that nobody seemed to care much about statistics...
Or about BE guests ;)
+ vb-stats[idx].val = cpu_to_virtio64(vb-vdev, val);
}
#define pages_to_bytes(x) ((u64)(x) PAGE_SHIFT)
With these changes merged in:
Acked-by: Cornelia Huck cornelia.h...@de.ibm.com
OK, here's an updated incremental patch: only comment
changed.
OK, I've merged this with one change:
+static void stats_sg_init(struct virtio_balloon *vb, struct scatterlist *sg)
+{
+ sg_init_one(sg, vb-stats, sizeof(vb-stats));
+}
+
...
- sg_init_one(sg, vb-stats, sizeof(vb-stats));
+ stats_sg_init(vb, sg);
This is no longer a meaningful change, so I removed it.
Here's the final result:
From: Michael S. Tsirkin m...@redhat.com
Subject: virtio_balloon: transitional interface
Virtio 1.0 doesn't include a modern balloon device.
But it's not a big change to support a transitional
balloon device: this has the advantage of supporting
existing drivers, transparently.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
Acked-by: Cornelia Huck cornelia.h...@de.ibm.com
Signed-off-by: Rusty Russell ru...@rustcorp.com.au
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 6a356e344f82..9db546ebe5a1 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -214,8 +219,8 @@ static inline void update_stat(struct virtio_balloon *vb,
int idx,
u16 tag, u64 val)
{
BUG_ON(idx = VIRTIO_BALLOON_S_NR);
- vb-stats[idx].tag = tag;
- vb-stats[idx].val = val;
+ vb-stats[idx].tag = cpu_to_virtio16(vb-vdev, tag);
+ vb-stats[idx].val = cpu_to_virtio64(vb-vdev, val);
}
#define pages_to_bytes(x) ((u64)(x) PAGE_SHIFT)
@@ -283,18 +288,27 @@ static void virtballoon_changed(struct virtio_device
*vdev)
static inline s64 towards_target(struct virtio_balloon *vb)
{
- __le32 v;
s64 target;
+ u32 num_pages;
- virtio_cread(vb-vdev, struct virtio_balloon_config, num_pages, v);
+ virtio_cread(vb-vdev, struct virtio_balloon_config, num_pages,
+num_pages);
- target = le32_to_cpu(v);
+ /* Legacy balloon config space is LE, unlike all other devices. */
+ if (!virtio_has_feature(vb-vdev, VIRTIO_F_VERSION_1))
+ num_pages = le32_to_cpu((__force __le32)num_pages);
+
+ target = num_pages;
return target - vb-num_pages;
}
static void update_balloon_size(struct virtio_balloon *vb)
{
- __le32 actual = cpu_to_le32(vb-num_pages);
+ u32 actual = vb-num_pages;
+
+ /* Legacy balloon config space is LE, unlike all other devices. */
+ if (!virtio_has_feature(vb-vdev, VIRTIO_F_VERSION_1))
+ actual = (__force u32)cpu_to_le32(actual);
virtio_cwrite(vb-vdev, struct virtio_balloon_config, actual,
actual);
diff --git a/include/uapi/linux/virtio_balloon.h
Re: [PATCH] virtio_balloon: drop virtio_balloon_stat_modern
Cornelia Huck cornelia.h...@de.ibm.com writes: On Tue, 14 Apr 2015 12:01:13 +0200 Michael S. Tsirkin m...@redhat.com wrote: Looks like we are better off sticking with the misaligned stat struct, to reduce the amount of virtio 1 specific code in balloon. So let's do it. Add a detailed comment to reduce the chance people copy this bad example. This also fixes a bug on BE architectures: tag should use cpu_to_le16, not cpu_to_le32. Acked-by: Cornelia Huck cornelia.h...@de.ibm.com Signed-off-by: Michael S. Tsirkin m...@redhat.com --- Just reposting so it's easier to apply. Feel free to squash into previous patch if you think it's neater. +1 for squashing from me. My A-b would also apply to the squashed patch. Yes, I already squashed. TBH, I would have squashed the next patches into one too, that simply got rid of the virtio_device_is_legacy_only() function and all callers. Thanks, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
