On 07/07/2018 12:05 PM, Paul Durrant wrote:
> ...for some uses of get_page_from_gfn().
>
> There are many occurences of the following pattern in the code:
>
> q = <readonly look-up> ? P2M_ALLOC : P2M_UNSHARE;
> page = get_page_from_gfn(d, gfn, &p2mt, q);
>
> if ( p2m_is_paging(p2mt) )
> {
> if ( page )
> put_page(page);
>
> p2m_mem_paging_populate(d, gfn);
> return <-ENOENT or equivalent>;
> }
>
> if ( (q & P2M_UNSHARE) && p2m_is_shared(p2mt) )
> {
> if ( page )
> put_page(page);
>
> return <-ENOENT or equivalent>;
> }
>
> if ( !page )
> return <-EINVAL or equivalent>;
>
> if ( !p2m_is_ram(p2mt) ||
> (!<readonly look-up> && p2m_is_readonly(p2mt)) )
> {
> put_page(page);
> return <-EINVAL or equivalent>;
> }
>
> There are some small differences between the exact way the occurrences are
> coded but the desired semantic is the same.
>
> This patch introduces a new common implementation of this code in
> get_paged_gfn() and then converts the various open-coded patterns into
> calls to this new function.
>
> Signed-off-by: Paul Durrant <paul.durr...@citrix.com>
This is a great idea.
It looks like this adds the restriction that the given gfn be ram (e.g.,
not MMIO) in all cases as well. It looks like that's what's wanted, but
it would be good to point this out in the commit message (so people can
verify that this change is warranted).
A few other comments...
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index c6b99c4116..510f37f100 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -375,39 +375,23 @@ static int get_paged_frame(unsigned long gfn, mfn_t
> *mfn,
> struct page_info **page, bool readonly,
> struct domain *rd)
> {
> - int rc = GNTST_okay;
> - p2m_type_t p2mt;
> -
> - *mfn = INVALID_MFN;
> - *page = get_page_from_gfn(rd, gfn, &p2mt,
> - readonly ? P2M_ALLOC : P2M_UNSHARE);
> - if ( !*page )
> - {
> -#ifdef P2M_SHARED_TYPES
> - if ( p2m_is_shared(p2mt) )
> - return GNTST_eagain;
> -#endif
> -#ifdef P2M_PAGES_TYPES
> - if ( p2m_is_paging(p2mt) )
> - {
> - p2m_mem_paging_populate(rd, gfn);
> - return GNTST_eagain;
> - }
> -#endif
> - return GNTST_bad_page;
> - }
> + int rc;
>
> - if ( p2m_is_foreign(p2mt) )
[snip]
> {
[snip]
> - put_page(*page);
> - *page = NULL;
> -
Comparing before-and-after, this seems to remove this 'p2m_is_foreign()'
check. Am I missing something?
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 35da9ca80e..419b76ac38 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1574,30 +1574,31 @@ void destroy_ring_for_helper(
> }
> }
>
> -int prepare_ring_for_helper(
> - struct domain *d, unsigned long gmfn, struct page_info **_page,
> - void **_va)
> +int get_paged_gfn(struct domain *d, gfn_t gfn, bool readonly,
> + p2m_type_t *p2mt_p, struct page_info **page_p)
This wants a comment somewhere describing exactly what the function does
and what it will return -- probably here above the function definition
itself would be the best.
> {
> - struct page_info *page;
> + p2m_query_t q = readonly ? P2M_ALLOC : P2M_UNSHARE;
> p2m_type_t p2mt;
> - void *va;
> + struct page_info *page;
>
> - page = get_page_from_gfn(d, gmfn, &p2mt, P2M_UNSHARE);
> + page = get_page_from_gfn(d, gfn_x(gfn), &p2mt, q);
>
> #ifdef CONFIG_HAS_MEM_PAGING
> if ( p2m_is_paging(p2mt) )
> {
> if ( page )
> put_page(page);
> - p2m_mem_paging_populate(d, gmfn);
> +
> + p2m_mem_paging_populate(d, gfn_x(gfn));
> return -ENOENT;
I realize you're copying the return values of prepare_ring_for_helper(),
but wouldn't -EAGAIN be more natural here?
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
