"Michael S. Tsirkin" <[email protected]> writes:
> 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.
You decided to fix the packed struct...
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 6a356e3..574267f 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -77,7 +77,7 @@ struct virtio_balloon {
>
> /* Memory statistics */
> int need_stats_update;
> - struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
> + struct virtio_balloon_stat_modern stats[VIRTIO_BALLOON_S_NR];
>
> /* To register callback in oom notifier call chain */
> struct notifier_block nb;
> @@ -269,7 +269,11 @@ 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));
> + if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
> + sg_init_one(&sg, vb->stats, sizeof(vb->stats));
> + else
> + sg_init_one(&sg, &vb->stats->tag, sizeof(vb->stats) -
> + offsetof(typeof(*vb->stats, tag);
This makes it compile, but definitely won't work.
> virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
> virtqueue_kick(vq);
> }
> @@ -283,21 +287,30 @@ 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(num_pages);
>
> - virtio_cwrite(vb->vdev, struct virtio_balloon_config, actual,
> - &actual);
> + virtio_cwrite(vb->vdev, struct virtio_balloon_config,
> + actual, &actual);
> }
Final line is gratitous reformatting.
I would leave the device *exactly* as is, ugly structure packing and
all.
Cheers,
Rusty.
On Wed, Apr 01, 2015 at 02:17:23PM +1030, Rusty Russell wrote:
> I would leave the device *exactly* as is, ugly structure packing and
> all.
But why? It's going to be used for years, might as well make it clean?
--
MST
"Michael S. Tsirkin" <[email protected]> writes:
> On Wed, Apr 01, 2015 at 02:17:23PM +1030, Rusty Russell wrote:
>> I would leave the device *exactly* as is, ugly structure packing and
>> all.
>
> But why? It's going to be used for years, might as well make it clean?
Because the only spec which currently exists says to do that. We do
need a new virtio memballoon spec, but it'll look nothing like this
anyway.
Cheers,
Rusty.
On Wed, Apr 01, 2015 at 07:53:14PM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" <[email protected]> writes:
> > On Wed, Apr 01, 2015 at 02:17:23PM +1030, Rusty Russell wrote:
> >> I would leave the device *exactly* as is, ugly structure packing and
> >> all.
> >
> > But why? It's going to be used for years, might as well make it clean?
>
> Because the only spec which currently exists says to do that.
OK but the only spec which currently exists also says it's a legacy only
device, so driver must not set VERSION_1. So surely, we can make minor
changes when VERSION_1 is set, like we did for other devices.
Let me post the latest patches I'm working on,
see what you think then.
> We do
> need a new virtio memballoon spec, but it'll look nothing like this
> anyway.
>
> Cheers,
> Rusty.
I think it's going to have significantly different semantics, too,
so not much value in making that one work with current
drivers, right?
--
MST
On Wed, 1 Apr 2015 11:43:46 +0200
"Michael S. Tsirkin" <[email protected]> wrote:
> On Wed, Apr 01, 2015 at 07:53:14PM +1030, Rusty Russell wrote:
> > "Michael S. Tsirkin" <[email protected]> writes:
> > > On Wed, Apr 01, 2015 at 02:17:23PM +1030, Rusty Russell wrote:
> > >> I would leave the device *exactly* as is, ugly structure packing and
> > >> all.
> > >
> > > But why? It's going to be used for years, might as well make it clean?
> >
> > Because the only spec which currently exists says to do that.
>
> OK but the only spec which currently exists also says it's a legacy only
> device, so driver must not set VERSION_1. So surely, we can make minor
> changes when VERSION_1 is set, like we did for other devices.
But we don't plan to replace the other devices, so it makes sense to do
some changes for 1.0.
>
> Let me post the latest patches I'm working on,
> see what you think then.
>
> > We do
> > need a new virtio memballoon spec, but it'll look nothing like this
> > anyway.
> >
> > Cheers,
> > Rusty.
>
> I think it's going to have significantly different semantics, too,
> so not much value in making that one work with current
> drivers, right?
>
So why not just keep virtio-balloon as-is and just specify endianness
etc. for 1.0? Keeps the old drivers going without hacks, and we can
start with a fresh driver for the new virtio-balloon.
On Wed, Apr 01, 2015 at 12:22:44PM +0200, Cornelia Huck wrote:
> On Wed, 1 Apr 2015 11:43:46 +0200
> "Michael S. Tsirkin" <[email protected]> wrote:
>
> > On Wed, Apr 01, 2015 at 07:53:14PM +1030, Rusty Russell wrote:
> > > "Michael S. Tsirkin" <[email protected]> writes:
> > > > On Wed, Apr 01, 2015 at 02:17:23PM +1030, Rusty Russell wrote:
> > > >> I would leave the device *exactly* as is, ugly structure packing and
> > > >> all.
> > > >
> > > > But why? It's going to be used for years, might as well make it clean?
> > >
> > > Because the only spec which currently exists says to do that.
> >
> > OK but the only spec which currently exists also says it's a legacy only
> > device, so driver must not set VERSION_1. So surely, we can make minor
> > changes when VERSION_1 is set, like we did for other devices.
>
> But we don't plan to replace the other devices, so it makes sense to do
> some changes for 1.0.
I'm not sure what the above says. Do you agree with
making minor changes in device behaviour?
Also to be clear, I think this is 1.1 material.
> >
> > Let me post the latest patches I'm working on,
> > see what you think then.
> >
> > > We do
> > > need a new virtio memballoon spec, but it'll look nothing like this
> > > anyway.
> > >
> > > Cheers,
> > > Rusty.
> >
> > I think it's going to have significantly different semantics, too,
> > so not much value in making that one work with current
> > drivers, right?
> >
>
> So why not just keep virtio-balloon as-is and just specify endianness
> etc. for 1.0? Keeps the old drivers going without hacks,
> and we can
> start with a fresh driver for the new virtio-balloon.
Well it doesn't really, we need cpu_to_virtio in a bunch of
places anyway.
So I kind of prefer making it clean, even just to avoid setting a bad
example for other devices.
Let me post the new patch where it's all fixed in a cleaner way, and
everyone can discuss whether it's too much work.
--
MST
On Wed, 1 Apr 2015 12:28:30 +0200
"Michael S. Tsirkin" <[email protected]> wrote:
> On Wed, Apr 01, 2015 at 12:22:44PM +0200, Cornelia Huck wrote:
> > On Wed, 1 Apr 2015 11:43:46 +0200
> > "Michael S. Tsirkin" <[email protected]> wrote:
> >
> > > On Wed, Apr 01, 2015 at 07:53:14PM +1030, Rusty Russell wrote:
> > > > "Michael S. Tsirkin" <[email protected]> writes:
> > > > > On Wed, Apr 01, 2015 at 02:17:23PM +1030, Rusty Russell wrote:
> > > > >> I would leave the device *exactly* as is, ugly structure packing and
> > > > >> all.
> > > > >
> > > > > But why? It's going to be used for years, might as well make it clean?
> > > >
> > > > Because the only spec which currently exists says to do that.
> > >
> > > OK but the only spec which currently exists also says it's a legacy only
> > > device, so driver must not set VERSION_1. So surely, we can make minor
> > > changes when VERSION_1 is set, like we did for other devices.
> >
> > But we don't plan to replace the other devices, so it makes sense to do
> > some changes for 1.0.
>
> I'm not sure what the above says. Do you agree with
> making minor changes in device behaviour?
The other way around.
> Also to be clear, I think this is 1.1 material.
Btw, I'd really like to see your proposed spec updates.
>
> > >
> > > Let me post the latest patches I'm working on,
> > > see what you think then.
> > >
> > > > We do
> > > > need a new virtio memballoon spec, but it'll look nothing like this
> > > > anyway.
> > > >
> > > > Cheers,
> > > > Rusty.
> > >
> > > I think it's going to have significantly different semantics, too,
> > > so not much value in making that one work with current
> > > drivers, right?
> > >
> >
> > So why not just keep virtio-balloon as-is and just specify endianness
> > etc. for 1.0? Keeps the old drivers going without hacks,
> > and we can
> > start with a fresh driver for the new virtio-balloon.
>
> Well it doesn't really, we need cpu_to_virtio in a bunch of
> places anyway.
Of course, but what about keeping changes minimal?
>
> So I kind of prefer making it clean, even just to avoid setting a bad
> example for other devices.
>
> Let me post the new patch where it's all fixed in a cleaner way, and
> everyone can discuss whether it's too much work.
>
On Wed, Apr 01, 2015 at 12:57:48PM +0200, Cornelia Huck wrote:
> On Wed, 1 Apr 2015 12:28:30 +0200
> "Michael S. Tsirkin" <[email protected]> wrote:
>
> > On Wed, Apr 01, 2015 at 12:22:44PM +0200, Cornelia Huck wrote:
> > > On Wed, 1 Apr 2015 11:43:46 +0200
> > > "Michael S. Tsirkin" <[email protected]> wrote:
> > >
> > > > On Wed, Apr 01, 2015 at 07:53:14PM +1030, Rusty Russell wrote:
> > > > > "Michael S. Tsirkin" <[email protected]> writes:
> > > > > > On Wed, Apr 01, 2015 at 02:17:23PM +1030, Rusty Russell wrote:
> > > > > >> I would leave the device *exactly* as is, ugly structure packing and
> > > > > >> all.
> > > > > >
> > > > > > But why? It's going to be used for years, might as well make it clean?
> > > > >
> > > > > Because the only spec which currently exists says to do that.
> > > >
> > > > OK but the only spec which currently exists also says it's a legacy only
> > > > device, so driver must not set VERSION_1. So surely, we can make minor
> > > > changes when VERSION_1 is set, like we did for other devices.
> > >
> > > But we don't plan to replace the other devices, so it makes sense to do
> > > some changes for 1.0.
> >
> > I'm not sure what the above says. Do you agree with
> > making minor changes in device behaviour?
>
> The other way around.
>
> > Also to be clear, I think this is 1.1 material.
>
> Btw, I'd really like to see your proposed spec updates.
Just sent.
> >
> > > >
> > > > Let me post the latest patches I'm working on,
> > > > see what you think then.
> > > >
> > > > > We do
> > > > > need a new virtio memballoon spec, but it'll look nothing like this
> > > > > anyway.
> > > > >
> > > > > Cheers,
> > > > > Rusty.
> > > >
> > > > I think it's going to have significantly different semantics, too,
> > > > so not much value in making that one work with current
> > > > drivers, right?
> > > >
> > >
> > > So why not just keep virtio-balloon as-is and just specify endianness
> > > etc. for 1.0? Keeps the old drivers going without hacks,
> > > and we can
> > > start with a fresh driver for the new virtio-balloon.
> >
> > Well it doesn't really, we need cpu_to_virtio in a bunch of
> > places anyway.
>
> Of course, but what about keeping changes minimal?
> >
> > So I kind of prefer making it clean, even just to avoid setting a bad
> > example for other devices.
> >
> > Let me post the new patch where it's all fixed in a cleaner way, and
> > everyone can discuss whether it's too much work.
> >