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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 17 March 2020 13:07
> To: paul@xxxxxxx
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Durrant, Paul <pdurrant@xxxxxxxxxxxx>; 
> Tamas K Lengyel
> <tamas@xxxxxxxxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Wei Liu 
> <wl@xxxxxxx>; Roger Pau
> Monné <roger.pau@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>; Ian 
> Jackson
> <ian.jackson@xxxxxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Konrad Rzeszutek 
> Wilk
> <konrad.wilk@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Tim 
> Deegan <tim@xxxxxxx>
> Subject: RE: [EXTERNAL] [PATCH v6 4/5] mm: add 'is_special_page' inline 
> function...
> 
> CAUTION: This email originated from outside of the organization. Do not click 
> links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> 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().
> 

I'll check again.

> > --- 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.

What's suspicious about it? I now realise the above comment also needs fixing.

> 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.
> 

I could go back to defining is_special_page() as a macro.

> > --- 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.
> 

Ok.

> > --- 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.
> 

Yes.

  Paul

> Jan


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