On 15/01/2021 13:26, Jan Beulich wrote: > On 15.01.2021 14:09, Andrew Cooper wrote: >> On 14/01/2021 15:23, Jan Beulich wrote: >>> @@ -1052,19 +1063,19 @@ map_grant_ref( >>> shah = shared_entry_header(rgt, ref); >>> act = active_entry_acquire(rgt, ref); >>> >>> - /* Make sure we do not access memory speculatively */ >>> - status = evaluate_nospec(rgt->gt_version == 1) ? &shah->flags >>> - : &status_entry(rgt, ref); >>> - >>> /* If already pinned, check the active domid and avoid refcnt >>> overflow. */ >>> if ( act->pin && >>> ((act->domid != ld->domain_id) || >>> - (act->pin & 0x80808080U) != 0 || >>> + (act->pin & (pin_incr << (GNTPIN_cntr_width - 1))) || >> This, I'm afraid, is not an improvement. What we want is: >> >> #define GNTPIN_overflow_mask 0x80808080U >> >> The reason for checking all at once is defence in depth (not strictly >> necessary, but also not a problem), > How is this not a problem? There is absolutely no reason to > reject a request just because some unrelated field may be > about to overflow. In fact I now think that I didn't even > leverage the full potential - the "act->pin != old_pin" check > could also be relaxed this way, I think. Just that it sits on > a path we probably don't really care about very much.
Hmm - I see your point. I'd missed the fact that a previous operation could leave this timebomb waiting for us. (Probably wants a note to this effect in the commit message). However we go about fixing it, "pin_incr << (GNTPIN_cntr_width - 1)" is too obscure IMO. Perhaps all we need is a #define GNTPIN_overflow_mask(x) ((x) << (GNTPIN_cntr_width - 1)) but it does occur to me that this logic only works to being with when pin_incr is strictly 1 in the relevant bytes. ~Andrew
