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
