Re: [Qemu-devel] [PATCH 2/2] intel_iommu: Fix unexpected unmaps during global unmap

[Paolo Bonzini]

On 24/06/19 11:09, Peter Xu wrote:
> On Mon, Jun 24, 2019 at 10:43:21AM +0200, Paolo Bonzini wrote:
>> On 24/06/19 10:06, Peter Xu wrote:
>>> Well, if with such an error we'd better fix it right away in this
>>> patch... :)
>>>
>>> Let me wait for some more comments, I'll touch that up too if I need a
>>> repost.
>>
>> Looks good to me, except for one minor issue in this patch.  But do not
>> attribute this one to me, it's basically all code from you.
> 
> OK.
> 
>>
>>> +static uint64_t vtd_get_next_mask(uint64_t start, uint64_t size, int gaw)
>>> +{
>>> +    /* Tries to find smallest mask from start first */
>>> +    uint64_t rmask = start & -start, max_mask = 1ULL << gaw;
>>> +
>>> +    assert(size && gaw > 0 && gaw < 64);
>>> +
>>> +    /* Zero start, or too big */
>>> +    if (!rmask || rmask > max_mask) {
>>> +        rmask = max_mask;
>>> +    }
>>
>> Perhaps simpler:
>>
>>     uint64_t max_mask = 1ULL << gaw;
>>     uint64_t alignment = start ? start & -start : max_mask;
>>
> 
> (I'll add another "alignment = MIN(alignment, max_mask)" here if no
>  one disagree...)

I do! :)  If alignment > max_mask, then it will also be > size below so
clamping is unnecessary.

There is another way which is to compute on the mask, so that start == 0
underflows to all-ones:

    uint64_t max_mask = (1ULL << gaw) - 1;
    uint64_t start_mask = (start & -start) - 1;
    uint64_t size_mask = pow2floor(size) - 1;
    return MIN(MIN(size_mask, start_mask), max_mask) + 1;

Paolo

>>     size = MIN(size, max_mask);
>>     if (alignment <= size) {
>>         /* Increase the alignment of start */
>>         return alignment;
>>     } else {
>>         /* Find the largest page mask from size */
>>         return 1ULL << (63 - clz64(size));
>>     }
>>
>> Also please rename it to get_naturally_aligned_size.
> 
> Will do.  Thanks,
>