Re: [Xen-devel] [PATCH v5 03/18] x86/p2m: Allow p2m_get_page_from_gfn to return shared entries

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.01.2020 18:49, Tamas K Lengyel wrote:
> The owner domain of shared pages is dom_cow, use that for get_page
> otherwise the function fails to return the correct page.

I think this description needs improvement: The function does the
special shared page dance in one place (on the "fast path")
already. This wants mentioning, either because it was a mistake
to have it just there, or because a new need has appeared to also
have it on the "slow path".

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -594,7 +594,10 @@ struct page_info *p2m_get_page_from_gfn(
>      if ( p2m_is_ram(*t) && mfn_valid(mfn) )
>      {
>          page = mfn_to_page(mfn);
> -        if ( !get_page(page, p2m->domain) )
> +        if ( !get_page(page, p2m->domain) &&
> +             /* Page could be shared */
> +             (!dom_cow || !p2m_is_shared(*t) ||
> +              !get_page(page, dom_cow)) )

While there may be a reason why on the fast path two get_page()
invocations are be necessary, couldn't you get away with just
one

        if ( !get_page(page, !dom_cow || !p2m_is_shared(*t) ? p2m->domain
                                                            : dom_cow) )

at least here? It's also not really clear to me why here and
there we need "!dom_cow || !p2m_is_shared(*t)" - wouldn't
p2m_is_shared() return consistently "false" when !dom_cow ?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel