diff mbox series

vhost: fix get hpa fail from guest pages

Message ID 20211111062725.108297-1-yuanx.wang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series vhost: fix get hpa fail from guest pages |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS

Commit Message

Wang, YuanX Nov. 11, 2021, 6:27 a.m. UTC 
  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(-)

Comments

Chenbo Xia Nov. 15, 2021, 8:04 a.m. UTC | #1 
> -----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?

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
Chenbo Xia Nov. 16, 2021, 8:07 a.m. UTC | #2 
> -----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.

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
Ding, Xuan Nov. 16, 2021, 8:47 a.m. UTC | #3 
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
Maxime Coquelin Nov. 16, 2021, 8:51 a.m. UTC | #4 
Hi,

On 11/16/21 09:47, Ding, Xuan wrote:
> 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.

I'm sorry, but I'm not sure to understand your explanation.
Could you please rephrase it?

Thanks,
Maxime

> 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
>
Chenbo Xia Nov. 16, 2021, 9:01 a.m. UTC | #5 
> -----Original Message-----
> From: Ding, Xuan <xuan.ding@intel.com>
> Sent: Tuesday, November 16, 2021 4:47 PM
> 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>; 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
> 
> 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.

I was not saying merging should be done, but merging with correct logic.

That is to say, the pages that should be merged will still be merged, but make the
Logic to handle all cases.

Thanks,
Chenbo

> 
> 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
Ding, Xuan Nov. 16, 2021, 10:08 a.m. UTC | #6 
Hi,

> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: 2021年11月16日 17:01
> To: Ding, Xuan <xuan.ding@intel.com>; Wang, YuanX <yuanx.wang@intel.com>;
> maxime.coquelin@redhat.com
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@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: Ding, Xuan <xuan.ding@intel.com>
> > Sent: Tuesday, November 16, 2021 4:47 PM
> > 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>; 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
> >
> > 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.
> 
> I was not saying merging should be done, but merging with correct logic.
> 
> That is to say, the pages that should be merged will still be merged, but make
> the Logic to handle all cases.

Sorry, I misunderstood your point. I was to add a supplement,
PA mode will have lots of pages to map for 4K/2M cases,
it may exceed IOMMU's capability. But it is rational, that's what I hope
to explain in the doc.

Thanks,
Xuan

> 
> Thanks,
> Chenbo
> 
> >
> > 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
Maxime Coquelin Nov. 16, 2021, 10:21 a.m. UTC | #7 
On 11/16/21 11:08, Ding, Xuan wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Xia, Chenbo <chenbo.xia@intel.com>
>> Sent: 2021年11月16日 17:01
>> To: Ding, Xuan <xuan.ding@intel.com>; Wang, YuanX <yuanx.wang@intel.com>;
>> maxime.coquelin@redhat.com
>> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@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: Ding, Xuan <xuan.ding@intel.com>
>>> Sent: Tuesday, November 16, 2021 4:47 PM
>>> 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>; 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
>>>
>>> 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.
>>
>> I was not saying merging should be done, but merging with correct logic.
>>
>> That is to say, the pages that should be merged will still be merged, but make
>> the Logic to handle all cases.
> 
> Sorry, I misunderstood your point. I was to add a supplement,
> PA mode will have lots of pages to map for 4K/2M cases,
> it may exceed IOMMU's capability. But it is rational, that's what I hope
> to explain in the doc.

We are running out of time for v21.11 release. Do you agree to postpone
the fix to new release? It can be backported to v21.11 LTS later.

> Thanks,
> Xuan
> 
>>
>> Thanks,
>> Chenbo
>>
>>>
>>> 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
>
diff mbox series

Patch

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;
+			}
+
+			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;
+			}
 		}
 	}