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

[Peter Xu <peterx@redhat.com>]

On Mon, Jun 24, 2019 at 12:11:54PM +0200, Paolo Bonzini wrote:
> 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.

You are right. ;)

> 
> 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;

The last line still seems problematic, but I just want to say the
calculation of size_mask is indeed a smart move! (I did think the zero
check was a bit ugly)

Thanks,

-- 
Peter Xu