Re: [PATCH v2 07/11] xen: move domain_use_host_layout() to common code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.03.2026 18:32, Oleksii Kurochko wrote:
> On 3/31/26 5:53 PM, Jan Beulich wrote:
>> On 31.03.2026 17:20, Oleksii Kurochko wrote:
>>> On 3/30/26 5:13 PM, Jan Beulich wrote:
>>>> On 23.03.2026 17:29, Oleksii Kurochko wrote:
>>>>> domain_use_host_layout() is not really architecture-specific, so move it
>>>>> from the Arm header to the common header xen/domain.h and provide a common
>>>>> implementation in xen/common/domain.c. domain_use_host_layout() 
>>>>> potentially
>>>>> is needed for x86 [1].
>>>>
>>>> No matter that this may indeed be true, ...
>>>>
>>>>> Turn the macro into a function to avoid header dependency issues.
>>>>
>>>> ... this introduces unreachable code on x86, i.e. a Misra rule 2.1 
>>>> violation.
>>>
>>> Do we have some deviation tag for such cases when the code temporary
>>> isn't used?
>>
>> I'm sorry, but it'll take me about as long as you to find out.
> 
> Sure, I will take a look. I just thought that maybe you have a solution 
> already just in your head.

Well, I do: Don't make this an out-of-line function.

>   I wonder
>> about "temporary" though: Do you have a clear understanding as to when
>> that will change?
> 
> No, I don't. As Stefano mentioned they will need this function one day. 
> Another option we could use ifndef x86 or ifdef DOM0_LESS and then when 
> someone will really need it on x86, this ifdef will be dropped. I don't 
> know if it is better solution.
> 
> It seems like the best one solution will still make a try to make 
> declare this function as macro.

Or an inline function. There's nothing ...

>>>>> @@ -2544,6 +2544,12 @@ void thaw_domains(void)
>>>>>    
>>>>>    #endif /* CONFIG_SYSTEM_SUSPEND */
>>>>>    
>>>>> +bool domain_use_host_layout(struct domain *d)
>>>>> +{
>>>>> +    return is_domain_direct_mapped(d) ||
>>>>> +           (paging_mode_translate(d) && is_hardware_domain(d));
>>>>> +}
>>>>
>>>> The placement of paging_mode_translate() doesn't match ...
>>>>
>>>>> --- a/xen/include/xen/domain.h
>>>>> +++ b/xen/include/xen/domain.h
>>>>> @@ -62,6 +62,22 @@ void domid_free(domid_t domid);
>>>>>    #define is_domain_direct_mapped(d) ((d)->cdf & CDF_directmap)
>>>>>    #define is_domain_using_staticmem(d) ((d)->cdf & CDF_staticmem)
>>>>>    
>>>>> +/*
>>>>> + * Is the auto-translated domain using the host memory layout?
>>>>> + *
>>>>> + * domain_use_host_layout() is always False for PV guests.
>>>>
>>>> ... the description of the function.
>>>
>>> But why the placement should be different?
>>
>> If you focus on auto-translated, then imo paging_mode_translate()
>> better would guard everything.
> 
> Then it make sense to do in the following way:
>   bool domain_use_host_layout(struct domain *d)
>   {
> -    return is_domain_direct_mapped(d) ||
> -           (paging_mode_translate(d) && is_hardware_domain(d));
> +    return paging_mode_translate(d) &&
> +           (is_domain_direct_mapped(d) || is_hardware_domain(d));
>   }

... in here which clearly speaks against doing so. And yes, this is what I
was asking for (with the function parameter also suitably constified).

>>> So if domain_use_host_layout() is fully depends on
>>> paging_mode_translate(d) && is_hardware_domain(d) and for which
>>> paging_mode_translate() is false if it is PV guest.
>>> Thereby domain_use_host_layout() is false too.
>>>
>>>>
>>>> Further, the first sentence above suggests the caller has to check
>>>> paging_mode_translate() before calling, which as per the implementation
>>>> clearly isn't the intention.
>>>
>>> Sorry, I don't follow you here.
>>
>> By starting the comment with "Is the auto-translated domain using", you
>> imply the caller checked for that aspect already. At least the way I
>> read it.
> 
> My understanding was that it is an explanation what function is checking.

For that you'd want to omit "auto-translated" from the first sentence, imo.

Jan