diff mbox

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

Message ID 1466515243-27264-8-git-send-email-julien.grall@arm.com
State Superseded
Headers show

Commit Message

Julien Grall June 21, 2016, 1:20 p.m. UTC
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@arm.com>
---
 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(-)

Comments

Julien Grall June 24, 2016, 1:58 p.m. UTC | #1
Hi Stefano,

On 23/06/16 15:14, Stefano Stabellini wrote:
> 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@arm.com>
>> ---
>>   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.

Good idea. It will be useful in other places too.

>
>>       paddr_t mask = 0;
>>       p2m_type_t _t;
>>       unsigned int level, root_table;


[...]

>> @@ -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

Will do.

Regards,
diff mbox

Patch

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);
     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 )
         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;