"Michael S. Tsirkin" <mst@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
>
>
> 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@xxxxxxxxxx>
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@xxxxxxxxxx>
Acked-by: Cornelia Huck <cornelia.huck@xxxxxxxxxx>
Signed-off-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
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 */
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
