| Message ID | 20181106191454.22143-3-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Add xentrace support | expand |
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.
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.
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,
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 --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);
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(-)