Re: [Xen-devel] [PATCH for-next 7/9] xen: Convert is_xen_heap_mfn to use typesafe MFN

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 13/03/2019 15:04, Jan Beulich wrote:
> > > > On 18.02.19 at 12:35, <julien.grall@xxxxxxx> wrote:
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -4300,7 +4300,8 @@ int xenmem_add_to_physmap_one(
> >   {
> >       struct page_info *page = NULL;
> >       unsigned long gfn = 0; /* gcc ... */
> > -    unsigned long prev_mfn, old_gpfn;
> > +    mfn_t prev_mfn;
> > +    unsigned long old_gpfn;
> Please can you put this together with "gfn"?
Sure.

> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2121,9 +2121,9 @@ void init_xenheap_pages(paddr_t ps, paddr_t pe)
> >        * Yuk! Ensure there is a one-page buffer between Xen and Dom zones, to
> >        * prevent merging of power-of-two blocks across the zone boundary.
> >        */
> > -    if ( ps && !is_xen_heap_mfn(paddr_to_pfn(ps)-1) )
> > +    if ( ps && !is_xen_heap_mfn(_mfn(paddr_to_pfn(ps)-1)) )
> Please add the missing blanks around - at the same time.
Ok.

> >           ps += PAGE_SIZE;
> > -    if ( !is_xen_heap_mfn(paddr_to_pfn(pe)) )
> > +    if ( !is_xen_heap_mfn(maddr_to_mfn(pe)) )
> Why maddr_to_mfn() here but still paddr_to_pfn() above? Oh,
> we don't have any mfn_sub(), I see.
Yes we don't have mfn_sub() (or even gfn_sub()). I only found a couple of places 
where such helpers might be useful. I can introduce the 2 helpers if you think 
it is worth it.
> > --- a/xen/include/asm-x86/mm.h
> > +++ b/xen/include/asm-x86/mm.h
> > @@ -278,7 +278,7 @@ struct page_info
> >   
> >   #define is_xen_heap_page(page) ((page)->count_info & PGC_xen_heap)
> >   #define is_xen_heap_mfn(mfn) \
> > -    (__mfn_valid(mfn) && is_xen_heap_page(mfn_to_page(_mfn(mfn))))
> > +    (__mfn_valid(mfn_x(mfn)) && is_xen_heap_page(mfn_to_page(mfn)))
> Please don't open code mfn_valid().
That's a mistake. Someone I thought there were place in the code that was 
overriding mfn_valid().
I will update it.

> With these minor issues taken care of
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
Thank you!

> Jan
--
Julien Grall

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