Re: [Xen-devel] [PATCH v6 1/9] x86/p2m: Allow p2m_get_page_from_gfn to return shared entries

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.01.2020 19:06, Tamas K Lengyel wrote:
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -574,11 +574,12 @@ struct page_info *p2m_get_page_from_gfn(
>                  if ( fdom == NULL )
>                      page = NULL;
>              }
> -            else if ( !get_page(page, p2m->domain) &&
> -                      /* Page could be shared */
> -                      (!dom_cow || !p2m_is_shared(*t) ||
> -                       !get_page(page, dom_cow)) )
> -                page = NULL;
> +            else
> +            {
> +                struct domain *d = !p2m_is_shared(*t) ? p2m->domain : 
> dom_cow;
> +                if ( !get_page(page, d) )
> +                    page = NULL;
> +            }
>          }
>          p2m_read_unlock(p2m);
>  
> @@ -594,8 +595,9 @@ struct page_info *p2m_get_page_from_gfn(
>      mfn = get_gfn_type_access(p2m, gfn_x(gfn), t, a, q, NULL);
>      if ( p2m_is_ram(*t) && mfn_valid(mfn) )
>      {
> +        struct domain *d = !p2m_is_shared(*t) ? p2m->domain : dom_cow;
>          page = mfn_to_page(mfn);
> -        if ( !get_page(page, p2m->domain) )
> +        if ( !get_page(page, d) )
>              page = NULL;
>      }
>      put_gfn(p2m->domain, gfn_x(gfn));

Personally I would have preferred to go without new local variables
here, but I'm not the maintainer of this code. However, at the very
least blank lines would need inserting between declaration and
statements. With at least this done (which could be done while
committing)

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

As an aside, I don't think the title accurately describes the
change, since there's just one out of two cases which shared
entries weren't taken care of. Similarly the description doesn't
reflect this at all.

Jan

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