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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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