From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH V8 for-4.5 4/4] xen: Handle resumed instruction based on previous mem_event reply Date: Mon, 15 Sep 2014 14:37:52 +0300 Message-ID: <5416CF90.4090500@bitdefender.com> References: <1410762248-23825-1-git-send-email-rcojocaru@bitdefender.com> <1410762248-23825-5-git-send-email-rcojocaru@bitdefender.com> <5416DC120200007800034EA8@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XTUbI-0006rt-Bn for xen-devel@lists.xenproject.org; Mon, 15 Sep 2014 11:37:44 +0000 Received: from smtp03.buh.bitdefender.org (smtp.bitdefender.biz [10.17.80.77]) by mx-sr.buh.bitdefender.com (Postfix) with ESMTP id A69E0800C4 for ; Mon, 15 Sep 2014 14:37:40 +0300 (EEST) In-Reply-To: <5416DC120200007800034EA8@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: kevin.tian@intel.com, keir@xen.org, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, eddie.dong@intel.com, tim@xen.org, jun.nakajima@intel.com, xen-devel@lists.xenproject.org, ian.jackson@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On 09/15/2014 01:31 PM, Jan Beulich wrote: >>>> On 15.09.14 at 08:24, wrote: >> In a scenario where a page fault that triggered a mem_event occured, >> p2m_mem_access_check() will now be able to either 1) emulate the >> current instruction, or 2) emulate it, but don't allow it to perform >> any writes. > > Purely from a technical perspective this patch (and hence now this > series) looks fine to me, with one nit: That's great news! Thanks for the review! >> --- a/xen/include/public/mem_event.h >> +++ b/xen/include/public/mem_event.h >> @@ -31,11 +31,13 @@ >> #include "io/ring.h" >> >> /* Memory event flags */ >> -#define MEM_EVENT_FLAG_VCPU_PAUSED (1 << 0) >> -#define MEM_EVENT_FLAG_DROP_PAGE (1 << 1) >> -#define MEM_EVENT_FLAG_EVICT_FAIL (1 << 2) >> -#define MEM_EVENT_FLAG_FOREIGN (1 << 3) >> -#define MEM_EVENT_FLAG_DUMMY (1 << 4) >> +#define MEM_EVENT_FLAG_VCPU_PAUSED (1 << 0) >> +#define MEM_EVENT_FLAG_DROP_PAGE (1 << 1) >> +#define MEM_EVENT_FLAG_EVICT_FAIL (1 << 2) >> +#define MEM_EVENT_FLAG_FOREIGN (1 << 3) >> +#define MEM_EVENT_FLAG_DUMMY (1 << 4) >> +#define MEM_EVENT_FLAG_EMULATE (1 << 5) >> +#define MEM_EVENT_FLAG_EMULATE_NOWRITE (1 << 6) > > While I think that most pre-existing types here are sufficiently > self-explaining, I don't think the new type is, and hence it > warrants a comment. Of course the final say on this will be with > Tim (being the maintainer). Of course, I'll prepare a comment while waiting for Tim's reply. > I also think that with the series having got reduced, there's no > longer a process problem, but I'd nevertheless like to point out two > things for you going forward (in the hope that this won't make you > drop your Xen efforts): With the larger pieces of code additions > you have pending on top of this series, we would really like to see > at least PoC in-tree users of any such addition, perhaps even going > as far as integrating them with osstest. This is (among other > aspects like helping understanding the purpose) so that the code > you add (and that's - at least initially - used only by you) won't > become stale sooner or later. > > The second aspect is that to help acceptance of the addition of > changes that are large and/or very special purpose it would be > beneficial if we would see previous smaller scale contributions by > the exact same people (e.g. bug fixes, code reviews). This is in > the spirit of, as is being said in the governance document, the > project being run as a meritocracy, not a democracy. > > And of course - as with any new functionality being added - it > always helps if from the very beginning you make clear why > existing functionality doesn't fit your needs. Noted, thank you for your help! Thanks, Razvan Cojocaru