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

From: Tamas K Lengyel <tamas@tklengyel.com>
To: George Dunlap <george.dunlap@citrix.com>
Cc: "wei.liu2@citrix.com" <wei.liu2@citrix.com>,
	"rcojocaru@bitdefender.com" <rcojocaru@bitdefender.com>,
	"george.dunlap@eu.citrix.com" <george.dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	"jbeulich@suse.com" <jbeulich@suse.com>,
	Alexandru Stefan ISAILA <aisaila@bitdefender.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"roger.pau@citrix.com" <roger.pau@citrix.com>
Subject: Re: [PATCH v5] x86/altp2m: Aggregate get entry and populate into common funcs
Date: Tue, 16 Apr 2019 08:25:52 -0600	[thread overview]
Message-ID: <CABfawhniLgxxid8Kw7mhmoN9m4zSxdHd10CK+HvvtTDQyFdBVQ@mail.gmail.com> (raw)
In-Reply-To: <92ff12b3-259f-d0d2-8ec7-e1554f5e4190@citrix.com>

On Tue, Apr 16, 2019 at 7:58 AM George Dunlap <george.dunlap@citrix.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel