diff mbox series

[Xen-devel,2/8] xen/arm: p2m: Introduce p2m_get_page_from_gfn

Message ID 20181106191454.22143-3-julien.grall@arm.com
State Superseded
Headers show
Series xen/arm: Add xentrace support | expand

Commit Message

Julien Grall Nov. 6, 2018, 7:14 p.m. UTC
In a follow-up patches, we will need to handle get_page_from_gfn
differently for DOMID_XEN. To keep the code simple move the current
content in a new separate helper p2m_get_page_from_gfn.

Note the new helper is a not anymore a static inline function as the helper
is quite complex.

Finally, take the opportunity to use typesafe gfn as the change is
minor.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/p2m.c        | 32 ++++++++++++++++++++++++++++++++
 xen/include/asm-arm/p2m.h | 33 ++++-----------------------------
 2 files changed, 36 insertions(+), 29 deletions(-)

Comments

Andrii Anisov Nov. 15, 2018, 1:31 p.m. UTC | #1
Hello Julien,

вт, 6 лист. 2018 о 21:16 Julien Grall <julien.grall@arm.com> пише:
>
> In a follow-up patches, we will need to handle get_page_from_gfn
> differently for DOMID_XEN. To keep the code simple move the current
> content in a new separate helper p2m_get_page_from_gfn.
>
> Note the new helper is a not anymore a static inline function as the helper
> is quite complex.

In the patch "[PATCH 4/8] xen/arm: Add support for read-only foreign
mappings" you make p2m_get_page_from_gfn() comparingly complex, but
still keep it as static inline. Could you please be consistent (making
both of those functions inline or non-inline)

Sincerely,
Andrii Anisov.
Andrii Anisov Nov. 15, 2018, 1:35 p.m. UTC | #2
Sorry,

Not "comparingly complex", but "equally complex". ;)

Sincerely,
Andrii Anisov.
чт, 15 лист. 2018 о 15:31 Andrii Anisov <andrii.anisov@gmail.com> пише:
>
> Hello Julien,
>
> вт, 6 лист. 2018 о 21:16 Julien Grall <julien.grall@arm.com> пише:
> >
> > In a follow-up patches, we will need to handle get_page_from_gfn
> > differently for DOMID_XEN. To keep the code simple move the current
> > content in a new separate helper p2m_get_page_from_gfn.
> >
> > Note the new helper is a not anymore a static inline function as the helper
> > is quite complex.
>
> In the patch "[PATCH 4/8] xen/arm: Add support for read-only foreign
> mappings" you make p2m_get_page_from_gfn() comparingly complex, but
> still keep it as static inline. Could you please be consistent (making
> both of those functions inline or non-inline)
>
> Sincerely,
> Andrii Anisov.
Julien Grall Nov. 15, 2018, 3:22 p.m. UTC | #3
On 11/15/18 1:31 PM, Andrii Anisov wrote:
> Hello Julien,

Hi,

> 
> вт, 6 лист. 2018 о 21:16 Julien Grall <julien.grall@arm.com> пише:
>>
>> In a follow-up patches, we will need to handle get_page_from_gfn
>> differently for DOMID_XEN. To keep the code simple move the current
>> content in a new separate helper p2m_get_page_from_gfn.
>>
>> Note the new helper is a not anymore a static inline function as the helper
>> is quite complex.
> 
> In the patch "[PATCH 4/8] xen/arm: Add support for read-only foreign
> mappings" you make p2m_get_page_from_gfn() comparingly complex, but
> still keep it as static inline. Could you please be consistent (making
> both of those functions inline or non-inline)

The reason I didn't move the other one in p2m.c is because so far p2m.c 
is only dealing with auto-translated guest. I didn't want to add 
function related with non-auto translated guest in it.

I also don't think introduce a new file for one 10 line function is 
really useful.

So that was the best solution. I am open to other suggestion.

Cheers,
Andrii Anisov Dec. 20, 2018, 11:20 a.m. UTC | #4
On 15.11.18 17:22, Julien Grall wrote:
> The reason I didn't move the other one in p2m.c is because so far p2m.c is only dealing with auto-translated guest. I didn't want to add function related with non-auto translated guest in it.
> 
> I also don't think introduce a new file for one 10 line function is really useful.
> 
> So that was the best solution. I am open to other suggestion.
> 
> Cheers,

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
diff mbox series

Patch

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 30cfb01498..04c8718e9f 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -379,6 +379,38 @@  mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
     return mfn;
 }
 
+struct page_info *p2m_get_page_from_gfn(struct domain *d, gfn_t gfn,
+                                        p2m_type_t *t)
+{
+    struct page_info *page;
+    p2m_type_t p2mt;
+    mfn_t mfn = p2m_lookup(d, gfn, &p2mt);
+
+    if (t)
+        *t = p2mt;
+
+    if ( !p2m_is_any_ram(p2mt) )
+        return NULL;
+
+    if ( !mfn_valid(mfn) )
+        return NULL;
+    page = mfn_to_page(mfn);
+
+    /*
+     * get_page won't work on foreign mapping because the page doesn't
+     * belong to the current domain.
+     */
+    if ( p2m_is_foreign(p2mt) )
+    {
+        struct domain *fdom = page_get_owner_and_reference(page);
+        ASSERT(fdom != NULL);
+        ASSERT(fdom != d);
+        return page;
+    }
+
+    return (get_page(page, d) ? page: NULL);
+}
+
 int guest_physmap_mark_populate_on_demand(struct domain *d,
                                           unsigned long gfn,
                                           unsigned int order)
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index c03557544a..a5914136e3 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -269,38 +269,13 @@  typedef unsigned int p2m_query_t;
 #define P2M_ALLOC    (1u<<0)   /* Populate PoD and paged-out entries */
 #define P2M_UNSHARE  (1u<<1)   /* Break CoW sharing */
 
+struct page_info *p2m_get_page_from_gfn(struct domain *d, gfn_t gfn,
+                                        p2m_type_t *t);
+
 static inline struct page_info *get_page_from_gfn(
     struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
 {
-    struct page_info *page;
-    p2m_type_t p2mt;
-    mfn_t mfn = p2m_lookup(d, _gfn(gfn), &p2mt);
-
-    if (t)
-        *t = p2mt;
-
-    if ( !p2m_is_any_ram(p2mt) )
-        return NULL;
-
-    if ( !mfn_valid(mfn) )
-        return NULL;
-    page = mfn_to_page(mfn);
-
-    /*
-     * get_page won't work on foreign mapping because the page doesn't
-     * belong to the current domain.
-     */
-    if ( p2m_is_foreign(p2mt) )
-    {
-        struct domain *fdom = page_get_owner_and_reference(page);
-        ASSERT(fdom != NULL);
-        ASSERT(fdom != d);
-        return page;
-    }
-
-    if ( !get_page(page, d) )
-        return NULL;
-    return page;
+    return p2m_get_page_from_gfn(d, _gfn(gfn), t);
 }
 
 int get_page_type(struct page_info *page, unsigned long type);