Re: [PATCH v1 2/3] xen/dom0less: refactor architecture-specific DomU construction

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 14/05/2025 13:18, Julien Grall wrote:
> 
> 
> On 14/05/2025 11:51, Orzel, Michal wrote:
>>
>>
>> On 14/05/2025 12:06, Julien Grall wrote:
>>> Hi,
>>>
>>> On 14/05/2025 10:57, Oleksii Kurochko wrote:
>>>>
>>>> On 5/14/25 9:25 AM, Julien Grall wrote:
>>>>> Hi Oleksii,
>>>>>
>>>>> On 13/05/2025 15:29, Oleksii Kurochko wrote:
>>>>>> Refactor construct_domU() to improve architecture separation and reduce
>>>>>> reliance on ARM-specific logic in common code:
>>>>>> - Drop set_domain_type() from generic code. This function is specific
>>>>>>     to ARM and serves no purpose on other architectures like RISC-V,
>>>>>>     which lack the arch.type field in kernel_info.
>>>>>
>>>>> So you will only ever boot one type of domain on RISC-V? IOW, no 32-bit
>>>>> or else?
>>>>
>>>> The bitness of the guest and host should match. So, an RV32 host should run
>>>> RV32 guests, and an RV64 host should run RV64 guests and so on.
>>>> (I'm not really sure that on RISC-V it is possible to run with RV64 host
>>>> an RV32 guest, but need to double-check)
>>>>
>>>> Therefore, my plan for RISC-V is to have the following:
>>>>     #ifdef CONFIG_RISCV_64
>>>>     #define is_32bit_domain(d) (0)
>>>>     #define is_64bit_domain(d) (1)
>>>>     #else
>>>>     #define is_32bit_domain(d) (1)
>>>>     #define is_64bit_domain(d) (0)
>>>>     #endif
>>>>
>>>> With this definition, there's no need to use|(d)->arch.type| for RISC-V.
>>>>
>>>>>
>>>>>> - Introduce arch_construct_domU() to encapsulate architecture-specific
>>>>>>     DomU construction steps.
>>>>>> - Implement arch_construct_domU() for ARM. This includes:
>>>>>>     - Setting the domain type for CONFIG_ARM64.
>>>>>>     - Handling static memory allocation if xen,static-mem is present in
>>>>>>       the device tree.
>>>>>>     - Processing static shared memory.
>>>>>> - Move call of make_resv_memory_node() to Arm's make_arch_nodes() as
>>>>>>     this call is specific to CONFIG_STATIC_SHM which is ARM specific,
>>>>>>     at least, now.
>>>>>
>>>>> This looks shortsighted. If there is a plan to use CONFIG_STATIC_SHM
>>>>> on RISC-V (I don't see why not today), then
>>>>> I think the code should stick in common/ even if it is not fully usable
>>>>> yet (that's the whole point of have CONFIG_* options).
>>>>
>>>> We don't use this feature in the downstream repo, but I can imagine some
>>>> cases where it could be useful. So, for now, its
>>>> use is purely theoretical and it is a reason why I agreed with Michal
>>>> and returned back these changes to Arm.
>>>
>>> I strongly disagree with this statement. If a feature is not
>>> architecture specific (like static shared memory), then the code ought
>>> to be in common code so it can be re-used by others.
>> But the code is not common. There are still 900 lines in arch spec dir.
> 
> Sure. But my point is rather more that static memory is not a feature 
> described by the Arm Arm. Instead, it is a feature of Xen that is ti to 
> device-tree. So inherently there is no reason to be implemented in arch/arm.
I agree, it can and should be made common. But exposing only callers makes no
sense at all for me. Callers should be exposed together with feature code 
movement.

> 
>>>
>>> Also, AFAIK, the bulk of the static shared memory code is in common. So
>>> it would be rather easy to add support for RISC-V if we wanted to.
>>>
>>> Given the code is already in common, it is rather silly to move the code
>> IMO it should not be made common in the first place. I don't like exposing
>> callers with the big chunk of code sitting in the arch specific directory.
> 
> So the concern is we didn't go far enough rather than the feature should 
> not be implemented in common for other archs, correct?
Yes. Oleksii exposed only callers. His intention was not to make static feature
common.

> 
>>
>>> back to Arm for then moving back to common potentially in a few weeks time.
>> What about static memory code? With the recent Oleksii code movement, the 
>> inline
>> helpers like allocate_static_memory() became unreachable when the feature is
>> disabled and the main purpose for helpers was to avoid ifdefery.
> 
> Sorry I am not sure which part you are referring to.
With the current code, allocate_static_memory(), assign_static_memory_11()
callers (that were moved to common) are protected with #ifdef. This causes the
helpers defined in case CONFIG_STATIC_MEMORY is not defined to be unreachable
(see static-memory.h).

~Michal