Re: [Xen-devel] [PATCH v6 4/5] mm: add 'is_special_page' inline function...

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.03.2020 18:49, paul@xxxxxxx wrote:
> In auditing open-coded tests of PGC_xen_heap, I am unsure if offline_page()
> needs to check for PGC_extra pages too.

"Extra" pages being the designated replacement for Xen heap ones,
I think it should. Then again the earlier

    if ( (owner = page_get_owner_and_reference(pg)) )

should succeed on them (as much as it should for Xen heap pages
shared with a domain), so perhaps simply say something to this
effect in the description?

> @@ -4216,8 +4216,7 @@ int steal_page(
>      if ( !(owner = page_get_owner_and_reference(page)) )
>          goto fail;
>  
> -    if ( owner != d || is_xen_heap_page(page) ||
> -         (page->count_info & PGC_extra) )
> +    if ( owner != d || is_special_page(page) )
>          goto fail_put;
>  
>      /*

A few hundred lines down from here in xenmem_add_to_physmap_one()
there is a use of is_xen_heap_mfn(). Any reason that doesn't get
converted? Same question - because of the code being similar -
then goes for mm/p2m.c:p2m_add_foreign().

> --- a/xen/arch/x86/mm/p2m-pod.c
> +++ b/xen/arch/x86/mm/p2m-pod.c
> @@ -749,8 +749,9 @@ p2m_pod_zero_check_superpage(struct p2m_domain *p2m, 
> gfn_t gfn)
>  
>          n = 1UL << min(cur_order, SUPERPAGE_ORDER + 0U);
>          for ( k = 0, page = mfn_to_page(mfn); k < n; ++k, ++page )
> -            if ( !(page->count_info & PGC_allocated) ||
> -                 (page->count_info & (PGC_page_table | PGC_xen_heap)) ||
> +            if ( is_special_page(page) ||
> +                 !(page->count_info & PGC_allocated) ||
> +                 (page->count_info & PGC_page_table) ||
>                   (page->count_info & PGC_count_mask) > max_ref )
>                  goto out;
>      }
> @@ -886,8 +887,9 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const gfn_t 
> *gfns, unsigned int count
>           * If this is ram, and not a pagetable or from the xen heap, and
>           * probably not mapped elsewhere, map it; otherwise, skip.
>           */
> -        if ( p2m_is_ram(types[i]) && (pg->count_info & PGC_allocated) &&
> -             !(pg->count_info & (PGC_page_table | PGC_xen_heap)) &&
> +        if ( p2m_is_ram(types[i]) && !is_special_page(pg) &&
> +             (pg->count_info & PGC_allocated) &&
> +             !(pg->count_info & PGC_page_table) &&
>               ((pg->count_info & PGC_count_mask) <= max_ref) )
>              map[i] = map_domain_page(mfns[i]);
>          else

I appreciate your desire to use the inline function you add, and
I also appreciate that you likely prefer to not make the other
suggested change (at least not right here), but then I think the
commit message would better gain a remark regarding the
suspicious uses of PGC_page_table here. I continue to think that
they should be dropped as being pointless. In any event I fear
the resulting code will be less efficient, as I'm unconvinced
that the compiler will fold the now split bit checks.

> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -559,7 +559,7 @@ _sh_propagate(struct vcpu *v,
>       * caching attributes in the shadows to match what was asked for.
>       */
>      if ( (level == 1) && is_hvm_domain(d) &&
> -         !is_xen_heap_mfn(target_mfn) )
> +         !is_special_page(mfn_to_page(target_mfn)) )

Careful - is_xen_heap_mfn() also includes an mfn_valid() check.
Code a few lines up from here suggests that MMIO MFNs can make
it here.

> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -285,6 +285,11 @@ extern struct domain *dom_cow;
>  
>  #include <asm/mm.h>
>  
> +static inline bool is_special_page(const struct page_info *page)
> +{
> +    return is_xen_heap_page(page) || (page->count_info & PGC_extra);

Seeing Arm32's implementation I understand why you need to use
|| here; it's a pity the two checks can't be folded. Hopefully
at least here the compiler recognizes the opportunity.

Jan

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