Re: [Xen-devel] [PATCH 04/17] xen: Convert virt_to_mfn() and mfn_to_virt() to use typesafe MFN

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 25/03/2020 15:27, Jan Beulich wrote:
> On 22.03.2020 17:14, julien@xxxxxxx wrote:
> > @@ -785,21 +781,21 @@ bool is_iomem_page(mfn_t mfn)
> >       return (page_get_owner(page) == dom_io);
> >   }
> >   
> > -static int update_xen_mappings(unsigned long mfn, unsigned int cacheattr)
> > +static int update_xen_mappings(mfn_t mfn, unsigned int cacheattr)
> >   {
> >       int err = 0;
> > -    bool alias = mfn >= PFN_DOWN(xen_phys_start) &&
> > -         mfn < PFN_UP(xen_phys_start + xen_virt_end - XEN_VIRT_START);
> > +    bool alias = mfn_x(mfn) >= PFN_DOWN(xen_phys_start) &&
> > +         mfn_x(mfn) < PFN_UP(xen_phys_start + xen_virt_end - XEN_VIRT_START);
> >       unsigned long xen_va =
> > -        XEN_VIRT_START + ((mfn - PFN_DOWN(xen_phys_start)) << PAGE_SHIFT);
> > +        XEN_VIRT_START + mfn_to_maddr(mfn_add(mfn, -PFN_DOWN(xen_phys_start)));
> Depending on the types involved (e.g. in PFN_DOWN()) this may
> or may not be safe, so I consider such a transformation at
> least fragile. I think we either want to gain mfn_sub() or
> keep this as a "real" subtraction.
I want to avoid mfn_x() as much as possible when everything can be done 
using typesafe operation. But i am not sure how mfn_sub() would solve 
the problem. Do you mind providing more information?
> > @@ -584,21 +584,21 @@ static unsigned long init_node_heap(int node, unsigned 
> > long mfn,
> >           needed = 0;
> >       }
> >       else if ( *use_tail && nr >= needed &&
> > -              arch_mfn_in_directmap(mfn + nr) &&
> > +              arch_mfn_in_directmap(mfn_x(mfn_add(mfn, nr))) &&
> >                 (!xenheap_bits ||
> > -               !((mfn + nr - 1) >> (xenheap_bits - PAGE_SHIFT))) )
> > +               !((mfn_x(mfn) + nr - 1) >> (xenheap_bits - PAGE_SHIFT))) )
> May I suggest consistency here: This one uses +, while ...
>
> >       {
> > -        _heap[node] = mfn_to_virt(mfn + nr - needed);
> > -        avail[node] = mfn_to_virt(mfn + nr - 1) +
> > +        _heap[node] = mfn_to_virt(mfn_add(mfn, nr - needed));
> > +        avail[node] = mfn_to_virt(mfn_add(mfn, nr - 1)) +
> >                         PAGE_SIZE - sizeof(**avail) * NR_ZONES;
> >       }
> >       else if ( nr >= needed &&
> > -              arch_mfn_in_directmap(mfn + needed) &&
> > +              arch_mfn_in_directmap(mfn_x(mfn_add(mfn, needed))) &&
> ... this one uses mfn_add() despite the mfn_x() around it, and ...
So the reason I used mfn_x(mfn_add(mfn, needed)) here is I plan to 
convert arch_mfn_in_directmap() to use typesafe soon. In the two others 
cases...
> >                 (!xenheap_bits ||
> > -               !((mfn + needed - 1) >> (xenheap_bits - PAGE_SHIFT))) )
> > +               !((mfn_x(mfn) + needed - 1) >> (xenheap_bits - PAGE_SHIFT))) )
> ... here you use + again. My personal preference would be to avoid
> constructs like mfn_x(mfn_add()).
... I am still unsure how to avoid mfn_x(). Do you have any ideas?
> > @@ -269,10 +270,10 @@ out_dealloc:
> >               continue;
> >           for ( i = 0; i < pages; i++ )
> >           {
> > -            uint32_t mfn = t_info_mfn_list[offset + i];
> > -            if ( !mfn )
> > +            mfn_t mfn = _mfn(t_info_mfn_list[offset + i]);
> > +            if ( mfn_eq(mfn, _mfn(0)) )
> Please could you take the opportunity and add the missing blank line
> between these two?
Sure.

> > --- a/xen/include/asm-x86/mm.h
> > +++ b/xen/include/asm-x86/mm.h
> > @@ -667,7 +667,7 @@ static inline bool arch_mfn_in_directmap(unsigned long mfn)
> >   {
> >       unsigned long eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END);
> >   
> > -    return mfn <= (virt_to_mfn(eva - 1) + 1);
> > +    return mfn <= mfn_x(mfn_add(virt_to_mfn(eva - 1),  1));
> Even if you wanted to stick to using mfn_add() here, there's one
> blank too many after the comma.
I will remove the extra blank. Regarding the construction, I have been 
wondering for a couple of years now whether we should introduce mfn_{lt, 
gt}. What do you think?

> With these taken care of
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
Thank you for the review.

Cheers,

--
Julien Grall