Re: [Xen-devel] [PATCH 2/8] xen: Use typesafe gfn/mfn in guest_physmap_* helpers

[Julien Grall <julien.grall@xxxxxxx>]

Hi Jan,

On 14/06/16 13:56, Jan Beulich wrote:
> > > > On 14.06.16 at 14:07, <julien.grall@xxxxxxx> wrote:
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -665,21 +665,21 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long 
> > gfn, unsigned long mfn,
> >   }
> >
> >   int
> > -guest_physmap_remove_page(struct domain *d, unsigned long gfn,
> > -                          unsigned long mfn, unsigned int page_order)
> > +guest_physmap_remove_page(struct domain *d, gfn_t gfn,
> > +                          mfn_t mfn, unsigned int page_order)
> >   {
> >       struct p2m_domain *p2m = p2m_get_hostp2m(d);
> >       int rc;
> >       gfn_lock(p2m, gfn, page_order);
> > -    rc = p2m_remove_page(p2m, gfn, mfn, page_order);
> > +    rc = p2m_remove_page(p2m, gfn_x(gfn), mfn_x(mfn), page_order);
> >       gfn_unlock(p2m, gfn, page_order);
> >       return rc;
> >   }
> >
> > -int
> > -guest_physmap_add_entry(struct domain *d, unsigned long gfn,
> > -                        unsigned long mfn, unsigned int page_order,
> > -                        p2m_type_t t)
> > +static int
> > +__guest_physmap_add_entry(struct domain *d, unsigned long gfn,
> At the very least only a single underscore please.
>
> > +                          unsigned long mfn, unsigned int page_order,
> > +                          p2m_type_t t)
> >   {
> >       struct p2m_domain *p2m = p2m_get_hostp2m(d);
> >       unsigned long i, ogfn;
> > @@ -838,6 +838,13 @@ out:
> >       return rc;
> >   }
> >
> > +/* XXX: To be removed when __guest_physmap_add_entry will use typesafe */
> But not just because of this (misleading) comment I really wonder
> what the point here is: Is it really that much more intrusive to
> change the function right away instead of introducing a wrapper?
I though it was intrusive because there are multiple place using adding 
a value to the mfn/gfn.
It might be less intrusive with your suggestion to add mfn_* wrappers 
for common operation. I will give another look.
> (The comment is misleading because __guest_physmap_add_entry
> shouldn't survive after the conversion to proper types, i.e. it is
> not the function below which is supposed to get removed.)
>
> > +int
> > +guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
> > +                        unsigned int page_order, p2m_type_t t)
> > +{
> > +    return __guest_physmap_add_entry(d, gfn_x(gfn), mfn_x(mfn), page_order, t);
> > +}
> Perhaps a better (wrapper-less) approach (if full conversion is indeed
> beyond scope) would be to simply rename the function parameters
> and have local variables named "gfn" and "mfn"?
>
> > @@ -2785,7 +2792,8 @@ int p2m_add_foreign(struct domain *tdom, unsigned long 
> > fgfn,
> >                       unsigned long gpfn, domid_t foreigndom)
> >   {
> >       p2m_type_t p2mt, p2mt_prev;
> > -    unsigned long prev_mfn, mfn;
> > +    mfn_t prev_mfn;
> > +    unsigned long mfn;
> This looks to make things more inconsistent rather than more consistent.
The usage of the variable in p2m_add_foreign seems limited to a couple 
of place. I will use mfn_t for it too.
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel