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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

>                (!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()).

> @@ -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?

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

With these taken care of
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan