Re: [Xen-devel] [PATCH v5] x86/altp2m: Aggregate get entry and populate into common funcs

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Tue, Apr 16, 2019 at 7:58 AM George Dunlap <george.dunlap@xxxxxxxxxx> wrote:
>
> On 4/16/19 2:51 PM, Andrew Cooper wrote:
> > On 16/04/2019 14:44, Tamas K Lengyel wrote:
> >>
> >>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> >>> index 2801a8ccca..8dc4353645 100644
> >>> --- a/xen/include/asm-x86/p2m.h
> >>> +++ b/xen/include/asm-x86/p2m.h
> >>> @@ -514,6 +514,23 @@ static inline unsigned long mfn_to_gfn(struct domain 
> >>> *d, mfn_t mfn)
> >>>          return mfn_x(mfn);
> >>>  }
> >>>
> >>> +int altp2m_get_entry(struct p2m_domain *ap2m, gfn_t gfn, mfn_t *mfn,
> >>> +                     p2m_type_t *t, p2m_access_t *a, bool prepopulate);
> >>> +
> >>> +static inline int altp2m_get_entry_direct(struct p2m_domain *ap2m,
> >>> +                                          gfn_t gfn, mfn_t *mfn,
> >>> +                                          p2m_type_t *t, p2m_access_t *a)
> >>> +{
> >>> +    return altp2m_get_entry(ap2m, gfn, mfn, t, a, false);
> >>> +}
> >>> +
> >>> +static inline int altp2m_get_entry_prepopulate(struct p2m_domain *ap2m,
> >>> +                                               gfn_t gfn, mfn_t *mfn,
> >>> +                                               p2m_type_t *t, 
> >>> p2m_access_t *a)
> >>> +{
> >>> +    return altp2m_get_entry(ap2m, gfn, mfn, t, a, true);
> >>> +}
> >> Are these wrappers really required? I don't think they add anything to
> >> readability, it's just yet another layer that does almost nothing.
> >
> > From a readability point of view, boolean parameters are about as opaque
> > as they come.  The same goes for all other scalar parameters which don't
> > have a mnemonic.
> >
> > For someone who is not an expert of the intricacies of the subsystem,
> > having the options spelt out in wrappers like this is extremely helpful
> > to reduce the cognitive load of trying to follow the code.
>
> This is exactly why I did it this way.
>
> The other pattern I think is tolerable is having a #define to use as an
> argument; for example:
>
> #define AP2MGET_prepopulate true
> #define AP2MGET_peek        false
>
> Then you get:
>
>   mfn = altp2m_get_entry(..., AP2MGET_prepopulate);
>
> But then you run into namespacing issues with the prefixes.
>
> I wouldn't object to doing it the above way, but I think the wrappers is
> probably simpler and clearer for this case.  I do object to passing in
> naked booleans.

Fair enough. I personally prefer the #define route though, it just
avoids creating lasagna code. With wrappers by the time I dig through
them trying to figure out where they actually land I tend to forget
why I was looking for it in the first place. Perhaps that's just me..

Tamas

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