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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.07.14 at 14:34, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> @@ -1434,6 +1444,37 @@ bool_t p2m_mem_access_check(paddr_t gpa, bool_t 
> gla_valid, unsigned long gla,
>              return 1;
>          }
>      }
> +    else
> +    {
> +        /* There's a mem_event listener */
> +        if ( hvm_funcs.exited_by_pagefault && 
> !hvm_funcs.exited_by_pagefault() ) /* don't send a mem_event */
> +        {
> +            if ( v->arch.mem_event.emulate_flags == 0 )

The five lines above (leaving aside the comment) should all become
a simple "else if()".

> +            {
> +                v->arch.mem_event.emulate_flags = MEM_EVENT_FLAG_EMULATE;
> +                v->arch.mem_event.gpa = gpa;
> +                v->arch.mem_event.eip = eip;
> +            }
> +        }
> +    }
> +
> +    if ( v->arch.mem_event.gpa != gpa || v->arch.mem_event.eip != eip )
> +    {
> +        v->arch.mem_event.emulate_flags = 0;
> +        v->arch.mem_event.gpa = gpa;
> +        v->arch.mem_event.eip = eip;
> +    }

So the default value for gpa and eip is zero - how do you deal with
guests having code/data at virtual/physical address zero? It would
seem to me that you need a "valid" qualification for these fields, but
since the purpose of this last block isn't really clear to me maybe I'm
simply missing something and a comment might help.

> +
> +    if ( v->arch.mem_event.emulate_flags )
> +    {
> +        if ( v->arch.mem_event.emulate_flags & 
> MEM_EVENT_FLAG_EMULATE_NOWRITE )
> +            hvm_emulate_one_full(1, TRAP_invalid_op, 
> HVM_DELIVER_NO_ERROR_CODE);
> +        else
> +            hvm_emulate_one_full(0, TRAP_invalid_op, 
> HVM_DELIVER_NO_ERROR_CODE);

I'd prefer such to be coded as a single call with the first argument
being suitable calculated (you wouldn't even need a conditional
operator here).

> @@ -1475,11 +1516,74 @@ void p2m_mem_access_resume(struct domain *d)
>      /* Pull all responses off the ring */
>      while( mem_event_get_response(d, &d->mem_event->access, &rsp) )
>      {
> +        struct vcpu *v;
> +
>          if ( rsp.flags & MEM_EVENT_FLAG_DUMMY )
>              continue;
> +
> +        /* Validate the vcpu_id in the response. */
> +        if ( (rsp.vcpu_id >= d->max_vcpus) || !d->vcpu[rsp.vcpu_id] )
> +            continue;
> +
> +        v = d->vcpu[rsp.vcpu_id];
> +
> +        /* Mark vcpu for skipping one instruction upon rescheduling */
> +        if ( rsp.flags & MEM_EVENT_FLAG_EMULATE )
> +        {
> +            xenmem_access_t access;
> +            int violation = 1;

bool_t?

> +
> +            v->arch.mem_event.emulate_flags = 0;
> +
> +            if ( p2m_get_mem_access(d, rsp.gfn, &access) == 0 )
> +            {
> +                violation = 0;
> +
> +                switch (access)

Coding style.

Jan


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