From mboxrd@z Thu Jan  1 00:00:00 1970
From: Razvan Cojocaru <rcojocaru@bitdefender.com>
Subject: Re: [PATCH V6 5/5] xen: Handle resumed instruction
 based on previous mem_event reply
Date: Wed, 10 Sep 2014 19:12:35 +0300
Message-ID: <54107873.1040001@bitdefender.com>
References: <1410258512-2955-1-git-send-email-rcojocaru@bitdefender.com>	<1410258512-2955-6-git-send-email-rcojocaru@bitdefender.com>
	<CAFLBxZYsAnLrLh8F02ofdaL+0+GnZZ2MmQEp-uUCAthmVXXtmQ@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta5.messagelabs.com ([195.245.231.135])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <rcojocaru@bitdefender.com>) id 1XRkVX-0004Y4-OY
	for xen-devel@lists.xenproject.org; Wed, 10 Sep 2014 16:12:35 +0000
Received: from smtp02.buh.bitdefender.net (smtp.bitdefender.biz [10.17.80.76])
	by mx-sr.buh.bitdefender.com (Postfix) with ESMTP id 4332180066
	for <xen-devel@lists.xenproject.org>;
	Wed, 10 Sep 2014 19:12:31 +0300 (EEST)
In-Reply-To: <CAFLBxZYsAnLrLh8F02ofdaL+0+GnZZ2MmQEp-uUCAthmVXXtmQ@mail.gmail.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: George Dunlap <dunlapg@umich.edu>
Cc: "Tian, Kevin" <kevin.tian@intel.com>, Keir Fraser <keir@xen.org>, Ian Campbell <ian.campbell@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>, "Dong,
	Eddie" <eddie.dong@intel.com>, Ian Jackson <ian.jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>, Jan Beulich <jbeulich@suse.com>, Jun Nakajima <jun.nakajima@intel.com>, Andrew Cooper <andrew.cooper3@citrix.com>, xen-devel <xen-devel@lists.xenproject.org>
List-Id: xen-devel@lists.xenproject.org

On 09/10/2014 07:03 PM, George Dunlap wrote:
> On Tue, Sep 9, 2014 at 11:28 AM, Razvan Cojocaru
> <rcojocaru@bitdefender.com> 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.
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> Acked-by: Kevin Tian <kevin.tian@intel.com>
> [snip]
> 
>> +    else if ( v->arch.mem_event.emulate_flags == 0 &&
>> +              npfec.kind != npfec_kind_with_gla ) /* don't send a mem_event */
>> +    {
>> +        v->arch.mem_event.emulate_flags = MEM_EVENT_FLAG_EMULATE;
>> +        v->arch.mem_event.gpa = gpa;
>> +        v->arch.mem_event.eip = eip;
>> +    }
> 
> It looks like the previous if() is true, that it will never get to
> this point (because it will either return 0 or 1 depending on whether
> p2m->access_required is set).  So you don't need to make this an
> "else" here -- you should just add a blank line and make this a normal
> if().
> 
> Also, maybe it's just because I'm not familiar with the mem_event
> interface, but I don't really see what this code is doing.  It seems
> to be changing the behavior even for clients that aren't using
> MEM_EVENT_FLAG_EMULATE*.  Is that intended?  In any case it seems like
> there could be a better comment here.

Thanks, those are very good points. I'll make that a regular if(), and
test also if introspection monitoring is enabled (please see patch 3/5:
d->arch.hvm_domain.introspection_enabled) before setting the emulate
flag, that way we won't alter the behaviour for other clients.

As for the previous if, I think that if it holds then it won't be
possible to send a mem_event anyway, hence the else.


Thanks,
Razvan Cojocaru