Re: [Xen-devel] [RFC 12/22] xen/arm: p2m: Introduce p2m_get_entry and use it to implement __p2m_lookup

[Tamas K Lengyel <tamas.k.lengyel@xxxxxxxxx>]

On Thu, Jul 28, 2016 at 8:51 AM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Currently, for a given GFN, the function __p2m_lookup will only return
> the associated MFN and the p2m type of the mapping.
>
> In some case we need the order of the mapping and the memaccess
> permission. Rather than providing separate function for this purpose,
> it is better to implement a generic function to return all the
> information.
>
> To avoid passing dummy parameter, a caller that does need a specific
> information can use NULL instead.
>
> The list of the informations retrieved is based on the x86 version. All
> of them will be used in follow-up patches.
>
> It might have been possible to extend __p2m_lookup, however I choose to
> reimplement it from scratch to allow sharing some helpers with the
> function that will update the P2M (will be added in a follow-up patch).
>
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> ---
>  xen/arch/arm/p2m.c         | 188 
> ++++++++++++++++++++++++++++++++++-----------
>  xen/include/asm-arm/page.h |   4 +
>  2 files changed, 149 insertions(+), 43 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index d4a4b62..8676b9d 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -36,6 +36,8 @@ static const paddr_t level_masks[] =
>      { ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK };
>  static const unsigned int level_shifts[] =
>      { ZEROETH_SHIFT, FIRST_SHIFT, SECOND_SHIFT, THIRD_SHIFT };
> +static const unsigned int level_orders[] =
> +    { ZEROETH_ORDER, FIRST_ORDER, SECOND_ORDER, THIRD_ORDER };
>
>  static bool_t p2m_valid(lpae_t pte)
>  {
> @@ -236,28 +238,99 @@ static lpae_t *p2m_get_root_pointer(struct p2m_domain 
> *p2m,
>
>  /*
>   * Lookup the MFN corresponding to a domain's GFN.
> + * Lookup mem access in the ratrix tree.
> + * The entries associated to the GFN is considered valid.
> + */
> +static p2m_access_t p2m_mem_access_radix_get(struct p2m_domain *p2m, gfn_t 
> gfn)
> +{
> +    void *ptr;
> +
> +    if ( !p2m->mem_access_enabled )
> +        return p2m_access_rwx;
> +
> +    ptr = radix_tree_lookup(&p2m->mem_access_settings, gfn_x(gfn));
> +    if ( !ptr )
> +        return p2m_access_rwx;
> +    else
> +        return radix_tree_ptr_to_int(ptr);
> +}

This looks fine from the mem_access side.

> +
> +#define GUEST_TABLE_MAP_FAILED 0
> +#define GUEST_TABLE_SUPER_PAGE 1
> +#define GUEST_TABLE_NORMAL_PAGE 2
> +
> +static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry,
> +                            int level_shift);
> +
> +/*
> + * Take the currently mapped table, find the corresponding GFN entry,
> + * and map the next table, if available.
>   *
> - * There are no processor functions to do a stage 2 only lookup therefore we
> - * do a a software walk.
> + * Return values:
> + *  GUEST_TABLE_MAP_FAILED: Either read_only was set and the entry
> + *  was empty, or allocating a new page failed.
> + *  GUEST_TABLE_NORMAL_PAGE: next level mapped normally
> + *  GUEST_TABLE_SUPER_PAGE: The next entry points to a superpage.
>   */
> -static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
> +static int p2m_next_level(struct p2m_domain *p2m, bool read_only,
> +                          lpae_t **table, unsigned int offset)
>  {
> -    struct p2m_domain *p2m = &d->arch.p2m;
> -    const paddr_t paddr = pfn_to_paddr(gfn_x(gfn));
> -    const unsigned int offsets[4] = {
> -        zeroeth_table_offset(paddr),
> -        first_table_offset(paddr),
> -        second_table_offset(paddr),
> -        third_table_offset(paddr)
> -    };
> -    const paddr_t masks[4] = {
> -        ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK
> -    };
> -    lpae_t pte, *map;
> +    lpae_t *entry;
> +    int ret;
> +    mfn_t mfn;
> +
> +    entry = *table + offset;
> +
> +    if ( !p2m_valid(*entry) )
> +    {
> +        if ( read_only )
> +            return GUEST_TABLE_MAP_FAILED;
> +
> +        ret = p2m_create_table(p2m, entry, /* not used */ ~0);
> +        if ( ret )
> +            return GUEST_TABLE_MAP_FAILED;
> +    }
> +
> +    /* The function p2m_next_level is never called at the 3rd level */
> +    if ( p2m_mapping(*entry) )
> +        return GUEST_TABLE_SUPER_PAGE;
> +
> +    mfn = _mfn(entry->p2m.base);
> +
> +    unmap_domain_page(*table);
> +    *table = map_domain_page(mfn);
> +
> +    return GUEST_TABLE_NORMAL_PAGE;
> +}
> +
> +/*
> + * Get the details of a given gfn.
> + *
> + * If the entry is present, the associated MFN will be returned and the
> + * access and type filled up. The page_order will correspond to the
> + * order of the mapping in the page table (i.e it could be a superpage).
> + *
> + * If the entry is not present, INVALID_MFN will be returned and the
> + * page_order will be set according to the order of the invalid range.
> + */
> +static mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
> +                           p2m_type_t *t, p2m_access_t *a,
> +                           unsigned int *page_order)

It is going to be necessary to make this function accessible outside
p2m.c to allow us to split mem_access components out of p2m. If you
could declare it as a non-static function and expose it in p2m.h that
would be great (x86 already has it accessible outside p2m.c).

Thanks,
Tamas

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