From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 04/20] x86/shadow: Only apply shadow heuristics when in guest context Date: Thu, 19 Feb 2015 14:18:20 +0000 Message-ID: <54E5F0AC.5090707@citrix.com> References: <1423761379-8520-1-git-send-email-andrew.cooper3@citrix.com> <1423761379-8520-5-git-send-email-andrew.cooper3@citrix.com> <20150219131014.GB21953@deinos.phlegethon.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150219131014.GB21953@deinos.phlegethon.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tim Deegan Cc: Jan Beulich , Xen-devel List-Id: xen-devel@lists.xenproject.org On 19/02/15 13:10, Tim Deegan wrote: > At 17:16 +0000 on 12 Feb (1423757763), Andrew Cooper wrote: >> It is incorrect to be applying these heuristics because of toolstack actions. > It might be incorrect if we did, but we don't. :P > > The guess_wrmap() heuristics should only be called on v == current() > because they use the shadow linear map, but this patch actually > _relaxes_ that constraint so that they could run on other vcpus of the > same guest (harmlessly). > > The second heuristic uses a stored smfn of a PTE, and uses the correct > callback for the shadow page's type, so is safe to call from any vcpu > so long as the domain's shadow lock is held. The check you add here > is needed _only_ because you are changing from using 'v' to using > 'curr' for the test. > > The changes you've made in multi.c are again only needed because you > are s/v/curr/ in the same patch, and again if anything are relaxing > the checks. Luckily they don't cover any mode-specific operations, so > AFAICT the code is still safe after all these changes, and at least no > worse than existing paths that use vcpu[0]. > > So this patch is safe, but it's not a bugfix, and I think the > changeset description needs to be rewritten. What you're actually > doing is removing uses of the vcpu parameter in these functions, > replacing them with uses of current. The changes to gating are a > side-effect. > > With that change, Reviewed-by: Tim Deegan Thanks for taking the time to review this. I will rewrite the commit message and submit a v2. ~Andrew