[Xen-devel,RFC,13/22] xen/arm: p2m: Replace all usage of __p2m_lookup with p2m_get_entry

Message ID 1469717505-8026-14-git-send-email-julien.grall@arm.com
State New
Headers show

Commit Message

Julien Grall July 28, 2016, 2:51 p.m.
__p2m_lookup is just a wrapper to p2m_get_entry.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Tamas K Lengyel <tamas@tklengyel.com>

---
    It might be possible to rework the memaccess code to take advantage
    of all the parameters. I will defer this to the memaccess folks.
---
 xen/arch/arm/p2m.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

Comments

Julien Grall July 28, 2016, 5:51 p.m. | #1
Hello Tamas,

On 28/07/2016 18:29, Tamas K Lengyel wrote:
> On Thu, Jul 28, 2016 at 8:51 AM, Julien Grall <julien.grall@arm.com> wrote:

[...]

>> ---
>>  xen/arch/arm/p2m.c | 18 ++++--------------
>>  1 file changed, 4 insertions(+), 14 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 8676b9d..9a9c85c 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -398,24 +398,13 @@ out:
>>      return mfn;
>>  }
>>
>> -/*
>> - * 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 mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
>> -{
>> -    return p2m_get_entry(&d->arch.p2m, gfn, t, NULL, NULL);
>> -}
>> -
>>  mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
>>  {
>>      mfn_t ret;
>>      struct p2m_domain *p2m = &d->arch.p2m;
>>
>>      p2m_read_lock(p2m);
>> -    ret = __p2m_lookup(d, gfn, t);
>> +    ret = p2m_get_entry(p2m, gfn, t, NULL, NULL);
>>      p2m_read_unlock(p2m);
>>
>>      return ret;
>> @@ -679,7 +668,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_get_entry(p2m, gfn, NULL, NULL, NULL);
>>
>>          if ( mfn_eq(mfn, INVALID_MFN) )
>>              return -ESRCH;
>> @@ -1595,6 +1584,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag)
>>      xenmem_access_t xma;
>>      p2m_type_t t;
>>      struct page_info *page = NULL;
>> +    struct p2m_domain *p2m = &current->domain->arch.p2m;
>
> I think this would be a good time to change this and pass p2m as an
> input to p2m_mem_access_check_and_get_page. This would help with our
> altp2m series as well.

This function can only work with the current p2m because of the call to 
gva_to_ipa. So I don't think it is a good idea to pass the p2m in parameter.

If you want to pass the p2m in parameter, you have to context switch 
properly all the registers (i.e VTTBR_EL2, TTBR{0,1}_EL1 and SCTLR_EL1).

Actually, this function is buggy if memaccess has changed the permission 
on memory holding the stage-1 page-table. Because we are using the 
hardware to translate the VA -> PA, the translate may fail due to memaccess.

Regards,
Julien Grall July 29, 2016, 3:06 p.m. | #2
On 28/07/16 18:36, Tamas K Lengyel wrote:
> On Thu, Jul 28, 2016 at 11:29 AM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
>> On Thu, Jul 28, 2016 at 8:51 AM, Julien Grall <julien.grall@arm.com> wrote:
>>> __p2m_lookup is just a wrapper to p2m_get_entry.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>> Cc: Tamas K Lengyel <tamas@tklengyel.com>
>>>
>>> ---
>>>     It might be possible to rework the memaccess code to take advantage
>>>     of all the parameters. I will defer this to the memaccess folks.
>>
>> Could you elaborate on what you mean?
>>
>
> Never mind, I see it. Yes, doing __p2m_get_mem_access and then
> p2m_get_entry later duplicates work. I would suggest just replacing
> __p2m_get_mem_access with a single call to p2m_get_entry to get both
> the type and the mem_access setting on the page in a single run.

I am not planning to clean-up the memaccess code. Feel free to send a 
follow-up patch for that.

Regards,

Patch

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 8676b9d..9a9c85c 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -398,24 +398,13 @@  out:
     return mfn;
 }
 
-/*
- * 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 mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
-{
-    return p2m_get_entry(&d->arch.p2m, gfn, t, NULL, NULL);
-}
-
 mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
 {
     mfn_t ret;
     struct p2m_domain *p2m = &d->arch.p2m;
 
     p2m_read_lock(p2m);
-    ret = __p2m_lookup(d, gfn, t);
+    ret = p2m_get_entry(p2m, gfn, t, NULL, NULL);
     p2m_read_unlock(p2m);
 
     return ret;
@@ -679,7 +668,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_get_entry(p2m, gfn, NULL, NULL, NULL);
 
         if ( mfn_eq(mfn, INVALID_MFN) )
             return -ESRCH;
@@ -1595,6 +1584,7 @@  p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag)
     xenmem_access_t xma;
     p2m_type_t t;
     struct page_info *page = NULL;
+    struct p2m_domain *p2m = &current->domain->arch.p2m;
 
     rc = gva_to_ipa(gva, &ipa, flag);
     if ( rc < 0 )
@@ -1655,7 +1645,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_get_entry(p2m, gfn, &t, NULL, NULL);
     if ( mfn_eq(mfn, INVALID_MFN) )
         goto err;