[PATCH v3 1/6] virtio_balloon: transitional interface

[PATCH v3 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     | 43 +++++++++++++++++++++++++++++--------
 2 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
index 4b0488f..f81b220 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -25,6 +25,7 @@
  * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
  * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE. */
+#include <linux/types.h>
 #include <linux/virtio_ids.h>
 #include <linux/virtio_config.h>
 
@@ -38,9 +39,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 */
@@ -56,4 +57,10 @@ struct virtio_balloon_stat {
 	__u64 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 6a356e3..0583720 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -77,7 +77,10 @@ struct virtio_balloon {
 
 	/* Memory statistics */
 	int need_stats_update;
-	struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
+	union {
+		struct virtio_balloon_stat_modern stats[VIRTIO_BALLOON_S_NR];
+		struct virtio_balloon_stat legacy_stats[VIRTIO_BALLOON_S_NR];
+	};
 
 	/* To register callback in oom notifier call chain */
 	struct notifier_block nb;
@@ -88,6 +91,14 @@ static struct virtio_device_id id_table[] = {
 	{ 0 },
 };
 
+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));
+}
+
 static u32 page_to_balloon_pfn(struct page *page)
 {
 	unsigned long pfn = page_to_pfn(page);
@@ -214,8 +225,13 @@ 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;
+	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;
+	}
 }
 
 #define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
@@ -269,7 +285,7 @@ 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));
+	stats_sg_init(vb, &sg);
 	virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
 	virtqueue_kick(vq);
 }
@@ -283,18 +299,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,
+		     &num_pages);
 
-	virtio_cread(vb->vdev, struct virtio_balloon_config, num_pages, &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 = le32_to_cpu(v);
+	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);
@@ -397,7 +422,7 @@ static int init_vqs(struct virtio_balloon *vb)
 		 * Prime this virtqueue with one buffer so the hypervisor can
 		 * use it to signal us later (it can't be broken yet!).
 		 */
-		sg_init_one(&sg, vb->stats, sizeof vb->stats);
+		stats_sg_init(vb, &sg);
 		if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL)
 		    < 0)
 			BUG();
-- 
MST

[PATCH v3 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 v3 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 v3 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 v3 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 v3 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: [virtio-dev] [PATCH v3 3/6] virtio_ccw: support non-legacy balloon devices

[Christian Borntraeger]

Am 01.04.2015 um 12:35 schrieb Michael S. Tsirkin:
> virtio_device_is_legacy_only is always false now,
> drop the test from virtio ccw.

Can you add the commit subject of patch2 here as a
prereq for this patch? This will hopefully avoid 
backport issues on distros that want to take this
patch but not the other for whatever reasons.


this patch is then.
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>


> 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) {
> 

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

[Michael S. Tsirkin]

On Wed, Apr 01, 2015 at 12:35:45PM +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@redhat.com>
> ---
>  include/uapi/linux/virtio_balloon.h | 11 ++++++++--
>  drivers/virtio/virtio_balloon.c     | 43 +++++++++++++++++++++++++++++--------
>  2 files changed, 43 insertions(+), 11 deletions(-)

So all in all 32 LOC added, and 9 out of these
deal with endian-ness differences.

Seems like a small cost for guests to pay for a clean spec, no?

-- 
MST

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

[Cornelia Huck]

On Wed, 1 Apr 2015 12:49:02 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Apr 01, 2015 at 12:35:45PM +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@redhat.com>
> > ---
> >  include/uapi/linux/virtio_balloon.h | 11 ++++++++--
> >  drivers/virtio/virtio_balloon.c     | 43 +++++++++++++++++++++++++++++--------
> >  2 files changed, 43 insertions(+), 11 deletions(-)
> 
> So all in all 32 LOC added, and 9 out of these
> deal with endian-ness differences.
> 
> Seems like a small cost for guests to pay for a clean spec, no?
> 

I'm not opposed to this per se, but I'm not totally convinced of the
usefulness :)

Seeing the qemu side would be helpful.

Re: [virtio-dev] [PATCH v3 3/6] virtio_ccw: support non-legacy balloon devices

[Cornelia Huck]

On Wed, 01 Apr 2015 12:40:10 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> Am 01.04.2015 um 12:35 schrieb Michael S. Tsirkin:
> > virtio_device_is_legacy_only is always false now,
> > drop the test from virtio ccw.
> 
> Can you add the commit subject of patch2 here as a
> prereq for this patch? This will hopefully avoid 
> backport issues on distros that want to take this
> patch but not the other for whatever reasons.

Seconded.

> 
> 
> this patch is then.
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

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

Michael, I assume you would take this patch through your tree?

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

[Cornelia Huck]

On Wed, 1 Apr 2015 12:35:48 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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(-)
> 

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [PATCH v3 6/6] virtio: drop virtio_device_is_legacy_only

[Cornelia Huck]

On Wed, 1 Apr 2015 12:36:03 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[PATCH v3 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 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.

changes from v2:
	fix up stats endian-ness
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     | 33 +++++++++++++++++++++++----------
 drivers/virtio/virtio_mmio.c        |  8 --------
 drivers/virtio/virtio_pci_modern.c  |  3 ---
 7 files changed, 35 insertions(+), 38 deletions(-)

-- 
MST

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

[Michael S. Tsirkin]

On Wed, Apr 01, 2015 at 01:52:44PM +0200, Cornelia Huck wrote:
> On Wed, 1 Apr 2015 12:49:02 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Apr 01, 2015 at 12:35:45PM +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@redhat.com>
> > > ---
> > >  include/uapi/linux/virtio_balloon.h | 11 ++++++++--
> > >  drivers/virtio/virtio_balloon.c     | 43 +++++++++++++++++++++++++++++--------
> > >  2 files changed, 43 insertions(+), 11 deletions(-)
> > 
> > So all in all 32 LOC added, and 9 out of these
> > deal with endian-ness differences.
> > 
> > Seems like a small cost for guests to pay for a clean spec, no?
> > 
> 
> I'm not opposed to this per se, but I'm not totally convinced of the
> usefulness :)
> 
> Seeing the qemu side would be helpful.

Cleaning this up for submission, should be ready RSN.

-- 
MST

Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support

[Michael S. Tsirkin]

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?


> changes from v2:
> 	fix up stats endian-ness
> 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     | 33 +++++++++++++++++++++++----------
>  drivers/virtio/virtio_mmio.c        |  8 --------
>  drivers/virtio/virtio_pci_modern.c  |  3 ---
>  7 files changed, 35 insertions(+), 38 deletions(-)
> 
> -- 
> MST

Re: [virtio-dev] [PATCH v3 3/6] virtio_ccw: support non-legacy balloon devices

[Rusty Russell]

Christian Borntraeger <borntraeger@de.ibm.com> writes:
> Am 01.04.2015 um 12:35 schrieb Michael S. Tsirkin:
>> virtio_device_is_legacy_only is always false now,
>> drop the test from virtio ccw.
>
> Can you add the commit subject of patch2 here as a
> prereq for this patch? This will hopefully avoid 
> backport issues on distros that want to take this
> patch but not the other for whatever reasons.

I changed commit wording to:

As of last patch, virtio_device_is_legacy_only is always false,
drop the test from virtio ccw.

That should be clear to anyone seeking -stable patches.

Thanks,
Rusty.

Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support

[Rusty Russell]

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

Thanks,
Rusty.

Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support

[Cornelia Huck]

On Tue, 14 Apr 2015 10:42:56 +0930
Rusty Russell <rusty@rustcorp.com.au> wrote:

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

Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support

[Michael S. Tsirkin]

On Tue, Apr 14, 2015 at 10:24:38AM +0200, Cornelia Huck wrote:
> On Tue, 14 Apr 2015 10:42:56 +0930
> Rusty Russell <rusty@rustcorp.com.au> wrote:
> 
> > "Michael S. Tsirkin" <mst@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 <mst@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, 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

Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support

[Cornelia Huck]

On Tue, 14 Apr 2015 11:21:11 +0200
"Michael S. Tsirkin" <mst@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.huck@de.ibm.com>

Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support

[Michael S. Tsirkin]

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" <mst@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.huck@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 @@ 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)

Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support

[Rusty Russell]

"Michael S. Tsirkin" <mst@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" <mst@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.huck@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 <mst@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 <mst@redhat.com>
Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Rusty Russell <rusty@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 b/include/uapi/linux/virtio_balloon.h
index 4b0488f20b2e..984169a819ee 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -25,6 +25,7 @@
  * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
  * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE. */
+#include <linux/types.h>
 #include <linux/virtio_ids.h>
 #include <linux/virtio_config.h>
 
@@ -38,9 +39,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 +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));
 
 #endif /* _LINUX_VIRTIO_BALLOON_H */

Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support

[Cornelia Huck]

On Wed, 15 Apr 2015 10:15:20 +0930
Rusty Russell <rusty@rustcorp.com.au> wrote:

> 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 <mst@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 <mst@redhat.com>
> Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> 
Looks good to me.

Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support

[Michael S. Tsirkin]

On Wed, Apr 15, 2015 at 10:15:20AM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@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" <mst@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.huck@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 <mst@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 <mst@redhat.com>
> Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Fine by me.


> 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 b/include/uapi/linux/virtio_balloon.h
> index 4b0488f20b2e..984169a819ee 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -25,6 +25,7 @@
>   * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
>   * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>   * SUCH DAMAGE. */
> +#include <linux/types.h>
>  #include <linux/virtio_ids.h>
>  #include <linux/virtio_config.h>
>  
> @@ -38,9 +39,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 +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));
>  
>  #endif /* _LINUX_VIRTIO_BALLOON_H */