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

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: [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 */