Re: [PATCH v4 1/3] xen/arm: Move some of the functions to common file

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

On 07/04/2025 16:34, Luca Fancellu wrote:
> 
> 
> Hi Ayan,
> 
> 
>> On 7 Apr 2025, at 15:29, Ayan Kumar Halder <ayankuma@xxxxxxx> wrote:
>>
>> Hi Luca,
>>
>> On 04/04/2025 10:06, Luca Fancellu wrote:
>>> Hi Ayan,
>>>
>>>> On 3 Apr 2025, at 18:12, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx> 
>>>> wrote:
>>>>
>>>> Added a new file prepare_xen_region.inc to hold the common earlyboot MPU 
>>>> regions
>>>> configurations across arm64 and arm32.
>>>>
>>>> prepare_xen_region, fail_insufficient_regions() will be used by both arm32 
>>>> and
>>>> arm64. Thus, they have been moved to prepare_xen_region.inc.
>>>>
>>>> enable_secondary_cpu_mm() is a stub which is moved to 
>>>> prepare_xen_region.inc as
>>>> SMP is currently not supported for MPU.
>>>>
>>>> *_PRBAR are moved to arm64/sysregs.h.
>>>> *_PRLAR are moved to prepare_xen_region.inc as they are common between 
>>>> arm32
>>>> and arm64.
>>>>
>>>> Introduce WRITE_SYSREG_ASM to write to the system registers from the 
>>>> common asm
>>>> file.
>>>>
>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
>>>> ---
>>> The split for the common code seems ok to me, but this patch is introducing 
>>> an issue for the arm64 compilation,
>> Sorry, I moved something at the last moment without testing. :(
>>> I’ve done an experiment and with these changes I’m able to compile both, 
>>> but feel free to ignore if it’s no what you
>>> had in mind.
>> The change looks good. However, ...
>>>
>>> diff --git a/xen/arch/arm/include/asm/arm32/sysregs.h 
>>> b/xen/arch/arm/include/asm/arm32/sysregs.h
>>> index 22871999afb3..a90d1610a155 100644
>>> --- a/xen/arch/arm/include/asm/arm32/sysregs.h
>>> +++ b/xen/arch/arm/include/asm/arm32/sysregs.h
>>> @@ -20,6 +20,13 @@
>>>   * uses r0 as a placeholder register. */
>>>  #define CMD_CP32(name...)      "mcr " __stringify(CP32(r0, name)) ";"
>>>  +#define REGION_TEXT_PRBAR       0x18    /* SH=11 AP=10 XN=0 */
>>> +#define REGION_RO_PRBAR         0x1D    /* SH=11 AP=10 XN=1 */
>>> +#define REGION_DATA_PRBAR       0x19    /* SH=11 AP=00 XN=1 */
>>> +#define REGION_DEVICE_PRBAR     0x11    /* SH=10 AP=00 XN=1 */
>>> +
>>> +#define WRITE_SYSREG_ASM(v, name) mcr CP32(v, name)
>>> +
>>>  #ifndef __ASSEMBLY__
>>>    /* C wrappers */
>>> diff --git a/xen/arch/arm/include/asm/cpregs.h 
>>> b/xen/arch/arm/include/asm/cpregs.h
>>> index 6019a2cbdd89..b909adc102a5 100644
>>> --- a/xen/arch/arm/include/asm/cpregs.h
>>> +++ b/xen/arch/arm/include/asm/cpregs.h
>>> @@ -1,10 +1,6 @@
>>>  #ifndef __ASM_ARM_CPREGS_H
>>>  #define __ASM_ARM_CPREGS_H
>>>  -#ifdef CONFIG_MPU
>>> -#include <asm/mpu/cpregs.h>
>>> -#endif
>>> -
>>>  /*
>>>   * AArch32 Co-processor registers.
>>>   *
>>> @@ -502,6 +498,12 @@
>>>  #define MVFR0_EL1               MVFR0
>>>  #define MVFR1_EL1               MVFR1
>>>  #define MVFR2_EL1               MVFR2
>>> +#define HMPUIR                  p15,4,c0,c0,4
>>> +#define HPRSELR                 p15,4,c6,c2,1
>>> +#define PRBAR_EL2               p15,4,c6,c3,0
>>> +#define PRLAR_EL2               p15,4,c6,c8,1
>>> +#define MPUIR_EL2               HMPUIR
>>> +#define PRSELR_EL2              HPRSELR
>>
>> Considering that there will be lots of arm32 MPU specific registers, do you 
>> want to move them to mpu/cpregs.h ?
>>
>> That would be my style preference.
> 
> I don’t have any strong opinion so it’s fine if you want them in cpregs.h.
If there are really a lot of new additions and the movement improves
readability, I'm ok with them being stored in mpu/cpregs.h

~Michal