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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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.

Cheers,
Luca