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

["Michael S. Tsirkin" <mst@redhat.com>]

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