[PATCH v2 0/6] virtio_balloon: virtio 1 support

[PATCH v2 0/6] virtio_balloon: virtio 1 support

[Michael S. Tsirkin]

Virtio 1.0 doesn't include a modern balloon device.  At some point we'll likely
define an incompatible interface with a different ID.  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.  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.

Changes from v1:
	correctly handle config space endian-ness

Michael S. Tsirkin (6):
  virtio_balloon: transitional interface
  virtio: balloon might not be a legacy device
  virtio_ccw: support non-legacy balloon devices
  virtio_mmio: support non-legacy balloon devices
  virtio_pci: support non-legacy balloon devices
  virtio: drop virtio_device_is_legacy_only

 include/linux/virtio.h              |  2 --
 include/uapi/linux/virtio_balloon.h | 11 +++++++++--
 drivers/s390/kvm/virtio_ccw.c       | 10 +++-------
 drivers/virtio/virtio.c             |  6 ------
 drivers/virtio/virtio_balloon.c     | 29 +++++++++++++++++++++--------
 drivers/virtio/virtio_mmio.c        |  8 --------
 drivers/virtio/virtio_pci_modern.c  |  3 ---
 7 files changed, 33 insertions(+), 36 deletions(-)

-- 
MST

[PATCH v2 1/6] virtio_balloon: transitional interface

[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 <mst@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

[PATCH v2 2/6] virtio: balloon might not be a legacy device

[Michael S. Tsirkin]

We added transitional device support to balloon driver,
so we don't need to black-list it in core anymore.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/virtio/virtio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 5ce2aa4..5fa67b5 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -280,7 +280,7 @@ static struct bus_type virtio_bus = {
 
 bool virtio_device_is_legacy_only(struct virtio_device_id id)
 {
-	return id.device == VIRTIO_ID_BALLOON;
+	return false;
 }
 EXPORT_SYMBOL_GPL(virtio_device_is_legacy_only);
 
-- 
MST

[PATCH v2 3/6] virtio_ccw: support non-legacy balloon devices

[Michael S. Tsirkin]

virtio_device_is_legacy_only is always false now,
drop the test from virtio ccw.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/s390/kvm/virtio_ccw.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index 71d7802..6f1fa17 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -1201,13 +1201,9 @@ static int virtio_ccw_online(struct ccw_device *cdev)
 	vcdev->vdev.id.vendor = cdev->id.cu_type;
 	vcdev->vdev.id.device = cdev->id.cu_model;
 
-	if (virtio_device_is_legacy_only(vcdev->vdev.id)) {
-		vcdev->revision = 0;
-	} else {
-		ret = virtio_ccw_set_transport_rev(vcdev);
-		if (ret)
-			goto out_free;
-	}
+	ret = virtio_ccw_set_transport_rev(vcdev);
+	if (ret)
+		goto out_free;
 
 	ret = register_virtio_device(&vcdev->vdev);
 	if (ret) {
-- 
MST

[PATCH v2 4/6] virtio_mmio: support non-legacy balloon devices

[Michael S. Tsirkin]

virtio_device_is_legacy_only is always false now,
drop the test from virtio mmio.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Pawel Moll <pawel.moll@arm.com>
---
 drivers/virtio/virtio_mmio.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 6010d7e..7a5e60d 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -581,14 +581,6 @@ static int virtio_mmio_probe(struct platform_device *pdev)
 	}
 	vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
 
-	/* Reject legacy-only IDs for version 2 devices */
-	if (vm_dev->version == 2 &&
-			virtio_device_is_legacy_only(vm_dev->vdev.id)) {
-		dev_err(&pdev->dev, "Version 2 not supported for devices %u!\n",
-				vm_dev->vdev.id.device);
-		return -ENODEV;
-	}
-
 	if (vm_dev->version == 1)
 		writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
 
-- 
MST

[PATCH v2 5/6] virtio_pci: support non-legacy balloon devices

[Michael S. Tsirkin]

virtio_device_is_legacy_only is always false now,
drop the test from virtio pci.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/virtio/virtio_pci_modern.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 2aa38e5..dfea17a 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -577,9 +577,6 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
 	}
 	vp_dev->vdev.id.vendor = pci_dev->subsystem_vendor;
 
-	if (virtio_device_is_legacy_only(vp_dev->vdev.id))
-		return -ENODEV;
-
 	/* check for a common config: if not, use legacy mode (bar 0). */
 	common = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_COMMON_CFG,
 					    IORESOURCE_IO | IORESOURCE_MEM);
-- 
MST

[PATCH v2 6/6] virtio: drop virtio_device_is_legacy_only

[Michael S. Tsirkin]

virtio_device_is_legacy_only is now unused, drop
it from core.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/linux/virtio.h  | 2 --
 drivers/virtio/virtio.c | 6 ------
 2 files changed, 8 deletions(-)

diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 28f0e65..8f4d4bf 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -108,8 +108,6 @@ struct virtio_device {
 	void *priv;
 };
 
-bool virtio_device_is_legacy_only(struct virtio_device_id id);
-
 static inline struct virtio_device *dev_to_virtio(struct device *_dev)
 {
 	return container_of(_dev, struct virtio_device, dev);
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 5fa67b5..b1877d7 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -278,12 +278,6 @@ static struct bus_type virtio_bus = {
 	.remove = virtio_dev_remove,
 };
 
-bool virtio_device_is_legacy_only(struct virtio_device_id id)
-{
-	return false;
-}
-EXPORT_SYMBOL_GPL(virtio_device_is_legacy_only);
-
 int register_virtio_driver(struct virtio_driver *driver)
 {
 	/* Catch this early. */
-- 
MST

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

[Rusty Russell]

"Michael S. Tsirkin" <mst@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.

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

[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

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

[Rusty Russell]

"Michael S. Tsirkin" <mst@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.  We do
need a new virtio memballoon spec, but it'll look nothing like this
anyway.

Cheers,
Rusty.

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

[Michael S. Tsirkin]

On Wed, Apr 01, 2015 at 07:53:14PM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@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

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

[Cornelia Huck]

On Wed, 1 Apr 2015 11:43:46 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Apr 01, 2015 at 07:53:14PM +1030, Rusty Russell wrote:
> > "Michael S. Tsirkin" <mst@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.

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

[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" <mst@redhat.com> wrote:
> 
> > On Wed, Apr 01, 2015 at 07:53:14PM +1030, Rusty Russell wrote:
> > > "Michael S. Tsirkin" <mst@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

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

[Cornelia Huck]

On Wed, 1 Apr 2015 12:28:30 +0200
"Michael S. Tsirkin" <mst@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" <mst@redhat.com> wrote:
> > 
> > > On Wed, Apr 01, 2015 at 07:53:14PM +1030, Rusty Russell wrote:
> > > > "Michael S. Tsirkin" <mst@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.

> 
> > > 
> > > 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.
> 

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

[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" <mst@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" <mst@redhat.com> wrote:
> > > 
> > > > On Wed, Apr 01, 2015 at 07:53:14PM +1030, Rusty Russell wrote:
> > > > > "Michael S. Tsirkin" <mst@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.
> >