From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nitesh Narayan Lal Subject: Re: [QEMU PATCH] kvm: Support for guest page hinting Date: Mon, 6 Nov 2017 09:21:32 -0500 Message-ID: <7290b1b1-5d39-9e07-3bab-fe7af985aec4@redhat.com> References: <20171103203013.9521-1-nilal@redhat.com> <20171103203701.9826-1-nilal@redhat.com> <20171103203701.9826-2-nilal@redhat.com> <1261138236.27784422.1509967286304.JavaMail.zimbra@redhat.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="g5bhE1Pq4b84AuhD00vVxn189d5Owc8dQ" Cc: kvm@vger.kernel.org, pbonzini@redhat.com, wei w wang , yang zhang wz , riel@redhat.com, david@redhat.com, mst@redhat.com, konrad wilk , dodgen@google.com To: Pankaj Gupta Return-path: Received: from mx1.redhat.com ([209.132.183.28]:51224 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752800AbdKFOVl (ORCPT ); Mon, 6 Nov 2017 09:21:41 -0500 In-Reply-To: <1261138236.27784422.1509967286304.JavaMail.zimbra@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --g5bhE1Pq4b84AuhD00vVxn189d5Owc8dQ Content-Type: multipart/mixed; boundary="rAElx8r0560vNqvM6H9b3UlRTdhCpmeNf"; protected-headers="v1" From: Nitesh Narayan Lal To: Pankaj Gupta Cc: kvm@vger.kernel.org, pbonzini@redhat.com, wei w wang , yang zhang wz , riel@redhat.com, david@redhat.com, mst@redhat.com, konrad wilk , dodgen@google.com Message-ID: <7290b1b1-5d39-9e07-3bab-fe7af985aec4@redhat.com> Subject: Re: [QEMU PATCH] kvm: Support for guest page hinting References: <20171103203013.9521-1-nilal@redhat.com> <20171103203701.9826-1-nilal@redhat.com> <20171103203701.9826-2-nilal@redhat.com> <1261138236.27784422.1509967286304.JavaMail.zimbra@redhat.com> In-Reply-To: <1261138236.27784422.1509967286304.JavaMail.zimbra@redhat.com> --rAElx8r0560vNqvM6H9b3UlRTdhCpmeNf Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Content-Language: en-US Hi Pankaj, On 11/06/2017 06:21 AM, Pankaj Gupta wrote: > Hi Nitesh, > >> This patch enables QEMU to handle page hinting requests >> from the guest. Once the guest kicks QEMU to free a page, >> QEMU retrives the guest physical address and converts it to >> host virtual address and then MADVISE that memory. >> >> Signed-off-by: Nitesh Narayan Lal >> --- >> hw/virtio/virtio-balloon.c | 81 >> ++++++++++++++++++++++++++++++++++++++ >> hw/virtio/virtio.c | 24 +++++++++++ >> include/hw/virtio/virtio-access.h | 1 + >> include/hw/virtio/virtio-balloon.h | 2 +- >> include/qemu/osdep.h | 7 ++++ >> 5 files changed, 114 insertions(+), 1 deletion(-) >> >> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c >> index 37cde38..de10350 100644 >> --- a/hw/virtio/virtio-balloon.c >> +++ b/hw/virtio/virtio-balloon.c >> @@ -33,6 +33,8 @@ >> =20 >> #define BALLOON_PAGE_SIZE (1 << VIRTIO_BALLOON_PFN_SHIFT) >> =20 >> +void page_hinting_request(uint64_t addr, uint32_t len); >> + >> static void balloon_page(void *addr, int deflate) >> { >> if (!qemu_balloon_is_inhibited() && (!kvm_enabled() || >> @@ -205,6 +207,84 @@ static void balloon_stats_set_poll_interval(Objec= t *obj, >> Visitor *v, >> balloon_stats_change_timer(s, 0); >> } >> =20 >> +static void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, Error **errp) >> +{ >> + MemoryRegionSection mrs =3D memory_region_find(get_system_memory(= ), >> + addr, 1); >> + >> + if (!mrs.mr) { >> + error_setg(errp, "No memory is mapped at address 0x%" HWADDR_= PRIx, >> addr); >> + return NULL; >> + } >> + >> + if (!memory_region_is_ram(mrs.mr) && !memory_region_is_romd(mrs.m= r)) { >> + error_setg(errp, "Memory at address 0x%" HWADDR_PRIx "is not = RAM", >> addr); >> + memory_region_unref(mrs.mr); >> + return NULL; >> + } >> + >> + *p_mr =3D mrs.mr; >> + return qemu_map_ram_ptr(mrs.mr->ram_block, mrs.offset_within_regi= on); >> +} >> + >> +struct guest_pages { >> + unsigned long pfn; >> + unsigned int pages; >> +}; >> + >> + >> +void page_hinting_request(uint64_t addr, uint32_t len) >> +{ >> + Error *local_err =3D NULL; >> + MemoryRegion *mr =3D NULL; >> + void *hvaddr; >> + int ret =3D 0; >> + struct guest_pages *guest_obj; >> + int i =3D 0; >> + void *hvaddr_to_free; >> + unsigned long pfn, pfn_end; >> + uint64_t gpaddr_to_free; >> + >> + /*ptr is the host physical address*/ > Could not understand the comment? It's a typo, It should have been something like "hvaddr is the the host virtual address obtained by converting guest physical address". I will correct this in the next patch-set. > >> + hvaddr =3D gpa2hva(&mr, addr, &local_err); >> + if (local_err) { >> + error_report_err(local_err); >> + return; >> + } >> + guest_obj =3D hvaddr; >> + >> + while (i < len) { >> + pfn =3D guest_obj[i].pfn; >> + pfn_end =3D guest_obj[i].pfn + guest_obj[i].pages - 1; >> + while (pfn <=3D pfn_end) { >> + gpaddr_to_free =3D pfn << VIRTIO_BALLOON_PFN_SHIFT; >> + hvaddr_to_free =3D gpa2hva(&mr, gpaddr_to_free, &local_err);= >> + if (local_err) { >> + error_report_err(local_err); >> + return; >> + } >> + ret =3D qemu_madvise((void *)hvaddr_to_free, 4096, QEMU_MADV_FREE);= >> + if (ret =3D=3D -1) >> + printf("\n%d:%s Error: Madvise failed with error:%d\n", __LINE_= _, >> __func__, ret); >> + pfn++; >> + } >> + i++; >> + } >> +} >> + >> + >> +static void virtio_balloon_page_hinting(VirtIODevice *vdev, VirtQueue= *vq) >> +{ >> + uint64_t addr; >> + uint32_t len; >> + VirtQueueElement elem =3D {}; >> + >> + pop_hinting_addr(vq, &addr, &len); >> + page_hinting_request(addr, len); >> + virtqueue_push(vq, &elem, 0); >> + virtio_notify(vdev, vq); >> +} >> + >> static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueu= e *vq) >> { >> VirtIOBalloon *s =3D VIRTIO_BALLOON(vdev); >> @@ -443,6 +523,7 @@ static void virtio_balloon_device_realize(DeviceSt= ate >> *dev, Error **errp) >> s->ivq =3D virtio_add_queue(vdev, 128, virtio_balloon_handle_outp= ut); >> s->dvq =3D virtio_add_queue(vdev, 128, virtio_balloon_handle_outp= ut); >> s->svq =3D virtio_add_queue(vdev, 128, virtio_balloon_receive_sta= ts); >> + s->hvq =3D virtio_add_queue(vdev, 128, virtio_balloon_page_hintin= g); >> =20 >> reset_stats(s); >> } >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> index 311929e..24e26ec 100644 >> --- a/hw/virtio/virtio.c >> +++ b/hw/virtio/virtio.c >> @@ -825,6 +825,30 @@ static void *virtqueue_alloc_element(size_t sz, u= nsigned >> out_num, unsigned in_nu >> return elem; >> } >> =20 >> +void pop_hinting_addr(VirtQueue *vq, uint64_t *addr, uint32_t *len) >> +{ >> + VRingMemoryRegionCaches *caches; >> + VRingDesc desc; >> + MemoryRegionCache *desc_cache; >> + VirtIODevice *vdev =3D vq->vdev; >> + unsigned int head, i, max; >> + >> + max =3D vq->vring.num; >> + if (!virtqueue_get_head(vq, vq->last_avail_idx++, &head)) { >> + printf("\n%d:%sError: Unable to read head\n", __LINE__, __func__); >> + } >> + i =3D head; > we can avoid 'i' here and directly pass head > 'to vring_desc_read' function? Yes, that's right. Thank you for pointing out. I will correct this in the next version. > >> + >> + caches =3D vring_get_region_caches(vq); >> + if (caches->desc.len < max * sizeof(VRingDesc)) { >> + virtio_error(vdev, "Cannot map descriptor ring"); >> + } >> + desc_cache =3D &caches->desc; >> + vring_desc_read(vdev, &desc, desc_cache, i); >> + *addr =3D desc.addr; >> + *len =3D desc.len; >> +} >> + >> void *virtqueue_pop(VirtQueue *vq, size_t sz) >> { >> unsigned int i, head, max; >> diff --git a/include/hw/virtio/virtio-access.h >> b/include/hw/virtio/virtio-access.h >> index 2e92074..568d71f 100644 >> --- a/include/hw/virtio/virtio-access.h >> +++ b/include/hw/virtio/virtio-access.h >> @@ -24,6 +24,7 @@ >> #define LEGACY_VIRTIO_IS_BIENDIAN 1 >> #endif >> =20 >> +void pop_hinting_addr(VirtQueue *vq, uint64_t *addr, uint32_t *len); >> static inline bool virtio_access_is_big_endian(VirtIODevice *vdev) >> { >> #if defined(LEGACY_VIRTIO_IS_BIENDIAN) >> diff --git a/include/hw/virtio/virtio-balloon.h >> b/include/hw/virtio/virtio-balloon.h >> index 1ea13bd..dfb5782 100644 >> --- a/include/hw/virtio/virtio-balloon.h >> +++ b/include/hw/virtio/virtio-balloon.h >> @@ -33,7 +33,7 @@ typedef struct virtio_balloon_stat_modern { >> =20 >> typedef struct VirtIOBalloon { >> VirtIODevice parent_obj; >> - VirtQueue *ivq, *dvq, *svq; >> + VirtQueue *ivq, *dvq, *svq, *hvq; >> uint32_t num_pages; >> uint32_t actual; >> uint64_t stats[VIRTIO_BALLOON_S_NR]; >> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h >> index 9dd318a..033d64c 100644 >> --- a/include/qemu/osdep.h >> +++ b/include/qemu/osdep.h >> @@ -278,6 +278,11 @@ void qemu_anon_ram_free(void *ptr, size_t size); >> #else >> #define QEMU_MADV_REMOVE QEMU_MADV_INVALID >> #endif >> +#ifdef MADV_FREE >> +#define QEMU_MADV_FREE MADV_FREE >> +#else >> +#define QEMU_MADV_FREE QEMU_MAD_INVALID >> +#endif >> =20 >> #elif defined(CONFIG_POSIX_MADVISE) >> =20 >> @@ -291,6 +296,7 @@ void qemu_anon_ram_free(void *ptr, size_t size); >> #define QEMU_MADV_HUGEPAGE QEMU_MADV_INVALID >> #define QEMU_MADV_NOHUGEPAGE QEMU_MADV_INVALID >> #define QEMU_MADV_REMOVE QEMU_MADV_INVALID >> +#define QEMU_MADV_FREE QEMU_MAD_INVALID >> =20 >> #else /* no-op */ >> =20 >> @@ -304,6 +310,7 @@ void qemu_anon_ram_free(void *ptr, size_t size); >> #define QEMU_MADV_HUGEPAGE QEMU_MADV_INVALID >> #define QEMU_MADV_NOHUGEPAGE QEMU_MADV_INVALID >> #define QEMU_MADV_REMOVE QEMU_MADV_INVALID >> +#define QEMU_MADV_FREE QEMU_MAD_INVALID >> =20 >> #endif >> =20 >> -- >> 2.9.4 >> >> --=20 Regards Nitesh --rAElx8r0560vNqvM6H9b3UlRTdhCpmeNf-- --g5bhE1Pq4b84AuhD00vVxn189d5Owc8dQ Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJaAG/tAAoJEKOGQNwGMqM5XucP/1yfuiTeg54/oLR7WDRhXiUg tvJYDcm6LaTeqIF/hiwFR8IJvWRC/yd6syHqev3TYhpRXYrUmEfV4nFEKb07chCA E5283B3ikDDl1xTWJIUnsKS3LwL4X8rCsy7/PLKST0SruqJuH79LXsmbpmeeqY4i xWAYQqEe7vPSADGamkoDqu5mpUxLHfJuFVcQ+1qAJKoGB/oulTNL0nUstTJKNCXG d4d6vZ6oxBH8UGy7ckD0jRG69Cyv9ryrWHzd5JErRBMBmzzsjvBAiXIl5F/w0/b8 s+Riv1dbEPvNYGpk+vThuZX6VNRTnCyeuTZJvev3ahxpptA97cXKHfOuaCNUisl4 B/gNwwczA16VRR3bTV2fugQCa+L7TS2d9IjFMNN7byg5MWvkbpXxHu7fPMFU89Ue PpBhVHkbQUtQIO2v0Y7EDKGlE+cQpwjpSOl/TBEYm/aDb5YTyVFSOr/k9yu5VffB dKCm3s0BijkWbsYqh+ffV3j28E6R/8Gv0/Ur7WyxUGhofHBlEIfBFABUqDU7/Mn7 RwLD4xmlUE/qFdM8y+bj08GD888SvOOMtJbe5dNskTlYyM7mUtIzI96P0TJexOuP ZpXEJ3oQBUXMHzau5oMGRg4tXKX3DOrdgmxD8PXEckqQlPJA6W0WNwuAQ9+idCcY +LBloLcNjBILZp/ue1GI =Jhbn -----END PGP SIGNATURE----- --g5bhE1Pq4b84AuhD00vVxn189d5Owc8dQ--