Re: [PATCH v4 02/21] IOMMU: simplify unmap-on-error in iommu_map()

[Jan Beulich <jbeulich@xxxxxxxx>]

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