search for: virtio_balloon_cmd_id_received

...h accessing to the config space inside an interrupt context, this patch series avoids that issue by moving the config register accesses to the related workqueue contexts. v2->v3 ChangeLog: - rename cmd_id_received to cmd_id_received_cache, and have call sites read the latest value via virtio_balloon_cmd_id_received. (Still kept Cornelia and Halil's reviewed-by as it's a minor change) - remove zeroing vb->num_free_page_blocks in probe since vb is allocated via kzalloc. v1->v2 ChangeLog: - add config_read_bitmap to indicate to the workqueue callbacks about the necessity o...

...ions(+), 1 deletion(-) diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 774deb65a9bb..798ec304fe3e 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -578,10 +578,14 @@ static int init_vqs(struct virtio_balloon *vb) static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb) { if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID, - &vb->config_read_bitmap)) + &vb->config_read_bitmap)) { virtio_cread(vb->vdev, struct virtio_balloon_config, free_page_hint_cmd_id, &vb->cmd_id_...

...io/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -596,7 +596,7 @@ static int send_cmd_id_start(struct virtio_balloon *vb) while (virtqueue_get_buf(vq, &unused)) ; - vb->cmd_id_active = virtio32_to_cpu(vb->vdev, + vb->cmd_id_active = cpu_to_virtio32(vb->vdev, virtio_balloon_cmd_id_received(vb)); sg_init_one(&sg, &vb->cmd_id_active, sizeof(vb->cmd_id_active)); err = virtqueue_add_outbuf(vq, &sg, 1, &vb->cmd_id_active, GFP_KERNEL); -- MST

...ss > maybe we should make certain to cast this as a le32 value. I can > submit a patch to do that. Thanks! But not yet - I am poking at the endian-ness things right now! > > As you say correctly the command id is actually assumed native endian: > > > > > > static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb) > > { > > if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID, > > &vb->config_read_bitmap)) > > virtio_cread(vb->vdev, struct virtio_balloon_config, > >...

...ss > maybe we should make certain to cast this as a le32 value. I can > submit a patch to do that. Thanks! But not yet - I am poking at the endian-ness things right now! > > As you say correctly the command id is actually assumed native endian: > > > > > > static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb) > > { > > if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID, > > &vb->config_read_bitmap)) > > virtio_cread(vb->vdev, struct virtio_balloon_config, > >...

...Duyck wrote: > > > > On Fri, Jul 10, 2020 at 4:31 AM Michael S. Tsirkin <mst at redhat.com> wrote: > > > > > <snip> > > > As you say correctly the command id is actually assumed native endian: > > > > > > > > > static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb) > > > { > > > if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID, > > > &vb->config_read_bitmap)) > > > virtio_cread(vb->vdev, struct virtio_balloon_config, > &gt...

....h | 11 +++++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 0ef16566c3f3..95d9c2f0a7be 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -580,7 +580,7 @@ static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb) if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID, &vb->config_read_bitmap)) virtio_cread(vb->vdev, struct virtio_balloon_config, - free_page_report_cmd_id, + free_page_hint_cmd_id, &vb->cmd_id_received_cache)...

...irtio_balloon_config, poison_val, &poison_val); actually this writes a native endian-ness value. All bytes happen to be the same though, and host only cares about 0 or non 0 ATM. As you say correctly the command id is actually assumed native endian: static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb) { if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID, &vb->config_read_bitmap)) virtio_cread(vb->vdev, struct virtio_balloon_config, free_page_hint_cmd_id,...

....h | 11 +++++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 0ef16566c3f3..95d9c2f0a7be 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -580,7 +580,7 @@ static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb) if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID, &vb->config_read_bitmap)) virtio_cread(vb->vdev, struct virtio_balloon_config, - free_page_report_cmd_id, + free_page_hint_cmd_id, &vb->cmd_id_received_cache)...

...irtio_balloon_config, poison_val, &poison_val); actually this writes a native endian-ness value. All bytes happen to be the same though, and host only cares about 0 or non 0 ATM. As you say correctly the command id is actually assumed native endian: static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb) { if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID, &vb->config_read_bitmap)) virtio_cread(vb->vdev, struct virtio_balloon_config, free_page_hint_cmd_id,...

...truct virtio_balloon, > + report_free_page_work); > + > + virtio_balloon_read_cmd_id_received(vb); This will not achieve what you are trying to do, which is cancel reporting if it's in progress. You need to re-read each time you compare to cmd_id_active. An API similar to u32 virtio_balloon_cmd_id_received(vb) seems to be called for, and I would rename cmd_id_received to cmd_id_received_cache to make sure we caught all users. > + if (vb->cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) { > + /* Pass ULONG_MAX to give back all the free pages */ > + return_free_pages_to_mm(vb, ULONG_MAX)...

...truct virtio_balloon, > + report_free_page_work); > + > + virtio_balloon_read_cmd_id_received(vb); This will not achieve what you are trying to do, which is cancel reporting if it's in progress. You need to re-read each time you compare to cmd_id_active. An API similar to u32 virtio_balloon_cmd_id_received(vb) seems to be called for, and I would rename cmd_id_received to cmd_id_received_cache to make sure we caught all users. > + if (vb->cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) { > + /* Pass ULONG_MAX to give back all the free pages */ > + return_free_pages_to_mm(vb, ULONG_MAX)...

...| 13 ++++++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 0ef16566c3f3..95d9c2f0a7be 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -580,7 +580,7 @@ static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb) if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID, &vb->config_read_bitmap)) virtio_cread(vb->vdev, struct virtio_balloon_config, - free_page_report_cmd_id, + free_page_hint_cmd_id, &vb->cmd_id_received_cache)...

...tio_balloon_read_cmd_id_received(vb); > > This will not achieve what you are trying to do, which is cancel reporting if it's > in progress. > > You need to re-read each time you compare to cmd_id_active. Yes, already did, please see [1] above > An API similar to > u32 virtio_balloon_cmd_id_received(vb) > seems to be called for, and I would rename cmd_id_received to > cmd_id_received_cache to make sure we caught all users. > I'm not sure about adding "cache" here, cmd_id_received refers to the cmd id that the driver just received from the device. There is one called &q...

...changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > index 0ef16566c3f3..95d9c2f0a7be 100644 > --- a/drivers/virtio/virtio_balloon.c > +++ b/drivers/virtio/virtio_balloon.c > @@ -580,7 +580,7 @@ static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb) > if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID, > &vb->config_read_bitmap)) > virtio_cread(vb->vdev, struct virtio_balloon_config, > - free_page_report_cmd_id, > + free_page_hint_cmd_id, > &amp...

...io/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -600,7 +600,7 @@ static int send_cmd_id_start(struct virtio_balloon *vb) while (virtqueue_get_buf(vq, &unused)) ; - vb->cmd_id_active = virtio32_to_cpu(vb->vdev, + vb->cmd_id_active = cpu_to_virtio32(vb->vdev, virtio_balloon_cmd_id_received(vb)); sg_init_one(&sg, &vb->cmd_id_active, sizeof(vb->cmd_id_active)); err = virtqueue_add_outbuf(vq, &sg, 1, &vb->cmd_id_active, GFP_KERNEL); -- MST

...io/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -600,7 +600,7 @@ static int send_cmd_id_start(struct virtio_balloon *vb) while (virtqueue_get_buf(vq, &unused)) ; - vb->cmd_id_active = virtio32_to_cpu(vb->vdev, + vb->cmd_id_active = cpu_to_virtio32(vb->vdev, virtio_balloon_cmd_id_received(vb)); sg_init_one(&sg, &vb->cmd_id_active, sizeof(vb->cmd_id_active)); err = virtqueue_add_outbuf(vq, &sg, 1, &vb->cmd_id_active, GFP_KERNEL); -- MST

...o_le32(actual); - - virtio_cwrite(vb->vdev, struct virtio_balloon_config, actual, - &actual); + virtio_cwrite_le(vb->vdev, struct virtio_balloon_config, actual, + &actual); } static void update_balloon_stats_func(struct work_struct *work) @@ -579,12 +573,10 @@ static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb) { if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID, &vb->config_read_bitmap)) { - virtio_cread(vb->vdev, struct virtio_balloon_config, - free_page_hint_cmd_id, - &vb->cmd_id_received_cache); /* Legacy balloon config...

...t only cares about 0 or non 0 ATM. So we are safe assuming it is a repeating value, but for correctness maybe we should make certain to cast this as a le32 value. I can submit a patch to do that. > As you say correctly the command id is actually assumed native endian: > > > static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb) > { > if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID, > &vb->config_read_bitmap)) > virtio_cread(vb->vdev, struct virtio_balloon_config, > free_page_hint...

Page reporting features were never supported by legacy hypervisors. Supporting them poses a problem: should we use native endian-ness (like current code assumes)? Or little endian-ness like the virtio spec says? Rather than try to figure out, and since results of incorrect endian-ness are dire, let's just block this configuration. Cc: stable at vger.kernel.org Signed-off-by: Michael S.