[PATCH v2 1/6] virtio_balloon: transitional interface

[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

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

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