Re: [Xen-devel] [RFC 19/22] xen/arm: p2m: Re-implement p2m_remove_using using p2m_set_entry

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 28 Jul 2016, Julien Grall wrote:
> The function p2m_insert_mapping can be re-implemented using the generic
> function p2m_set_entry.
> 
> Also drop the operation REMOVE in apply_* as nobody is using it anymore.
> Note that the functions could have been dropped in one go at the end,
> however I find easier to drop the operations one by one avoiding a big
> deletion in the patch that converts the last operation.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


>  xen/arch/arm/p2m.c | 127 
> ++++++-----------------------------------------------
>  1 file changed, 13 insertions(+), 114 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 297b176..0920222 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -725,7 +725,6 @@ static int p2m_mem_access_radix_set(struct p2m_domain 
> *p2m, gfn_t gfn,
>  
>  enum p2m_operation {
>      INSERT,
> -    REMOVE,
>      MEMACCESS,
>  };
>  
> @@ -750,7 +749,6 @@ static void p2m_put_l3_page(mfn_t mfn, p2m_type_t type)
>      }
>  }
>  
> -#if 0
>  /* Free lpae sub-tree behind an entry */
>  static void p2m_free_entry(struct p2m_domain *p2m,
>                             lpae_t entry, unsigned int level)
> @@ -1083,7 +1081,6 @@ static int p2m_set_entry(struct p2m_domain *p2m,
>  
>      return rc;
>  }
> -#endif
>  
>  /*
>   * Returns true if start_gpaddr..end_gpaddr contains at least one
> @@ -1161,7 +1158,6 @@ static int apply_one_level(struct domain *d,
>                             p2m_access_t a)
>  {
>      const paddr_t level_size = level_sizes[level];
> -    const paddr_t level_mask = level_masks[level];
>  
>      struct p2m_domain *p2m = &d->arch.p2m;
>      lpae_t pte;
> @@ -1247,74 +1243,6 @@ static int apply_one_level(struct domain *d,
>  
>          break;
>  
> -    case REMOVE:
> -        if ( !p2m_valid(orig_pte) )
> -        {
> -            /* Progress up to next boundary */
> -            *addr = (*addr + level_size) & level_mask;
> -            *maddr = (*maddr + level_size) & level_mask;
> -            return P2M_ONE_PROGRESS_NOP;
> -        }
> -
> -        if ( level < 3 )
> -        {
> -            if ( p2m_table(orig_pte) )
> -                return P2M_ONE_DESCEND;
> -
> -            if ( op == REMOVE &&
> -                 !is_mapping_aligned(*addr, end_gpaddr,
> -                                     0, /* maddr doesn't matter for remove */
> -                                     level_size) )
> -            {
> -                /*
> -                 * Removing a mapping from the middle of a superpage. Shatter
> -                 * and descend.
> -                 */
> -                *flush = true;
> -                rc = p2m_shatter_page(p2m, entry, level);
> -                if ( rc < 0 )
> -                    return rc;
> -
> -                return P2M_ONE_DESCEND;
> -            }
> -        }
> -
> -        /*
> -         * Ensure that the guest address addr currently being
> -         * handled (that is in the range given as argument to
> -         * this function) is actually mapped to the corresponding
> -         * machine address in the specified range. maddr here is
> -         * the machine address given to the function, while
> -         * orig_pte.p2m.base is the machine frame number actually
> -         * mapped to the guest address: check if the two correspond.
> -         */
> -         if ( op == REMOVE &&
> -              pfn_to_paddr(orig_pte.p2m.base) != *maddr )
> -             printk(XENLOG_G_WARNING
> -                    "p2m_remove dom%d: mapping at %"PRIpaddr" is of maddr 
> %"PRIpaddr" not %"PRIpaddr" as expected\n",
> -                    d->domain_id, *addr, pfn_to_paddr(orig_pte.p2m.base),
> -                    *maddr);
> -
> -        *flush = true;
> -
> -        p2m_remove_pte(entry, p2m->clean_pte);
> -        p2m_mem_access_radix_set(p2m, _gfn(paddr_to_pfn(*addr)),
> -                                 p2m_access_rwx);
> -
> -        *addr += level_size;
> -        *maddr += level_size;
> -
> -        p2m->stats.mappings[level]--;
> -
> -        if ( level == 3 )
> -            p2m_put_l3_page(_mfn(orig_pte.p2m.base), orig_pte.p2m.type);
> -
> -        /*
> -         * This is still a single pte write, no matter the level, so no need 
> to
> -         * scale.
> -         */
> -        return P2M_ONE_PROGRESS;
> -
>      case MEMACCESS:
>          if ( level < 3 )
>          {
> @@ -1526,43 +1454,6 @@ static int apply_p2m_changes(struct domain *d,
>          }
>  
>          BUG_ON(level > 3);
> -
> -        if ( op == REMOVE )
> -        {
> -            for ( ; level > P2M_ROOT_LEVEL; level-- )
> -            {
> -                lpae_t old_entry;
> -                lpae_t *entry;
> -                unsigned int offset;
> -
> -                pg = pages[level];
> -
> -                /*
> -                 * No need to try the previous level if the current one
> -                 * still contains some mappings.
> -                 */
> -                if ( pg->u.inuse.p2m_refcount )
> -                    break;
> -
> -                offset = offsets[level - 1];
> -                entry = &mappings[level - 1][offset];
> -                old_entry = *entry;
> -
> -                page_list_del(pg, &p2m->pages);
> -
> -                p2m_remove_pte(entry, p2m->clean_pte);
> -
> -                p2m->stats.mappings[level - 1]--;
> -                update_reference_mapping(pages[level - 1], old_entry, 
> *entry);
> -
> -                /*
> -                 * We can't free the page now because it may be present
> -                 * in the guest TLB. Queue it and free it after the TLB
> -                 * has been flushed.
> -                 */
> -                page_list_add(pg, &free_pages);
> -            }
> -        }
>      }
>  
>      if ( op == INSERT )
> @@ -1604,8 +1495,10 @@ out:
>           * addr keeps the address of the end of the last 
> successfully-inserted
>           * mapping.
>           */
> -        apply_p2m_changes(d, REMOVE, sgfn, gfn - gfn_x(sgfn), smfn,
> -                          0, p2m_invalid, d->arch.p2m.default_access);
> +        p2m_write_lock(p2m);
> +        p2m_set_entry(p2m, sgfn, gfn - gfn_x(sgfn), INVALID_MFN,
> +                      p2m_invalid, p2m_access_rwx);
> +        p2m_write_unlock(p2m);
>      }
>  
>      return rc;
> @@ -1626,9 +1519,15 @@ static inline int p2m_remove_mapping(struct domain *d,
>                                       unsigned long nr,
>                                       mfn_t mfn)
>  {
> -    return apply_p2m_changes(d, REMOVE, start_gfn, nr, mfn,
> -                             /* arguments below not used when removing 
> mapping */
> -                             0, p2m_invalid, d->arch.p2m.default_access);
> +    struct p2m_domain *p2m = &d->arch.p2m;
> +    int rc;
> +
> +    p2m_write_lock(p2m);
> +    rc = p2m_set_entry(p2m, start_gfn, nr, INVALID_MFN,
> +                       p2m_invalid, p2m_access_rwx);
> +    p2m_write_unlock(p2m);
> +
> +    return rc;
>  }
>  
>  int map_regions_rw_cache(struct domain *d,
> -- 
> 1.9.1
> 

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