Re: [PATCH V6 5/5] xen: Handle resumed instruction based on previous mem_event reply

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

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