Re: [PATCH v2 1/6] virtio_balloon: transitional interface

2015-04-01 Thread Michael S. Tsirkin 
On Wed, Apr 01, 2015 at 02:17:23PM +1030, Rusty Russell wrote:
 I would leave the device *exactly* as is, ugly structure packing and
 all.

But why?  It's going to be used for years, might as well make it clean?

-- 
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/6] virtio_balloon: transitional interface

2015-04-01 Thread Michael S. Tsirkin 
On Wed, Apr 01, 2015 at 07:53:14PM +1030, Rusty Russell wrote:
 Michael S. Tsirkin m...@redhat.com writes:
  On Wed, Apr 01, 2015 at 02:17:23PM +1030, Rusty Russell wrote:
  I would leave the device *exactly* as is, ugly structure packing and
  all.
 
  But why?  It's going to be used for years, might as well make it clean?
 
 Because the only spec which currently exists says to do that.

OK but the only spec which currently exists also says it's a legacy only
device, so driver must not set VERSION_1.  So surely, we can make minor
changes when VERSION_1 is set, like we did for other devices.

Let me post the latest patches I'm working on,
see what you think then.

  We do
 need a new virtio memballoon spec, but it'll look nothing like this
 anyway.
 
 Cheers,
 Rusty.

I think it's going to have significantly different semantics, too,
so not much value in making that one work with current
drivers, right?

-- 
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/6] virtio_balloon: transitional interface

2015-04-01 Thread Cornelia Huck 
On Wed, 1 Apr 2015 11:43:46 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 On Wed, Apr 01, 2015 at 07:53:14PM +1030, Rusty Russell wrote:
  Michael S. Tsirkin m...@redhat.com writes:
   On Wed, Apr 01, 2015 at 02:17:23PM +1030, Rusty Russell wrote:
   I would leave the device *exactly* as is, ugly structure packing and
   all.
  
   But why?  It's going to be used for years, might as well make it clean?
  
  Because the only spec which currently exists says to do that.
 
 OK but the only spec which currently exists also says it's a legacy only
 device, so driver must not set VERSION_1.  So surely, we can make minor
 changes when VERSION_1 is set, like we did for other devices.

But we don't plan to replace the other devices, so it makes sense to do
some changes for 1.0.

 
 Let me post the latest patches I'm working on,
 see what you think then.
 
   We do
  need a new virtio memballoon spec, but it'll look nothing like this
  anyway.
  
  Cheers,
  Rusty.
 
 I think it's going to have significantly different semantics, too,
 so not much value in making that one work with current
 drivers, right?
 

So why not just keep virtio-balloon as-is and just specify endianness
etc. for 1.0? Keeps the old drivers going without hacks, and we can
start with a fresh driver for the new virtio-balloon.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/6] virtio_balloon: transitional interface

2015-04-01 Thread Michael S. Tsirkin 
On Wed, Apr 01, 2015 at 12:22:44PM +0200, Cornelia Huck wrote:
 On Wed, 1 Apr 2015 11:43:46 +0200
 Michael S. Tsirkin m...@redhat.com wrote:
 
  On Wed, Apr 01, 2015 at 07:53:14PM +1030, Rusty Russell wrote:
   Michael S. Tsirkin m...@redhat.com writes:
On Wed, Apr 01, 2015 at 02:17:23PM +1030, Rusty Russell wrote:
I would leave the device *exactly* as is, ugly structure packing and
all.
   
But why?  It's going to be used for years, might as well make it clean?
   
   Because the only spec which currently exists says to do that.
  
  OK but the only spec which currently exists also says it's a legacy only
  device, so driver must not set VERSION_1.  So surely, we can make minor
  changes when VERSION_1 is set, like we did for other devices.
 
 But we don't plan to replace the other devices, so it makes sense to do
 some changes for 1.0.

I'm not sure what the above says. Do you agree with
making minor changes in device behaviour?
Also to be clear, I think this is 1.1 material.

  
  Let me post the latest patches I'm working on,
  see what you think then.
  
We do
   need a new virtio memballoon spec, but it'll look nothing like this
   anyway.
   
   Cheers,
   Rusty.
  
  I think it's going to have significantly different semantics, too,
  so not much value in making that one work with current
  drivers, right?
  
 
 So why not just keep virtio-balloon as-is and just specify endianness
 etc. for 1.0? Keeps the old drivers going without hacks,
 and we can
 start with a fresh driver for the new virtio-balloon.

Well it doesn't really, we need cpu_to_virtio in a bunch of
places anyway.

So I kind of prefer making it clean, even just to avoid setting a bad
example for other devices.

Let me post the new patch where it's all fixed in a cleaner way, and
everyone can discuss whether it's too much work.

-- 
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/6] virtio_balloon: transitional interface

2015-04-01 Thread Michael S. Tsirkin 
On Wed, Apr 01, 2015 at 12:57:48PM +0200, Cornelia Huck wrote:
 On Wed, 1 Apr 2015 12:28:30 +0200
 Michael S. Tsirkin m...@redhat.com wrote:
 
  On Wed, Apr 01, 2015 at 12:22:44PM +0200, Cornelia Huck wrote:
   On Wed, 1 Apr 2015 11:43:46 +0200
   Michael S. Tsirkin m...@redhat.com wrote:
   
On Wed, Apr 01, 2015 at 07:53:14PM +1030, Rusty Russell wrote:
 Michael S. Tsirkin m...@redhat.com writes:
  On Wed, Apr 01, 2015 at 02:17:23PM +1030, Rusty Russell wrote:
  I would leave the device *exactly* as is, ugly structure packing 
  and
  all.
 
  But why?  It's going to be used for years, might as well make it 
  clean?
 
 Because the only spec which currently exists says to do that.

OK but the only spec which currently exists also says it's a legacy only
device, so driver must not set VERSION_1.  So surely, we can make minor
changes when VERSION_1 is set, like we did for other devices.
   
   But we don't plan to replace the other devices, so it makes sense to do
   some changes for 1.0.
  
  I'm not sure what the above says. Do you agree with
  making minor changes in device behaviour?
 
 The other way around.
 
  Also to be clear, I think this is 1.1 material.
 
 Btw, I'd really like to see your proposed spec updates.

Just sent.

  

Let me post the latest patches I'm working on,
see what you think then.

  We do
 need a new virtio memballoon spec, but it'll look nothing like this
 anyway.
 
 Cheers,
 Rusty.

I think it's going to have significantly different semantics, too,
so not much value in making that one work with current
drivers, right?

   
   So why not just keep virtio-balloon as-is and just specify endianness
   etc. for 1.0? Keeps the old drivers going without hacks,
   and we can
   start with a fresh driver for the new virtio-balloon.
  
  Well it doesn't really, we need cpu_to_virtio in a bunch of
  places anyway.
 
 Of course, but what about keeping changes minimal?
  
  So I kind of prefer making it clean, even just to avoid setting a bad
  example for other devices.
  
  Let me post the new patch where it's all fixed in a cleaner way, and
  everyone can discuss whether it's too much work.
  
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/6] virtio_balloon: transitional interface

2015-03-31 Thread Rusty Russell 
Michael S. Tsirkin m...@redhat.com writes:
 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.

You decided to fix the packed struct...

 diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
 index 6a356e3..574267f 100644
 --- a/drivers/virtio/virtio_balloon.c
 +++ b/drivers/virtio/virtio_balloon.c
 @@ -77,7 +77,7 @@ struct virtio_balloon {
  
   /* Memory statistics */
   int need_stats_update;
 - struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
 + struct virtio_balloon_stat_modern stats[VIRTIO_BALLOON_S_NR];
  
   /* To register callback in oom notifier call chain */
   struct notifier_block nb;
 @@ -269,7 +269,11 @@ static void stats_handle_request(struct virtio_balloon 
 *vb)
   vq = vb-stats_vq;
   if (!virtqueue_get_buf(vq, len))
   return;
 - sg_init_one(sg, vb-stats, sizeof(vb-stats));
 + if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
 + sg_init_one(sg, vb-stats, sizeof(vb-stats));
 + else
 + sg_init_one(sg, vb-stats-tag, sizeof(vb-stats) -
 + offsetof(typeof(*vb-stats, tag);

This makes it compile, but definitely won't work.

   virtqueue_add_outbuf(vq, sg, 1, vb, GFP_KERNEL);
   virtqueue_kick(vq);
  }
 @@ -283,21 +287,30 @@ 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(num_pages);
  
 - virtio_cwrite(vb-vdev, struct virtio_balloon_config, actual,
 -   actual);
 + virtio_cwrite(vb-vdev, struct virtio_balloon_config,
 +   actual, actual);
  }

Final line is gratitous reformatting.

I would leave the device *exactly* as is, ugly structure packing and
all.

Cheers,
Rusty.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v2 1/6] virtio_balloon: transitional interface

2015-03-31 Thread Michael S. Tsirkin 
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
---
 include/uapi/linux/virtio_balloon.h | 11 +--
 drivers/virtio/virtio_balloon.c | 29 +
 2 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/include/uapi/linux/virtio_balloon.h 
b/include/uapi/linux/virtio_balloon.h
index 4b0488f..71ef93b 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -38,9 +38,9 @@
 
 struct virtio_balloon_config {
/* Number of pages host wants Guest to give up. */
-   __le32 num_pages;
+   __u32 num_pages;
/* Number of pages we've actually got in balloon. */
-   __le32 actual;
+   __u32 actual;
 };
 
 #define VIRTIO_BALLOON_S_SWAP_IN  0   /* Amount of memory swapped in */
@@ -51,9 +51,16 @@ struct virtio_balloon_config {
 #define VIRTIO_BALLOON_S_MEMTOT   5   /* Total amount of memory */
 #define VIRTIO_BALLOON_S_NR   6
 
+/* Legacy stat structure. We keep it around to avoid breaking old userspace. */
 struct virtio_balloon_stat {
__u16 tag;
__u64 val;
 } __attribute__((packed));
 
+struct virtio_balloon_stat_modern {
+   __u8 reserved[6];
+   __virtio16 tag;
+   __virtio64 val;
+};
+
 #endif /* _LINUX_VIRTIO_BALLOON_H */
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 6a356e3..574267f 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -77,7 +77,7 @@ struct virtio_balloon {
 
/* Memory statistics */
int need_stats_update;
-   struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
+   struct virtio_balloon_stat_modern stats[VIRTIO_BALLOON_S_NR];
 
/* To register callback in oom notifier call chain */
struct notifier_block nb;
@@ -269,7 +269,11 @@ static void stats_handle_request(struct virtio_balloon *vb)
vq = vb-stats_vq;
if (!virtqueue_get_buf(vq, len))
return;
-   sg_init_one(sg, vb-stats, sizeof(vb-stats));
+   if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
+   sg_init_one(sg, vb-stats, sizeof(vb-stats));
+   else
+   sg_init_one(sg, vb-stats-tag, sizeof(vb-stats) -
+   offsetof(typeof(*vb-stats, tag);
virtqueue_add_outbuf(vq, sg, 1, vb, GFP_KERNEL);
virtqueue_kick(vq);
 }
@@ -283,21 +287,30 @@ 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(num_pages);
 
-   virtio_cwrite(vb-vdev, struct virtio_balloon_config, actual,
- actual);
+   virtio_cwrite(vb-vdev, struct virtio_balloon_config,
+ actual, actual);
 }
 
 /*
-- 
MST

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization