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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.08.16 at 18:52, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1707,7 +1707,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> long gla,
>      int rc, fall_through = 0, paged = 0;
>      int sharing_enomem = 0;
>      vm_event_request_t *req_ptr = NULL;
> -    bool_t ap2m_active;
> +    bool_t ap2m_active, sync = 0;
>  
>      /* On Nested Virtualization, walk the guest page table.
>       * If this succeeds, all is fine.
> @@ -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 ) {

Coding style. If you dropped the brace entirely, you could at once
adjust ...

>                  fall_through = 1;
>              } else {

... coding style here.

> -                /* Rights not promoted, vcpu paused, work here is done */
> +                /* Rights not promoted (aka. sync event), work here is done 
> */

Comment style. More of these elsewhere.

> +int hvm_monitor_mem_access(struct vcpu* v, bool_t sync,

Coding style.

> +                           vm_event_request_t *req)
> +{
> +    return monitor_traps(v, sync, req);
> +}

Overall - is this a useful wrapper? Why can't the caller(s) call
monitor_traps() directly? And if you really want to keep it, it would
probably better be an inline one.

Jan


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