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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Jan,

On 02/03/18 15:34, Jan Beulich wrote:
> > > > On 21.02.18 at 15:02, <julien.grall@xxxxxxx> wrote:
> > @@ -95,11 +101,18 @@ 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); \
> > +    })
> Hmm, not really nice, but what do you do.
I am open to better suggestion. I wanted to avoid the conversion all 
over the code.
Also, do you have an opinion on Wei's suggestion:

"What I meant was to make copy_{to,from}_guest* type-safe. I just feel it
a bit strange you only created a wrapper for this file. I wonder why.

Note I'm just asking question. That's not necessarily a good idea to
turn them all in the end."

> >   static void increase_reservation(struct memop_args *a)
> >   {
> >       struct page_info *page;
> >       unsigned long i;
> > -    xen_pfn_t mfn;
> > +    mfn_t mfn;
> Please move this declaration ...
>
> > @@ -133,7 +146,7 @@ static void increase_reservation(struct memop_args *a)
> >                !guest_handle_is_null(a->extent_list) )
> >           {
> >               mfn = page_to_mfn(page);
> ... here, making the assignment its initializer. Or even avoid the
> local variable altogether, as the macro has already got one. Same
> elsewhere (whichever of the two variants fits), albeit maybe in the
> other cases the scope can't be shrunk much.
I will have a look.

Cheers,

--
Julien Grall

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