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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Jan,

On 15/03/18 08:06, Jan Beulich wrote:
> > > > 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?
I thought the handle was different type at each call site. I was wrong, 
so turned it to static inline.
> 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).
I am quite reluctant to turn this function as nop for Arm. This is 
common code and should not assume the implementation of 
paging_mode_translate. Furthermore, I can't see the real benefits as the 
compiler will optimize out it.
> > @@ -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?
I didn't do it because the line was getting too long and quite difficult 
to read if you split it.
Also, technically the compiler will be clever enough to optimize the 
code. So I don't see the benefits of removing the variable.
Cheers,

--
Julien Grall

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