On 07/04/2025 16:34, Luca Fancellu wrote:
>
>
> Hi Ayan,
>
>
>> On 7 Apr 2025, at 15:29, Ayan Kumar Halder <ayank...@amd.com> 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.hal...@amd.com>
>>>> 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.hal...@amd.com>
>>>> ---
>>> 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
