Re: [PATCH V14 4/7] xen/arm: Data abort exception (R/W) mem_access events

[Julien Grall <julien.grall@citrix.com>]

Hi Tamas,

The code looks good. See few typoes and coding style issue below.

On 26/03/15 22:05, Tamas K Lengyel wrote:
> +static int p2m_mem_access_radix_set(struct p2m_domain *p2m, unsigned long pfn,
> +                                    p2m_access_t a)
> +{
> +    int rc;
> +
> +    if ( !p2m->mem_access_enabled )
> +        return 0;
> +
> +    if ( p2m_access_rwx == a )
> +    {
> +        radix_tree_delete(&p2m->mem_access_settings, pfn);
> +        return 0;
> +    }
> +
> +    rc = radix_tree_insert(&p2m->mem_access_settings, pfn,
> +                           radix_tree_int_to_ptr(a));
> +    if ( rc == -EEXIST )
> +    {
> +        /* If a setting existed already, change it to the new one */

s/existed already/already exists/?

> +        radix_tree_replace_slot(
> +            radix_tree_lookup_slot(
> +                &p2m->mem_access_settings, pfn),
> +            radix_tree_int_to_ptr(a));
> +        rc = 0;
> +    }
> +
> +    return rc;
> +}
> +
>  enum p2m_operation {
>      INSERT,
>      ALLOCATE,
>      REMOVE,
>      RELINQUISH,
>      CACHEFLUSH,
> +    MEMACCESS,
>  };
>  
>  /* Put any references on the single 4K page referenced by pte.  TODO:
> @@ -560,13 +593,22 @@ static int apply_one_level(struct domain *d,
>          if ( p2m_valid(orig_pte) )
>              return P2M_ONE_DESCEND;
>  
> -        if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) )
> +        if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) &&
> +           /* We only create superpages when mem_access is not in use. */
> +             (level == 3 || (level < 3 && !p2m->mem_access_enabled)) )

I'm wondering if it would be better to check "a != p2m_access_rwx"
rather than "p2m->mem_access_enabled" in order to avoid split when it's
unnecessary.

Although given the status of this series. I won't bother you to ask you
this change now :).

>          {
>              struct page_info *page;
>  
>              page = alloc_domheap_pages(d, level_shift - PAGE_SHIFT, 0);
>              if ( page )
>              {
> +                rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a);
> +                if ( rc < 0 )
> +                {
> +                    free_domheap_page(page);
> +                    return rc;
> +                }
> +
>                  pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t, a);
>                  if ( level < 3 )
>                      pte.p2m.table = 0;
> @@ -587,8 +629,8 @@ static int apply_one_level(struct domain *d,
>          /*
>           * If we get here then we failed to allocate a sufficiently
>           * large contiguous region for this level (which can't be
> -         * L3). Create a page table and continue to descend so we try
> -         * smaller allocations.
> +         * L3) or mem_access is in use. Create a page table and
> +         * continue to descend so we try smaller allocations.
>           */
>          rc = p2m_create_table(d, entry, 0, flush_cache);
>          if ( rc < 0 )
> @@ -598,9 +640,14 @@ static int apply_one_level(struct domain *d,
>  
>      case INSERT:
>          if ( is_mapping_aligned(*addr, end_gpaddr, *maddr, level_size) &&
> -           /* We do not handle replacing an existing table with a superpage */
> -             (level == 3 || !p2m_table(orig_pte)) )
> +           /* We do not handle replacing an existing table with a superpage
> +            * or when mem_access is in use. */

Coding style:
/*
 * blah blah
 */

[..]

> +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)

[..]

> +    /* Otherwise, check if there is a memory event listener, and send the message along */
> +    if ( !mem_event_check_ring(&v->domain->mem_event->access) )
> +    {
> +        /* No listener */
> +        if ( p2m->access_required )
> +        {
> +            gdprintk(XENLOG_INFO, "Memory access permissions failure, "
> +                                  "no mem_event listener VCPU %d, dom %d\n",
> +                                  v->vcpu_id, v->domain->domain_id);

You may want to use gprintk as gdprintk call will be dropped on
non-debug build.

[..]

> +/* Set access type for a region of pfns.
> + * If start_pfn == -1ul, sets the default access type */

Coding style:

/*
 * Blah blah
 */

> +long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
> +                        uint32_t start, uint32_t mask, xenmem_access_t access)

[..]

> +int p2m_get_mem_access(struct domain *d, unsigned long gpfn,
> +                       xenmem_access_t *access)


[..]

> +    /* If request to get default access */
> +    if ( gpfn == ~0ull )

gpfn is an unsigned long. You may want to use ~0ul here.

[..]

> diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
> index 29f3628..56d1afe 100644
> --- a/xen/include/xen/p2m-common.h
> +++ b/xen/include/xen/p2m-common.h
> @@ -44,4 +44,14 @@ int unmap_mmio_regions(struct domain *d,
>                         unsigned long nr,
>                         unsigned long mfn);
>  
> +/* Set access type for a region of pfns.
> + * If start_pfn == -1ul, sets the default access type */
> +long p2m_set_mem_access(struct domain *d, unsigned long start_pfn, uint32_t nr,
> +                        uint32_t start, uint32_t mask, xenmem_access_t access);
> +

Coding style:

/*
 * Blah blah
 */

> +/* Get access type for a pfn
> + * If pfn == -1ul, gets the default access type */
> +int p2m_get_mem_access(struct domain *d, unsigned long pfn,
> +                       xenmem_access_t *access);
> +

Ditto


>  #endif /* _XEN_P2M_COMMON_H */
> 

Regards,

-- 
Julien Grall