Re: [PATCH v3 2/5] xen/arm: Move some of the functions to common file

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

On 30/03/2025 23:06, Julien Grall wrote:
> 
> 
> Hi Ayan,
> 
> On 30/03/2025 19:03, Ayan Kumar Halder wrote:
>> Added a new file prepare_xen_region.inc to hold the common earlyboot MPU 
>> regions
>> configurations across arm64 and arm32.
> 
> While I understand the desire to consolidate the code, I am quite
> unconvinced this should be done for assembly code. A few examples below why.
> 
> I would be interested to hear the view of the other Arm maintainers.
I think consolidating the code is ok as long as we don't go to extremes.
Consolidation should be done for things only where it makes really sense
(macros, helpers). For example, in places where we don't predict that given
implementation might be different due to certain arch optimizations, etc. (like
you mentioned for mov_w). If we worry that it'll be difficult to asses what to
make common, I might suggest to consolidate only common macros.

In case of this patch, I think we could have a single implementation for
prepare_xen_region and fail_insufficient_regions, whereas enable_boot_cpu should
be implementation specific. I also think that a common .inc file is a good place
for storing stubs such as enable_secondary_cpu_mm(). One day, if let's say arm64
decide to add support for SMP, it will add it's own implementation, leaving the
stub only for arm32.

> 
>>
>> prepare_xen_region, enable_boot_cpu, fail_insufficient_regions() will be 
>> used by
>> both arm32 and arm64. Thus, they have been moved to prepare_xen_region.inc.
>>
>> REGION_* are moved to arm64/sysregs.h. Introduced LOAD_SYSREG and 
>> STORE_SYSREG
That's not a good name as it makes me think it's ldr/str.

>> to read/write to the system registers from the common asm file. One could not
>> reuse READ_SYSREG and WRITE_SYSREG as they have been defined to be invoked 
>> from
>> C files.
Therefore {READ/WRTIE}_SYSREG_ASM() could be used to denote ASM only usage.

~Michal