As of 68a8aa5d7264 ("iommu: make map and unmap take a page count,
similar to flush") there's no need anymore to have a loop here.

Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: New.

--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -308,11 +308,9 @@ int iommu_map(struct domain *d, dfn_t df
                    d->domain_id, dfn_x(dfn_add(dfn, i)),
                    mfn_x(mfn_add(mfn, i)), rc);
 
-        while ( i-- )
-            /* if statement to satisfy __must_check */
-            if ( iommu_call(hd->platform_ops, unmap_page, d, dfn_add(dfn, i),
-                            flush_flags) )
-                continue;
+        /* while statement to satisfy __must_check */
+        while ( iommu_unmap(d, dfn, i, flush_flags) )
+            break;
 
         if ( !is_hardware_domain(d) )
             domain_crash(d);

On Mon, Apr 25, 2022 at 10:32:10AM +0200, Jan Beulich wrote:
> As of 68a8aa5d7264 ("iommu: make map and unmap take a page count,
> similar to flush") there's no need anymore to have a loop here.
> 
> Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I wonder whether we should have a macro to ignore returns from
__must_check attributed functions.  Ie:

#define IGNORE_RETURN(exp) while ( exp ) break;

As to avoid confusion (and having to reason) whether the usage of
while is correct.  I always find it confusing to assert such loop
expressions are correct.

Thanks, Roger.

On 03.05.2022 12:25, Roger Pau Monné wrote:
> On Mon, Apr 25, 2022 at 10:32:10AM +0200, Jan Beulich wrote:
>> As of 68a8aa5d7264 ("iommu: make map and unmap take a page count,
>> similar to flush") there's no need anymore to have a loop here.
>>
>> Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> I wonder whether we should have a macro to ignore returns from
> __must_check attributed functions.  Ie:
> 
> #define IGNORE_RETURN(exp) while ( exp ) break;
> 
> As to avoid confusion (and having to reason) whether the usage of
> while is correct.  I always find it confusing to assert such loop
> expressions are correct.

I've been considering some form of wrapper macro (not specifically
the one you suggest), but I'm of two minds: On one hand I agree it
would help readers, but otoh I fear it may make it more attractive
to actually override the __must_check (which really ought to be an
exception).

Jan

On Tue, May 03, 2022 at 04:37:29PM +0200, Jan Beulich wrote:
> On 03.05.2022 12:25, Roger Pau Monné wrote:
> > On Mon, Apr 25, 2022 at 10:32:10AM +0200, Jan Beulich wrote:
> >> As of 68a8aa5d7264 ("iommu: make map and unmap take a page count,
> >> similar to flush") there's no need anymore to have a loop here.
> >>
> >> Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> > I wonder whether we should have a macro to ignore returns from
> > __must_check attributed functions.  Ie:
> > 
> > #define IGNORE_RETURN(exp) while ( exp ) break;
> > 
> > As to avoid confusion (and having to reason) whether the usage of
> > while is correct.  I always find it confusing to assert such loop
> > expressions are correct.
> 
> I've been considering some form of wrapper macro (not specifically
> the one you suggest), but I'm of two minds: On one hand I agree it
> would help readers, but otoh I fear it may make it more attractive
> to actually override the __must_check (which really ought to be an
> exception).

Well, I think anyone reviewing the code would realize that the error
is being ignored, and hence check that this is actually intended.

Thanks, Roger.

On 25/04/2022 09:32, Jan Beulich wrote:
> As of 68a8aa5d7264 ("iommu: make map and unmap take a page count,
> similar to flush") there's no need anymore to have a loop here.
>
> Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v3: New.
>
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -308,11 +308,9 @@ int iommu_map(struct domain *d, dfn_t df
>                     d->domain_id, dfn_x(dfn_add(dfn, i)),
>                     mfn_x(mfn_add(mfn, i)), rc);
>  
> -        while ( i-- )
> -            /* if statement to satisfy __must_check */
> -            if ( iommu_call(hd->platform_ops, unmap_page, d, dfn_add(dfn, i),
> -                            flush_flags) )
> -                continue;
> +        /* while statement to satisfy __must_check */
> +        while ( iommu_unmap(d, dfn, i, flush_flags) )
> +            break;

How can this possibly be correct?

The map_page() calls are made one 4k page at a time, and this while loop
is undoing every iteration, one 4k page at a time.

Without this while loop, any failure after the first page will end up
not being unmapped.

~Andrew

On 27.04.2022 15:16, Andrew Cooper wrote:
> On 25/04/2022 09:32, Jan Beulich wrote:
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -308,11 +308,9 @@ int iommu_map(struct domain *d, dfn_t df
>>                     d->domain_id, dfn_x(dfn_add(dfn, i)),
>>                     mfn_x(mfn_add(mfn, i)), rc);
>>  
>> -        while ( i-- )
>> -            /* if statement to satisfy __must_check */
>> -            if ( iommu_call(hd->platform_ops, unmap_page, d, dfn_add(dfn, i),
>> -                            flush_flags) )
>> -                continue;
>> +        /* while statement to satisfy __must_check */
>> +        while ( iommu_unmap(d, dfn, i, flush_flags) )
>> +            break;
> 
> How can this possibly be correct?
> 
> The map_page() calls are made one 4k page at a time, and this while loop
> is undoing every iteration, one 4k page at a time.
> 
> Without this while loop, any failure after the first page will end up
> not being unmapped.

There's no real "while loop" here, it's effectively

        if ( iommu_unmap(d, dfn, i, flush_flags) )
            /* nothing */;

just that I wanted to avoid the empty body (but I could switch if
that's preferred).

Note that the 3rd argument to iommu_unmap() is i, not 1.

But I have to admit that I also have trouble interpreting your last
sentence - how would it matter if there was no code here at all? Or
did you maybe mean "With ..." instead of "Without ..."?

Jan