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

[George Dunlap <dunlapg@xxxxxxxxx>]

On Wed, Sep 10, 2014 at 5:12 PM, Razvan Cojocaru
<rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 09/10/2014 07:03 PM, George Dunlap wrote:
>> On Tue, Sep 9, 2014 at 11:28 AM, Razvan Cojocaru
>> <rcojocaru@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>>> Acked-by: Kevin Tian <kevin.tian@xxxxxxxxx>
>> [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.

...and you should also put a comment there explaining why someone with
introspection enabled wouldn't want an event here (something I'm still
not clear on).

Are you *sure* that everyone who enables introspection will want that
event suppressed (not just you), and that no one else will?
Otherwise, it might make more sense to add some kind of flag to enable
or disable it, rather than gating it on introspection.  Or it's
possible everyone actually does want that event suppressed -- in which
case making it universal is the best option.

Andres, any opinions here?

> 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.

Sure, it won't be possible to send a mem event, because that code will
not be executed at all. :-)

Putting an "else" there sort of implies to someone reading the code
that you think the if() block might be executed and then continue
executing, which is misleading.  In your patch it's even more
misleading, because the else only covers the first if() and not the
subsequent conditionals you've added right after; which implies that
the if() block might be executed and then execute the conditionals
below this one, but not this one.

The less things a programmer has to remember / figure out /  keep in
her head, the less likely she is to make a mistake. :-)

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel