Linux Virtualization - Mar 2015 - [PATCH 1/6] virtio_balloon: transitional interface

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 change to support a transitional balloon
device: this has the advantage of supporting existing drivers, transparently.

The only issue is with the stats buffer, which has misaligned fields.
Seems easy to fix by prepending a 6 byte reserved field.

I'll post spec patch and qemu patches shortly.

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 |  6 ++++++
 drivers/s390/kvm/virtio_ccw.c       | 10 +++-------
 drivers/virtio/virtio.c             |  6 ------
 drivers/virtio/virtio_balloon.c     |  8 ++++++--
 drivers/virtio/virtio_mmio.c        |  8 --------
 drivers/virtio/virtio_pci_modern.c  |  3 ---
 7 files changed, 15 insertions(+), 28 deletions(-)

-- 
MST

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 at redhat.com>
---
 include/uapi/linux/virtio_balloon.h | 6 ++++++
 drivers/virtio/virtio_balloon.c     | 8 ++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/virtio_balloon.h
b/include/uapi/linux/virtio_balloon.h
index 4b0488f..5d83902 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -56,4 +56,10 @@ struct virtio_balloon_stat {
 	__u64 val;
 } __attribute__((packed));
 
+struct virtio_balloon_stat_modern {
+	__u8 reserved[6];
+	__u16 tag;
+	__u64 val;
+};
+
 #endif /* _LINUX_VIRTIO_BALLOON_H */
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 6a356e3..68e937f 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);
 }
-- 
MST

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

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

Signed-off-by: Michael S. Tsirkin <mst at 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

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

Signed-off-by: Michael S. Tsirkin <mst at redhat.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

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

Signed-off-by: Michael S. Tsirkin <mst at 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

virtio_device_is_legacy_only is now unused, drop
it from core.

Signed-off-by: Michael S. Tsirkin <mst at 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

On Mon, Mar 30, 2015 at 07:37:12PM +0200, Michael S. Tsirkin
wrote:
> 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 at redhat.com>
> ---
Hmm, re-reading the code, this won't be enough for BE
guests. We are doing one extra byteswap there.

I'll fix up and repost.
>  include/uapi/linux/virtio_balloon.h | 6 ++++++
>  drivers/virtio/virtio_balloon.c     | 8 ++++++--
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/virtio_balloon.h
b/include/uapi/linux/virtio_balloon.h
> index 4b0488f..5d83902 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -56,4 +56,10 @@ struct virtio_balloon_stat {
>  	__u64 val;
>  } __attribute__((packed));
>  
> +struct virtio_balloon_stat_modern {
> +	__u8 reserved[6];
> +	__u16 tag;
> +	__u64 val;
> +};
> +
>  #endif /* _LINUX_VIRTIO_BALLOON_H */
> diff --git a/drivers/virtio/virtio_balloon.c
b/drivers/virtio/virtio_balloon.c
> index 6a356e3..68e937f 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);
>  }
> -- 
> MST
>

On Mon, 30 Mar 2015 19:37:08 +0200
"Michael S. Tsirkin" <mst at redhat.com> 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.  But for now, it's not a big change to support a transitional
balloon
> device: this has the advantage of supporting existing drivers,
transparently.
This is needed to support a ballooner on transports that don't support
mixing legacy and 1.0, right?
> 
> The only issue is with the stats buffer, which has misaligned fields.
> Seems easy to fix by prepending a 6 byte reserved field.
I assume you also define the stats fields to be le for 1.0?
> 
> I'll post spec patch and qemu patches shortly.
I'll defer review until then.
> 
> 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 |  6 ++++++
>  drivers/s390/kvm/virtio_ccw.c       | 10 +++-------
>  drivers/virtio/virtio.c             |  6 ------
>  drivers/virtio/virtio_balloon.c     |  8 ++++++--
>  drivers/virtio/virtio_mmio.c        |  8 --------
>  drivers/virtio/virtio_pci_modern.c  |  3 ---
>  7 files changed, 15 insertions(+), 28 deletions(-)
>

On Tue, Mar 31, 2015 at 10:46:18AM +0200, Cornelia Huck
wrote:
> On Mon, 30 Mar 2015 19:37:08 +0200
> "Michael S. Tsirkin" <mst at redhat.com> 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.  But for now, it's not a big change to support a transitional
balloon
> > device: this has the advantage of supporting existing drivers,
transparently.
> 
> This is needed to support a ballooner on transports that don't support
> mixing legacy and 1.0, right?
That too. But it's also useful if you don't know whether your guest
supports virtio 1 or not - which is the point of using
a transitional device.
> > 
> > The only issue is with the stats buffer, which has misaligned fields.
> > Seems easy to fix by prepending a 6 byte reserved field.
> 
> I assume you also define the stats fields to be le for 1.0?
They were always le for the balloon.
> > 
> > I'll post spec patch and qemu patches shortly.
> 
> I'll defer review until then.
> 
> > 
> > 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 |  6 ++++++
> >  drivers/s390/kvm/virtio_ccw.c       | 10 +++-------
> >  drivers/virtio/virtio.c             |  6 ------
> >  drivers/virtio/virtio_balloon.c     |  8 ++++++--
> >  drivers/virtio/virtio_mmio.c        |  8 --------
> >  drivers/virtio/virtio_pci_modern.c  |  3 ---
> >  7 files changed, 15 insertions(+), 28 deletions(-)
> >

On Mon, 2015-03-30 at 18:37 +0100, Michael S. Tsirkin
wrote:
> virtio_device_is_legacy_only is always false now,
> drop the test from virtio mmio.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
Slightly ironic ack ;-) after all the battle you fought for this:

Acked-by: Pawel Moll <pawel.moll at arm.com>

Thanks!

Pawel