Re: [PATCH for-4.5 v8 15/19] xen/arm: Data abort exception (R/W) mem_events.

[Ian Campbell <Ian.Campbell@citrix.com>]

On Tue, 2014-09-23 at 15:14 +0200, Tamas K Lengyel wrote:
> This patch enables to store, set, check and deliver LPAE R/W mem_events.
> As the LPAE PTE's lack enough available software programmable bits,
> we store the permissions in a Radix tree. A custom boolean, access_in_use,
> specifies if the tree is in use to avoid uneccessary lookups on an empty tree.

"unnecessary".

> @@ -414,12 +417,40 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
>      return 0;
>  }
>  
> +static long p2m_mem_access_radix_set(struct p2m_domain *p2m, unsigned long pfn,
> +                                     p2m_access_t a)
> +{
> +    long rc;
> +    if ( p2m_access_rwx == a )

I think I mentioned this before, but please write your conditionals the
more conventional way around.

> +    {
> +        if ( p2m->access_in_use )
> +            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 ( -EEXIST == rc )
> +    {
> +        /* If a setting existed already, change it to the new one */
> +        radix_tree_replace_slot(
> +            radix_tree_lookup_slot(
> +                &p2m->mem_access_settings, pfn),
> +            radix_tree_int_to_ptr(a));

What a shame that the API doesn't allow you to do an insert or replace
in one go!

> @@ -591,9 +631,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. */
> +             (level == 3 || (!p2m_table(orig_pte) && !p2m->access_in_use)) )
>          {
> +            rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a);
> +            if ( rc < 0 )
> +                return rc;

I expect the answer is yes, but just to check: It is desirable to be
able to insert with a specific non-defualt access perms, as opposed to
INSERT with defaults then MEMACCESS?

> @@ -753,6 +799,49 @@ static int apply_one_level(struct domain *d,
>              *addr += PAGE_SIZE;
>              return P2M_ONE_PROGRESS_NOP;
>          }
> +
> +    case MEMACCESS:
> +        if ( level < 3 )
> +        {
> +            if ( !p2m_valid(orig_pte) )
> +            {
> +                *addr += level_size;
> +                return P2M_ONE_PROGRESS_NOP;
> +            }
> +
> +            /* Shatter large pages as we descend */
> +            if ( p2m_mapping(orig_pte) )
> +            {
> +                rc = p2m_shatter_page(d, entry, level, flush_cache);
> +
> +                if ( rc < 0 )
> +                    return rc;

I don't know if that matter for this specific sub op but what is
supposed to happen on error? Should partial work be undone?

> @@ -776,6 +865,8 @@ static int apply_p2m_changes(struct domain *d,
>      unsigned int cur_root_table = ~0;
>      unsigned int cur_offset[4] = { ~0, };
>      unsigned int count = 0;
> +    unsigned long start_gpfn = paddr_to_pfn(start_gpaddr),
> +                  end_gpfn = paddr_to_pfn(end_gpaddr);

You don't need to update end_gpaddr up, do you?

>      bool_t flush = false;
>      bool_t flush_pt;
>  
> @@ -821,6 +912,21 @@ static int apply_p2m_changes(struct domain *d,
>              count = 0;
>          }
>  
> +        /*
> +         * Preempt setting mem_access permissions as required by XSA-89,
> +         * if it's not the last iteration.
> +         */
> +        if ( op == MEMACCESS && count )
> +        {
> +            int progress = paddr_to_pfn(addr) - start_gpfn + 1;
> +            if ( (end_gpfn-start_gpfn) > progress && !(progress & mask)

This differs from the x86 equivalent in that it is not "++progress".
Doesn't that mean that we can fail to make any progress at all?

> +int p2m_get_mem_access(struct domain *d, unsigned long gpfn,
> +                       xenmem_access_t *access)
> +{
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    void *i;
> +    unsigned int index;
> +
> +    static const xenmem_access_t memaccess[] = {
> +#define ACCESS(ac) [p2m_access_##ac] = XENMEM_access_##ac
> +            ACCESS(n),
> +            ACCESS(r),
> +            ACCESS(w),
> +            ACCESS(rw),
> +            ACCESS(x),
> +            ACCESS(rx),
> +            ACCESS(wx),
> +            ACCESS(rwx),
> +            ACCESS(rx2rw),
> +            ACCESS(n2rwx),
> +#undef ACCESS

Unlike x86 I think you have pretty much complete freedom to define
p2m_access_*? Why not just make them 1:1?

(one for a future cleanup if you prefer)

> +        /* Seting was found in the Radix tree. */

"Setting"