Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.03.2020 13:45, pdurrant@xxxxxxxx wrote:
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -2087,19 +2087,22 @@ static int sh_remove_all_mappings(struct domain *d, 
> mfn_t gmfn, gfn_t gfn)
>           * The qemu helper process has an untyped mapping of this dom's RAM
>           * and the HVM restore program takes another.
>           * Also allow one typed refcount for
> -         * - Xen heap pages, to match share_xen_page_with_guest(),
> -         * - ioreq server pages, to match prepare_ring_for_helper().
> +         * - special pages, which are explicitly referenced and mapped by
> +         *   Xen.
> +         * - ioreq server pages, which may be special pages or normal
> +         *   guest pages with an extra reference taken by
> +         *   prepare_ring_for_helper().
>           */
>          if ( !(shadow_mode_external(d)
>                 && (page->count_info & PGC_count_mask) <= 3
>                 && ((page->u.inuse.type_info & PGT_count_mask)
> -                   == (is_xen_heap_page(page) ||
> +                   == (is_special_page(page) ||
>                         (is_hvm_domain(d) && is_ioreq_server_page(d, 
> page))))) )

Shouldn't you delete most of this line, after the previous patch
converted the ioreq server pages to PGC_extra ones?

>              printk(XENLOG_G_ERR "can't find all mappings of mfn %"PRI_mfn
> -                   " (gfn %"PRI_gfn"): c=%lx t=%lx x=%d i=%d\n",
> +                   " (gfn %"PRI_gfn"): c=%lx t=%lx s=%d i=%d\n",
>                     mfn_x(gmfn), gfn_x(gfn),
>                     page->count_info, page->u.inuse.type_info,
> -                   !!is_xen_heap_page(page),
> +                   !!is_special_page(page),

The !! would be nice to go away at this occasion:

> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -285,6 +285,9 @@ extern struct domain *dom_cow;
>  
>  #include <asm/mm.h>
>  
> +#define is_special_page(page) \
> +    (is_xen_heap_page(page) || ((page)->count_info & PGC_extra))

Can this become an inline function returning bool?

Also I notice this construct is used by x86 code only - is there
a particular reason it doesn't get placed in an x86 header (at
least for the time being)?

Further I notice you neither take care of is_xen_heap_mfn(), nor
does the description explain why that would not also need at
least considering conversion. _sh_propagate(), for example, has
an instance that I think would need changing.

Finally I notice there are two is_xen_heap_page() uses in tboot.c,
both of which look like they also want converting.

Jan

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