virtio-balloon: add support for providing free page reporting

From: Alexander Duyck <[email protected]>

Add support for the page reporting feature provided by virtio-balloon. Reporting differs from the regular balloon functionality in that is is much less durable than a standard memory balloon. Instead of creating a list of pages that cannot be accessed the pages are only inaccessible while they are being indicated to the virtio interface. Once the interface has acknowledged them they are placed back into their respective free lists and are once again accessible by the guest system.

Unlike a standard balloon we don't inflate and deflate the pages. Instead we perform the reporting, and once the reporting is completed it is assumed that the page has been dropped from the guest and will be faulted back in the next time the page is accessed.

This patch is a subset of the UAPI patch that was submitted for the Linux kernel. The original patch can be found at:
https://lore.kernel.org/lkml/[email protected]/

Quoted 15 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Add support for the page reporting feature provided by virtio-balloon. Reporting differs from the regular balloon functionality in that is is much less durable than a standard memory balloon. Instead of creating a list of pages that cannot be accessed the pages are only inaccessible while they are being indicated to the virtio interface. Once the interface has acknowledged them they are placed back into their respective free lists and are once again accessible by the guest system.

Unlike a standard balloon we don't inflate and deflate the pages. Instead we perform the reporting, and once the reporting is completed it is assumed that the page has been dropped from the guest and will be faulted back in the next time the page is accessed.

Quoted 3 lines

This patch is a subset of the UAPI patch that was submitted for the Linux kernel. The original patch can be found at:
https://lore.kernel.org/lkml/[email protected]/

You don't need all these comments.

Usually we do

"linux-headers: update to contain virito-balloon free page reporting

Let's sync the latest upstream changes for free page reporting. To be replaced by a full linux header sync.

Signed-off-by Alexander Duyck <[email protected]>

"

mst will replace this by a full header sync (if necessary) when sending it upstream

Quoted 21 lines · David Hildenbrand

Quoted 18 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Add support for the page reporting feature provided by virtio-balloon. Reporting differs from the regular balloon functionality in that is is much less durable than a standard memory balloon. Instead of creating a list of pages that cannot be accessed the pages are only inaccessible while they are being indicated to the virtio interface. Once the interface has acknowledged them they are placed back into their respective free lists and are once again accessible by the guest system.

Unlike a standard balloon we don't inflate and deflate the pages. Instead we perform the reporting, and once the reporting is completed it is assumed that the page has been dropped from the guest and will be faulted back in the next time the page is accessed.

This patch is a subset of the UAPI patch that was submitted for the Linux kernel. The original patch can be found at:
https://lore.kernel.org/lkml/[email protected]/

Quoted 1 line

You don't need all these comments.

Sorry about that. Those are basically the same comments from the original upstream patch. I just wasn't aware of the process so I just copied that over and added the comment/link to the original patch.

From: Alexander Duyck <[email protected]>

Add support for what I am referring to as "free page reporting".

Basically the idea is to function very similar to how the balloon works in that we basically end up madvising the page as not being used. However

we don't really need to bother with any deflate type logic since the page will be faulted back into the guest when it is read or written to.

This is meant to be a simplification of the existing balloon interface to use for providing hints to what memory needs to be freed. I am assuming

this is safe to do as the deflate logic does not actually appear to do very much other than tracking what subpages have been released and which ones haven't.

Signed-off-by Alexander Duyck <[email protected]>

---
 hw/virtio/virtio-balloon.c         |   48 +++++++++++++++++++++++++++++++++++-
 include/hw/virtio/virtio-balloon.h |    2 +-
 2 files changed, 47 insertions(+), 3 deletions(-)

hw/virtio/virtio-balloon.c +8 -0

index 1c6d36a29a04..297b267198ac 100644
@@ -321,6 +321,42 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v,
     balloon_stats_change_timer(s, 0);
 }
 
+static void virtio_balloon_handle_report(VirtIODevice *vdev, VirtQueue *vq)
+{
+    VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
+    VirtQueueElement *elem;
+
+    while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) {
+        unsigned int i;
+

hw/virtio/virtio-balloon.c +12 -0

+                continue;
+            }
+
+            ram_block_discard_range(rb, ram_offset, size);
+        }
+
+        virtqueue_push(vq, elem, 0);
+        virtio_notify(vdev, vq);
+        g_free(elem);
+    }
+}
+

Quoted 3 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Add support for what I am referring to as "free page reporting".

"Add support for "free page reporting".

Quoted 2 lines

Basically the idea is to function very similar to how the balloon works in that we basically end up madvising the page as not being used. However

I'd get rid of one "basically".

Quoted 5 lines

we don't really need to bother with any deflate type logic since the page will be faulted back into the guest when it is read or written to.

This is meant to be a simplification of the existing balloon interface to use for providing hints to what memory needs to be freed. I am assuming

It's not really a simplification, it's something new. It's a new way of letting the guest automatically report free pages to the hypervisor, so the hypervisor can reuse them. In contrast to inflate/deflate, that's triggered via the hypervisor explicitly.

Quoted 3 lines

this is safe to do as the deflate logic does not actually appear to do very much other than tracking what subpages have been released and which ones haven't.

"I assume this is safe" does not sound very confident. Can we just drop the last sentence?

Quoted 24 lines

Signed-off-by Alexander Duyck <[email protected]>

---
 hw/virtio/virtio-balloon.c         |   48 +++++++++++++++++++++++++++++++++++-
 include/hw/virtio/virtio-balloon.h |    2 +-
 2 files changed, 47 insertions(+), 3 deletions(-)

hw/virtio/virtio-balloon.c +8 -0

index 1c6d36a29a04..297b267198ac 100644
@@ -321,6 +321,42 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v,
     balloon_stats_change_timer(s, 0);
 }
 
+static void virtio_balloon_handle_report(VirtIODevice *vdev, VirtQueue *vq)
+{
+    VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
+    VirtQueueElement *elem;
+
+    while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) {
+        unsigned int i;
+

hw/virtio/virtio-balloon.c static void balloon_stats_set_poll_interval(Object *obj, Visitor *v, · line 341

Quoted 10 lines

hw/virtio/virtio-balloon.c +10 -0

+        for (i = 0; i < elem->in_num; i++) {
+            void *addr = elem->in_sg[i].iov_base;
+            size_t size = elem->in_sg[i].iov_len;
+            ram_addr_t ram_offset;
+            size_t rb_page_size;
+            RAMBlock *rb;
+
+            if (qemu_balloon_is_inhibited() || dev->poison_val) {
+                continue;
+            }

These checks are not sufficient. See virtio_balloon_handle_output(), where we e.g., check that somebody doesn't try to discard the bios.

Maybe we can factor our/unify the handling in both code paths.

hw/virtio/virtio-balloon.c static void balloon_stats_set_poll_interval(Object *obj, Visitor *v, · line 347

Quoted 6 lines

hw/virtio/virtio-balloon.c +6 -0

+
+            rb = qemu_ram_block_from_host(addr, false, &ram_offset);
+            rb_page_size = qemu_ram_pagesize(rb);
+
+            /* For now we will simply ignore unaligned memory regions */
+            if ((ram_offset | size) & (rb_page_size - 1)) {

"!QEMU_IS_ALIGNED()" please to make this easier to read.

Quoted 12 lines

hw/virtio/virtio-balloon.c +12 -0

+                continue;
+            }
+
+            ram_block_discard_range(rb, ram_offset, size);
+        }
+
+        virtqueue_push(vq, elem, 0);
+        virtio_notify(vdev, vq);
+        g_free(elem);
+    }
+}
+

[...]

hw/virtio/virtio-balloon.c static Property virtio_balloon_properties[] = { · line 985

Quoted 8 lines

     if (virtio_has_feature(s->host_features,
                            VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
         s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,

hw/virtio/virtio-balloon.c +1 -0

@@ -940,6 +982,8 @@ static Property virtio_balloon_properties[] = {
      */
     DEFINE_PROP_BOOL("qemu-4-0-config-size", VirtIOBalloon,
                      qemu_4_0_config_size, false),
+    DEFINE_PROP_BIT("unused-page-reporting", VirtIOBalloon, host_features,

"free-page-reporting"

Quoted 24 lines · David Hildenbrand

Quoted 3 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Add support for what I am referring to as "free page reporting".

"Add support for "free page reporting".

Quoted 2 lines

Basically the idea is to function very similar to how the balloon works in that we basically end up madvising the page as not being used. However

I'd get rid of one "basically".

Quoted 5 lines

we don't really need to bother with any deflate type logic since the page will be faulted back into the guest when it is read or written to.

This is meant to be a simplification of the existing balloon interface to use for providing hints to what memory needs to be freed. I am assuming

It's not really a simplification, it's something new. It's a new way of letting the guest automatically report free pages to the hypervisor, so the hypervisor can reuse them. In contrast to inflate/deflate, that's triggered via the hypervisor explicitly.

Quoted 6 lines

Quoted 3 lines

this is safe to do as the deflate logic does not actually appear to do very much other than tracking what subpages have been released and which ones haven't.

"I assume this is safe" does not sound very confident. Can we just drop the last sentence?

Agreed. I will make the requested changes to the patch description.

Quoted 38 lines

Quoted 24 lines

Signed-off-by Alexander Duyck <[email protected]>

---
 hw/virtio/virtio-balloon.c         |   48 +++++++++++++++++++++++++++++++++++-
 include/hw/virtio/virtio-balloon.h |    2 +-
 2 files changed, 47 insertions(+), 3 deletions(-)

hw/virtio/virtio-balloon.c +8 -0

index 1c6d36a29a04..297b267198ac 100644
@@ -321,6 +321,42 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v,
     balloon_stats_change_timer(s, 0);
 }
 
+static void virtio_balloon_handle_report(VirtIODevice *vdev, VirtQueue *vq)
+{
+    VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
+    VirtQueueElement *elem;
+
+    while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) {
+        unsigned int i;
+

hw/virtio/virtio-balloon.c static void balloon_stats_set_poll_interval(Object *obj, Visitor *v, · line 341

Quoted 10 lines

hw/virtio/virtio-balloon.c +10 -0

+        for (i = 0; i < elem->in_num; i++) {
+            void *addr = elem->in_sg[i].iov_base;
+            size_t size = elem->in_sg[i].iov_len;
+            ram_addr_t ram_offset;
+            size_t rb_page_size;
+            RAMBlock *rb;
+
+            if (qemu_balloon_is_inhibited() || dev->poison_val) {
+                continue;
+            }

These checks are not sufficient. See virtio_balloon_handle_output(), where we e.g., check that somebody doesn't try to discard the bios.

Quoted 1 line

Maybe we can factor our/unify the handling in both code paths.

I am going to have to look at this further. If I understand correctly you are asking me to add all of the memory section checks that are in virtio_balloon_handle_output correct?

I'm not sure it makes sense with the scatterlist approach I have taken here. All of the mappings were created as a scatterlist of writable DMA mappings unlike the regular balloon which is just stuffing PFN numbers into an array and then sending the array across. As such I would think there are already such protections in place. For instance, you wouldn't want to let virtio-net map the BIOS and request data be written into that either, correct? So I am assuming there is something there to prevent that already isn't there?

Quoted 40 lines · Alexander Duyck

Quoted 34 lines

Quoted 20 lines

 hw/virtio/virtio-balloon.c         |   48 +++++++++++++++++++++++++++++++++++-
 include/hw/virtio/virtio-balloon.h |    2 +-
 2 files changed, 47 insertions(+), 3 deletions(-)

hw/virtio/virtio-balloon.c +8 -0

index 1c6d36a29a04..297b267198ac 100644
@@ -321,6 +321,42 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v,
     balloon_stats_change_timer(s, 0);
 }
 
+static void virtio_balloon_handle_report(VirtIODevice *vdev, VirtQueue *vq)
+{
+    VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
+    VirtQueueElement *elem;
+
+    while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) {
+        unsigned int i;
+

hw/virtio/virtio-balloon.c static void balloon_stats_set_poll_interval(Object *obj, Visitor *v, · line 341

Quoted 10 lines

hw/virtio/virtio-balloon.c +10 -0

+        for (i = 0; i < elem->in_num; i++) {
+            void *addr = elem->in_sg[i].iov_base;
+            size_t size = elem->in_sg[i].iov_len;
+            ram_addr_t ram_offset;
+            size_t rb_page_size;
+            RAMBlock *rb;
+
+            if (qemu_balloon_is_inhibited() || dev->poison_val) {
+                continue;
+            }

These checks are not sufficient. See virtio_balloon_handle_output(), where we e.g., check that somebody doesn't try to discard the bios.

Quoted 1 line

Maybe we can factor our/unify the handling in both code paths.

I am going to have to look at this further. If I understand correctly you are asking me to add all of the memory section checks that are in virtio_balloon_handle_output correct?

Quoted 8 lines

I'm not sure it makes sense with the scatterlist approach I have taken here. All of the mappings were created as a scatterlist of writable DMA mappings unlike the regular balloon which is just stuffing PFN numbers into an array and then sending the array across. As such I would think there are already such protections in place. For instance, you wouldn't want to let virtio-net map the BIOS and request data be written into that either, correct? So I am assuming there is something there to prevent that already isn't there?

Good point, let's find out :)