Re: [Xen-devel] [PATCH v5 12/16] xen/mm: Switch common/memory.c to use typesafe MFN

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.03.18 at 19:20, <julien.grall@xxxxxxx> wrote:
> @@ -95,11 +101,17 @@ static unsigned int max_order(const struct domain *d)
>      return min(order, MAX_ORDER + 0U);
>  }
>  
> +/* Helper to copy a typesafe MFN to guest */
> +#define copy_mfn_to_guest(hnd, off, mfn)            \
> +    ({                                              \
> +        xen_pfn_t mfn_ = mfn_x(mfn);                \
> +        __copy_to_guest_offset(hnd, off, &mfn_, 1); \
> +    })

However much I dislike introduction of new name space violations,
I think following the global naming principle here is more important:
As a function not validating the address range, this should have
two leading underscores. Also - was there a reason for this not to
be an inline function?

The other thing I notice only now is perhaps a little much too ask
for a mostly mechanical change like this one: All uses of this sit
inside !paging_mode_translate() checks, hence these could do
nothing on ARM and resolve to __copy_to_user() on x86 (with
the type checking suitably lifted to here).

> @@ -132,8 +144,9 @@ static void increase_reservation(struct memop_args *a)
>          if ( !paging_mode_translate(d) &&
>               !guest_handle_is_null(a->extent_list) )
>          {
> -            mfn = page_to_mfn(page);
> -            if ( unlikely(__copy_to_guest_offset(a->extent_list, i, &mfn, 
> 1)) )
> +            mfn_t mfn = page_to_mfn(page);
> +
> +            if ( unlikely(copy_mfn_to_guest(a->extent_list, i, mfn)) )

Can't you avoid the intermediate variable altogether now?

Jan


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