The IOTLB that it returned didn't guarantee that page_mask is indeed a
so-called page mask. That won't affect current usage since now only
vhost is using it (vhost API allows arbitary IOTLB range). However we
have IOTLB scemantic and we should best follow it. This patch fixes this
issue to make sure the page_mask is always a valid page mask.

Fixes: a764040 ("exec: abstract address_space_do_translate()")
Signed-off-by: Peter Xu <peterx@redhat.com>
---
Sorry please use this one. The codes is merely the same, just avoided
moving the codes around unnecessarily.
---
 exec.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/exec.c b/exec.c
index ff16f04..3db247c 100644
--- a/exec.c
+++ b/exec.c
@@ -529,16 +529,14 @@ IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
         section.offset_within_region;
 
     if (plen == (hwaddr)-1) {
-        /*
-         * We use default page size here. Logically it only happens
-         * for identity mappings.
-         */
-        plen = TARGET_PAGE_SIZE;
+        /* If not specified during translation, use default mask */
+        plen = TARGET_PAGE_MASK;
+    } else {
+        /* Make it a valid page mask */
+        assert(plen);
+        plen = (1ULL << (63 - clz64(plen))) - 1;
     }
 
-    /* Convert to address mask */
-    plen -= 1;
-
     return (IOMMUTLBEntry) {
         .target_as = section.address_space,
         .iova = addr & ~plen,
-- 
2.7.4

On 29/05/2017 06:02, Peter Xu wrote:
> The IOTLB that it returned didn't guarantee that page_mask is indeed a
> so-called page mask. That won't affect current usage since now only
> vhost is using it (vhost API allows arbitary IOTLB range). However we
> have IOTLB scemantic and we should best follow it. This patch fixes this
> issue to make sure the page_mask is always a valid page mask.
> 
> Fixes: a764040 ("exec: abstract address_space_do_translate()")
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> Sorry please use this one. The codes is merely the same, just avoided
> moving the codes around unnecessarily.
> ---
>  exec.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index ff16f04..3db247c 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -529,16 +529,14 @@ IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
>          section.offset_within_region;
>  
>      if (plen == (hwaddr)-1) {
> -        /*
> -         * We use default page size here. Logically it only happens
> -         * for identity mappings.
> -         */
> -        plen = TARGET_PAGE_SIZE;
> +        /* If not specified during translation, use default mask */
> +        plen = TARGET_PAGE_MASK;
> +    } else {
> +        /* Make it a valid page mask */
> +        assert(plen);
> +        plen = (1ULL << (63 - clz64(plen))) - 1;

Would pow2floor be better?

Thanks,

Paolo

>      }
>  
> -    /* Convert to address mask */
> -    plen -= 1;
> -
>      return (IOMMUTLBEntry) {
>          .target_as = section.address_space,
>          .iova = addr & ~plen,
> 

On Tue, May 30, 2017 at 06:23:14PM +0200, Paolo Bonzini wrote:
> 
> 
> On 29/05/2017 06:02, Peter Xu wrote:
> > The IOTLB that it returned didn't guarantee that page_mask is indeed a
> > so-called page mask. That won't affect current usage since now only
> > vhost is using it (vhost API allows arbitary IOTLB range). However we
> > have IOTLB scemantic and we should best follow it. This patch fixes this
> > issue to make sure the page_mask is always a valid page mask.
> > 
> > Fixes: a764040 ("exec: abstract address_space_do_translate()")
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > Sorry please use this one. The codes is merely the same, just avoided
> > moving the codes around unnecessarily.
> > ---
> >  exec.c | 14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
> > 
> > diff --git a/exec.c b/exec.c
> > index ff16f04..3db247c 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -529,16 +529,14 @@ IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
> >          section.offset_within_region;
> >  
> >      if (plen == (hwaddr)-1) {
> > -        /*
> > -         * We use default page size here. Logically it only happens
> > -         * for identity mappings.
> > -         */
> > -        plen = TARGET_PAGE_SIZE;
> > +        /* If not specified during translation, use default mask */
> > +        plen = TARGET_PAGE_MASK;
> > +    } else {
> > +        /* Make it a valid page mask */
> > +        assert(plen);
> > +        plen = (1ULL << (63 - clz64(plen))) - 1;
> 
> Would pow2floor be better?

Yes. :) Will respin. Thanks!

-- 
Peter Xu