Re: [Xen-devel] [PATCH v2 15/25] arm/altp2m: Extend __p2m_lookup.

[Julien Grall <julien.grall@xxxxxxx>]

Hello Sergej,

On 01/08/16 18:10, Sergej Proskurin wrote:
> This commit extends the functionality of the function "__p2m_lookup".
> The function "__p2m_lookup" performs the necessary steps gathering
> information concerning memory attributes and the p2m table level a
> specific gfn is mapped to. Thus, we extend the function's prototype so
> that the caller can optionally get these information for further
> processing.
>
> Also, we extend the function prototype of "__p2m_lookup" to hold an
> argument of type "struct p2m_domain*", as we need to distinguish between
> the host's p2m and different altp2m views. While doing so, we needed to
> extend the function's prototypes of the following functions:
>
> * __p2m_get_mem_access
> * p2m_mem_access_and_get_page
>
> Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
> ---
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> ---
>  xen/arch/arm/p2m.c | 59 ++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 39 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 784f8da..326e343 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -168,15 +168,22 @@ void p2m_flush_tlb(struct p2m_domain *p2m)
>      }
>  }
>
> +static int __p2m_get_mem_access(struct p2m_domain*, gfn_t, xenmem_access_t*);
> +
>  /*
>   * Lookup the MFN corresponding to a domain's GFN.
>   *
>   * There are no processor functions to do a stage 2 only lookup therefore we
>   * do a a software walk.
> + *
> + * Optionally, __p2m_lookup takes arguments to provide information about the
> + * p2m type, the p2m table level the paddr is mapped to, associated mem
> + * attributes, and memory access rights.
>   */
> -static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
> +static mfn_t __p2m_lookup(struct p2m_domain *p2m, gfn_t gfn, p2m_type_t *t,
> +                          unsigned int *level, unsigned int *mattr,
> +                          xenmem_access_t *xma)
Please give a look at my series "xen/arm: Rework the P2M code to follow 
break-before-make sequence" [1], it will expose a clean interface to 
retrieve all the necessary information.
>  mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
>  {
>      mfn_t ret;
> -    struct p2m_domain *p2m = &d->arch.p2m;
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>
>      p2m_read_lock(p2m);
> -    ret = __p2m_lookup(d, gfn, t);
> +    ret = __p2m_lookup(p2m, gfn, t, NULL, NULL, NULL);
>      p2m_read_unlock(p2m);
>
>      return ret;
> @@ -479,10 +499,9 @@ static int p2m_create_table(struct p2m_domain *p2m, lpae_t 
> *entry,
>      return 0;
>  }
>
> -static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
> +static int __p2m_get_mem_access(struct p2m_domain *p2m, gfn_t gfn,
>                                  xenmem_access_t *access)
>  {
> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>      void *i;
>      unsigned int index;
>
> @@ -525,7 +544,7 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
>           * No setting was found in the Radix tree. Check if the
>           * entry exists in the page-tables.
>           */
> -        mfn_t mfn = __p2m_lookup(d, gfn, NULL);
> +        mfn_t mfn = __p2m_lookup(p2m, gfn, NULL, NULL, NULL, NULL);
>
>          if ( mfn_eq(mfn, INVALID_MFN) )
>              return -ESRCH;
> @@ -1519,7 +1538,7 @@ mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn)
>   * we indeed found a conflicting mem_access setting.
>   */
>  static struct page_info*
> -p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag)
> +p2m_mem_access_check_and_get_page(struct p2m_domain *p2m, vaddr_t gva, 
> unsigned long flag)
>  {
>      long rc;
>      paddr_t ipa;
> @@ -1539,7 +1558,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned 
> long flag)
>       * We do this first as this is faster in the default case when no
>       * permission is set on the page.
>       */
> -    rc = __p2m_get_mem_access(current->domain, gfn, &xma);
> +    rc = __p2m_get_mem_access(p2m, gfn, &xma);
>      if ( rc < 0 )
>          goto err;
>
> @@ -1588,7 +1607,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned 
> long flag)
>       * We had a mem_access permission limiting the access, but the page type
>       * could also be limiting, so we need to check that as well.
>       */
> -    mfn = __p2m_lookup(current->domain, gfn, &t);
> +    mfn = __p2m_lookup(p2m, gfn, &t, NULL, NULL, NULL);
>      if ( mfn_eq(mfn, INVALID_MFN) )
>          goto err;
>
> @@ -1671,7 +1690,7 @@ struct page_info *get_page_from_gva(struct vcpu *v, 
> vaddr_t va,
>
>  err:
>      if ( !page && p2m->mem_access_enabled )
> -        page = p2m_mem_access_check_and_get_page(va, flags);
> +        page = p2m_mem_access_check_and_get_page(p2m, va, flags);
p2m_mem_access_check_and_get_page should take a vCPU in parameter and 
not a p2m.
I know the function is already buggy because it assumes that the vCPU == 
current. But let's not broken more this function.
>      p2m_read_unlock(p2m);
>
> @@ -1948,7 +1967,7 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn,
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>
>      p2m_read_lock(p2m);
> -    ret = __p2m_get_mem_access(d, gfn, access);
> +    ret = __p2m_get_mem_access(p2m, gfn, access);
>      p2m_read_unlock(p2m);
>
>      return ret;
Regards,

[1] 
https://lists.xenproject.org/archives/html/xen-devel/2016-07/msg02952.html
--
Julien Grall

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