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

[George Dunlap <dunlapg@xxxxxxxxx>]

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.

 -George

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