* [PATCH v3 1/6] virtio_balloon: transitional interface
2015-04-01 12:57 [PATCH v3 0/6] virtio_balloon: virtio 1 support Michael S. Tsirkin
@ 2015-04-01 10:35 ` Michael S. Tsirkin
2015-04-01 10:49 ` Michael S. Tsirkin
2015-04-01 10:35 ` [PATCH v3 2/6] virtio: balloon might not be a legacy device Michael S. Tsirkin
` (5 subsequent siblings)
6 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2015-04-01 10:35 UTC (permalink / raw)
To: linux-kernel
Cc: Cornelia Huck, Pawel Moll, virtio-dev, Rusty Russell,
virtualization, linux-api
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/6] virtio_balloon: transitional interface
2015-04-01 10:35 ` [PATCH v3 1/6] virtio_balloon: transitional interface Michael S. Tsirkin
@ 2015-04-01 10:49 ` Michael S. Tsirkin
2015-04-01 11:52 ` Cornelia Huck
0 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2015-04-01 10:49 UTC (permalink / raw)
To: linux-kernel
Cc: Cornelia Huck, Pawel Moll, virtio-dev, Rusty Russell,
virtualization, linux-api
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/6] virtio_balloon: transitional interface
2015-04-01 10:49 ` Michael S. Tsirkin
@ 2015-04-01 11:52 ` Cornelia Huck
2015-04-01 13:01 ` Michael S. Tsirkin
0 siblings, 1 reply; 24+ messages in thread
From: Cornelia Huck @ 2015-04-01 11:52 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel, Pawel Moll, virtio-dev, Rusty Russell,
virtualization, linux-api
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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/6] virtio_balloon: transitional interface
2015-04-01 11:52 ` Cornelia Huck
@ 2015-04-01 13:01 ` Michael S. Tsirkin
0 siblings, 0 replies; 24+ messages in thread
From: Michael S. Tsirkin @ 2015-04-01 13:01 UTC (permalink / raw)
To: Cornelia Huck
Cc: linux-kernel, Pawel Moll, virtio-dev, Rusty Russell,
virtualization, linux-api
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 2/6] virtio: balloon might not be a legacy device
2015-04-01 12:57 [PATCH v3 0/6] virtio_balloon: virtio 1 support Michael S. Tsirkin
2015-04-01 10:35 ` [PATCH v3 1/6] virtio_balloon: transitional interface Michael S. Tsirkin
@ 2015-04-01 10:35 ` Michael S. Tsirkin
2015-04-01 11:57 ` Cornelia Huck
2015-04-01 10:35 ` [PATCH v3 3/6] virtio_ccw: support non-legacy balloon devices Michael S. Tsirkin
` (4 subsequent siblings)
6 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2015-04-01 10:35 UTC (permalink / raw)
To: linux-kernel
Cc: Cornelia Huck, Pawel Moll, virtio-dev, Rusty Russell, virtualization
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/6] virtio: balloon might not be a legacy device
2015-04-01 10:35 ` [PATCH v3 2/6] virtio: balloon might not be a legacy device Michael S. Tsirkin
@ 2015-04-01 11:57 ` Cornelia Huck
0 siblings, 0 replies; 24+ messages in thread
From: Cornelia Huck @ 2015-04-01 11:57 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel, Pawel Moll, virtio-dev, Rusty Russell, virtualization
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>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 3/6] virtio_ccw: support non-legacy balloon devices
2015-04-01 12:57 [PATCH v3 0/6] virtio_balloon: virtio 1 support Michael S. Tsirkin
2015-04-01 10:35 ` [PATCH v3 1/6] virtio_balloon: transitional interface Michael S. Tsirkin
2015-04-01 10:35 ` [PATCH v3 2/6] virtio: balloon might not be a legacy device Michael S. Tsirkin
@ 2015-04-01 10:35 ` Michael S. Tsirkin
2015-04-01 10:40 ` [virtio-dev] " Christian Borntraeger
2015-04-01 10:35 ` [PATCH v3 4/6] virtio_mmio: " Michael S. Tsirkin
` (3 subsequent siblings)
6 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2015-04-01 10:35 UTC (permalink / raw)
To: linux-kernel
Cc: Cornelia Huck, Pawel Moll, virtio-dev, Christian Borntraeger,
linux390, Martin Schwidefsky, Heiko Carstens, linux-s390
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [virtio-dev] [PATCH v3 3/6] virtio_ccw: support non-legacy balloon devices
2015-04-01 10:35 ` [PATCH v3 3/6] virtio_ccw: support non-legacy balloon devices Michael S. Tsirkin
@ 2015-04-01 10:40 ` Christian Borntraeger
2015-04-01 11:55 ` Cornelia Huck
2015-04-14 1:09 ` Rusty Russell
0 siblings, 2 replies; 24+ messages in thread
From: Christian Borntraeger @ 2015-04-01 10:40 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: Cornelia Huck, Pawel Moll, virtio-dev, linux390,
Martin Schwidefsky, Heiko Carstens, linux-s390
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) {
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [virtio-dev] [PATCH v3 3/6] virtio_ccw: support non-legacy balloon devices
2015-04-01 10:40 ` [virtio-dev] " Christian Borntraeger
@ 2015-04-01 11:55 ` Cornelia Huck
2015-04-14 1:09 ` Rusty Russell
1 sibling, 0 replies; 24+ messages in thread
From: Cornelia Huck @ 2015-04-01 11:55 UTC (permalink / raw)
To: Christian Borntraeger
Cc: Michael S. Tsirkin, linux-kernel, Pawel Moll, virtio-dev,
linux390, Martin Schwidefsky, Heiko Carstens, linux-s390
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?
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [virtio-dev] [PATCH v3 3/6] virtio_ccw: support non-legacy balloon devices
2015-04-01 10:40 ` [virtio-dev] " Christian Borntraeger
2015-04-01 11:55 ` Cornelia Huck
@ 2015-04-14 1:09 ` Rusty Russell
1 sibling, 0 replies; 24+ messages in thread
From: Rusty Russell @ 2015-04-14 1:09 UTC (permalink / raw)
To: Christian Borntraeger, Michael S. Tsirkin, linux-kernel
Cc: Cornelia Huck, Pawel Moll, virtio-dev, linux390,
Martin Schwidefsky, Heiko Carstens, linux-s390
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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 4/6] virtio_mmio: support non-legacy balloon devices
2015-04-01 12:57 [PATCH v3 0/6] virtio_balloon: virtio 1 support Michael S. Tsirkin
` (2 preceding siblings ...)
2015-04-01 10:35 ` [PATCH v3 3/6] virtio_ccw: support non-legacy balloon devices Michael S. Tsirkin
@ 2015-04-01 10:35 ` Michael S. Tsirkin
2015-04-01 10:35 ` [PATCH v3 5/6] virtio_pci: " Michael S. Tsirkin
` (2 subsequent siblings)
6 siblings, 0 replies; 24+ messages in thread
From: Michael S. Tsirkin @ 2015-04-01 10:35 UTC (permalink / raw)
To: linux-kernel
Cc: Cornelia Huck, Pawel Moll, virtio-dev, Rusty Russell, virtualization
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 5/6] virtio_pci: support non-legacy balloon devices
2015-04-01 12:57 [PATCH v3 0/6] virtio_balloon: virtio 1 support Michael S. Tsirkin
` (3 preceding siblings ...)
2015-04-01 10:35 ` [PATCH v3 4/6] virtio_mmio: " Michael S. Tsirkin
@ 2015-04-01 10:35 ` Michael S. Tsirkin
2015-04-01 10:36 ` [PATCH v3 6/6] virtio: drop virtio_device_is_legacy_only Michael S. Tsirkin
2015-04-12 15:02 ` [PATCH v3 0/6] virtio_balloon: virtio 1 support Michael S. Tsirkin
6 siblings, 0 replies; 24+ messages in thread
From: Michael S. Tsirkin @ 2015-04-01 10:35 UTC (permalink / raw)
To: linux-kernel
Cc: Cornelia Huck, Pawel Moll, virtio-dev, Rusty Russell, virtualization
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 6/6] virtio: drop virtio_device_is_legacy_only
2015-04-01 12:57 [PATCH v3 0/6] virtio_balloon: virtio 1 support Michael S. Tsirkin
` (4 preceding siblings ...)
2015-04-01 10:35 ` [PATCH v3 5/6] virtio_pci: " Michael S. Tsirkin
@ 2015-04-01 10:36 ` Michael S. Tsirkin
2015-04-01 12:04 ` Cornelia Huck
2015-04-12 15:02 ` [PATCH v3 0/6] virtio_balloon: virtio 1 support Michael S. Tsirkin
6 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2015-04-01 10:36 UTC (permalink / raw)
To: linux-kernel
Cc: Cornelia Huck, Pawel Moll, virtio-dev, Rusty Russell,
David Hildenbrand, Paul Bolle, virtualization
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v3 6/6] virtio: drop virtio_device_is_legacy_only
2015-04-01 10:36 ` [PATCH v3 6/6] virtio: drop virtio_device_is_legacy_only Michael S. Tsirkin
@ 2015-04-01 12:04 ` Cornelia Huck
0 siblings, 0 replies; 24+ messages in thread
From: Cornelia Huck @ 2015-04-01 12:04 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel, Pawel Moll, virtio-dev, Rusty Russell,
David Hildenbrand, Paul Bolle, virtualization
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>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support
2015-04-01 12:57 [PATCH v3 0/6] virtio_balloon: virtio 1 support Michael S. Tsirkin
` (5 preceding siblings ...)
2015-04-01 10:36 ` [PATCH v3 6/6] virtio: drop virtio_device_is_legacy_only Michael S. Tsirkin
@ 2015-04-12 15:02 ` Michael S. Tsirkin
2015-04-14 1:12 ` Rusty Russell
6 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2015-04-12 15:02 UTC (permalink / raw)
To: linux-kernel
Cc: Cornelia Huck, Pawel Moll, virtio-dev, Rusty Russell, virtualization
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support
2015-04-12 15:02 ` [PATCH v3 0/6] virtio_balloon: virtio 1 support Michael S. Tsirkin
@ 2015-04-14 1:12 ` Rusty Russell
2015-04-14 8:24 ` Cornelia Huck
0 siblings, 1 reply; 24+ messages in thread
From: Rusty Russell @ 2015-04-14 1:12 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: Cornelia Huck, Pawel Moll, virtio-dev, virtualization
"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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support
2015-04-14 1:12 ` Rusty Russell
@ 2015-04-14 8:24 ` Cornelia Huck
2015-04-14 9:21 ` Michael S. Tsirkin
0 siblings, 1 reply; 24+ messages in thread
From: Cornelia Huck @ 2015-04-14 8:24 UTC (permalink / raw)
To: Rusty Russell
Cc: Michael S. Tsirkin, linux-kernel, Pawel Moll, virtio-dev, virtualization
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?
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support
2015-04-14 8:24 ` Cornelia Huck
@ 2015-04-14 9:21 ` Michael S. Tsirkin
2015-04-14 9:50 ` Cornelia Huck
0 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2015-04-14 9:21 UTC (permalink / raw)
To: Cornelia Huck
Cc: Rusty Russell, linux-kernel, Pawel Moll, virtio-dev, virtualization
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support
2015-04-14 9:21 ` Michael S. Tsirkin
@ 2015-04-14 9:50 ` Cornelia Huck
2015-04-14 9:58 ` Michael S. Tsirkin
0 siblings, 1 reply; 24+ messages in thread
From: Cornelia Huck @ 2015-04-14 9:50 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rusty Russell, linux-kernel, Pawel Moll, virtio-dev, virtualization
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>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support
2015-04-14 9:50 ` Cornelia Huck
@ 2015-04-14 9:58 ` Michael S. Tsirkin
2015-04-15 0:45 ` Rusty Russell
0 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2015-04-14 9:58 UTC (permalink / raw)
To: Cornelia Huck
Cc: Rusty Russell, linux-kernel, Pawel Moll, virtio-dev, virtualization
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)
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support
2015-04-14 9:58 ` Michael S. Tsirkin
@ 2015-04-15 0:45 ` Rusty Russell
2015-04-15 15:32 ` Cornelia Huck
2015-04-15 15:45 ` Michael S. Tsirkin
0 siblings, 2 replies; 24+ messages in thread
From: Rusty Russell @ 2015-04-15 0:45 UTC (permalink / raw)
To: Michael S. Tsirkin, Cornelia Huck
Cc: linux-kernel, Pawel Moll, virtio-dev, virtualization
"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 */
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support
2015-04-15 0:45 ` Rusty Russell
@ 2015-04-15 15:32 ` Cornelia Huck
2015-04-15 15:45 ` Michael S. Tsirkin
1 sibling, 0 replies; 24+ messages in thread
From: Cornelia Huck @ 2015-04-15 15:32 UTC (permalink / raw)
To: Rusty Russell
Cc: Michael S. Tsirkin, linux-kernel, Pawel Moll, virtio-dev, virtualization
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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support
2015-04-15 0:45 ` Rusty Russell
2015-04-15 15:32 ` Cornelia Huck
@ 2015-04-15 15:45 ` Michael S. Tsirkin
1 sibling, 0 replies; 24+ messages in thread
From: Michael S. Tsirkin @ 2015-04-15 15:45 UTC (permalink / raw)
To: Rusty Russell
Cc: Cornelia Huck, linux-kernel, Pawel Moll, virtio-dev, virtualization
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 */
^ permalink raw reply [flat|nested] 24+ messages in thread