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
