[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v3 7/8] xen/arm: Rework the interface of p2m_lookup and use typesafe gfn and mfn



On Tue, 21 Jun 2016, Julien Grall wrote:
> The prototype and the declaration of p2m_lookup disagree on how the
> function should be used. One expect a frame number whilst the other
> an address.
> 
> Thankfully, everyone is using with an address today. However, most of
> the callers have to convert a guest frame to an address. Modify
> the interface to take a guest physical frame in parameter and return
> a machine frame.
> 
> Whilst modifying the interface, use typesafe gfn and mfn for clarity
> and catching possible misusage.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> ---
>  xen/arch/arm/p2m.c        | 37 ++++++++++++++++++++-----------------
>  xen/arch/arm/traps.c      | 21 +++++++++++----------
>  xen/include/asm-arm/p2m.h |  7 +++----
>  3 files changed, 34 insertions(+), 31 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 47cb383..f3330dd 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -140,14 +140,15 @@ void flush_tlb_domain(struct domain *d)
>  }
>  
>  /*
> - * Lookup the MFN corresponding to a domain's PFN.
> + * 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.
>   */
> -static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
> +static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
>  {
>      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),
> @@ -158,7 +159,7 @@ static paddr_t __p2m_lookup(struct domain *d, paddr_t 
> paddr, p2m_type_t *t)
>          ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK
>      };
>      lpae_t pte, *map;
> -    paddr_t maddr = INVALID_PADDR;
> +    mfn_t mfn = _mfn(INVALID_MFN);

It might be worth defining INVALID_MFN_T and just assign that to mfn.


>      paddr_t mask = 0;
>      p2m_type_t _t;
>      unsigned int level, root_table;
> @@ -216,21 +217,22 @@ static paddr_t __p2m_lookup(struct domain *d, paddr_t 
> paddr, p2m_type_t *t)
>      {
>          ASSERT(mask);
>          ASSERT(pte.p2m.type != p2m_invalid);
> -        maddr = (pte.bits & PADDR_MASK & mask) | (paddr & ~mask);
> +        mfn = _mfn(paddr_to_pfn((pte.bits & PADDR_MASK & mask) |
> +                                (paddr & ~mask)));
>          *t = pte.p2m.type;
>      }
>  
>  err:
> -    return maddr;
> +    return mfn;
>  }
>  
> -paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
> +mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
>  {
> -    paddr_t ret;
> +    mfn_t ret;
>      struct p2m_domain *p2m = &d->arch.p2m;
>  
>      spin_lock(&p2m->lock);
> -    ret = __p2m_lookup(d, paddr, t);
> +    ret = __p2m_lookup(d, gfn, t);
>      spin_unlock(&p2m->lock);
>  
>      return ret;
> @@ -493,8 +495,9 @@ 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.
>           */
> -        paddr_t maddr = __p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
> -        if ( INVALID_PADDR == maddr )
> +        mfn_t mfn = __p2m_lookup(d, gfn, NULL);
> +
> +        if ( mfn_x(mfn) == INVALID_MFN )
>              return -ESRCH;
>  
>          /* If entry exists then its rwx. */
> @@ -1483,8 +1486,7 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t 
> start_mfn, xen_pfn_t end_mfn)
>  
>  mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn)
>  {
> -    paddr_t p = p2m_lookup(d, pfn_to_paddr(gfn_x(gfn)), NULL);
> -    return _mfn(p >> PAGE_SHIFT);
> +    return p2m_lookup(d, gfn, NULL);
>  }
>  
>  /*
> @@ -1498,7 +1500,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned 
> long flag)
>  {
>      long rc;
>      paddr_t ipa;
> -    unsigned long maddr;
> +    gfn_t gfn;
>      unsigned long mfn;
>      xenmem_access_t xma;
>      p2m_type_t t;
> @@ -1508,11 +1510,13 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, 
> unsigned long flag)
>      if ( rc < 0 )
>          goto err;
>  
> +    gfn = _gfn(paddr_to_pfn(ipa));
> +
>      /*
>       * 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(paddr_to_pfn(ipa)), 
> &xma);
> +    rc = __p2m_get_mem_access(current->domain, gfn, &xma);
>      if ( rc < 0 )
>          goto err;
>  
> @@ -1561,11 +1565,10 @@ 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.
>       */
> -    maddr = __p2m_lookup(current->domain, ipa, &t);
> -    if ( maddr == INVALID_PADDR )
> +    mfn = mfn_x(__p2m_lookup(current->domain, gfn, &t));
> +    if ( mfn == INVALID_MFN )

The conversion would go away if we had an INVALID_MFN which is mfn_t


>          goto err;
>  
> -    mfn = maddr >> PAGE_SHIFT;
>      if ( !mfn_valid(mfn) )
>          goto err;
>  
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 44926ca..02fe117 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2319,14 +2319,16 @@ void dump_guest_s1_walk(struct domain *d, vaddr_t 
> addr)
>  {
>      register_t ttbcr = READ_SYSREG(TCR_EL1);
>      uint64_t ttbr0 = READ_SYSREG64(TTBR0_EL1);
> -    paddr_t paddr;
>      uint32_t offset;
>      uint32_t *first = NULL, *second = NULL;
> +    mfn_t mfn;
> +
> +    mfn = p2m_lookup(d, _gfn(paddr_to_pfn(ttbr0)), NULL);
>  
>      printk("dom%d VA 0x%08"PRIvaddr"\n", d->domain_id, addr);
>      printk("    TTBCR: 0x%08"PRIregister"\n", ttbcr);
>      printk("    TTBR0: 0x%016"PRIx64" = 0x%"PRIpaddr"\n",
> -           ttbr0, p2m_lookup(d, ttbr0 & PAGE_MASK, NULL));
> +           ttbr0, pfn_to_paddr(mfn_x(mfn)));
>  
>      if ( ttbcr & TTBCR_EAE )
>      {
> @@ -2339,32 +2341,31 @@ void dump_guest_s1_walk(struct domain *d, vaddr_t 
> addr)
>          return;
>      }
>  
> -    paddr = p2m_lookup(d, ttbr0 & PAGE_MASK, NULL);
> -    if ( paddr == INVALID_PADDR )
> +    if ( mfn_x(mfn) == INVALID_MFN )
>      {
>          printk("Failed TTBR0 maddr lookup\n");
>          goto done;
>      }
> -    first = map_domain_page(_mfn(paddr_to_pfn(paddr)));
> +    first = map_domain_page(mfn);
>  
>      offset = addr >> (12+10);
>      printk("1ST[0x%"PRIx32"] (0x%"PRIpaddr") = 0x%08"PRIx32"\n",
> -           offset, paddr, first[offset]);
> +           offset, pfn_to_paddr(mfn_x(mfn)), first[offset]);
>      if ( !(first[offset] & 0x1) ||
>           !(first[offset] & 0x2) )
>          goto done;
>  
> -    paddr = p2m_lookup(d, first[offset] & PAGE_MASK, NULL);
> +    mfn = p2m_lookup(d, _gfn(paddr_to_pfn(first[offset])), NULL);
>  
> -    if ( paddr == INVALID_PADDR )
> +    if ( mfn_x(mfn) == INVALID_MFN )
>      {
>          printk("Failed L1 entry maddr lookup\n");
>          goto done;
>      }
> -    second = map_domain_page(_mfn(paddr_to_pfn(paddr)));
> +    second = map_domain_page(mfn);
>      offset = (addr >> 12) & 0x3FF;
>      printk("2ND[0x%"PRIx32"] (0x%"PRIpaddr") = 0x%08"PRIx32"\n",
> -           offset, paddr, second[offset]);
> +           offset, pfn_to_paddr(mfn_x(mfn)), second[offset]);
>  
>  done:
>      if (second) unmap_domain_page(second);
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 0d1e61e..f204482 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -135,8 +135,8 @@ void p2m_restore_state(struct vcpu *n);
>  /* Print debugging/statistial info about a domain's p2m */
>  void p2m_dump_info(struct domain *d);
>  
> -/* Look up the MFN corresponding to a domain's PFN. */
> -paddr_t p2m_lookup(struct domain *d, paddr_t gpfn, p2m_type_t *t);
> +/* Look up the MFN corresponding to a domain's GFN. */
> +mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);
>  
>  /* Clean & invalidate caches corresponding to a region of guest address 
> space */
>  int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t 
> end_mfn);
> @@ -201,8 +201,7 @@ static inline struct page_info *get_page_from_gfn(
>  {
>      struct page_info *page;
>      p2m_type_t p2mt;
> -    paddr_t maddr = p2m_lookup(d, pfn_to_paddr(gfn), &p2mt);
> -    unsigned long mfn = maddr >> PAGE_SHIFT;
> +    unsigned long mfn = mfn_x(p2m_lookup(d, _gfn(gfn), &p2mt));
>  
>      if (t)
>          *t = p2mt;
> -- 
> 1.9.1
> 

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.