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