Re: [PATCH] xen/common: Do not allocate magic pages 1:1 for direct mapped domains

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.02.2024 14:35, Henry Wang wrote:
> Hi Jan,
> 
> On 2/27/2024 9:27 PM, Jan Beulich wrote:
>> On 27.02.2024 14:24, Henry Wang wrote:
>>> On 2/26/2024 4:25 PM, Jan Beulich wrote:
>>>> On 26.02.2024 02:19, Henry Wang wrote:
>>>>> --- a/xen/common/memory.c
>>>>> +++ b/xen/common/memory.c
>>>>> @@ -219,7 +219,7 @@ static void populate_physmap(struct memop_args *a)
>>>>>            }
>>>>>            else
>>>>>            {
>>>>> -            if ( is_domain_direct_mapped(d) )
>>>>> +            if ( is_domain_direct_mapped(d) && !is_magic_gpfn(gpfn) )
>>>>>                {
>>>>>                    mfn = _mfn(gpfn);
>>>>>    
>>>> I wonder whether is_domain_direct_mapped() shouldn't either be cloned
>>>> into e.g. is_gfn_direct_mapped(d, gfn), or be adjusted in-place to gain
>>>> such a (then optional) 2nd parameter. (Of course there again shouldn't be
>>>> a need for every domain to define a stub is_domain_direct_mapped() - if
>>>> not defined by an arch header, the stub can be supplied in a single
>>>> central place.)
>>> Same here, it looks like you prefer the centralized
>>> is_domain_direct_mapped() now, as we are having more archs. I can do the
>>> clean-up when sending v2. Just out of curiosity, do you think it is a
>>> good practice to place the is_domain_direct_mapped() implementation in
>>> xen/domain.h with proper arch #ifdefs?
>> arch #ifdefs? I'd like to avoid such, if at all possible. Generic fallbacks
>> generally ought to key their conditionals to the very identifier not
>> (already) being defined.
> 
> I meant something like this (as I saw CDF_directmap is currently gated 
> in this way in xen/domain.h):
> 
> #ifdef CONFIG_ARM
> #define is_domain_direct_mapped(d) ((d)->cdf & CDF_directmap)
> #else
> #define is_domain_direct_mapped(d) ((void)(d), 0)
> #endif
> 
> I am having trouble to think of another way to keep the function in a 
> centralized place while
> avoiding the #ifdefs. Would you mind elaborating a bit? Thanks!

What is already there is fine to change. I took your earlier reply to
mean that you want to add an "#ifndef CONFIG_ARM" to put in place some
new fallback handler.

Of course the above could also be done without any direct CONFIG_ARM
dependency. For example, CDF_directmap could simply evaluate to zero
when direct mapped memory isn't supported.

Jan