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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.12.18 at 20:38, <sstabellini@xxxxxxxxxx> wrote:
> On Tue, 4 Dec 2018, Jan Beulich wrote:
>> >>> On 03.12.18 at 22:03, <sstabellini@xxxxxxxxxx> 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?
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. The term
"node" of course is confusing too, considering its NUMA meaning
elsewhere in the hypervisor.

Jan



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