RE: [PATCH] vhost: fix get hpa fail from guest pages

["Ding, Xuan" <xuan.ding@intel.com>]

Hi Chenbo,

> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: 2021年11月16日 16:07
> To: Xia, Chenbo <chenbo.xia@intel.com>; Wang, YuanX
> <yuanx.wang@intel.com>; maxime.coquelin@redhat.com
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; Ding, Xuan
> <xuan.ding@intel.com>; Ma, WenwuX <wenwux.ma@intel.com>; He,
> Xingguang <xingguang.he@intel.com>; Yang, YvonneX
> <yvonnex.yang@intel.com>
> Subject: RE: [PATCH] vhost: fix get hpa fail from guest pages
> 
> > -----Original Message-----
> > From: Xia, Chenbo <chenbo.xia@intel.com>
> > Sent: Monday, November 15, 2021 4:04 PM
> > To: Wang, YuanX <yuanx.wang@intel.com>; maxime.coquelin@redhat.com
> > Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; Ding, Xuan
> > <xuan.ding@intel.com>; Ma, WenwuX <wenwux.ma@intel.com>; He,
> Xingguang
> > <xingguang.he@intel.com>; Yang, YvonneX <yvonnex.yang@intel.com>
> > Subject: RE: [PATCH] vhost: fix get hpa fail from guest pages
> >
> > > -----Original Message-----
> > > From: Wang, YuanX <yuanx.wang@intel.com>
> > > Sent: Thursday, November 11, 2021 2:27 PM
> > > To: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
> > > Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; Ding, Xuan
> > > <xuan.ding@intel.com>; Ma, WenwuX <wenwux.ma@intel.com>; He,
> > > Xingguang <xingguang.he@intel.com>; Yang, YvonneX
> > > <yvonnex.yang@intel.com>; Wang,
> > YuanX
> > > <yuanx.wang@intel.com>
> > > Subject: [PATCH] vhost: fix get hpa fail from guest pages
> > >
> > > When processing front-end memory regions messages, vhost saves the
> > > guest/host physical address mappings to guest pages and merges
> > > adjacent contiguous pages if hpa is contiguous, however gpa is
> > > likely not contiguous in PA mode and merging will cause the gpa
> > > range to change.
> > > This patch distinguishes the case of discontinuous gpa and does a
> > > range lookup on gpa when doing a binary search.
> > >
> > > Fixes: e246896178e("vhost: get guest/host physical address
> > > mappings")
> > > Fixes: 6563cf92380 ("vhost: fix async copy on multi-page buffers")
> > >
> > > Signed-off-by: Yuan Wang <yuanx.wang@intel.com>
> > > ---
> > >  lib/vhost/vhost.h      | 18 ++++++++++++++++--
> > >  lib/vhost/vhost_user.c | 15 +++++++++++----
> > >  2 files changed, 27 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/lib/vhost/vhost.h b/lib/vhost/vhost.h index
> > > 7085e0885c..b3f0c1d07c 100644
> > > --- a/lib/vhost/vhost.h
> > > +++ b/lib/vhost/vhost.h
> > > @@ -587,6 +587,20 @@ static __rte_always_inline int
> > > guest_page_addrcmp(const void *p1,
> > >  	return 0;
> > >  }
> > >
> > > +static __rte_always_inline int guest_page_rangecmp(const void *p1,
> > > +const
> > void
> > > *p2)
> > > +{
> > > +	const struct guest_page *page1 = (const struct guest_page *)p1;
> > > +	const struct guest_page *page2 = (const struct guest_page *)p2;
> > > +
> > > +	if (page1->guest_phys_addr >= page2->guest_phys_addr) {
> > > +		if (page1->guest_phys_addr < page2->guest_phys_addr +
> page2->size)
> > > +			return 0;
> > > +		else
> > > +			return 1;
> > > +	} else
> > > +		return -1;
> > > +}
> > > +
> > >  static __rte_always_inline rte_iova_t  gpa_to_first_hpa(struct
> > > virtio_net *dev, uint64_t gpa,
> > >  	uint64_t gpa_size, uint64_t *hpa_size) @@ -597,9 +611,9 @@
> > > gpa_to_first_hpa(struct virtio_net *dev, uint64_t gpa,
> > >
> > >  	*hpa_size = gpa_size;
> > >  	if (dev->nr_guest_pages >= VHOST_BINARY_SEARCH_THRESH) {
> > > -		key.guest_phys_addr = gpa & ~(dev->guest_pages[0].size - 1);
> > > +		key.guest_phys_addr = gpa;
> > >  		page = bsearch(&key, dev->guest_pages, dev->nr_guest_pages,
> > > -			       sizeof(struct guest_page), guest_page_addrcmp);
> > > +			       sizeof(struct guest_page), guest_page_rangecmp);
> > >  		if (page) {
> > >  			if (gpa + gpa_size <=
> > >  					page->guest_phys_addr + page->size)
> { diff --git
> > > a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c index
> > > a781346c4d..7d58fde458 100644
> > > --- a/lib/vhost/vhost_user.c
> > > +++ b/lib/vhost/vhost_user.c
> > > @@ -999,10 +999,17 @@ add_one_guest_page(struct virtio_net *dev,
> > > uint64_t guest_phys_addr,
> > >  	if (dev->nr_guest_pages > 0) {
> > >  		last_page = &dev->guest_pages[dev->nr_guest_pages - 1];
> > >  		/* merge if the two pages are continuous */
> > > -		if (host_phys_addr == last_page->host_phys_addr +
> > > -				      last_page->size) {
> > > -			last_page->size += size;
> > > -			return 0;
> > > +		if (host_phys_addr == last_page->host_phys_addr +
> > > +last_page->size)
> > > {
> > > +			if (rte_eal_iova_mode() == RTE_IOVA_VA) {
> > > +				last_page->size += size;
> > > +				return 0;
> > > +			}
> >
> > This makes me think about a question: In IOVA_VA mode, what ensures
> > HPA and GPA are both contiguous?
> 
> When I wrote this email, I thought host_phys_addr is HPA but in VA mode, it's
> Host IOVA, aka VA in DPDK's case. So in most cases GPA and VA will be both
> contiguous when the contiguous pages are all in one memory region. But I think
> we should not make such assumption as some vhost master may send GPA-
> contiguous pages in different memory region, then the VA after mmap may not
> be contiguous. 

Now we do async vfio mapping at page granularity.
For 4K/2M pages, without this assumption, pages not be merged may
exceed the IOMMU's capability. This limits only use 1G hugepage for DMA device.

Thanks,
Xuan

> 
> Maxime, What do you think?
> 
> Thanks,
> Chenbo
> 
> >
> > Maxime & Yuan, any thought?
> >
> > Thanks,
> > Chenbo
> >
> > > +
> > > +			if (rte_eal_iova_mode() == RTE_IOVA_PA &&
> > > +				guest_phys_addr == last_page-
> >guest_phys_addr +
> > > last_page->size) {
> > > +				last_page->size += size;
> > > +				return 0;
> > > +			}
> > >  		}
> > >  	}
> > >
> > > --
> > > 2.25.1