Re: [Xen-devel] [PATCH v2] mem_access: sanitize code around sending vm_event request

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 01/08/16 17:52, Tamas K Lengyel wrote:
> The two functions monitor_traps and mem_access_send_req duplicate some of the
> same functionality. The mem_access_send_req however leaves a lot of the
> standard vm_event fields to be filled by other functions.
> 
> Remove mem_access_send_req() completely, making use of monitor_traps() to put
> requests into the monitor ring.  This in turn causes some cleanup around the
> old callsites of mem_access_send_req(), and on ARM, the introduction of the
> __p2m_mem_access_send_req() helper to fill in common mem_access information.
> We also update monitor_traps to now include setting the common vcpu_id field
> so that all other call-sites can ommit this step.
> 
> Finally, this change identifies that errors from mem_access_send_req() were
> never checked.  As errors constitute a problem with the monitor ring,
> crashing the domain is the most appropriate action to take.
> 
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

This appears to be v3, not v2?

> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 812dbf6..27f9d26 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1728,13 +1728,8 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long 
> gla,
>      if ( req )
>      {
>          *req_ptr = req;
> -        req->reason = VM_EVENT_REASON_MEM_ACCESS;
> -
> -        /* Pause the current VCPU */
> -        if ( p2ma != p2m_access_n2rwx )
> -            req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
>  
> -        /* Send request to mem event */
> +        req->reason = VM_EVENT_REASON_MEM_ACCESS;
>          req->u.mem_access.gfn = gfn;
>          req->u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1);
>          if ( npfec.gla_valid )
> @@ -1750,23 +1745,10 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned 
> long gla,
>          req->u.mem_access.flags |= npfec.read_access    ? MEM_ACCESS_R : 0;
>          req->u.mem_access.flags |= npfec.write_access   ? MEM_ACCESS_W : 0;
>          req->u.mem_access.flags |= npfec.insn_fetch     ? MEM_ACCESS_X : 0;
> -        req->vcpu_id = v->vcpu_id;
> -
> -        vm_event_fill_regs(req);
> -
> -        if ( altp2m_active(v->domain) )
> -        {
> -            req->flags |= VM_EVENT_FLAG_ALTERNATE_P2M;
> -            req->altp2m_idx = vcpu_altp2m(v).p2midx;
> -        }
>      }
>  
> -    /* Pause the current VCPU */
> -    if ( p2ma != p2m_access_n2rwx )
> -        vm_event_vcpu_pause(v);
> -
> -    /* VCPU may be paused, return whether we promoted automatically */
> -    return (p2ma == p2m_access_n2rwx);
> +    /* Return whether vCPU pause is required (aka. sync event) */
> +    return (p2ma != p2m_access_n2rwx);
>  }
>  
>  static inline

p2m-bits:

Acked-by: George Dunlap <george.dunlap@xxxxxxxxxx>

But I agree with Julien -- this patch has several independent changes
which makes it quite difficult to tell what's going on.  I'm sure it's
taken the two of us a lot more time together to figure out what is and
is not happening than it would have for you to break it down into
several little chunks.

If you're not already familiar with it, I would recommend looking into
stackgit.  My modus operandi for things like this is to get things
working in one big patch, then pop it off the stack and apply bits of it
at a time to make a series.

It's not only more considerate of your reviewers, but it's also a
helpful exercise for yourself.

 -George


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