On 10.04.20 05:41, Alexander Duyck wrote: > From: Alexander Duyck <alexander.h.du...@linux.intel.com> > > We need to make certain to advertise support for page poison tracking if > we want to actually get data on if the guest will be poisoning pages. So > if free page hinting is active we should add page poisoning support and > let the guest disable it if it isn't using it. > > Page poisoning will result in a page being dirtied on free. As such we > cannot really avoid having to copy the page at least one more time since > we will need to write the poison value to the destination. As such we can > just ignore free page hinting if page poisoning is enabled as it will > actually reduce the work we have to do. > > Signed-off-by: Alexander Duyck <alexander.h.du...@linux.intel.com> > --- > hw/virtio/virtio-balloon.c | 26 ++++++++++++++++++++++---- > include/hw/virtio/virtio-balloon.h | 1 + > 2 files changed, 23 insertions(+), 4 deletions(-) > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > index a4729f7fc930..1c6d36a29a04 100644 > --- a/hw/virtio/virtio-balloon.c > +++ b/hw/virtio/virtio-balloon.c > @@ -531,6 +531,15 @@ static void virtio_balloon_free_page_start(VirtIOBalloon > *s) > return; > } > > + /* > + * If page poisoning is enabled then we probably shouldn't bother with > + * the hinting since the poisoning will dirty the page and invalidate > + * the work we are doing anyway. > + */ > + if (virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
Why not check for the poison value instead? (as you do in patch #3) ? > + return; > + } > + > if (s->free_page_report_cmd_id == UINT_MAX) { > s->free_page_report_cmd_id = > VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN; We should rename all "free_page_report" stuff we can to "free_page_hint"/"free_page_hinting" to avoid confusion (e.g., on my side :) ) before adding free page reporting . (looking at the virtio-balloon linux header, it's also confusing but we're stuck with that - maybe we should add better comments) > @@ -618,12 +627,10 @@ static size_t virtio_balloon_config_size(VirtIOBalloon > *s) > if (s->qemu_4_0_config_size) { > return sizeof(struct virtio_balloon_config); > } > - if (virtio_has_feature(features, VIRTIO_BALLOON_F_PAGE_POISON)) { > + if (virtio_has_feature(features, VIRTIO_BALLOON_F_PAGE_POISON) || > + virtio_has_feature(features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) { > return sizeof(struct virtio_balloon_config); > } > - if (virtio_has_feature(features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) { > - return offsetof(struct virtio_balloon_config, poison_val); > - } I am not sure this change is completely sane. Why is that necessary at all? > return offsetof(struct virtio_balloon_config, free_page_report_cmd_id); > } > > @@ -634,6 +641,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, > uint8_t *config_data) > > config.num_pages = cpu_to_le32(dev->num_pages); > config.actual = cpu_to_le32(dev->actual); > + config.poison_val = cpu_to_le32(dev->poison_val); > > if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED) { > config.free_page_report_cmd_id = > @@ -697,6 +705,9 @@ static void virtio_balloon_set_config(VirtIODevice *vdev, > qapi_event_send_balloon_change(vm_ram_size - > ((ram_addr_t) dev->actual << > VIRTIO_BALLOON_PFN_SHIFT)); > } > + dev->poison_val = virtio_vdev_has_feature(vdev, > + VIRTIO_BALLOON_F_PAGE_POISON) ? > + le32_to_cpu(config.poison_val) : 0; Can we just do a dev->poison_val = 0; if (virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) { dev->poison_val = le32_to_cpu(config.poison_val); } instead? > trace_virtio_balloon_set_config(dev->actual, oldactual); > } > > @@ -706,6 +717,9 @@ static uint64_t virtio_balloon_get_features(VirtIODevice > *vdev, uint64_t f, > VirtIOBalloon *dev = VIRTIO_BALLOON(vdev); > f |= dev->host_features; > virtio_add_feature(&f, VIRTIO_BALLOON_F_STATS_VQ); > + if (virtio_has_feature(f, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) { > + virtio_add_feature(&f, VIRTIO_BALLOON_F_PAGE_POISON); > + } > > return f; > } > @@ -854,6 +868,8 @@ static void virtio_balloon_device_reset(VirtIODevice > *vdev) > g_free(s->stats_vq_elem); > s->stats_vq_elem = NULL; > } > + > + s->poison_val = 0; > } > > static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status) > @@ -916,6 +932,8 @@ static Property virtio_balloon_properties[] = { > VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false), > DEFINE_PROP_BIT("free-page-hint", VirtIOBalloon, host_features, > VIRTIO_BALLOON_F_FREE_PAGE_HINT, false), > + DEFINE_PROP_BIT("x-page-poison", VirtIOBalloon, host_features, > + VIRTIO_BALLOON_F_PAGE_POISON, false), Just curious, why an x- feature? -- Thanks, David / dhildenb