[PATCH v4 0/7] First chunk for Arm R82 and MPU support

[PATCH v4 0/7] First chunk for Arm R82 and MPU support

[Luca Fancellu]

Hi all,

This is the first chunk of work to support MPU and R82 on Xen, this serie
reaches the early boot stages just before early_fdt_map(), just to give an idea
about which stage of the boot is reached.

v4:
 - dropped setup_mpu() patch and early_fdt_map() patch (needs rework)
 - add new patches: boot protocol and early asm MPU structure update
 - general fixes listed on each patch
v3 changes:
 - stated on each patch
v2 changes for this serie:
 - rebased serie on the MPU skeleton that allow compilation
 - removed some patches already merged in the MPU skeleton

Luca Fancellu (6):
  docs/arm: Document Xen booting protocol on Armv8-R
  arm/mpu: Provide and populate MPU C data structures
  arm/mpu: Provide access to the MPU region from the C code
  arm/mpu: Introduce utility functions for the pr_t type
  arm/mpu: Provide a constructor for pr_t type
  arm/mpu: Introduce MPU memory mapping flags

Penny Zheng (1):
  arm/mpu: Introduce MPU memory region map structure

 docs/misc/arm/booting.txt                |   8 +
 xen/arch/arm/arm64/mpu/head.S            |  13 ++
 xen/arch/arm/include/asm/arm32/mpu.h     |  25 +++
 xen/arch/arm/include/asm/arm64/mpu.h     |  61 +++++++
 xen/arch/arm/include/asm/bitmap-op.inc   |  67 ++++++++
 xen/arch/arm/include/asm/mpu.h           |  78 +++++++++
 xen/arch/arm/include/asm/mpu/mm.h        |  51 ++++++
 xen/arch/arm/include/asm/mpu/regions.inc |  71 ++++++--
 xen/arch/arm/include/asm/page.h          |  25 +++
 xen/arch/arm/mpu/mm.c                    | 201 +++++++++++++++++++++++
 10 files changed, 590 insertions(+), 10 deletions(-)
 create mode 100644 xen/arch/arm/include/asm/arm32/mpu.h
 create mode 100644 xen/arch/arm/include/asm/arm64/mpu.h
 create mode 100644 xen/arch/arm/include/asm/bitmap-op.inc

-- 
2.34.1

[PATCH v4 1/7] docs/arm: Document Xen booting protocol on Armv8-R

[Luca Fancellu]

Document the requirement needed to boot Xen on Armv8-R platforms.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v4 changes:
 - New patch
---
 docs/misc/arm/booting.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
index 21ae74837dcc..719af74f1e69 100644
--- a/docs/misc/arm/booting.txt
+++ b/docs/misc/arm/booting.txt
@@ -62,6 +62,14 @@ Xen relies on some settings the firmware has to configure in EL3 before starting
 
 * The bit SCR_EL3.HCE (resp. SCR.HCE for 32-bit ARM) must be set to 1.
 
+When Xen runs on Armv8-R, the highest exception level is EL2 and the only
+available state is secure (S) on Arm64 and non secure (NS) on Arm32, hence the
+above requirements need to be adjusted to this case:
+
+* Xen must be entered in S EL2 mode on Arm64 and in NS EL2 mode on Arm32.
+
+* Xen must be entered with MPU off and data cache disabled (SCTLR_EL2.M bit and
+  SCTLR_EL2.C set to 0).
 
 [1] linux/Documentation/arm/booting.rst
 Latest version: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arch/arm/booting.rst
-- 
2.34.1

Re: [PATCH v4 1/7] docs/arm: Document Xen booting protocol on Armv8-R

[Ayan Kumar Halder]

Hi Luca,

On 29/04/2025 16:20, Luca Fancellu wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> Document the requirement needed to boot Xen on Armv8-R platforms.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> v4 changes:
>   - New patch
> ---
>   docs/misc/arm/booting.txt | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
> index 21ae74837dcc..719af74f1e69 100644
> --- a/docs/misc/arm/booting.txt
> +++ b/docs/misc/arm/booting.txt
> @@ -62,6 +62,14 @@ Xen relies on some settings the firmware has to configure in EL3 before starting
>
>   * The bit SCR_EL3.HCE (resp. SCR.HCE for 32-bit ARM) must be set to 1.
>
> +When Xen runs on Armv8-R, the highest exception level is EL2 and the only
> +available state is secure (S) on Arm64 and non secure (NS) on Arm32, hence the
> +above requirements need to be adjusted to this case:
> +
> +* Xen must be entered in S EL2 mode on Arm64 and in NS EL2 mode on Arm32.
> +
> +* Xen must be entered with MPU off and data cache disabled (SCTLR_EL2.M bit and
> +  SCTLR_EL2.C set to 0).
>
>   [1] linux/Documentation/arm/booting.rst
>   Latest version: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arch/arm/booting.rst
> --
> 2.34.1
>
- Ayan

Re: [PATCH v4 1/7] docs/arm: Document Xen booting protocol on Armv8-R

[Orzel, Michal]

On 29/04/2025 17:20, Luca Fancellu wrote:
> Document the requirement needed to boot Xen on Armv8-R platforms.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v4 changes:
>  - New patch
> ---
>  docs/misc/arm/booting.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
> index 21ae74837dcc..719af74f1e69 100644
> --- a/docs/misc/arm/booting.txt
> +++ b/docs/misc/arm/booting.txt
> @@ -62,6 +62,14 @@ Xen relies on some settings the firmware has to configure in EL3 before starting
>  
>  * The bit SCR_EL3.HCE (resp. SCR.HCE for 32-bit ARM) must be set to 1.
>  
> +When Xen runs on Armv8-R, the highest exception level is EL2 and the only
> +available state is secure (S) on Arm64 and non secure (NS) on Arm32, hence the
> +above requirements need to be adjusted to this case:
> +
> +* Xen must be entered in S EL2 mode on Arm64 and in NS EL2 mode on Arm32.
> +
> +* Xen must be entered with MPU off and data cache disabled (SCTLR_EL2.M bit and
> +  SCTLR_EL2.C set to 0).
>  
>  [1] linux/Documentation/arm/booting.rst
>  Latest version: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arch/arm/booting.rst

Is the goal of this patch to only write firmware/bootloader requirements or
accommodate booting protocol in general? Asking because in this file there is a
requirement that Xen should be loaded below 10TB which afaict does not fit Armv8-R.

Other than that:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH v4 1/7] docs/arm: Document Xen booting protocol on Armv8-R

[Luca Fancellu]

Hi Michal,

> On 30 Apr 2025, at 07:37, Orzel, Michal <Michal.Orzel@amd.com> wrote:
> 
> 
> 
> On 29/04/2025 17:20, Luca Fancellu wrote:
>> Document the requirement needed to boot Xen on Armv8-R platforms.
>> 
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> ---
>> v4 changes:
>> - New patch
>> ---
>> docs/misc/arm/booting.txt | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>> 
>> diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
>> index 21ae74837dcc..719af74f1e69 100644
>> --- a/docs/misc/arm/booting.txt
>> +++ b/docs/misc/arm/booting.txt
>> @@ -62,6 +62,14 @@ Xen relies on some settings the firmware has to configure in EL3 before starting
>> 
>> * The bit SCR_EL3.HCE (resp. SCR.HCE for 32-bit ARM) must be set to 1.
>> 
>> +When Xen runs on Armv8-R, the highest exception level is EL2 and the only
>> +available state is secure (S) on Arm64 and non secure (NS) on Arm32, hence the
>> +above requirements need to be adjusted to this case:
>> +
>> +* Xen must be entered in S EL2 mode on Arm64 and in NS EL2 mode on Arm32.
>> +
>> +* Xen must be entered with MPU off and data cache disabled (SCTLR_EL2.M bit and
>> +  SCTLR_EL2.C set to 0).
>> 
>> [1] linux/Documentation/arm/booting.rst
>> Latest version: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arch/arm/booting.rst
> 
> Is the goal of this patch to only write firmware/bootloader requirements or
> accommodate booting protocol in general? Asking because in this file there is a
> requirement that Xen should be loaded below 10TB which afaict does not fit Armv8-R.

Yes you are right, but I think this still holds since we support 1TB on Armv8-R at the moment?

Cheers,
Luca

Re: [PATCH v4 1/7] docs/arm: Document Xen booting protocol on Armv8-R

[Orzel, Michal]

On 30/04/2025 08:58, Luca Fancellu wrote:
> Hi Michal,
> 
>> On 30 Apr 2025, at 07:37, Orzel, Michal <Michal.Orzel@amd.com> wrote:
>>
>>
>>
>> On 29/04/2025 17:20, Luca Fancellu wrote:
>>> Document the requirement needed to boot Xen on Armv8-R platforms.
>>>
>>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>>> ---
>>> v4 changes:
>>> - New patch
>>> ---
>>> docs/misc/arm/booting.txt | 8 ++++++++
>>> 1 file changed, 8 insertions(+)
>>>
>>> diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
>>> index 21ae74837dcc..719af74f1e69 100644
>>> --- a/docs/misc/arm/booting.txt
>>> +++ b/docs/misc/arm/booting.txt
>>> @@ -62,6 +62,14 @@ Xen relies on some settings the firmware has to configure in EL3 before starting
>>>
>>> * The bit SCR_EL3.HCE (resp. SCR.HCE for 32-bit ARM) must be set to 1.
>>>
>>> +When Xen runs on Armv8-R, the highest exception level is EL2 and the only
>>> +available state is secure (S) on Arm64 and non secure (NS) on Arm32, hence the
>>> +above requirements need to be adjusted to this case:
>>> +
>>> +* Xen must be entered in S EL2 mode on Arm64 and in NS EL2 mode on Arm32.
>>> +
>>> +* Xen must be entered with MPU off and data cache disabled (SCTLR_EL2.M bit and
>>> +  SCTLR_EL2.C set to 0).
>>>
>>> [1] linux/Documentation/arm/booting.rst
>>> Latest version: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arch/arm/booting.rst
>>
>> Is the goal of this patch to only write firmware/bootloader requirements or
>> accommodate booting protocol in general? Asking because in this file there is a
>> requirement that Xen should be loaded below 10TB which afaict does not fit Armv8-R.
> 
> Yes you are right, but I think this still holds since we support 1TB on Armv8-R at the moment?
No, this value denotes max RAM size you support and not where Xen must be
loaded. On ARMv8-A, we support max 2TB of RAM but require Xen to be loaded
anywhere up to 10TB. Do we have the same limitation on ARMv8-R?

~Michal

Re: [PATCH v4 1/7] docs/arm: Document Xen booting protocol on Armv8-R

[Luca Fancellu]

Hi Michal,

>>> 
>>> Is the goal of this patch to only write firmware/bootloader requirements or
>>> accommodate booting protocol in general? Asking because in this file there is a
>>> requirement that Xen should be loaded below 10TB which afaict does not fit Armv8-R.
>> 
>> Yes you are right, but I think this still holds since we support 1TB on Armv8-R at the moment?
> No, this value denotes max RAM size you support and not where Xen must be
> loaded. On ARMv8-A, we support max 2TB of RAM but require Xen to be loaded
> anywhere up to 10TB. Do we have the same limitation on ARMv8-R?

Uhm, right, I’m not aware of limitations

> 
> ~Michal
> 

Re: [PATCH v4 1/7] docs/arm: Document Xen booting protocol on Armv8-R

[Julien Grall]

On 29/04/2025 16:20, Luca Fancellu wrote:
> Document the requirement needed to boot Xen on Armv8-R platforms.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v4 changes:
>   - New patch
> ---
>   docs/misc/arm/booting.txt | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
> index 21ae74837dcc..719af74f1e69 100644
> --- a/docs/misc/arm/booting.txt
> +++ b/docs/misc/arm/booting.txt
> @@ -62,6 +62,14 @@ Xen relies on some settings the firmware has to configure in EL3 before starting
>   
>   * The bit SCR_EL3.HCE (resp. SCR.HCE for 32-bit ARM) must be set to 1.
>   
> +When Xen runs on Armv8-R, the highest exception level is EL2 and the only
> +available state is secure (S) on Arm64 and non secure (NS) on Arm32, hence the
> +above requirements need to be adjusted to this case:
 > +> +* Xen must be entered in S EL2 mode on Arm64 and in NS EL2 mode 
on Arm32.

I think it would be better to update the line "Xen must be entered in NS 
EL2 mode" to clarify the state for 64-bit Arm.

 > +> +* Xen must be entered with MPU off and data cache disabled 
(SCTLR_EL2.M bit and
> +  SCTLR_EL2.C set to 0).

This line is valid for Armv8-A/Armv7-A when using the Image/zImage protocol.

>   
>   [1] linux/Documentation/arm/booting.rst
>   Latest version: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arch/arm/booting.rst

-- 
Julien Grall

Re: [PATCH v4 1/7] docs/arm: Document Xen booting protocol on Armv8-R

[Luca Fancellu]

Hi Julien,

> On 6 May 2025, at 12:44, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 29/04/2025 16:20, Luca Fancellu wrote:
>> Document the requirement needed to boot Xen on Armv8-R platforms.
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> ---
>> v4 changes:
>>  - New patch
>> ---
>>  docs/misc/arm/booting.txt | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>> diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
>> index 21ae74837dcc..719af74f1e69 100644
>> --- a/docs/misc/arm/booting.txt
>> +++ b/docs/misc/arm/booting.txt
>> @@ -62,6 +62,14 @@ Xen relies on some settings the firmware has to configure in EL3 before starting
>>    * The bit SCR_EL3.HCE (resp. SCR.HCE for 32-bit ARM) must be set to 1.
>>  +When Xen runs on Armv8-R, the highest exception level is EL2 and the only
>> +available state is secure (S) on Arm64 and non secure (NS) on Arm32, hence the
>> +above requirements need to be adjusted to this case:
> > +> +* Xen must be entered in S EL2 mode on Arm64 and in NS EL2 mode on Arm32.
> 
> I think it would be better to update the line "Xen must be entered in NS EL2 mode" to clarify the state for 64-bit Arm.
> 
> > +> +* Xen must be entered with MPU off and data cache disabled (SCTLR_EL2.M bit and
>> +  SCTLR_EL2.C set to 0).
> 
> This line is valid for Armv8-A/Armv7-A when using the Image/zImage protocol.
> 
>>    [1] linux/Documentation/arm/booting.rst
>>  Latest version: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arch/arm/booting.rst

Just to be sure to be on the same page, are you suggesting these changes on the original file?

diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
index 21ae74837dcc..c00c651805d7 100644
--- a/docs/misc/arm/booting.txt
+++ b/docs/misc/arm/booting.txt
@@ -58,10 +58,14 @@ Firmware/bootloader requirements
 
 Xen relies on some settings the firmware has to configure in EL3 before starting Xen.
 
-* Xen must be entered in NS EL2 mode
+* Xen must be entered in:
+  * Non-Secure EL2 mode for Armv8-A Arm64 and Arm32, Armv8-R Arm32.
+  * Secure EL2 mode for Armv8-R Arm64.
 
 * The bit SCR_EL3.HCE (resp. SCR.HCE for 32-bit ARM) must be set to 1.
 
+* Xen must be entered with MMU/MPU off and data cache disabled (SCTLR_EL2.M bit
+  and SCTLR_EL2.C set to 0).
 
 [1] linux/Documentation/arm/booting.rst
 Latest version: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arch/arm/booting.rst

Cheers,
Luca

Re: [PATCH v4 1/7] docs/arm: Document Xen booting protocol on Armv8-R

[Julien Grall]

On 06/05/2025 13:24, Luca Fancellu wrote:
> Hi Julien,

Hi Luca,

> 
>> On 6 May 2025, at 12:44, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 29/04/2025 16:20, Luca Fancellu wrote:
>>> Document the requirement needed to boot Xen on Armv8-R platforms.
>>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>>> ---
>>> v4 changes:
>>>   - New patch
>>> ---
>>>   docs/misc/arm/booting.txt | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>> diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
>>> index 21ae74837dcc..719af74f1e69 100644
>>> --- a/docs/misc/arm/booting.txt
>>> +++ b/docs/misc/arm/booting.txt
>>> @@ -62,6 +62,14 @@ Xen relies on some settings the firmware has to configure in EL3 before starting
>>>     * The bit SCR_EL3.HCE (resp. SCR.HCE for 32-bit ARM) must be set to 1.
>>>   +When Xen runs on Armv8-R, the highest exception level is EL2 and the only
>>> +available state is secure (S) on Arm64 and non secure (NS) on Arm32, hence the
>>> +above requirements need to be adjusted to this case:
>>> +> +* Xen must be entered in S EL2 mode on Arm64 and in NS EL2 mode on Arm32.
>>
>> I think it would be better to update the line "Xen must be entered in NS EL2 mode" to clarify the state for 64-bit Arm.
>>
>>> +> +* Xen must be entered with MPU off and data cache disabled (SCTLR_EL2.M bit and
>>> +  SCTLR_EL2.C set to 0).
>>
>> This line is valid for Armv8-A/Armv7-A when using the Image/zImage protocol.
>>
>>>     [1] linux/Documentation/arm/booting.rst
>>>   Latest version: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arch/arm/booting.rst
> 
> Just to be sure to be on the same page, are you suggesting these changes on the original file?

Yes with one tweak.

 > > diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
> index 21ae74837dcc..c00c651805d7 100644
> --- a/docs/misc/arm/booting.txt
> +++ b/docs/misc/arm/booting.txt
> @@ -58,10 +58,14 @@ Firmware/bootloader requirements
>   
>   Xen relies on some settings the firmware has to configure in EL3 before starting Xen.

I think you want to update this sentence to remove the reference to EL3. 
Even on A-profile EL3 is not mandatory (I vaguely remember one of the 
platform I worked on had no EL3).

>   
> -* Xen must be entered in NS EL2 mode
> +* Xen must be entered in:
> +  * Non-Secure EL2 mode for Armv8-A Arm64 and Arm32, Armv8-R Arm32.
> +  * Secure EL2 mode for Armv8-R Arm64.
>   
>   * The bit SCR_EL3.HCE (resp. SCR.HCE for 32-bit ARM) must be set to 1.

And then here: "When EL3 is supported, ...". This would also cover the 
R-profile change.

Cheers,

>   
> +* Xen must be entered with MMU/MPU off and data cache disabled (SCTLR_EL2.M bit
> +  and SCTLR_EL2.C set to 0).
>   
>   [1] linux/Documentation/arm/booting.rst
>   Latest version: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arch/arm/booting.rst
> 
> Cheers,
> Luca
> 

-- 
Julien Grall

Re: [PATCH v4 1/7] docs/arm: Document Xen booting protocol on Armv8-R

[Luca Fancellu]

Hi Julien,

>> Just to be sure to be on the same page, are you suggesting these changes on the original file?
> 
> Yes with one tweak.
> 
> > > diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
>> index 21ae74837dcc..c00c651805d7 100644
>> --- a/docs/misc/arm/booting.txt
>> +++ b/docs/misc/arm/booting.txt
>> @@ -58,10 +58,14 @@ Firmware/bootloader requirements
>>    Xen relies on some settings the firmware has to configure in EL3 before starting Xen.
> 
> I think you want to update this sentence to remove the reference to EL3. Even on A-profile EL3 is not mandatory (I vaguely remember one of the platform I worked on had no EL3).
> 
>>  -* Xen must be entered in NS EL2 mode
>> +* Xen must be entered in:
>> +  * Non-Secure EL2 mode for Armv8-A Arm64 and Arm32, Armv8-R Arm32.
>> +  * Secure EL2 mode for Armv8-R Arm64.
>>    * The bit SCR_EL3.HCE (resp. SCR.HCE for 32-bit ARM) must be set to 1.
> 
> And then here: "When EL3 is supported, ...". This would also cover the R-profile change.
> 

Thanks for the clarification, @Michal, @Ayan, are you ok if I retain your R-by with these changes or
should I drop it?


> Cheers,
> 
>>  +* Xen must be entered with MMU/MPU off and data cache disabled (SCTLR_EL2.M bit
>> +  and SCTLR_EL2.C set to 0).
>>    [1] linux/Documentation/arm/booting.rst
>>  Latest version: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arch/arm/booting.rst

Cheers,
Luca

[PATCH v4 2/7] arm/mpu: Introduce MPU memory region map structure

[Luca Fancellu]

From: Penny Zheng <Penny.Zheng@arm.com>

Introduce pr_t typedef which is a structure having the prbar
and prlar members, each being structured as the registers of
the aarch64 armv8-r architecture.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Wei Chen <wei.chen@arm.com>
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
Changes in v4:
 - Fixed typos, changed name for reserved bitfields, add emacs bits
   to arm64/mpu.h.
   Now base and limit are 42 bits as we consider FEAT_LPA disabled,
   since we support max 1TB of memory.
   Moved data structure in commit that uses it
---
 xen/arch/arm/include/asm/arm64/mpu.h | 50 ++++++++++++++++++++++++++++
 xen/arch/arm/include/asm/mpu.h       |  4 +++
 2 files changed, 54 insertions(+)
 create mode 100644 xen/arch/arm/include/asm/arm64/mpu.h

diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/arm64/mpu.h
new file mode 100644
index 000000000000..b27fccd77550
--- /dev/null
+++ b/xen/arch/arm/include/asm/arm64/mpu.h
@@ -0,0 +1,50 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __ARM_ARM64_MPU_H__
+#define __ARM_ARM64_MPU_H__
+
+#ifndef __ASSEMBLY__
+
+/* Protection Region Base Address Register */
+typedef union {
+    struct __packed {
+        unsigned long xn:2;       /* Execute-Never */
+        unsigned long ap:2;       /* Access Permission */
+        unsigned long sh:2;       /* Shareability */
+        unsigned long base:42;    /* Base Address */
+        unsigned long res0:16;    /* RES0 */
+    } reg;
+    uint64_t bits;
+} prbar_t;
+
+/* Protection Region Limit Address Register */
+typedef union {
+    struct __packed {
+        unsigned long en:1;     /* Region enable */
+        unsigned long ai:3;     /* Memory Attribute Index */
+        unsigned long ns:1;     /* Not-Secure */
+        unsigned long res0:1;   /* RES0 */
+        unsigned long limit:42; /* Limit Address */
+        unsigned long res1:16;  /* RES0 */
+    } reg;
+    uint64_t bits;
+} prlar_t;
+
+/* MPU Protection Region */
+typedef struct {
+    prbar_t prbar;
+    prlar_t prlar;
+} pr_t;
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* __ARM_ARM64_MPU_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
index d4ec4248b62b..bb83f5a5f580 100644
--- a/xen/arch/arm/include/asm/mpu.h
+++ b/xen/arch/arm/include/asm/mpu.h
@@ -6,6 +6,10 @@
 #ifndef __ARM_MPU_H__
 #define __ARM_MPU_H__
 
+#if defined(CONFIG_ARM_64)
+# include <asm/arm64/mpu.h>
+#endif
+
 #define MPU_REGION_SHIFT  6
 #define MPU_REGION_ALIGN  (_AC(1, UL) << MPU_REGION_SHIFT)
 #define MPU_REGION_MASK   (~(MPU_REGION_ALIGN - 1))
-- 
2.34.1

Re: [PATCH v4 2/7] arm/mpu: Introduce MPU memory region map structure

[Orzel, Michal]

On 29/04/2025 17:20, Luca Fancellu wrote:
> From: Penny Zheng <Penny.Zheng@arm.com>
> 
> Introduce pr_t typedef which is a structure having the prbar
> and prlar members, each being structured as the registers of
> the aarch64 armv8-r architecture.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Wei Chen <wei.chen@arm.com>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH v4 2/7] arm/mpu: Introduce MPU memory region map structure

[Julien Grall]

On 29/04/2025 16:20, Luca Fancellu wrote:
> From: Penny Zheng <Penny.Zheng@arm.com>
> 
> Introduce pr_t typedef which is a structure having the prbar
> and prlar members, each being structured as the registers of
> the aarch64 armv8-r architecture.

Typo: AArch64 Armv8-R

> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Wei Chen <wei.chen@arm.com>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

[PATCH v4 3/7] arm/mpu: Provide and populate MPU C data structures

[Luca Fancellu]

Provide some data structure in the C world to track the MPU
status, these structures will be filled at boot by the assembly
early code with the boot MPU regions and afterwards they will be
used at runtime.

Provide methods to update a bitmap created with DECLARE_BITMAP
from the assembly code for both Arm32 and Arm64.

Modify Arm64 assembly boot code to reset any unused MPU region,
initialise 'max_xen_mpumap' with the number of supported MPU
regions and modify the common asm macro 'prepare_xen_region' to
load into xen_mpumap the MPU status and set/clear the bitmap
'xen_mpumap_mask' used to track the enabled regions.

Changed parameters name from 'base', 'limit' to 'tmp1' and 'tmp2'
in order to use them also for intermediate operations on the
MPU and the bitmap C data structures, the help on top of the macro
is enough to understand how the macro will work and this will
save some registers.

Provide a stub implementation for the pr_t type and few asm
macro for the Arm32 to prevent compilation break, they will
be implemented later.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v4 changes:
 - new patch
---
 xen/arch/arm/arm64/mpu/head.S            | 13 +++++
 xen/arch/arm/include/asm/arm32/mpu.h     | 25 +++++++++
 xen/arch/arm/include/asm/bitmap-op.inc   | 67 ++++++++++++++++++++++
 xen/arch/arm/include/asm/mpu.h           |  5 ++
 xen/arch/arm/include/asm/mpu/mm.h        |  7 +++
 xen/arch/arm/include/asm/mpu/regions.inc | 71 ++++++++++++++++++++----
 xen/arch/arm/mpu/mm.c                    | 16 ++++++
 7 files changed, 194 insertions(+), 10 deletions(-)
 create mode 100644 xen/arch/arm/include/asm/arm32/mpu.h
 create mode 100644 xen/arch/arm/include/asm/bitmap-op.inc

diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
index 6d336cafbbaf..c0cac06b015f 100644
--- a/xen/arch/arm/arm64/mpu/head.S
+++ b/xen/arch/arm/arm64/mpu/head.S
@@ -40,6 +40,9 @@ FUNC(enable_boot_cpu_mm)
     mrs   x5, MPUIR_EL2
     and   x5, x5, #NUM_MPU_REGIONS_MASK
 
+    ldr   x0, =max_xen_mpumap
+    strb  w5, [x0]
+
     /* x0: region sel */
     mov   x0, xzr
     /* Xen text section. */
@@ -74,6 +77,16 @@ FUNC(enable_boot_cpu_mm)
     prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_DEVICE_PRBAR, attr_prlar=REGION_DEVICE_PRLAR
 #endif
 
+zero_mpu:
+    /* Reset remaining MPU regions */
+    cmp   x0, x5
+    beq   out_zero_mpu
+    mov   x1, #0
+    mov   x2, #1
+    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prlar=REGION_DISABLED_PRLAR
+    b     zero_mpu
+
+out_zero_mpu:
     b    enable_mpu
     ret
 END(enable_boot_cpu_mm)
diff --git a/xen/arch/arm/include/asm/arm32/mpu.h b/xen/arch/arm/include/asm/arm32/mpu.h
new file mode 100644
index 000000000000..1bdae4c309dc
--- /dev/null
+++ b/xen/arch/arm/include/asm/arm32/mpu.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __ARM_ARM32_MPU_H__
+#define __ARM_ARM32_MPU_H__
+
+#ifndef __ASSEMBLY__
+
+/* MPU Protection Region */
+typedef struct {
+    uint32_t prbar;
+    uint32_t prlar;
+} pr_t;
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* __ARM_ARM32_MPU_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/include/asm/bitmap-op.inc b/xen/arch/arm/include/asm/bitmap-op.inc
new file mode 100644
index 000000000000..e316d9417bb9
--- /dev/null
+++ b/xen/arch/arm/include/asm/bitmap-op.inc
@@ -0,0 +1,67 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/*
+ * Sets a bit in a bitmap declared by DECLARE_BITMAP, symbol name passed through
+ * bitmap_symbol.
+ *
+ * bitmap_set_bit:      symbol of the bitmap declared by DECLARE_BITMAP
+ * bit:                 bit number to be set in the bitmap
+ * tmp1-tmp4:           temporary registers used for the computation
+ *
+ * Preserves bit.
+ * Output:
+ *  tmp1: Address of the word containing the changed bit.
+ * Clobbers: bit, tmp1, tmp2, tmp3, tmp4.
+ */
+.macro bitmap_set_bit bitmap_symbol, bit, tmp1, tmp2, tmp3, tmp4
+    adr_l   \tmp1, \bitmap_symbol
+    mov     \tmp2, #(BYTES_PER_LONG - 1)
+    mvn     \tmp2, \tmp2
+    lsr     \tmp3, \bit, #3
+    and     \tmp2, \tmp3, \tmp2
+    add     \tmp1, \tmp1, \tmp2                 // bitmap_symbol + (bit/BITS_PER_LONG)*BYTES_PER_LONG
+    and     \tmp2, \bit, #(BITS_PER_LONG - 1)   // bit offset inside word
+
+    ldr     \tmp3, [\tmp1]
+    mov     \tmp4, #1
+    lsl     \tmp4, \tmp4, \tmp2                 // (1 << offset)
+    orr     \tmp3, \tmp3, \tmp4                 // set the bit
+    str     \tmp3, [\tmp1]
+.endm
+
+/*
+ * Clears a bit in a bitmap declared by DECLARE_BITMAP, symbol name passed
+ * through bitmap_symbol.
+ *
+ * bitmap_set_bit:      symbol of the bitmap declared by DECLARE_BITMAP
+ * bit:                 bit number to be set in the bitmap
+ * tmp1-tmp4:           temporary registers used for the computation
+ *
+ * Preserves bit.
+ * Output:
+ *  tmp1: Address of the word containing the changed bit.
+ * Clobbers: bit, tmp1, tmp2, tmp3, tmp4.
+ */
+.macro bitmap_clear_bit bitmap_symbol, bit, tmp1, tmp2, tmp3, tmp4
+    adr_l   \tmp1, \bitmap_symbol
+    mov     \tmp2, #(BYTES_PER_LONG - 1)
+    mvn     \tmp2, \tmp2
+    lsr     \tmp3, \bit, #3
+    and     \tmp2, \tmp3, \tmp2
+    add     \tmp1, \tmp1, \tmp2                 // bitmap_symbol + (bit/BITS_PER_LONG)*BYTES_PER_LONG
+    and     \tmp2, \bit, #(BITS_PER_LONG - 1)   // bit offset inside word
+
+    ldr     \tmp3, [\tmp1]
+    mov     \tmp4, #1
+    lsl     \tmp4, \tmp4, \tmp2                 // (1 << offset)
+    mvn     \tmp4, \tmp4                        // ~(1 << offset)
+    and     \tmp3, \tmp3, \tmp4                 // clear the bit
+    str     \tmp3, [\tmp1]
+.endm
+
+/*
+ * Local variables:
+ * mode: ASM
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
index bb83f5a5f580..1368b2eb990f 100644
--- a/xen/arch/arm/include/asm/mpu.h
+++ b/xen/arch/arm/include/asm/mpu.h
@@ -8,6 +8,10 @@
 
 #if defined(CONFIG_ARM_64)
 # include <asm/arm64/mpu.h>
+#elif defined(CONFIG_ARM_32)
+# include <asm/arm32/mpu.h>
+#else
+# error "unknown ARM variant"
 #endif
 
 #define MPU_REGION_SHIFT  6
@@ -17,6 +21,7 @@
 #define NUM_MPU_REGIONS_SHIFT   8
 #define NUM_MPU_REGIONS         (_AC(1, UL) << NUM_MPU_REGIONS_SHIFT)
 #define NUM_MPU_REGIONS_MASK    (NUM_MPU_REGIONS - 1)
+#define MAX_MPU_REGION_NR       255
 
 #endif /* __ARM_MPU_H__ */
 
diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
index bfd840fa5d31..28339259c458 100644
--- a/xen/arch/arm/include/asm/mpu/mm.h
+++ b/xen/arch/arm/include/asm/mpu/mm.h
@@ -8,9 +8,16 @@
 #include <xen/page-size.h>
 #include <xen/types.h>
 #include <asm/mm.h>
+#include <asm/mpu.h>
 
 extern struct page_info *frame_table;
 
+extern uint8_t max_xen_mpumap;
+
+extern DECLARE_BITMAP(xen_mpumap_mask, MAX_MPU_REGION_NR);
+
+extern pr_t xen_mpumap[MAX_MPU_REGION_NR];
+
 #define virt_to_maddr(va) ((paddr_t)((vaddr_t)(va) & PADDR_MASK))
 
 #ifdef CONFIG_ARM_32
diff --git a/xen/arch/arm/include/asm/mpu/regions.inc b/xen/arch/arm/include/asm/mpu/regions.inc
index 47868a152662..dc0306f8c5fc 100644
--- a/xen/arch/arm/include/asm/mpu/regions.inc
+++ b/xen/arch/arm/include/asm/mpu/regions.inc
@@ -1,22 +1,50 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 
+#include <asm/bitmap-op.inc>
 #include <asm/mpu.h>
 #include <asm/sysregs.h>
 
 /* Backgroud region enable/disable */
 #define SCTLR_ELx_BR    BIT(17, UL)
 
+#define REGION_DISABLED_PRLAR   0x00    /* NS=0 ATTR=000 EN=0 */
 #define REGION_NORMAL_PRLAR     0x0f    /* NS=0 ATTR=111 EN=1 */
 #define REGION_DEVICE_PRLAR     0x09    /* NS=0 ATTR=100 EN=1 */
 
+#define PRLAR_ELx_EN            0x1
+
+#ifdef CONFIG_ARM_64
+#define XEN_MPUMAP_ENTRY_SHIFT  0x4     /* 16 byte structure */
+
+.macro store_pair reg1, reg2, dst
+        stp \reg1, \reg2, [\dst]
+.endm
+
+.macro invalidate_dcache_one reg
+        dc ivac, \reg
+.endm
+
+#else
+#define XEN_MPUMAP_ENTRY_SHIFT  0x2     /* 8 byte structure */
+
+.macro store_pair reg1, reg2, dst
+        nop
+.endm
+
+.macro invalidate_dcache_one reg
+        nop
+.endm
+
+#endif
+
 /*
  * Macro to prepare and set a EL2 MPU memory region.
  * We will also create an according MPU memory region entry, which
  * is a structure of pr_t,  in table \prmap.
  *
  * sel:         region selector
- * base:        reg storing base address
- * limit:       reg storing limit address
+ * tmp1:        reg storing base address
+ * tmp2:        reg storing limit address
  * prbar:       store computed PRBAR_EL2 value
  * prlar:       store computed PRLAR_EL2 value
  * maxcount:    maximum number of EL2 regions supported
@@ -28,13 +56,13 @@
  * Preserves maxcount
  * Output:
  *  sel: Next available region selector index.
- * Clobbers base, limit, prbar, prlar
+ * Clobbers tmp1, tmp2, prbar, prlar
  *
  * Note that all parameters using registers should be distinct.
  */
-.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR
+.macro prepare_xen_region, sel, tmp1, tmp2, prbar, prlar, maxcount, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR
     /* Check if the region is empty */
-    cmp   \base, \limit
+    cmp   \tmp1, \tmp2
     beq   1f
 
     /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */
@@ -42,20 +70,43 @@
     bge   fail_insufficient_regions
 
     /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/
-    and   \base, \base, #MPU_REGION_MASK
+    and   \tmp1, \tmp1, #MPU_REGION_MASK
     mov   \prbar, #\attr_prbar
-    orr   \prbar, \prbar, \base
+    orr   \prbar, \prbar, \tmp1
 
     /* Limit address should be inclusive */
-    sub   \limit, \limit, #1
-    and   \limit, \limit, #MPU_REGION_MASK
+    sub   \tmp2, \tmp2, #1
+    and   \tmp2, \tmp2, #MPU_REGION_MASK
     mov   \prlar, #\attr_prlar
-    orr   \prlar, \prlar, \limit
+    orr   \prlar, \prlar, \tmp2
 
     WRITE_SYSREG_ASM(\sel, PRSELR_EL2)
     isb
     WRITE_SYSREG_ASM(\prbar, PRBAR_EL2)
     WRITE_SYSREG_ASM(\prlar, PRLAR_EL2)
+
+    /* Load pair into xen_mpumap and invalidate cache */
+    mov   \tmp1, \sel
+    lsl   \tmp1, \tmp1, #XEN_MPUMAP_ENTRY_SHIFT
+    adr_l \tmp2, xen_mpumap
+    add   \tmp2, \tmp2, \tmp1
+    store_pair \prbar, \prlar, \tmp2
+    invalidate_dcache_one \tmp2
+
+    /* Set/clear xen_mpumap_mask bitmap */
+    tst   \prlar, #PRLAR_ELx_EN
+    bne   2f
+    // Region is disabled, clear the bit in the bitmap
+    bitmap_clear_bit xen_mpumap_mask, \sel, \tmp1, \tmp2, \prbar, \prlar
+    b     3f
+
+2:
+    // Region is enabled, set the bit in the bitmap
+    bitmap_set_bit xen_mpumap_mask, \sel, \tmp1, \tmp2, \prbar, \prlar
+
+3:
+    invalidate_dcache_one \tmp1
+
     dsb   sy
     isb
 
diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index 07c8959f4ee9..9eab09ff2044 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -7,9 +7,25 @@
 #include <xen/mm.h>
 #include <xen/sizes.h>
 #include <xen/types.h>
+#include <asm/mpu.h>
 
 struct page_info *frame_table;
 
+/* Maximum number of supported MPU memory regions by the EL2 MPU. */
+uint8_t __ro_after_init max_xen_mpumap;
+
+/*
+ * Bitmap xen_mpumap_mask is to record the usage of EL2 MPU memory regions.
+ * Bit 0 represents MPU memory region 0, bit 1 represents MPU memory
+ * region 1, ..., and so on.
+ * If a MPU memory region gets enabled, set the according bit to 1.
+ */
+DECLARE_BITMAP(xen_mpumap_mask, MAX_MPU_REGION_NR) \
+    __section(".data.page_aligned");
+
+/* EL2 Xen MPU memory region mapping table. */
+pr_t __section(".data.page_aligned") xen_mpumap[MAX_MPU_REGION_NR];
+
 static void __init __maybe_unused build_assertions(void)
 {
     /*
-- 
2.34.1

Re: [PATCH v4 3/7] arm/mpu: Provide and populate MPU C data structures

[Luca Fancellu]

Hi all,

> diff --git a/xen/arch/arm/include/asm/mpu/regions.inc b/xen/arch/arm/include/asm/mpu/regions.inc
> index 47868a152662..dc0306f8c5fc 100644
> --- a/xen/arch/arm/include/asm/mpu/regions.inc
> +++ b/xen/arch/arm/include/asm/mpu/regions.inc
> @@ -1,22 +1,50 @@
> /* SPDX-License-Identifier: GPL-2.0-only */
> 
> +#include <asm/bitmap-op.inc>
> #include <asm/mpu.h>
> #include <asm/sysregs.h>
> 
> /* Backgroud region enable/disable */
> #define SCTLR_ELx_BR    BIT(17, UL)
> 
> +#define REGION_DISABLED_PRLAR   0x00    /* NS=0 ATTR=000 EN=0 */
> #define REGION_NORMAL_PRLAR     0x0f    /* NS=0 ATTR=111 EN=1 */
> #define REGION_DEVICE_PRLAR     0x09    /* NS=0 ATTR=100 EN=1 */
> 
> +#define PRLAR_ELx_EN            0x1
> +
> +#ifdef CONFIG_ARM_64
> +#define XEN_MPUMAP_ENTRY_SHIFT  0x4     /* 16 byte structure */
> +
> +.macro store_pair reg1, reg2, dst
> +        stp \reg1, \reg2, [\dst]
> +.endm
> +
> +.macro invalidate_dcache_one reg
> +        dc ivac, \reg
> +.endm
> +
> +#else
> +#define XEN_MPUMAP_ENTRY_SHIFT  0x2     /* 8 byte structure */

This should be 0x3, my mistake! 

Cheers,
Luca

Re: [PATCH v4 3/7] arm/mpu: Provide and populate MPU C data structures

[Orzel, Michal]

On 29/04/2025 17:20, Luca Fancellu wrote:
> Provide some data structure in the C world to track the MPU
> status, these structures will be filled at boot by the assembly
> early code with the boot MPU regions and afterwards they will be
> used at runtime.
> 
> Provide methods to update a bitmap created with DECLARE_BITMAP
> from the assembly code for both Arm32 and Arm64.
> 
> Modify Arm64 assembly boot code to reset any unused MPU region,
> initialise 'max_xen_mpumap' with the number of supported MPU
IMO this is not a good name because there's nothing there suggesting that this
variable stores the number. Maybe max_mpu_regions or max_xen_mpumap_regions.

> regions and modify the common asm macro 'prepare_xen_region' to
> load into xen_mpumap the MPU status and set/clear the bitmap
> 'xen_mpumap_mask' used to track the enabled regions.
> 
> Changed parameters name from 'base', 'limit' to 'tmp1' and 'tmp2'
> in order to use them also for intermediate operations on the
> MPU and the bitmap C data structures, the help on top of the macro
> is enough to understand how the macro will work and this will
> save some registers.
> 
> Provide a stub implementation for the pr_t type and few asm
> macro for the Arm32 to prevent compilation break, they will
> be implemented later.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v4 changes:
>  - new patch
> ---
>  xen/arch/arm/arm64/mpu/head.S            | 13 +++++
>  xen/arch/arm/include/asm/arm32/mpu.h     | 25 +++++++++
>  xen/arch/arm/include/asm/bitmap-op.inc   | 67 ++++++++++++++++++++++
>  xen/arch/arm/include/asm/mpu.h           |  5 ++
>  xen/arch/arm/include/asm/mpu/mm.h        |  7 +++
>  xen/arch/arm/include/asm/mpu/regions.inc | 71 ++++++++++++++++++++----
>  xen/arch/arm/mpu/mm.c                    | 16 ++++++
>  7 files changed, 194 insertions(+), 10 deletions(-)
>  create mode 100644 xen/arch/arm/include/asm/arm32/mpu.h
>  create mode 100644 xen/arch/arm/include/asm/bitmap-op.inc
> 
> diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
> index 6d336cafbbaf..c0cac06b015f 100644
> --- a/xen/arch/arm/arm64/mpu/head.S
> +++ b/xen/arch/arm/arm64/mpu/head.S
> @@ -40,6 +40,9 @@ FUNC(enable_boot_cpu_mm)
>      mrs   x5, MPUIR_EL2
>      and   x5, x5, #NUM_MPU_REGIONS_MASK
>  
> +    ldr   x0, =max_xen_mpumap
> +    strb  w5, [x0]
> +
>      /* x0: region sel */
>      mov   x0, xzr
>      /* Xen text section. */
> @@ -74,6 +77,16 @@ FUNC(enable_boot_cpu_mm)
>      prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_DEVICE_PRBAR, attr_prlar=REGION_DEVICE_PRLAR
>  #endif
>  
> +zero_mpu:
> +    /* Reset remaining MPU regions */
> +    cmp   x0, x5
> +    beq   out_zero_mpu
> +    mov   x1, #0
> +    mov   x2, #1
Shouldn't we mark the region as emtpy (base == limit) when doing region clear?

> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prlar=REGION_DISABLED_PRLAR
> +    b     zero_mpu
> +
> +out_zero_mpu:
>      b    enable_mpu
>      ret
>  END(enable_boot_cpu_mm)
> diff --git a/xen/arch/arm/include/asm/arm32/mpu.h b/xen/arch/arm/include/asm/arm32/mpu.h
> new file mode 100644
> index 000000000000..1bdae4c309dc
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/arm32/mpu.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __ARM_ARM32_MPU_H__
> +#define __ARM_ARM32_MPU_H__
> +
> +#ifndef __ASSEMBLY__
> +
> +/* MPU Protection Region */
> +typedef struct {
> +    uint32_t prbar;
> +    uint32_t prlar;
> +} pr_t;
> +
> +#endif /* __ASSEMBLY__ */
> +
> +#endif /* __ARM_ARM32_MPU_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/include/asm/bitmap-op.inc b/xen/arch/arm/include/asm/bitmap-op.inc
> new file mode 100644
> index 000000000000..e316d9417bb9
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/bitmap-op.inc
I'll just skim through this file for now. I won't check for algorithm being correct.

> @@ -0,0 +1,67 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +/*
> + * Sets a bit in a bitmap declared by DECLARE_BITMAP, symbol name passed through
> + * bitmap_symbol.
> + *
> + * bitmap_set_bit:      symbol of the bitmap declared by DECLARE_BITMAP
> + * bit:                 bit number to be set in the bitmap
> + * tmp1-tmp4:           temporary registers used for the computation
> + *
> + * Preserves bit.
Here you say it is preserved, yet...

> + * Output:
> + *  tmp1: Address of the word containing the changed bit.
> + * Clobbers: bit, tmp1, tmp2, tmp3, tmp4.
... here you list is as clobbered.

> + */
> +.macro bitmap_set_bit bitmap_symbol, bit, tmp1, tmp2, tmp3, tmp4
> +    adr_l   \tmp1, \bitmap_symbol
> +    mov     \tmp2, #(BYTES_PER_LONG - 1)
> +    mvn     \tmp2, \tmp2
> +    lsr     \tmp3, \bit, #3
> +    and     \tmp2, \tmp3, \tmp2
> +    add     \tmp1, \tmp1, \tmp2                 // bitmap_symbol + (bit/BITS_PER_LONG)*BYTES_PER_LONG
We don't use // style comments. Please use /* */

> +    and     \tmp2, \bit, #(BITS_PER_LONG - 1)   // bit offset inside word
> +
> +    ldr     \tmp3, [\tmp1]
> +    mov     \tmp4, #1
> +    lsl     \tmp4, \tmp4, \tmp2                 // (1 << offset)
> +    orr     \tmp3, \tmp3, \tmp4                 // set the bit
> +    str     \tmp3, [\tmp1]
> +.endm
> +
> +/*
> + * Clears a bit in a bitmap declared by DECLARE_BITMAP, symbol name passed
> + * through bitmap_symbol.
> + *
> + * bitmap_set_bit:      symbol of the bitmap declared by DECLARE_BITMAP
> + * bit:                 bit number to be set in the bitmap
> + * tmp1-tmp4:           temporary registers used for the computation
> + *
> + * Preserves bit.
> + * Output:
> + *  tmp1: Address of the word containing the changed bit.
> + * Clobbers: bit, tmp1, tmp2, tmp3, tmp4.
> + */
> +.macro bitmap_clear_bit bitmap_symbol, bit, tmp1, tmp2, tmp3, tmp4
> +    adr_l   \tmp1, \bitmap_symbol
> +    mov     \tmp2, #(BYTES_PER_LONG - 1)
> +    mvn     \tmp2, \tmp2
> +    lsr     \tmp3, \bit, #3
> +    and     \tmp2, \tmp3, \tmp2
> +    add     \tmp1, \tmp1, \tmp2                 // bitmap_symbol + (bit/BITS_PER_LONG)*BYTES_PER_LONG
> +    and     \tmp2, \bit, #(BITS_PER_LONG - 1)   // bit offset inside word
> +
> +    ldr     \tmp3, [\tmp1]
> +    mov     \tmp4, #1
> +    lsl     \tmp4, \tmp4, \tmp2                 // (1 << offset)
> +    mvn     \tmp4, \tmp4                        // ~(1 << offset)
> +    and     \tmp3, \tmp3, \tmp4                 // clear the bit
> +    str     \tmp3, [\tmp1]
> +.endm
> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
> index bb83f5a5f580..1368b2eb990f 100644
> --- a/xen/arch/arm/include/asm/mpu.h
> +++ b/xen/arch/arm/include/asm/mpu.h
> @@ -8,6 +8,10 @@
>  
>  #if defined(CONFIG_ARM_64)
>  # include <asm/arm64/mpu.h>
> +#elif defined(CONFIG_ARM_32)
> +# include <asm/arm32/mpu.h>
> +#else
> +# error "unknown ARM variant"
>  #endif
>  
>  #define MPU_REGION_SHIFT  6
> @@ -17,6 +21,7 @@
>  #define NUM_MPU_REGIONS_SHIFT   8
>  #define NUM_MPU_REGIONS         (_AC(1, UL) << NUM_MPU_REGIONS_SHIFT)
>  #define NUM_MPU_REGIONS_MASK    (NUM_MPU_REGIONS - 1)
> +#define MAX_MPU_REGION_NR       255
Shouldn't you define it using NUM_MPU_REGIONS? It should have the same
definition as mask.

>  
>  #endif /* __ARM_MPU_H__ */
>  
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
> index bfd840fa5d31..28339259c458 100644
> --- a/xen/arch/arm/include/asm/mpu/mm.h
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -8,9 +8,16 @@
>  #include <xen/page-size.h>
>  #include <xen/types.h>
>  #include <asm/mm.h>
> +#include <asm/mpu.h>
>  
>  extern struct page_info *frame_table;
>  
> +extern uint8_t max_xen_mpumap;
> +
> +extern DECLARE_BITMAP(xen_mpumap_mask, MAX_MPU_REGION_NR);
> +
> +extern pr_t xen_mpumap[MAX_MPU_REGION_NR];
> +
>  #define virt_to_maddr(va) ((paddr_t)((vaddr_t)(va) & PADDR_MASK))
>  
>  #ifdef CONFIG_ARM_32
> diff --git a/xen/arch/arm/include/asm/mpu/regions.inc b/xen/arch/arm/include/asm/mpu/regions.inc
> index 47868a152662..dc0306f8c5fc 100644
> --- a/xen/arch/arm/include/asm/mpu/regions.inc
> +++ b/xen/arch/arm/include/asm/mpu/regions.inc
> @@ -1,22 +1,50 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
>  
> +#include <asm/bitmap-op.inc>
>  #include <asm/mpu.h>
>  #include <asm/sysregs.h>
>  
>  /* Backgroud region enable/disable */
>  #define SCTLR_ELx_BR    BIT(17, UL)
>  
> +#define REGION_DISABLED_PRLAR   0x00    /* NS=0 ATTR=000 EN=0 */
>  #define REGION_NORMAL_PRLAR     0x0f    /* NS=0 ATTR=111 EN=1 */
>  #define REGION_DEVICE_PRLAR     0x09    /* NS=0 ATTR=100 EN=1 */
>  
> +#define PRLAR_ELx_EN            0x1
> +
> +#ifdef CONFIG_ARM_64
> +#define XEN_MPUMAP_ENTRY_SHIFT  0x4     /* 16 byte structure */
> +
> +.macro store_pair reg1, reg2, dst
> +        stp \reg1, \reg2, [\dst]
Why 8 instead of 4 spaces?

> +.endm
> +
> +.macro invalidate_dcache_one reg
> +        dc ivac, \reg
> +.endm
> +
> +#else
> +#define XEN_MPUMAP_ENTRY_SHIFT  0x2     /* 8 byte structure */
> +
> +.macro store_pair reg1, reg2, dst
> +        nop
> +.endm
> +
> +.macro invalidate_dcache_one reg
> +        nop
> +.endm
> +
> +#endif
> +
>  /*
>   * Macro to prepare and set a EL2 MPU memory region.
>   * We will also create an according MPU memory region entry, which
>   * is a structure of pr_t,  in table \prmap.
>   *
>   * sel:         region selector
> - * base:        reg storing base address
> - * limit:       reg storing limit address
> + * tmp1:        reg storing base address
> + * tmp2:        reg storing limit address
I think this change is not needed. The parameters should be named base and limit
because this is what you expect caller to pass. Inside the function, you can do
whatever you want with these registers and caller does not care as long as you
mention if they are clobbered or not. Same in C world. You can reuse the
parameter for a different internal purpose inside a function.

>   * prbar:       store computed PRBAR_EL2 value
>   * prlar:       store computed PRLAR_EL2 value
>   * maxcount:    maximum number of EL2 regions supported
> @@ -28,13 +56,13 @@
>   * Preserves maxcount
>   * Output:
>   *  sel: Next available region selector index.
> - * Clobbers base, limit, prbar, prlar
> + * Clobbers tmp1, tmp2, prbar, prlar
>   *
>   * Note that all parameters using registers should be distinct.
>   */
> -.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR
> +.macro prepare_xen_region, sel, tmp1, tmp2, prbar, prlar, maxcount, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR
>      /* Check if the region is empty */
> -    cmp   \base, \limit
> +    cmp   \tmp1, \tmp2
>      beq   1f
>  
>      /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */
> @@ -42,20 +70,43 @@
>      bge   fail_insufficient_regions
>  
>      /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/
> -    and   \base, \base, #MPU_REGION_MASK
> +    and   \tmp1, \tmp1, #MPU_REGION_MASK
>      mov   \prbar, #\attr_prbar
> -    orr   \prbar, \prbar, \base
> +    orr   \prbar, \prbar, \tmp1
>  
>      /* Limit address should be inclusive */
> -    sub   \limit, \limit, #1
> -    and   \limit, \limit, #MPU_REGION_MASK
> +    sub   \tmp2, \tmp2, #1
> +    and   \tmp2, \tmp2, #MPU_REGION_MASK
>      mov   \prlar, #\attr_prlar
> -    orr   \prlar, \prlar, \limit
> +    orr   \prlar, \prlar, \tmp2
>  
>      WRITE_SYSREG_ASM(\sel, PRSELR_EL2)
>      isb
>      WRITE_SYSREG_ASM(\prbar, PRBAR_EL2)
>      WRITE_SYSREG_ASM(\prlar, PRLAR_EL2)
> +
> +    /* Load pair into xen_mpumap and invalidate cache */
> +    mov   \tmp1, \sel
> +    lsl   \tmp1, \tmp1, #XEN_MPUMAP_ENTRY_SHIFT
You could get rid of these 2 extra instructions and instead do:

> +    adr_l \tmp2, xen_mpumap
> +    add   \tmp2, \tmp2, \tmp1
add \tmp2, \tmp2, \sel, lsl #XEN_MPUMAP_ENTRY_SHIFT
which combines everything in one go.

> +    store_pair \prbar, \prlar, \tmp2
> +    invalidate_dcache_one \tmp2
> +
> +    /* Set/clear xen_mpumap_mask bitmap */
> +    tst   \prlar, #PRLAR_ELx_EN
> +    bne   2f
> +    // Region is disabled, clear the bit in the bitmap
Comment style, here and elsewhere

> +    bitmap_clear_bit xen_mpumap_mask, \sel, \tmp1, \tmp2, \prbar, \prlar
> +    b     3f
> +
> +2:
> +    // Region is enabled, set the bit in the bitmap
> +    bitmap_set_bit xen_mpumap_mask, \sel, \tmp1, \tmp2, \prbar, \prlar
Wouldn't it be better to first clear the entire bitmap before setting up the
regions (i.e. all regions disabled) and then only have the set part here?

> +
> +3:
> +    invalidate_dcache_one \tmp1
> +
>      dsb   sy
>      isb
>  
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index 07c8959f4ee9..9eab09ff2044 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -7,9 +7,25 @@
>  #include <xen/mm.h>
>  #include <xen/sizes.h>
>  #include <xen/types.h>
> +#include <asm/mpu.h>
>  
>  struct page_info *frame_table;
>  
> +/* Maximum number of supported MPU memory regions by the EL2 MPU. */
> +uint8_t __ro_after_init max_xen_mpumap;
> +
> +/*
> + * Bitmap xen_mpumap_mask is to record the usage of EL2 MPU memory regions.
> + * Bit 0 represents MPU memory region 0, bit 1 represents MPU memory
> + * region 1, ..., and so on.
> + * If a MPU memory region gets enabled, set the according bit to 1.
> + */
> +DECLARE_BITMAP(xen_mpumap_mask, MAX_MPU_REGION_NR) \
> +    __section(".data.page_aligned");
> +
> +/* EL2 Xen MPU memory region mapping table. */
> +pr_t __section(".data.page_aligned") xen_mpumap[MAX_MPU_REGION_NR];
> +
>  static void __init __maybe_unused build_assertions(void)
>  {
>      /*

~Michal

Re: [PATCH v4 3/7] arm/mpu: Provide and populate MPU C data structures

[Luca Fancellu]

Hi Michal,

> On 30 Apr 2025, at 11:57, Orzel, Michal <Michal.Orzel@amd.com> wrote:
> 
> 
> 
> On 29/04/2025 17:20, Luca Fancellu wrote:
>> Provide some data structure in the C world to track the MPU
>> status, these structures will be filled at boot by the assembly
>> early code with the boot MPU regions and afterwards they will be
>> used at runtime.
>> 
>> Provide methods to update a bitmap created with DECLARE_BITMAP
>> from the assembly code for both Arm32 and Arm64.
>> 
>> Modify Arm64 assembly boot code to reset any unused MPU region,
>> initialise 'max_xen_mpumap' with the number of supported MPU
> IMO this is not a good name because there's nothing there suggesting that this
> variable stores the number. Maybe max_mpu_regions or max_xen_mpumap_regions.

ok I will change it

>> 
>>     /* x0: region sel */
>>     mov   x0, xzr
>>     /* Xen text section. */
>> @@ -74,6 +77,16 @@ FUNC(enable_boot_cpu_mm)
>>     prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_DEVICE_PRBAR, attr_prlar=REGION_DEVICE_PRLAR
>> #endif
>> 
>> +zero_mpu:
>> +    /* Reset remaining MPU regions */
>> +    cmp   x0, x5
>> +    beq   out_zero_mpu
>> +    mov   x1, #0
>> +    mov   x2, #1
> Shouldn't we mark the region as emtpy (base == limit) when doing region clear?

So the macro takes an exclusive range, inside it will change to inclusive by doing limit-1,
so the region will be empty.

> 
> 
>> @@ -0,0 +1,67 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +/*
>> + * Sets a bit in a bitmap declared by DECLARE_BITMAP, symbol name passed through
>> + * bitmap_symbol.
>> + *
>> + * bitmap_set_bit:      symbol of the bitmap declared by DECLARE_BITMAP
>> + * bit:                 bit number to be set in the bitmap
>> + * tmp1-tmp4:           temporary registers used for the computation
>> + *
>> + * Preserves bit.
> Here you say it is preserved, yet...
> 
>> + * Output:
>> + *  tmp1: Address of the word containing the changed bit.
>> + * Clobbers: bit, tmp1, tmp2, tmp3, tmp4.
> ... here you list is as clobbered.

right, I’ll fix that

> 
>> + */
>> +.macro bitmap_set_bit bitmap_symbol, bit, tmp1, tmp2, tmp3, tmp4
>> +    adr_l   \tmp1, \bitmap_symbol
>> +    mov     \tmp2, #(BYTES_PER_LONG - 1)
>> +    mvn     \tmp2, \tmp2
>> +    lsr     \tmp3, \bit, #3
>> +    and     \tmp2, \tmp3, \tmp2
>> +    add     \tmp1, \tmp1, \tmp2                 // bitmap_symbol + (bit/BITS_PER_LONG)*BYTES_PER_LONG
> We don't use // style comments. Please use /* */

sure, I’ll change here and in the rest of the patch

>> 
>> 
>> #define MPU_REGION_SHIFT  6
>> @@ -17,6 +21,7 @@
>> #define NUM_MPU_REGIONS_SHIFT   8
>> #define NUM_MPU_REGIONS         (_AC(1, UL) << NUM_MPU_REGIONS_SHIFT)
>> #define NUM_MPU_REGIONS_MASK    (NUM_MPU_REGIONS - 1)
>> +#define MAX_MPU_REGION_NR       255
> Shouldn't you define it using NUM_MPU_REGIONS? It should have the same
> definition as mask.

Maybe I misunderstood your comment in the previous patch, ok I will use:

#define MAX_MPU_REGION_NR      NUM_MPU_REGIONS_MASK

>> 
>> +
>> +.macro store_pair reg1, reg2, dst
>> +        stp \reg1, \reg2, [\dst]
> Why 8 instead of 4 spaces?

I’ll fix 

>> 
>> /*
>>  * Macro to prepare and set a EL2 MPU memory region.
>>  * We will also create an according MPU memory region entry, which
>>  * is a structure of pr_t,  in table \prmap.
>>  *
>>  * sel:         region selector
>> - * base:        reg storing base address
>> - * limit:       reg storing limit address
>> + * tmp1:        reg storing base address
>> + * tmp2:        reg storing limit address
> I think this change is not needed. The parameters should be named base and limit
> because this is what you expect caller to pass. Inside the function, you can do
> whatever you want with these registers and caller does not care as long as you
> mention if they are clobbered or not. Same in C world. You can reuse the
> parameter for a different internal purpose inside a function.

ok I’ll revert back

>> 
>> +
>> +    /* Load pair into xen_mpumap and invalidate cache */
>> +    mov   \tmp1, \sel
>> +    lsl   \tmp1, \tmp1, #XEN_MPUMAP_ENTRY_SHIFT
> You could get rid of these 2 extra instructions and instead do:
> 
>> +    adr_l \tmp2, xen_mpumap
>> +    add   \tmp2, \tmp2, \tmp1
> add \tmp2, \tmp2, \sel, lsl #XEN_MPUMAP_ENTRY_SHIFT

yep, I’ll use it

> which combines everything in one go.
> 
>> +    store_pair \prbar, \prlar, \tmp2
>> +    invalidate_dcache_one \tmp2
>> +
>> +    /* Set/clear xen_mpumap_mask bitmap */
>> +    tst   \prlar, #PRLAR_ELx_EN
>> +    bne   2f
>> +    // Region is disabled, clear the bit in the bitmap
> Comment style, here and elsewhere
> 
>> +    bitmap_clear_bit xen_mpumap_mask, \sel, \tmp1, \tmp2, \prbar, \prlar
>> +    b     3f
>> +
>> +2:
>> +    // Region is enabled, set the bit in the bitmap
>> +    bitmap_set_bit xen_mpumap_mask, \sel, \tmp1, \tmp2, \prbar, \prlar
> Wouldn't it be better to first clear the entire bitmap before setting up the
> regions (i.e. all regions disabled) and then only have the set part here?

well we are going to set up all region anyway, doing that here will save some time
spent on barriers and cache invalidation, maybe not much, but still…

Cheers,
Luca

[PATCH v4 4/7] arm/mpu: Provide access to the MPU region from the C code

[Luca Fancellu]

Implement some utility function in order to access the MPU regions
from the C world.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v4 changes:
 - moved back PRBAR0_EL2/PRLAR0_EL2 to mm.c and protect
   them with CONFIG_ARM_64, changed comments, fixed typos and code
   style
 - Add PRBAR_EL2_(n) definition, to be overriden by Arm32
 - protect prepare_selector, read_protection_region,
   write_protection_region by Arm64 to ensure compilation on both
   arm32 and arm64, Arm32 will modify that later while introducing
   the arm32 bits.
v3 changes:
 - Moved PRBAR0_EL2/PRLAR0_EL2 to arm64 specific
 - Modified prepare_selector() to be easily made a NOP
   for Arm32, which can address up to 32 region without
   changing selector and it is also its maximum amount
   of MPU regions.
---
---
 xen/arch/arm/include/asm/mpu.h    |   1 +
 xen/arch/arm/include/asm/mpu/mm.h |  34 +++++++++
 xen/arch/arm/mpu/mm.c             | 117 ++++++++++++++++++++++++++++++
 3 files changed, 152 insertions(+)

diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
index 1368b2eb990f..40a86140b6cc 100644
--- a/xen/arch/arm/include/asm/mpu.h
+++ b/xen/arch/arm/include/asm/mpu.h
@@ -17,6 +17,7 @@
 #define MPU_REGION_SHIFT  6
 #define MPU_REGION_ALIGN  (_AC(1, UL) << MPU_REGION_SHIFT)
 #define MPU_REGION_MASK   (~(MPU_REGION_ALIGN - 1))
+#define MPU_REGION_RES0   (0xFFFFULL << 48)
 
 #define NUM_MPU_REGIONS_SHIFT   8
 #define NUM_MPU_REGIONS         (_AC(1, UL) << NUM_MPU_REGIONS_SHIFT)
diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
index 28339259c458..e2235e568e81 100644
--- a/xen/arch/arm/include/asm/mpu/mm.h
+++ b/xen/arch/arm/include/asm/mpu/mm.h
@@ -41,6 +41,40 @@ static inline struct page_info *virt_to_page(const void *v)
     return mfn_to_page(mfn);
 }
 
+/* Utility function to be used whenever MPU regions are modified */
+static inline void context_sync_mpu(void)
+{
+    /*
+     * ARM DDI 0600B.a, C1.7.1
+     * Writes to MPU registers are only guaranteed to be visible following a
+     * Context synchronization event and DSB operation.
+     */
+    dsb(sy);
+    isb();
+}
+
+/*
+ * The following API requires context_sync_mpu() after being used to modify MPU
+ * regions:
+ *  - write_protection_region
+ */
+
+/*
+ * Reads the MPU region with index 'sel' from the HW.
+ *
+ * @pr_read: mpu protection region returned by read op.
+ * @sel: mpu protection region selector
+ */
+extern void read_protection_region(pr_t *pr_read, uint8_t sel);
+
+/*
+ * Writes the MPU region with index 'sel' to the HW.
+ *
+ * @pr_write: const mpu protection region passed through write op.
+ * @sel: mpu protection region selector
+ */
+extern void write_protection_region(const pr_t *pr_write, uint8_t sel);
+
 #endif /* __ARM_MPU_MM_H__ */
 
 /*
diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index 9eab09ff2044..40ccf99adc94 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -8,6 +8,8 @@
 #include <xen/sizes.h>
 #include <xen/types.h>
 #include <asm/mpu.h>
+#include <asm/mpu/mm.h>
+#include <asm/sysregs.h>
 
 struct page_info *frame_table;
 
@@ -26,6 +28,35 @@ DECLARE_BITMAP(xen_mpumap_mask, MAX_MPU_REGION_NR) \
 /* EL2 Xen MPU memory region mapping table. */
 pr_t __section(".data.page_aligned") xen_mpumap[MAX_MPU_REGION_NR];
 
+#ifdef CONFIG_ARM_64
+/*
+ * The following are needed for the case generators GENERATE_WRITE_PR_REG_CASE
+ * and GENERATE_READ_PR_REG_CASE with num==0
+ */
+#define PRBAR0_EL2 PRBAR_EL2
+#define PRLAR0_EL2 PRLAR_EL2
+
+#define PRBAR_EL2_(n)   PRBAR##n##_EL2
+#define PRLAR_EL2_(n)   PRLAR##n##_EL2
+
+#endif
+
+#define GENERATE_WRITE_PR_REG_CASE(num, pr)                                 \
+    case num:                                                               \
+    {                                                                       \
+        WRITE_SYSREG(pr->prbar.bits & ~MPU_REGION_RES0, PRBAR_EL2_(num));   \
+        WRITE_SYSREG(pr->prlar.bits & ~MPU_REGION_RES0, PRLAR_EL2_(num));   \
+        break;                                                              \
+    }
+
+#define GENERATE_READ_PR_REG_CASE(num, pr)                      \
+    case num:                                                   \
+    {                                                           \
+        pr->prbar.bits = READ_SYSREG(PRBAR_EL2_(num));          \
+        pr->prlar.bits = READ_SYSREG(PRLAR_EL2_(num));          \
+        break;                                                  \
+    }
+
 static void __init __maybe_unused build_assertions(void)
 {
     /*
@@ -36,6 +67,92 @@ static void __init __maybe_unused build_assertions(void)
     BUILD_BUG_ON(PAGE_SIZE != SZ_4K);
 }
 
+#ifdef CONFIG_ARM_64
+/*
+ * Armv8-R supports direct access and indirect access to the MPU regions through
+ * registers, indirect access involves changing the MPU region selector, issuing
+ * an isb barrier and accessing the selected region through specific registers;
+ * instead direct access involves accessing specific registers that points to
+ * a specific MPU region, without changing the selector (in some cases) and
+ * issuing barriers because of that.
+ * For Arm64 the PR{B,L}AR_ELx (for n=0) and PR{B,L}AR<n>_ELx, n=1..15, are used
+ * for the direct access to the regions selected by PRSELR_EL2.REGION<7:4>:n, so
+ * 16 regions can be directly access when the selector is multiple of 16, giving
+ * access to all the supported memory regions.
+ */
+static void prepare_selector(uint8_t *sel)
+{
+    uint8_t cur_sel = *sel;
+
+    /*
+     * {read,write}_protection_region works using the direct access to the 0..15
+     * regions, so in order to save the isb() overhead, change the PRSELR_EL2
+     * only when needed, so when the upper 4 bits of the selector will change.
+     */
+    cur_sel &= 0xF0U;
+    if ( READ_SYSREG(PRSELR_EL2) != cur_sel )
+    {
+        WRITE_SYSREG(cur_sel, PRSELR_EL2);
+        isb();
+    }
+    *sel = *sel & 0xFU;
+}
+
+void read_protection_region(pr_t *pr_read, uint8_t sel)
+{
+    prepare_selector(&sel);
+
+    switch ( sel )
+    {
+        GENERATE_READ_PR_REG_CASE(0, pr_read);
+        GENERATE_READ_PR_REG_CASE(1, pr_read);
+        GENERATE_READ_PR_REG_CASE(2, pr_read);
+        GENERATE_READ_PR_REG_CASE(3, pr_read);
+        GENERATE_READ_PR_REG_CASE(4, pr_read);
+        GENERATE_READ_PR_REG_CASE(5, pr_read);
+        GENERATE_READ_PR_REG_CASE(6, pr_read);
+        GENERATE_READ_PR_REG_CASE(7, pr_read);
+        GENERATE_READ_PR_REG_CASE(8, pr_read);
+        GENERATE_READ_PR_REG_CASE(9, pr_read);
+        GENERATE_READ_PR_REG_CASE(10, pr_read);
+        GENERATE_READ_PR_REG_CASE(11, pr_read);
+        GENERATE_READ_PR_REG_CASE(12, pr_read);
+        GENERATE_READ_PR_REG_CASE(13, pr_read);
+        GENERATE_READ_PR_REG_CASE(14, pr_read);
+        GENERATE_READ_PR_REG_CASE(15, pr_read);
+    default:
+        BUG(); /* Can't happen */
+    }
+}
+
+void write_protection_region(const pr_t *pr_write, uint8_t sel)
+{
+    prepare_selector(&sel);
+
+    switch ( sel )
+    {
+        GENERATE_WRITE_PR_REG_CASE(0, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(1, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(2, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(3, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(4, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(5, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(6, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(7, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(8, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(9, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(10, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(11, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(12, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(13, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(14, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(15, pr_write);
+    default:
+        BUG(); /* Can't happen */
+    }
+}
+#endif
+
 void __init setup_mm(void)
 {
     BUG_ON("unimplemented");
-- 
2.34.1

Re: [PATCH v4 4/7] arm/mpu: Provide access to the MPU region from the C code

[Orzel, Michal]

On 29/04/2025 17:20, Luca Fancellu wrote:
> Implement some utility function in order to access the MPU regions
s/function/functions/

> from the C world.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v4 changes:
>  - moved back PRBAR0_EL2/PRLAR0_EL2 to mm.c and protect
>    them with CONFIG_ARM_64, changed comments, fixed typos and code
>    style
>  - Add PRBAR_EL2_(n) definition, to be overriden by Arm32
>  - protect prepare_selector, read_protection_region,
>    write_protection_region by Arm64 to ensure compilation on both
>    arm32 and arm64, Arm32 will modify that later while introducing
>    the arm32 bits.
> v3 changes:
>  - Moved PRBAR0_EL2/PRLAR0_EL2 to arm64 specific
>  - Modified prepare_selector() to be easily made a NOP
>    for Arm32, which can address up to 32 region without
>    changing selector and it is also its maximum amount
>    of MPU regions.
> ---
> ---
>  xen/arch/arm/include/asm/mpu.h    |   1 +
>  xen/arch/arm/include/asm/mpu/mm.h |  34 +++++++++
>  xen/arch/arm/mpu/mm.c             | 117 ++++++++++++++++++++++++++++++
>  3 files changed, 152 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
> index 1368b2eb990f..40a86140b6cc 100644
> --- a/xen/arch/arm/include/asm/mpu.h
> +++ b/xen/arch/arm/include/asm/mpu.h
> @@ -17,6 +17,7 @@
>  #define MPU_REGION_SHIFT  6
>  #define MPU_REGION_ALIGN  (_AC(1, UL) << MPU_REGION_SHIFT)
>  #define MPU_REGION_MASK   (~(MPU_REGION_ALIGN - 1))
> +#define MPU_REGION_RES0   (0xFFFFULL << 48)
This does not look like a common macro. It's arm64 specific.
Also, it looks like you use it in macros that are common too.

>  
>  #define NUM_MPU_REGIONS_SHIFT   8
>  #define NUM_MPU_REGIONS         (_AC(1, UL) << NUM_MPU_REGIONS_SHIFT)
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
> index 28339259c458..e2235e568e81 100644
> --- a/xen/arch/arm/include/asm/mpu/mm.h
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -41,6 +41,40 @@ static inline struct page_info *virt_to_page(const void *v)
>      return mfn_to_page(mfn);
>  }
>  
> +/* Utility function to be used whenever MPU regions are modified */
> +static inline void context_sync_mpu(void)
> +{
> +    /*
> +     * ARM DDI 0600B.a, C1.7.1
> +     * Writes to MPU registers are only guaranteed to be visible following a
> +     * Context synchronization event and DSB operation.
> +     */
> +    dsb(sy);
> +    isb();
> +}
> +
> +/*
> + * The following API requires context_sync_mpu() after being used to modify MPU
> + * regions:
> + *  - write_protection_region
> + */
> +
> +/*
> + * Reads the MPU region with index 'sel' from the HW.
If you use @foo style, you should use @sel here.
But IMO this comment does not bring any usefulness.
The name of the helper and parameter description is enough.

> + *
> + * @pr_read: mpu protection region returned by read op.
> + * @sel: mpu protection region selector
> + */
> +extern void read_protection_region(pr_t *pr_read, uint8_t sel);
> +
> +/*
> + * Writes the MPU region with index 'sel' to the HW.
> + *
> + * @pr_write: const mpu protection region passed through write op.
No need to say const in parameter description

> + * @sel: mpu protection region selector
Same here.

> + */
> +extern void write_protection_region(const pr_t *pr_write, uint8_t sel);
> +
>  #endif /* __ARM_MPU_MM_H__ */
>  
>  /*
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index 9eab09ff2044..40ccf99adc94 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -8,6 +8,8 @@
>  #include <xen/sizes.h>
>  #include <xen/types.h>
>  #include <asm/mpu.h>
> +#include <asm/mpu/mm.h>
> +#include <asm/sysregs.h>
>  
>  struct page_info *frame_table;
>  
> @@ -26,6 +28,35 @@ DECLARE_BITMAP(xen_mpumap_mask, MAX_MPU_REGION_NR) \
>  /* EL2 Xen MPU memory region mapping table. */
>  pr_t __section(".data.page_aligned") xen_mpumap[MAX_MPU_REGION_NR];
>  
> +#ifdef CONFIG_ARM_64
> +/*
> + * The following are needed for the case generators GENERATE_WRITE_PR_REG_CASE
It's read a bit odd. Perhaps remove 'generators' word and use 'cases:'

> + * and GENERATE_READ_PR_REG_CASE with num==0
> + */
> +#define PRBAR0_EL2 PRBAR_EL2
> +#define PRLAR0_EL2 PRLAR_EL2
> +
> +#define PRBAR_EL2_(n)   PRBAR##n##_EL2
> +#define PRLAR_EL2_(n)   PRLAR##n##_EL2
> +
> +#endif
> +
> +#define GENERATE_WRITE_PR_REG_CASE(num, pr)                                 \
> +    case num:                                                               \
> +    {                                                                       \
> +        WRITE_SYSREG(pr->prbar.bits & ~MPU_REGION_RES0, PRBAR_EL2_(num));   \
> +        WRITE_SYSREG(pr->prlar.bits & ~MPU_REGION_RES0, PRLAR_EL2_(num));   \
> +        break;                                                              \
> +    }
> +
> +#define GENERATE_READ_PR_REG_CASE(num, pr)                      \
> +    case num:                                                   \
> +    {                                                           \
> +        pr->prbar.bits = READ_SYSREG(PRBAR_EL2_(num));          \
> +        pr->prlar.bits = READ_SYSREG(PRLAR_EL2_(num));          \
> +        break;                                                  \
> +    }
> +
>  static void __init __maybe_unused build_assertions(void)
>  {
>      /*
> @@ -36,6 +67,92 @@ static void __init __maybe_unused build_assertions(void)
>      BUILD_BUG_ON(PAGE_SIZE != SZ_4K);
>  }
>  
> +#ifdef CONFIG_ARM_64
> +/*
> + * Armv8-R supports direct access and indirect access to the MPU regions through
> + * registers, indirect access involves changing the MPU region selector, issuing
s/registers,/registers:/ and perhaps use bullet points

> + * an isb barrier and accessing the selected region through specific registers;
> + * instead direct access involves accessing specific registers that points to
> + * a specific MPU region, without changing the selector (in some cases) and
What do you mean by "in some cases"?

> + * issuing barriers because of that.
> + * For Arm64 the PR{B,L}AR_ELx (for n=0) and PR{B,L}AR<n>_ELx, n=1..15, are used
If for n==0 you used (), why not following the same style for 1..15?
It all improves readability of such long comments.

> + * for the direct access to the regions selected by PRSELR_EL2.REGION<7:4>:n, so
> + * 16 regions can be directly access when the selector is multiple of 16, giving
s/access/accessed/
s/is multiple/is a multiple/

> + * access to all the supported memory regions.
> + */
> +static void prepare_selector(uint8_t *sel)
> +{
> +    uint8_t cur_sel = *sel;
> +
> +    /*
> +     * {read,write}_protection_region works using the direct access to the 0..15
> +     * regions, so in order to save the isb() overhead, change the PRSELR_EL2
> +     * only when needed, so when the upper 4 bits of the selector will change.
> +     */
> +    cur_sel &= 0xF0U;
> +    if ( READ_SYSREG(PRSELR_EL2) != cur_sel )
> +    {
> +        WRITE_SYSREG(cur_sel, PRSELR_EL2);
> +        isb();
> +    }
> +    *sel = *sel & 0xFU;
> +}
> +
> +void read_protection_region(pr_t *pr_read, uint8_t sel)
> +{
> +    prepare_selector(&sel);
> +
> +    switch ( sel )
> +    {
> +        GENERATE_READ_PR_REG_CASE(0, pr_read);
> +        GENERATE_READ_PR_REG_CASE(1, pr_read);
> +        GENERATE_READ_PR_REG_CASE(2, pr_read);
> +        GENERATE_READ_PR_REG_CASE(3, pr_read);
> +        GENERATE_READ_PR_REG_CASE(4, pr_read);
> +        GENERATE_READ_PR_REG_CASE(5, pr_read);
> +        GENERATE_READ_PR_REG_CASE(6, pr_read);
> +        GENERATE_READ_PR_REG_CASE(7, pr_read);
> +        GENERATE_READ_PR_REG_CASE(8, pr_read);
> +        GENERATE_READ_PR_REG_CASE(9, pr_read);
> +        GENERATE_READ_PR_REG_CASE(10, pr_read);
> +        GENERATE_READ_PR_REG_CASE(11, pr_read);
> +        GENERATE_READ_PR_REG_CASE(12, pr_read);
> +        GENERATE_READ_PR_REG_CASE(13, pr_read);
> +        GENERATE_READ_PR_REG_CASE(14, pr_read);
> +        GENERATE_READ_PR_REG_CASE(15, pr_read);
> +    default:
> +        BUG(); /* Can't happen */
> +    }
> +}
> +
> +void write_protection_region(const pr_t *pr_write, uint8_t sel)
> +{
> +    prepare_selector(&sel);
> +
> +    switch ( sel )
> +    {
> +        GENERATE_WRITE_PR_REG_CASE(0, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(1, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(2, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(3, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(4, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(5, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(6, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(7, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(8, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(9, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(10, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(11, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(12, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(13, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(14, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(15, pr_write);
> +    default:
> +        BUG(); /* Can't happen */
> +    }
> +}
> +#endif
> +
>  void __init setup_mm(void)
>  {
>      BUG_ON("unimplemented");

~Michal

Re: [PATCH v4 4/7] arm/mpu: Provide access to the MPU region from the C code

[Luca Fancellu]

Hi Michal,

>> 
>> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
>> index 1368b2eb990f..40a86140b6cc 100644
>> --- a/xen/arch/arm/include/asm/mpu.h
>> +++ b/xen/arch/arm/include/asm/mpu.h
>> @@ -17,6 +17,7 @@
>> #define MPU_REGION_SHIFT  6
>> #define MPU_REGION_ALIGN  (_AC(1, UL) << MPU_REGION_SHIFT)
>> #define MPU_REGION_MASK   (~(MPU_REGION_ALIGN - 1))
>> +#define MPU_REGION_RES0   (0xFFFFULL << 48)
> This does not look like a common macro. It's arm64 specific.
> Also, it looks like you use it in macros that are common too.

Yes right, I’ll move that into asm/arm64/mpu.h

>> 
>> +/*
>> + * Reads the MPU region with index 'sel' from the HW.
> If you use @foo style, you should use @sel here.
> But IMO this comment does not bring any usefulness.
> The name of the helper and parameter description is enough.
> 
>> + *
>> + * @pr_read: mpu protection region returned by read op.
>> + * @sel: mpu protection region selector
>> + */
>> +extern void read_protection_region(pr_t *pr_read, uint8_t sel);
>> +
>> +/*
>> + * Writes the MPU region with index 'sel' to the HW.
>> + *
>> + * @pr_write: const mpu protection region passed through write op.
> No need to say const in parameter description
> 
>> + * @sel: mpu protection region selector
> Same here.

I’ll modify these above

>> 
>> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
>> index 9eab09ff2044..40ccf99adc94 100644
>> --- a/xen/arch/arm/mpu/mm.c
>> +++ b/xen/arch/arm/mpu/mm.c
>> @@ -8,6 +8,8 @@
>> #include <xen/sizes.h>
>> #include <xen/types.h>
>> #include <asm/mpu.h>
>> +#include <asm/mpu/mm.h>
>> +#include <asm/sysregs.h>
>> 
>> struct page_info *frame_table;
>> 
>> @@ -26,6 +28,35 @@ DECLARE_BITMAP(xen_mpumap_mask, MAX_MPU_REGION_NR) \
>> /* EL2 Xen MPU memory region mapping table. */
>> pr_t __section(".data.page_aligned") xen_mpumap[MAX_MPU_REGION_NR];
>> 
>> +#ifdef CONFIG_ARM_64
>> +/*
>> + * The following are needed for the case generators GENERATE_WRITE_PR_REG_CASE
> It's read a bit odd. Perhaps remove 'generators' word and use 'cases:'

ok

>> 
>> 
>> +#ifdef CONFIG_ARM_64
>> +/*
>> + * Armv8-R supports direct access and indirect access to the MPU regions through
>> + * registers, indirect access involves changing the MPU region selector, issuing
> s/registers,/registers:/ and perhaps use bullet points
> 
>> + * an isb barrier and accessing the selected region through specific registers;
>> + * instead direct access involves accessing specific registers that points to
>> + * a specific MPU region, without changing the selector (in some cases) and
> What do you mean by "in some cases"?

what I had in mind was that eventually you’ll need to change the selector at some point,
like arm64 every 16 regions or on arm32 from region 32 onwards, but maybe I can simplify
and remove this part.

> 
>> + * issuing barriers because of that.
>> + * For Arm64 the PR{B,L}AR_ELx (for n=0) and PR{B,L}AR<n>_ELx, n=1..15, are used
> If for n==0 you used (), why not following the same style for 1..15?
> It all improves readability of such long comments.
> 
>> + * for the direct access to the regions selected by PRSELR_EL2.REGION<7:4>:n, so
>> + * 16 regions can be directly access when the selector is multiple of 16, giving
> s/access/accessed/
> s/is multiple/is a multiple/

ok all the above

Cheers,
Luca

[PATCH v4 5/7] arm/mpu: Introduce utility functions for the pr_t type

[Luca Fancellu]

Introduce few utility function to manipulate and handle the
pr_t type.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v4 changes:
 - Modify comment on top of the helpers. Clarify pr_set_limit
   takes exclusive address.
   Protected common code with #ifdef Arm64 until Arm32 is ready
   with pr_t
---
 xen/arch/arm/include/asm/mpu.h | 64 ++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
index 40a86140b6cc..0e0a7f05ade9 100644
--- a/xen/arch/arm/include/asm/mpu.h
+++ b/xen/arch/arm/include/asm/mpu.h
@@ -24,6 +24,70 @@
 #define NUM_MPU_REGIONS_MASK    (NUM_MPU_REGIONS - 1)
 #define MAX_MPU_REGION_NR       255
 
+#ifndef __ASSEMBLY__
+
+#ifdef CONFIG_ARM_64
+/*
+ * Set base address of MPU protection region.
+ *
+ * @pr: pointer to the protection region structure.
+ * @base: base address as base of the protection region.
+ */
+static inline void pr_set_base(pr_t *pr, paddr_t base)
+{
+    pr->prbar.reg.base = (base >> MPU_REGION_SHIFT);
+}
+
+/*
+ * Set limit address of MPU protection region.
+ *
+ * @pr: pointer to the protection region structure.
+ * @limit: exclusive address as limit of the protection region.
+ */
+static inline void pr_set_limit(pr_t *pr, paddr_t limit)
+{
+    pr->prlar.reg.limit = ((limit - 1) >> MPU_REGION_SHIFT);
+}
+
+/*
+ * Access to get base address of MPU protection region.
+ * The base address shall be zero extended.
+ *
+ * @pr: pointer to the protection region structure.
+ * @return: Base address configured for the passed protection region.
+ */
+static inline paddr_t pr_get_base(pr_t *pr)
+{
+    return (paddr_t)(pr->prbar.reg.base << MPU_REGION_SHIFT);
+}
+
+/*
+ * Access to get limit address of MPU protection region.
+ * The limit address shall be concatenated with 0x3f.
+ *
+ * @pr: pointer to the protection region structure.
+ * @return: Inclusive limit address configured for the passed protection region.
+ */
+static inline paddr_t pr_get_limit(pr_t *pr)
+{
+    return (paddr_t)((pr->prlar.reg.limit << MPU_REGION_SHIFT)
+                     | ~MPU_REGION_MASK);
+}
+
+/*
+ * Checks if the protection region is valid (enabled).
+ *
+ * @pr: pointer to the protection region structure.
+ * @return: True if the region is valid (enabled), false otherwise.
+ */
+static inline bool region_is_valid(pr_t *pr)
+{
+    return pr->prlar.reg.en;
+}
+#endif
+
+#endif /* __ASSEMBLY__ */
+
 #endif /* __ARM_MPU_H__ */
 
 /*
-- 
2.34.1

Re: [PATCH v4 5/7] arm/mpu: Introduce utility functions for the pr_t type

[Orzel, Michal]

On 29/04/2025 17:20, Luca Fancellu wrote:
> Introduce few utility function to manipulate and handle the
s/few/a few/
s/function/functions/

> pr_t type.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v4 changes:
>  - Modify comment on top of the helpers. Clarify pr_set_limit
>    takes exclusive address.
>    Protected common code with #ifdef Arm64 until Arm32 is ready
>    with pr_t
> ---
>  xen/arch/arm/include/asm/mpu.h | 64 ++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
> index 40a86140b6cc..0e0a7f05ade9 100644
> --- a/xen/arch/arm/include/asm/mpu.h
> +++ b/xen/arch/arm/include/asm/mpu.h
> @@ -24,6 +24,70 @@
>  #define NUM_MPU_REGIONS_MASK    (NUM_MPU_REGIONS - 1)
>  #define MAX_MPU_REGION_NR       255
>  
> +#ifndef __ASSEMBLY__
> +
> +#ifdef CONFIG_ARM_64
> +/*
> + * Set base address of MPU protection region.
> + *
> + * @pr: pointer to the protection region structure.
> + * @base: base address as base of the protection region.
> + */
> +static inline void pr_set_base(pr_t *pr, paddr_t base)
> +{
> +    pr->prbar.reg.base = (base >> MPU_REGION_SHIFT);
Shouldn't you take MPU_REGION_RES0 into account?

~Michal

Re: [PATCH v4 5/7] arm/mpu: Introduce utility functions for the pr_t type

[Luca Fancellu]

Hi Michal,

> On 5 May 2025, at 13:08, Orzel, Michal <Michal.Orzel@amd.com> wrote:
> 
> 
> 
> On 29/04/2025 17:20, Luca Fancellu wrote:
>> Introduce few utility function to manipulate and handle the
> s/few/a few/
> s/function/functions/

Ok

>> 
>> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
>> index 40a86140b6cc..0e0a7f05ade9 100644
>> --- a/xen/arch/arm/include/asm/mpu.h
>> +++ b/xen/arch/arm/include/asm/mpu.h
>> @@ -24,6 +24,70 @@
>> #define NUM_MPU_REGIONS_MASK    (NUM_MPU_REGIONS - 1)
>> #define MAX_MPU_REGION_NR       255
>> 
>> +#ifndef __ASSEMBLY__
>> +
>> +#ifdef CONFIG_ARM_64
>> +/*
>> + * Set base address of MPU protection region.
>> + *
>> + * @pr: pointer to the protection region structure.
>> + * @base: base address as base of the protection region.
>> + */
>> +static inline void pr_set_base(pr_t *pr, paddr_t base)
>> +{
>> +    pr->prbar.reg.base = (base >> MPU_REGION_SHIFT);
> Shouldn't you take MPU_REGION_RES0 into account?

Yes indeed, I’ll fix

Cheers,
Luca

[PATCH v4 6/7] arm/mpu: Provide a constructor for pr_t type

[Luca Fancellu]

Provide a function that creates a pr_t object from a memory
range and some attributes.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v4 changes:
 - update helper comments
 - rename XN_EL2_ENABLED to PRBAR_EL2_XN_ENABLED
 - protected pr_of_xenaddr() with #ifdef Arm64 until Arm32
   can build with it
---
 xen/arch/arm/include/asm/arm64/mpu.h | 11 +++++
 xen/arch/arm/include/asm/mpu.h       |  4 ++
 xen/arch/arm/include/asm/mpu/mm.h    | 10 ++++
 xen/arch/arm/mpu/mm.c                | 68 ++++++++++++++++++++++++++++
 4 files changed, 93 insertions(+)

diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/arm64/mpu.h
index b27fccd77550..39233b43a5aa 100644
--- a/xen/arch/arm/include/asm/arm64/mpu.h
+++ b/xen/arch/arm/include/asm/arm64/mpu.h
@@ -3,6 +3,17 @@
 #ifndef __ARM_ARM64_MPU_H__
 #define __ARM_ARM64_MPU_H__
 
+/*
+ * Excute never.
+ * Stage 1 EL2 translation regime.
+ * XN[1] determines whether execution of the instruction fetched from the MPU
+ * memory region is permitted.
+ * Stage 2 EL1/EL0 translation regime.
+ * XN[0] determines whether execution of the instruction fetched from the MPU
+ * memory region is permitted.
+ */
+#define PRBAR_EL2_XN_ENABLED  0x2
+
 #ifndef __ASSEMBLY__
 
 /* Protection Region Base Address Register */
diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
index 0e0a7f05ade9..7b82f10d336b 100644
--- a/xen/arch/arm/include/asm/mpu.h
+++ b/xen/arch/arm/include/asm/mpu.h
@@ -24,6 +24,10 @@
 #define NUM_MPU_REGIONS_MASK    (NUM_MPU_REGIONS - 1)
 #define MAX_MPU_REGION_NR       255
 
+/* Access permission attributes. */
+/* Read/Write at EL2, No Access at EL1/EL0. */
+#define AP_RW_EL2 0x0
+
 #ifndef __ASSEMBLY__
 
 #ifdef CONFIG_ARM_64
diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
index e2235e568e81..296fe74c5d61 100644
--- a/xen/arch/arm/include/asm/mpu/mm.h
+++ b/xen/arch/arm/include/asm/mpu/mm.h
@@ -75,6 +75,16 @@ extern void read_protection_region(pr_t *pr_read, uint8_t sel);
  */
 extern void write_protection_region(const pr_t *pr_write, uint8_t sel);
 
+/*
+ * Creates a pr_t structure describing a protection region.
+ *
+ * @base: base address as base of the protection region.
+ * @limit: exclusive address as limit of the protection region.
+ * @attr: attribute index for the memory type.
+ * @return: pr_t structure describing a protection region.
+ */
+extern pr_t pr_of_xenaddr(paddr_t base, paddr_t limit, unsigned int attr_idx);
+
 #endif /* __ARM_MPU_MM_H__ */
 
 /*
diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index 40ccf99adc94..2e0aeb486ff8 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -9,6 +9,7 @@
 #include <xen/types.h>
 #include <asm/mpu.h>
 #include <asm/mpu/mm.h>
+#include <asm/page.h>
 #include <asm/sysregs.h>
 
 struct page_info *frame_table;
@@ -151,6 +152,73 @@ void write_protection_region(const pr_t *pr_write, uint8_t sel)
         BUG(); /* Can't happen */
     }
 }
+
+pr_t pr_of_xenaddr(paddr_t base, paddr_t limit, unsigned int attr_idx)
+{
+    prbar_t prbar;
+    prlar_t prlar;
+    pr_t region;
+
+    /* Build up value for PRBAR_EL2. */
+    prbar = (prbar_t) {
+        .reg = {
+            .ap = AP_RW_EL2,      /* Read/Write at EL2, no access at EL1/EL0. */
+            .xn = PRBAR_EL2_XN_ENABLED,   /* No need to execute outside .text */
+        }};
+
+    switch ( attr_idx )
+    {
+    case MT_NORMAL_NC:
+        /*
+         * ARM ARM: Overlaying the shareability attribute (DDI
+         * 0406C.b B3-1376 to 1377)
+         *
+         * A memory region with a resultant memory type attribute of normal,
+         * and a resultant cacheability attribute of Inner non-cacheable,
+         * outer non-cacheable, must have a resultant shareability attribute
+         * of outer shareable, otherwise shareability is UNPREDICTABLE.
+         *
+         * On ARMv8 sharability is ignored and explicitly treated as outer
+         * shareable for normal inner non-cacheable, outer non-cacheable.
+         */
+        prbar.reg.sh = LPAE_SH_OUTER;
+        break;
+    case MT_DEVICE_nGnRnE:
+    case MT_DEVICE_nGnRE:
+        /*
+         * Shareability is ignored for non-normal memory, Outer is as
+         * good as anything.
+         *
+         * On ARMv8 sharability is ignored and explicitly treated as outer
+         * shareable for any device memory type.
+         */
+        prbar.reg.sh = LPAE_SH_OUTER;
+        break;
+    default:
+        /* Xen mappings are SMP coherent */
+        prbar.reg.sh = LPAE_SH_INNER;
+    }
+
+    /* Build up value for PRLAR_EL2. */
+    prlar = (prlar_t) {
+        .reg = {
+            .ns = 0,        /* Hyp mode is in secure world */
+            .ai = attr_idx,
+            .en = 1,        /* Region enabled */
+        }};
+
+    /* Build up MPU memory region. */
+    region = (pr_t) {
+        .prbar = prbar,
+        .prlar = prlar,
+    };
+
+    /* Set base address and limit address. */
+    pr_set_base(&region, base);
+    pr_set_limit(&region, limit);
+
+    return region;
+}
 #endif
 
 void __init setup_mm(void)
-- 
2.34.1

Re: [PATCH v4 6/7] arm/mpu: Provide a constructor for pr_t type

[Ayan Kumar Halder]

Hi Luca,

On 29/04/2025 16:20, Luca Fancellu wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> Provide a function that creates a pr_t object from a memory
> range and some attributes.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v4 changes:
>   - update helper comments
>   - rename XN_EL2_ENABLED to PRBAR_EL2_XN_ENABLED
>   - protected pr_of_xenaddr() with #ifdef Arm64 until Arm32
>     can build with it
> ---
>   xen/arch/arm/include/asm/arm64/mpu.h | 11 +++++
>   xen/arch/arm/include/asm/mpu.h       |  4 ++
>   xen/arch/arm/include/asm/mpu/mm.h    | 10 ++++
>   xen/arch/arm/mpu/mm.c                | 68 ++++++++++++++++++++++++++++
>   4 files changed, 93 insertions(+)
>
> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/arm64/mpu.h
> index b27fccd77550..39233b43a5aa 100644
> --- a/xen/arch/arm/include/asm/arm64/mpu.h
> +++ b/xen/arch/arm/include/asm/arm64/mpu.h
> @@ -3,6 +3,17 @@
>   #ifndef __ARM_ARM64_MPU_H__
>   #define __ARM_ARM64_MPU_H__
>
> +/*
> + * Excute never.
> + * Stage 1 EL2 translation regime.
> + * XN[1] determines whether execution of the instruction fetched from the MPU
> + * memory region is permitted.
> + * Stage 2 EL1/EL0 translation regime.
> + * XN[0] determines whether execution of the instruction fetched from the MPU
> + * memory region is permitted.
> + */
> +#define PRBAR_EL2_XN_ENABLED  0x2
> +
>   #ifndef __ASSEMBLY__
>
>   /* Protection Region Base Address Register */
> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
> index 0e0a7f05ade9..7b82f10d336b 100644
> --- a/xen/arch/arm/include/asm/mpu.h
> +++ b/xen/arch/arm/include/asm/mpu.h
> @@ -24,6 +24,10 @@
>   #define NUM_MPU_REGIONS_MASK    (NUM_MPU_REGIONS - 1)
>   #define MAX_MPU_REGION_NR       255
>
> +/* Access permission attributes. */
> +/* Read/Write at EL2, No Access at EL1/EL0. */
> +#define AP_RW_EL2 0x0
> +
>   #ifndef __ASSEMBLY__
>
>   #ifdef CONFIG_ARM_64
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
> index e2235e568e81..296fe74c5d61 100644
> --- a/xen/arch/arm/include/asm/mpu/mm.h
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -75,6 +75,16 @@ extern void read_protection_region(pr_t *pr_read, uint8_t sel);
>    */
>   extern void write_protection_region(const pr_t *pr_write, uint8_t sel);
>
> +/*
> + * Creates a pr_t structure describing a protection region.
> + *
> + * @base: base address as base of the protection region.
> + * @limit: exclusive address as limit of the protection region.
> + * @attr: attribute index for the memory type.
> + * @return: pr_t structure describing a protection region.
> + */
> +extern pr_t pr_of_xenaddr(paddr_t base, paddr_t limit, unsigned int attr_idx);
> +
>   #endif /* __ARM_MPU_MM_H__ */
>
>   /*
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index 40ccf99adc94..2e0aeb486ff8 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -9,6 +9,7 @@
>   #include <xen/types.h>
>   #include <asm/mpu.h>
>   #include <asm/mpu/mm.h>
> +#include <asm/page.h>
>   #include <asm/sysregs.h>
>
>   struct page_info *frame_table;
> @@ -151,6 +152,73 @@ void write_protection_region(const pr_t *pr_write, uint8_t sel)
>           BUG(); /* Can't happen */
>       }
>   }
> +
> +pr_t pr_of_xenaddr(paddr_t base, paddr_t limit, unsigned int attr_idx)
> +{
> +    prbar_t prbar;
> +    prlar_t prlar;
> +    pr_t region;
> +
> +    /* Build up value for PRBAR_EL2. */
> +    prbar = (prbar_t) {
> +        .reg = {
> +            .ap = AP_RW_EL2,      /* Read/Write at EL2, no access at EL1/EL0. */
> +            .xn = PRBAR_EL2_XN_ENABLED,   /* No need to execute outside .text */
> +        }};
> +
> +    switch ( attr_idx )
> +    {
> +    case MT_NORMAL_NC:
> +        /*
> +         * ARM ARM: Overlaying the shareability attribute (DDI
> +         * 0406C.b B3-1376 to 1377)
> +         *
> +         * A memory region with a resultant memory type attribute of normal,
> +         * and a resultant cacheability attribute of Inner non-cacheable,
> +         * outer non-cacheable, must have a resultant shareability attribute
> +         * of outer shareable, otherwise shareability is UNPREDICTABLE.
> +         *
> +         * On ARMv8 sharability is ignored and explicitly treated as outer
> +         * shareable for normal inner non-cacheable, outer non-cacheable.
> +         */
> +        prbar.reg.sh = LPAE_SH_OUTER;
> +        break;
> +    case MT_DEVICE_nGnRnE:
> +    case MT_DEVICE_nGnRE:
> +        /*
> +         * Shareability is ignored for non-normal memory, Outer is as
> +         * good as anything.
> +         *
> +         * On ARMv8 sharability is ignored and explicitly treated as outer
> +         * shareable for any device memory type.
> +         */
> +        prbar.reg.sh = LPAE_SH_OUTER;
> +        break;
> +    default:
> +        /* Xen mappings are SMP coherent */
> +        prbar.reg.sh = LPAE_SH_INNER;
> +    }
> +
> +    /* Build up value for PRLAR_EL2. */
> +    prlar = (prlar_t) {
> +        .reg = {
#ifdef CONFIG_ARM_64
> +            .ns = 0,        /* Hyp mode is in secure world */
#endif
> +            .ai = attr_idx,
> +            .en = 1,        /* Region enabled */
> +        }};
> +
> +    /* Build up MPU memory region. */
> +    region = (pr_t) {
> +        .prbar = prbar,
> +        .prlar = prlar,
> +    };
> +
> +    /* Set base address and limit address. */
> +    pr_set_base(&region, base);
> +    pr_set_limit(&region, limit);
> +
> +    return region;
> +}
>   #endif
>
>   void __init setup_mm(void)
> --
> 2.34.1
>
>
- Ayan

Re: [PATCH v4 6/7] arm/mpu: Provide a constructor for pr_t type

[Luca Fancellu]

Hi Ayan,

>> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
>> index 40ccf99adc94..2e0aeb486ff8 100644
>> --- a/xen/arch/arm/mpu/mm.c
>> +++ b/xen/arch/arm/mpu/mm.c
>> @@ -9,6 +9,7 @@
>>  #include <xen/types.h>
>>  #include <asm/mpu.h>
>>  #include <asm/mpu/mm.h>
>> +#include <asm/page.h>
>>  #include <asm/sysregs.h>
>> 
>>  struct page_info *frame_table;
>> @@ -151,6 +152,73 @@ void write_protection_region(const pr_t *pr_write, uint8_t sel)
>>          BUG(); /* Can't happen */
>>      }
>>  }
>> +
>> +pr_t pr_of_xenaddr(paddr_t base, paddr_t limit, unsigned int attr_idx)
>> +{
>> +    prbar_t prbar;
>> +    prlar_t prlar;
>> +    pr_t region;
>> +
>> +    /* Build up value for PRBAR_EL2. */
>> +    prbar = (prbar_t) {
>> +        .reg = {
>> +            .ap = AP_RW_EL2,      /* Read/Write at EL2, no access at EL1/EL0. */
>> +            .xn = PRBAR_EL2_XN_ENABLED,   /* No need to execute outside .text */
>> +        }};
>> +
>> +    switch ( attr_idx )
>> +    {
>> +    case MT_NORMAL_NC:
>> +        /*
>> +         * ARM ARM: Overlaying the shareability attribute (DDI
>> +         * 0406C.b B3-1376 to 1377)
>> +         *
>> +         * A memory region with a resultant memory type attribute of normal,
>> +         * and a resultant cacheability attribute of Inner non-cacheable,
>> +         * outer non-cacheable, must have a resultant shareability attribute
>> +         * of outer shareable, otherwise shareability is UNPREDICTABLE.
>> +         *
>> +         * On ARMv8 sharability is ignored and explicitly treated as outer
>> +         * shareable for normal inner non-cacheable, outer non-cacheable.
>> +         */
>> +        prbar.reg.sh = LPAE_SH_OUTER;
>> +        break;
>> +    case MT_DEVICE_nGnRnE:
>> +    case MT_DEVICE_nGnRE:
>> +        /*
>> +         * Shareability is ignored for non-normal memory, Outer is as
>> +         * good as anything.
>> +         *
>> +         * On ARMv8 sharability is ignored and explicitly treated as outer
>> +         * shareable for any device memory type.
>> +         */
>> +        prbar.reg.sh = LPAE_SH_OUTER;
>> +        break;
>> +    default:
>> +        /* Xen mappings are SMP coherent */
>> +        prbar.reg.sh = LPAE_SH_INNER;
>> +    }
>> +
>> +    /* Build up value for PRLAR_EL2. */
>> +    prlar = (prlar_t) {
>> +        .reg = {
> #ifdef CONFIG_ARM_64
>> +            .ns = 0,        /* Hyp mode is in secure world */
> #endif

yes this part will be introduced by you together with the pr_t definition,
in this patch the overall pr_of_xenaddr is protected by CONFIG_ARM_64

Cheers,
Luca

Re: [PATCH v4 6/7] arm/mpu: Provide a constructor for pr_t type

[Orzel, Michal]

On 29/04/2025 17:20, Luca Fancellu wrote:
> Provide a function that creates a pr_t object from a memory
> range and some attributes.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v4 changes:
>  - update helper comments
>  - rename XN_EL2_ENABLED to PRBAR_EL2_XN_ENABLED
>  - protected pr_of_xenaddr() with #ifdef Arm64 until Arm32
>    can build with it
> ---
>  xen/arch/arm/include/asm/arm64/mpu.h | 11 +++++
>  xen/arch/arm/include/asm/mpu.h       |  4 ++
>  xen/arch/arm/include/asm/mpu/mm.h    | 10 ++++
>  xen/arch/arm/mpu/mm.c                | 68 ++++++++++++++++++++++++++++
>  4 files changed, 93 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/arm64/mpu.h
> index b27fccd77550..39233b43a5aa 100644
> --- a/xen/arch/arm/include/asm/arm64/mpu.h
> +++ b/xen/arch/arm/include/asm/arm64/mpu.h
> @@ -3,6 +3,17 @@
>  #ifndef __ARM_ARM64_MPU_H__
>  #define __ARM_ARM64_MPU_H__
>  
> +/*
> + * Excute never.
> + * Stage 1 EL2 translation regime.
> + * XN[1] determines whether execution of the instruction fetched from the MPU
> + * memory region is permitted.
> + * Stage 2 EL1/EL0 translation regime.
> + * XN[0] determines whether execution of the instruction fetched from the MPU
> + * memory region is permitted.
> + */
Why do we need this comment? If any, why do we need EL1 description if the macro
is EL2?

> +#define PRBAR_EL2_XN_ENABLED  0x2
> +
>  #ifndef __ASSEMBLY__
>  
>  /* Protection Region Base Address Register */
> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
> index 0e0a7f05ade9..7b82f10d336b 100644
> --- a/xen/arch/arm/include/asm/mpu.h
> +++ b/xen/arch/arm/include/asm/mpu.h
> @@ -24,6 +24,10 @@
>  #define NUM_MPU_REGIONS_MASK    (NUM_MPU_REGIONS - 1)
>  #define MAX_MPU_REGION_NR       255
>  
> +/* Access permission attributes. */
> +/* Read/Write at EL2, No Access at EL1/EL0. */
> +#define AP_RW_EL2 0x0
This macro and the previous one are used only once (I also checked your full
tree) and cannot be set by the caller. What's the purpose of the macros then?
Why can't we set these values in pr_of_xenaddr() and add comment next to value
there?

> +
>  #ifndef __ASSEMBLY__
>  
>  #ifdef CONFIG_ARM_64
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
> index e2235e568e81..296fe74c5d61 100644
> --- a/xen/arch/arm/include/asm/mpu/mm.h
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -75,6 +75,16 @@ extern void read_protection_region(pr_t *pr_read, uint8_t sel);
>   */
>  extern void write_protection_region(const pr_t *pr_write, uint8_t sel);
Here and ...

>  
> +/*
> + * Creates a pr_t structure describing a protection region.
> + *
> + * @base: base address as base of the protection region.
> + * @limit: exclusive address as limit of the protection region.
> + * @attr: attribute index for the memory type.
> + * @return: pr_t structure describing a protection region.
> + */
> +extern pr_t pr_of_xenaddr(paddr_t base, paddr_t limit, unsigned int attr_idx);
here. Please don't use extern in prototypes. It's not needed.

> +
>  #endif /* __ARM_MPU_MM_H__ */
>  
>  /*
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index 40ccf99adc94..2e0aeb486ff8 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -9,6 +9,7 @@
>  #include <xen/types.h>
>  #include <asm/mpu.h>
>  #include <asm/mpu/mm.h>
> +#include <asm/page.h>
>  #include <asm/sysregs.h>
>  
>  struct page_info *frame_table;
> @@ -151,6 +152,73 @@ void write_protection_region(const pr_t *pr_write, uint8_t sel)
>          BUG(); /* Can't happen */
>      }
>  }
> +
> +pr_t pr_of_xenaddr(paddr_t base, paddr_t limit, unsigned int attr_idx)
> +{
> +    prbar_t prbar;
> +    prlar_t prlar;
> +    pr_t region;
> +
> +    /* Build up value for PRBAR_EL2. */
> +    prbar = (prbar_t) {
> +        .reg = {
> +            .ap = AP_RW_EL2,      /* Read/Write at EL2, no access at EL1/EL0. */
> +            .xn = PRBAR_EL2_XN_ENABLED,   /* No need to execute outside .text */
> +        }};
> +
> +    switch ( attr_idx )
> +    {
> +    case MT_NORMAL_NC:
> +        /*
> +         * ARM ARM: Overlaying the shareability attribute (DDI
> +         * 0406C.b B3-1376 to 1377)
It's a bit odd to provide here the manual for Armv7.
Also, our general advice is to use the latest revision.

> +         *
> +         * A memory region with a resultant memory type attribute of normal,
> +         * and a resultant cacheability attribute of Inner non-cacheable,
> +         * outer non-cacheable, must have a resultant shareability attribute
> +         * of outer shareable, otherwise shareability is UNPREDICTABLE.
> +         *
> +         * On ARMv8 sharability is ignored and explicitly treated as outer
> +         * shareable for normal inner non-cacheable, outer non-cacheable.
> +         */
> +        prbar.reg.sh = LPAE_SH_OUTER;
> +        break;
> +    case MT_DEVICE_nGnRnE:
> +    case MT_DEVICE_nGnRE:
> +        /*
> +         * Shareability is ignored for non-normal memory, Outer is as
> +         * good as anything.
> +         *
> +         * On ARMv8 sharability is ignored and explicitly treated as outer
Does this Armv8 comments make sense? We don't support Armv7 MPU.

> +         * shareable for any device memory type.
> +         */
> +        prbar.reg.sh = LPAE_SH_OUTER;
> +        break;
> +    default:
> +        /* Xen mappings are SMP coherent */
> +        prbar.reg.sh = LPAE_SH_INNER;
AFAIR MISRA C requires every clause to be terminated with break.
That said I don't remember if we accepted this rule...

> +    }
> +
> +    /* Build up value for PRLAR_EL2. */
> +    prlar = (prlar_t) {
> +        .reg = {
> +            .ns = 0,        /* Hyp mode is in secure world */
> +            .ai = attr_idx,
> +            .en = 1,        /* Region enabled */
> +        }};
> +
> +    /* Build up MPU memory region. */
> +    region = (pr_t) {
> +        .prbar = prbar,
> +        .prlar = prlar,
> +    };
> +
> +    /* Set base address and limit address. */
> +    pr_set_base(&region, base);
> +    pr_set_limit(&region, limit);
> +
> +    return region;
> +}
>  #endif
>  
>  void __init setup_mm(void)

~Michal

Re: [PATCH v4 6/7] arm/mpu: Provide a constructor for pr_t type

[Luca Fancellu]

Hi Michal,

>> 
>> +/*
>> + * Excute never.
>> + * Stage 1 EL2 translation regime.
>> + * XN[1] determines whether execution of the instruction fetched from the MPU
>> + * memory region is permitted.
>> + * Stage 2 EL1/EL0 translation regime.
>> + * XN[0] determines whether execution of the instruction fetched from the MPU
>> + * memory region is permitted.
>> + */
> Why do we need this comment? If any, why do we need EL1 description if the macro
> is EL2?

Uhm yes I think I can remove altogether this comment

> 
>> +#define PRBAR_EL2_XN_ENABLED  0x2
>> +
>> #ifndef __ASSEMBLY__
>> 
>> /* Protection Region Base Address Register */
>> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
>> index 0e0a7f05ade9..7b82f10d336b 100644
>> --- a/xen/arch/arm/include/asm/mpu.h
>> +++ b/xen/arch/arm/include/asm/mpu.h
>> @@ -24,6 +24,10 @@
>> #define NUM_MPU_REGIONS_MASK    (NUM_MPU_REGIONS - 1)
>> #define MAX_MPU_REGION_NR       255
>> 
>> +/* Access permission attributes. */
>> +/* Read/Write at EL2, No Access at EL1/EL0. */
>> +#define AP_RW_EL2 0x0
> This macro and the previous one are used only once (I also checked your full
> tree) and cannot be set by the caller. What's the purpose of the macros then?
> Why can't we set these values in pr_of_xenaddr() and add comment next to value
> there?

Sure, I’ll do that

> 
>> +
>> #ifndef __ASSEMBLY__
>> 
>> #ifdef CONFIG_ARM_64
>> diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
>> index e2235e568e81..296fe74c5d61 100644
>> --- a/xen/arch/arm/include/asm/mpu/mm.h
>> +++ b/xen/arch/arm/include/asm/mpu/mm.h
>> @@ -75,6 +75,16 @@ extern void read_protection_region(pr_t *pr_read, uint8_t sel);
>>  */
>> extern void write_protection_region(const pr_t *pr_write, uint8_t sel);
> Here and ...
> 
>> 
>> +/*
>> + * Creates a pr_t structure describing a protection region.
>> + *
>> + * @base: base address as base of the protection region.
>> + * @limit: exclusive address as limit of the protection region.
>> + * @attr: attribute index for the memory type.
>> + * @return: pr_t structure describing a protection region.
>> + */
>> +extern pr_t pr_of_xenaddr(paddr_t base, paddr_t limit, unsigned int attr_idx);
> here. Please don't use extern in prototypes. It's not needed.

I see we have a mixed usage of this in arch/arm and it’s not documented to do otherwise
in the code style, in this case I would prefer to be explicit unless it’s a strong objection on your side,
let me know.


>> 
>> +pr_t pr_of_xenaddr(paddr_t base, paddr_t limit, unsigned int attr_idx)
>> +{
>> +    prbar_t prbar;
>> +    prlar_t prlar;
>> +    pr_t region;
>> +
>> +    /* Build up value for PRBAR_EL2. */
>> +    prbar = (prbar_t) {
>> +        .reg = {
>> +            .ap = AP_RW_EL2,      /* Read/Write at EL2, no access at EL1/EL0. */
>> +            .xn = PRBAR_EL2_XN_ENABLED,   /* No need to execute outside .text */
>> +        }};
>> +
>> +    switch ( attr_idx )
>> +    {
>> +    case MT_NORMAL_NC:
>> +        /*
>> +         * ARM ARM: Overlaying the shareability attribute (DDI
>> +         * 0406C.b B3-1376 to 1377)
> It's a bit odd to provide here the manual for Armv7.
> Also, our general advice is to use the latest revision.

I suspect this was copied from mfn_to_xen_entry, I’ll try to find the latest reference to this.

> 
>> +         *
>> +         * A memory region with a resultant memory type attribute of normal,
>> +         * and a resultant cacheability attribute of Inner non-cacheable,
>> +         * outer non-cacheable, must have a resultant shareability attribute
>> +         * of outer shareable, otherwise shareability is UNPREDICTABLE.
>> +         *
>> +         * On ARMv8 sharability is ignored and explicitly treated as outer
>> +         * shareable for normal inner non-cacheable, outer non-cacheable.
>> +         */
>> +        prbar.reg.sh = LPAE_SH_OUTER;
>> +        break;
>> +    case MT_DEVICE_nGnRnE:
>> +    case MT_DEVICE_nGnRE:
>> +        /*
>> +         * Shareability is ignored for non-normal memory, Outer is as
>> +         * good as anything.
>> +         *
>> +         * On ARMv8 sharability is ignored and explicitly treated as outer
> Does this Armv8 comments make sense? We don't support Armv7 MPU.
> 
>> +         * shareable for any device memory type.
>> +         */
>> +        prbar.reg.sh = LPAE_SH_OUTER;
>> +        break;
>> +    default:
>> +        /* Xen mappings are SMP coherent */
>> +        prbar.reg.sh = LPAE_SH_INNER;
> AFAIR MISRA C requires every clause to be terminated with break.
> That said I don't remember if we accepted this rule...

I’ll add a break, it doesn’t hurt.

Cheers,
Luca

Re: [PATCH v4 6/7] arm/mpu: Provide a constructor for pr_t type

[Julien Grall]

Hi Luca,

On 06/05/2025 13:56, Luca Fancellu wrote:
>>> +/*
>>> + * Creates a pr_t structure describing a protection region.
>>> + *
>>> + * @base: base address as base of the protection region.
>>> + * @limit: exclusive address as limit of the protection region.
>>> + * @attr: attribute index for the memory type.
>>> + * @return: pr_t structure describing a protection region.
>>> + */
>>> +extern pr_t pr_of_xenaddr(paddr_t base, paddr_t limit, unsigned int attr_idx);
>> here. Please don't use extern in prototypes. It's not needed.
> 
> I see we have a mixed usage of this in arch/arm and it’s not documented to do otherwise
> in the code style, in this case I would prefer to be explicit unless it’s a strong objection on your side,
> let me know.

Old Arm code is using "extern". But new code should avoid it for prototypes.

Cheers,

-- 
Julien Grall

Re: [PATCH v4 6/7] arm/mpu: Provide a constructor for pr_t type

[Luca Fancellu]

Hi Julien,

> On 6 May 2025, at 14:51, Julien Grall <julien@xen.org> wrote:
> 
> Hi Luca,
> 
> On 06/05/2025 13:56, Luca Fancellu wrote:
>>>> +/*
>>>> + * Creates a pr_t structure describing a protection region.
>>>> + *
>>>> + * @base: base address as base of the protection region.
>>>> + * @limit: exclusive address as limit of the protection region.
>>>> + * @attr: attribute index for the memory type.
>>>> + * @return: pr_t structure describing a protection region.
>>>> + */
>>>> +extern pr_t pr_of_xenaddr(paddr_t base, paddr_t limit, unsigned int attr_idx);
>>> here. Please don't use extern in prototypes. It's not needed.
>> I see we have a mixed usage of this in arch/arm and it’s not documented to do otherwise
>> in the code style, in this case I would prefer to be explicit unless it’s a strong objection on your side,
>> let me know.
> 
> Old Arm code is using "extern". But new code should avoid it for prototypes.

ok I see, I’ll drop it then. 

Cheers,
Luca

Re: [PATCH v4 6/7] arm/mpu: Provide a constructor for pr_t type

[Luca Fancellu]

>> +    switch ( attr_idx )
>> +    {
>> +    case MT_NORMAL_NC:
>> +        /*
>> +         * ARM ARM: Overlaying the shareability attribute (DDI
>> +         * 0406C.b B3-1376 to 1377)
> It's a bit odd to provide here the manual for Armv7.
> Also, our general advice is to use the latest revision.

I’m thinking about restructuring in this way:

    switch ( attr_idx )
    {
    /*
     * ARM ARM: Shareable, Inner Shareable, and Outer Shareable Normal memory
     * (DDI 0487L.a B2.10.1.1.1 Note section):
     *
     * Because all data accesses to Non-cacheable locations are data coherent
     * to all observers, Non-cacheable locations are always treated as Outer
     * Shareable
     *
     * ARM ARM: Device memory (DDI 0487L.a B2.10.2)
     *
     * All of these memory types have the following properties:
     * [...]
     *  - Data accesses to memory locations are coherent for all observers in
     *    the system, and correspondingly are treated as being Outer Shareable
     */
    case MT_NORMAL_NC:
        /* Fall through */
    case MT_DEVICE_nGnRnE:
        /* Fall through */
    case MT_DEVICE_nGnRE:
        prbar.reg.sh = LPAE_SH_OUTER;
        break;
    default:
        /* Xen mappings are SMP coherent */
        prbar.reg.sh = LPAE_SH_INNER;
        break;
    }

what do you think? It will have the fall through comment and will also explain the LPAE_SH_OUTER value set
for both normal memory nc and device memory.

Please let me know.

Cheers,
Luca

[PATCH v4 7/7] arm/mpu: Introduce MPU memory mapping flags

[Luca Fancellu]

Introduce the MPU memory mapping flags in asm/page.h.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v4 changes:
 - no changes, I'm not sure how I can merge XN and Permission flags.
---
 xen/arch/arm/include/asm/page.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/xen/arch/arm/include/asm/page.h b/xen/arch/arm/include/asm/page.h
index 69f817d1e68a..22f7d2c6cb30 100644
--- a/xen/arch/arm/include/asm/page.h
+++ b/xen/arch/arm/include/asm/page.h
@@ -62,6 +62,7 @@
 
 #define MAIRVAL (MAIR1VAL << 32 | MAIR0VAL)
 
+#ifdef CONFIG_MMU
 /*
  * Layout of the flags used for updating the hypervisor page tables
  *
@@ -90,6 +91,30 @@
 #define _PAGE_CONTIG_BIT    8
 #define _PAGE_CONTIG        (1U << _PAGE_CONTIG_BIT)
 
+#else /* !CONFIG_MMU */
+
+/*
+ * Layout of the flags used for updating MPU memory region attributes
+ * [0:2] Memory attribute Index
+ * [3:4] Execute Never
+ * [5:6] Access Permission
+ * [7]   Region Present
+ */
+#define _PAGE_AI_BIT            0
+#define _PAGE_XN_BIT            3
+#define _PAGE_AP_BIT            5
+#define _PAGE_PRESENT_BIT       7
+#define _PAGE_AI                (7U << _PAGE_AI_BIT)
+#define _PAGE_XN                (2U << _PAGE_XN_BIT)
+#define _PAGE_RO                (2U << _PAGE_AP_BIT)
+#define _PAGE_PRESENT           (1U << _PAGE_PRESENT_BIT)
+#define PAGE_AI_MASK(x)         (((x) >> _PAGE_AI_BIT) & 0x7U)
+#define PAGE_XN_MASK(x)         (((x) >> _PAGE_XN_BIT) & 0x3U)
+#define PAGE_AP_MASK(x)         (((x) >> _PAGE_AP_BIT) & 0x3U)
+#define PAGE_RO_MASK(x)         (((x) >> _PAGE_AP_BIT) & 0x2U)
+
+#endif /* CONFIG_MMU */
+
 /*
  * _PAGE_DEVICE and _PAGE_NORMAL are convenience defines. They are not
  * meant to be used outside of this header.
-- 
2.34.1