Re: [Xen-devel] [PATCH] mem_access: Use monitor_traps instead of mem_access_send_req

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/07/2016 20:35, 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.
>
> Since mem_access events go on the monitor ring in this patch we consolidate
> all paths to use monitor_traps to place events on the ring and to fill in
> the common parts of the requests.
>
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
> ---
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>

Common and x86 bits Reviewed-by: Andrew Cooper
<andrew.cooper3@xxxxxxxxxx>, but a few suggestions.

> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index d82349c..df898a3 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1642,12 +1642,41 @@ void __init setup_virt_paging(void)
>      smp_call_function(setup_virt_paging_one, (void *)val, 1);
>  }
>  
> +static int
> +__p2m_mem_access_send_req(paddr_t gpa, vaddr_t gla, const struct npfec npfec,
> +                          xenmem_access_t xma)
> +{
> +    struct vcpu *v = current;
> +    vm_event_request_t req = {};
> +    bool_t sync = (xma == XENMEM_access_n2rwx) ? 0 : 1;
> +
> +    req.reason = VM_EVENT_REASON_MEM_ACCESS;
> +
> +    /* Send request to mem access subscriber */
> +    req.u.mem_access.gfn = gpa >> PAGE_SHIFT;
> +    req.u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1);

I see this is only code motion, but ~PAGE_MASK here instead of
open-coding it.

> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index daaee1d..688370d 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1846,11 +1846,12 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> long gla,
>                  }
>              }
>  
> -            if ( p2m_mem_access_check(gpa, gla, npfec, &req_ptr) )
> -            {
> +            sync = p2m_mem_access_check(gpa, gla, npfec, &req_ptr);
> +
> +            if ( !sync ) {

Please keep this brace on the newline (inline with the style), even if
it doesn't match the style of the else clause.

>                  fall_through = 1;
>              } else {
> -                /* Rights not promoted, vcpu paused, work here is done */
> +                /* Rights not promoted (aka. sync event), work here is done 
> */
>                  rc = 1;
>                  goto out_put_gfn;
>              }
> @@ -1956,7 +1957,12 @@ out:
>      }
>      if ( req_ptr )
>      {
> -        mem_access_send_req(currd, req_ptr);
> +        if ( hvm_monitor_mem_access(curr, sync, req_ptr) < 0 )
> +        {
> +            /* Crash the domain */
> +            rc = 0;
> +        }

It is reasonable to omit the braces here.

> +
>          xfree(req_ptr);
>      }
>      return rc;
> diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
> index 7277c12..c7285c6 100644
> --- a/xen/arch/x86/hvm/monitor.c
> +++ b/xen/arch/x86/hvm/monitor.c
> @@ -152,6 +152,12 @@ int hvm_monitor_cpuid(unsigned long insn_length)
>      return monitor_traps(curr, 1, &req);
>  }
>  
> +int hvm_monitor_mem_access(struct vcpu* v, bool_t sync,

vcpu *v.

~Andrew

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