Re: [Xen-devel] [PATCH 04/14] xen/x86: Use mfn_to_gfn rather than mfn_to_gmfn

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.05.19 at 17:14, <julien.grall@xxxxxxx> wrote:
> mfn_to_gfn and mfn_to_gmfn are doing exactly the same except the former
> is using mfn_t.

... and gfn_t (return type) as of patch 3.

> Furthermore, the naming of the former is more consistent with the
> current naming scheme (GFN/MFN). So use replace mfn_to_gmfn with
> mfn_to_gfn in x86 code.

Nit: Either "use" or "replace with", but not both.

> @@ -713,19 +713,20 @@ int arch_domain_soft_reset(struct domain *d)
>      ASSERT( owner == d );
>  
>      mfn = page_to_mfn(page);
> -    gfn = mfn_to_gmfn(d, mfn_x(mfn));
> +    gfn = mfn_to_gfn(d, mfn);
>  
>      /*
>       * gfn == INVALID_GFN indicates that the shared_info page was never 
> mapped
>       * to the domain's address space and there is nothing to replace.
>       */
> -    if ( gfn == gfn_x(INVALID_GFN) )
> +    if ( gfn_eq(gfn, INVALID_GFN) )
>          goto exit_put_page;
>  
> -    if ( !mfn_eq(get_gfn_query(d, gfn, &p2mt), mfn) )
> +    if ( !mfn_eq(get_gfn_query(d, gfn_x(gfn), &p2mt), mfn) )
>      {
> -        printk(XENLOG_G_ERR "Failed to get Dom%d's shared_info GFN (%lx)\n",
> -               d->domain_id, gfn);
> +        printk(XENLOG_G_ERR
> +               "Failed to get %pd's shared_info GFN (%"PRI_gfn")\n",

I'd recommend to drop the parentheses from the format string at the
same time.

> @@ -733,31 +734,34 @@ int arch_domain_soft_reset(struct domain *d)
>      new_page = alloc_domheap_page(d, 0);
>      if ( !new_page )
>      {
> -        printk(XENLOG_G_ERR "Failed to alloc a page to replace"
> -               " Dom%d's shared_info frame %lx\n", d->domain_id, gfn);
> +        printk(XENLOG_G_ERR
> +               "Failed to alloc a page to replace %pd's shared_info frame 
> %"PRI_gfn"\n",

s/frame/GFN/ to better match the earlier one? Same in the further log
messages here then.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2632,19 +2632,20 @@ int free_page_type(struct page_info *page, unsigned 
> long type,
>  {
>  #ifdef CONFIG_PV
>      struct domain *owner = page_get_owner(page);
> -    unsigned long gmfn;
>      int rc;
>  
>      if ( likely(owner != NULL) && unlikely(paging_mode_enabled(owner)) )
>      {
> +        gfn_t gfn;
> +
>          /* A page table is dirtied when its type count becomes zero. */
>          paging_mark_dirty(owner, page_to_mfn(page));
>  
>          ASSERT(!shadow_mode_refcounts(owner));
>  
> -        gmfn = mfn_to_gmfn(owner, mfn_x(page_to_mfn(page)));
> -        if ( VALID_M2P(gmfn) )
> -            shadow_remove_all_shadows(owner, _mfn(gmfn));
> +        gfn = mfn_to_gfn(owner, page_to_mfn(page));
> +        if ( VALID_M2P(gfn_x(gfn)) )
> +            shadow_remove_all_shadows(owner, _mfn(gfn_x(gfn)));
>      }

This is a highly suspicious change imo (albeit the code was bogus
already before): It certainly isn't GFN here even if we were to assume
translated mode could be in use. One other caller of
the function, sh_page_fault() passes a variable named gmfn as well,
but typed mfn_t (and this gmfn gets set from get_gfn(), i.e. is _not_
a GFN). The 3rd one, shadow_prepare_page_type_change(), clearly
passes an MFN.

I think the best course of action here is to split out the change,
just to explain why removing the mfn_to_gmfn() here altogether
is appropriate nowadays: PV guests can't be in translated mode
anymore, and hence mfn_to_gmfn() doesn't do any translation. At
that point the VALID_M2P() check can go away as well, so you'll be
able to simply do

        shadow_remove_all_shadows(owner, page_to_mfn(page));

perhaps with another !shadow_mode_translate() assertion added
next to the one that's already there. Tim, thoughts?

With this split out and irrespective of whether you decide to follow
the format string suggestions further up
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan



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