On Tue, 14 Apr 2015 11:21:11 +0200 "Michael S. Tsirkin" <mst at 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 at de.ibm.com>
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 at 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 at 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)
"Michael S. Tsirkin" <mst at 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 at 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 at 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 at 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 at redhat.com> Acked-by: Cornelia Huck <cornelia.huck at de.ibm.com> Signed-off-by: Rusty Russell <rusty at 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 */