Re: [PATCH v5 4/7] xen: introduce mfn_init macro

[Stefano Stabellini <sstabellini@kernel.org>]

On Wed, 5 Dec 2018, Jan Beulich wrote:
> >>> On 04.12.18 at 20:38, <sstabellini@kernel.org> wrote:
> > On Tue, 4 Dec 2018, Jan Beulich wrote:
> >> >>> On 03.12.18 at 22:03, <sstabellini@kernel.org> wrote:
> >> > To be used in constant initializations of mfn_t variables, such as:
> >> > 
> >> > static mfn_t node = mfn_init(MM_ADDR);
> >> > 
> >> > It is necessary because static inline functions cannot be used as static
> >> > initializers.
> >> 
> >> We had been at this point once (quite some time ago), and got
> >> away without such an addition. Did you try to find that old
> >> discussion? Are there any new reasons to have such a construct?
> >> Do you need this for other than setting a value to INVALID_MFN,
> >> in which case INVALID_MFN_INITIALIZER ought to be suitable?
> >> 
> >> This is not to say I'm entirely opposed.
> >> 
> >> If we were to have such a construct, I wonder though whether
> >> mfn_init() is suitable as a name. Simply MFN() perhaps, and then
> >> also consistently have GFN() and DFN()?
> > 
> > Hi Jan,
> > 
> > I am happy with any name, and MFN() together with GFN() and DFN() look
> > like a good option.
> > 
> > The reason why it is needed is that without it I cannot introduce a
> > statically initialized array of mfn_t type like the one in the following
> > patch in the series:
> > 
> > +static const struct pm_access pm_node_access[] = {
> > +    /* MM_RPU grants access to all RPU Nodes.  */
> > +    [NODE_RPU] = { mfn_init(MM_RPU) },
> > +    [NODE_RPU_0] = { mfn_init(MM_RPU) },
> > +    [NODE_RPU_1] = { mfn_init(MM_RPU) },
> > +    [NODE_IPI_RPU_0] = { mfn_init(MM_RPU) },
> > 
> > [...]
> > 
> > Where MM_RPU is a mfn, and the NODE_* are IDs defined as enum:
> > 
> > #define MM_RPU  0xff9a0
> > 
> > enum pm_node_id {
> > 	NODE_RPU = 6,
> > 	NODE_RPU_0,
> > 	NODE_RPU_1,
> > 
> > [...]
> > 
> > 
> > Originally I had:
> > 
> >   [NODE_RPU] = { MM_RPU },
> > 
> > but I changed the type to be mfn_t to address one of Julien's comments.
> > You might get a better idea of the issue if you give a look at this
> > branch:
> > 
> > http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git zynqmp-v5
> 
> Well, I have to admit that I'd rather not see ways to embed hard-coded
> MFNs into code made available generically. May I suggest that you use
> a macro with a name to your liking just locally to that one source file?

OK, no problem


> As a side note, I'm also puzzled by there being entries in the table which
> don't have their MFNs specified. Oddly enough it looks as if
> .hwdom_access was true if and only if no MFN is specified.

Yes, the hwdom_access check could be turned into a check for
MFN(INVALID_MFN). I'll do that it will actually make the array size
smaller.


> The term "node" of course is confusing too, considering its NUMA
> meaning elsewhere in the hypervisor.
 
That comes from the EEMI firmware specification: they use the term
"node" to address a power domain "unit".

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