Introduce pr_t typedef which is a structure having the prbar and prlar members,
each being structured as the registers of the AArch32 Armv8-R architecture.
Also, define MPU_REGION_RES0 to 0 as there are no reserved 0 bits beyond the
BASE or LIMIT bitfields in prbar or prlar respectively.
Move pr_t definition to common code.
Also, enclose xn_0 within ARM64 as it is not present for ARM32.
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
xen/arch/arm/include/asm/arm32/mpu.h | 30 +++++++++++++++++++++++-----
xen/arch/arm/include/asm/arm64/mpu.h | 6 ------
xen/arch/arm/include/asm/mpu.h | 6 ++++++
xen/arch/arm/mpu/mm.c | 2 ++
4 files changed, 33 insertions(+), 11 deletions(-)
diff --git a/xen/arch/arm/include/asm/arm32/mpu.h b/xen/arch/arm/include/asm/arm32/mpu.h
index f0d4d4055c..ae3b661fde 100644
--- a/xen/arch/arm/include/asm/arm32/mpu.h
+++ b/xen/arch/arm/include/asm/arm32/mpu.h
@@ -5,11 +5,31 @@
#ifndef __ASSEMBLY__
-/* MPU Protection Region */
-typedef struct {
- uint32_t prbar;
- uint32_t prlar;
-} pr_t;
+#define MPU_REGION_RES0 0x0
+
+/* Hypervisor Protection Region Base Address Register */
+typedef union {
+ struct {
+ unsigned int xn:1; /* Execute-Never */
+ unsigned int ap_0:1; /* Acess Permission */
+ unsigned long ro:1; /* Access Permission AP[1] */
+ unsigned int sh:2; /* Sharebility */
+ unsigned int res0:1; /* Reserved as 0 */
+ unsigned int base:26; /* Base Address */
+ } reg;
+ uint32_t bits;
+} prbar_t;
+
+/* Hypervisor Protection Region Limit Address Register */
+typedef union {
+ struct {
+ unsigned int en:1; /* Region enable */
+ unsigned int ai:3; /* Memory Attribute Index */
+ unsigned int res0:2; /* Reserved 0 by hardware */
+ unsigned int limit:26; /* Limit Address */
+ } reg;
+ uint32_t bits;
+} prlar_t;
#endif /* __ASSEMBLY__ */
diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/arm64/mpu.h
index f0ce344e78..df46774dcb 100644
--- a/xen/arch/arm/include/asm/arm64/mpu.h
+++ b/xen/arch/arm/include/asm/arm64/mpu.h
@@ -34,12 +34,6 @@ typedef union {
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 */
diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
index 8f06ddac0f..c8573a5980 100644
--- a/xen/arch/arm/include/asm/mpu.h
+++ b/xen/arch/arm/include/asm/mpu.h
@@ -25,6 +25,12 @@
#ifndef __ASSEMBLY__
+/* MPU Protection Region */
+typedef struct {
+ prbar_t prbar;
+ prlar_t prlar;
+} pr_t;
+
#ifdef CONFIG_ARM_64
/*
* Set base address of MPU protection region.
diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index 86fbe105af..2fb6b822c6 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -167,7 +167,9 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags)
/* Build up value for PRBAR_EL2. */
prbar = (prbar_t) {
.reg = {
+#ifdef CONFIG_ARM64
.xn_0 = 0,
+#endif
.xn = PAGE_XN_MASK(flags),
.ap_0 = 0,
.ro = PAGE_RO_MASK(flags)
--
2.25.1
On 04/06/2025 19:43, Ayan Kumar Halder wrote:
> Introduce pr_t typedef which is a structure having the prbar and prlar members,
> each being structured as the registers of the AArch32 Armv8-R architecture.
>
> Also, define MPU_REGION_RES0 to 0 as there are no reserved 0 bits beyond the
> BASE or LIMIT bitfields in prbar or prlar respectively.
>
> Move pr_t definition to common code.
> Also, enclose xn_0 within ARM64 as it is not present for ARM32.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> xen/arch/arm/include/asm/arm32/mpu.h | 30 +++++++++++++++++++++++-----
> xen/arch/arm/include/asm/arm64/mpu.h | 6 ------
> xen/arch/arm/include/asm/mpu.h | 6 ++++++
> xen/arch/arm/mpu/mm.c | 2 ++
> 4 files changed, 33 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/arm32/mpu.h b/xen/arch/arm/include/asm/arm32/mpu.h
> index f0d4d4055c..ae3b661fde 100644
> --- a/xen/arch/arm/include/asm/arm32/mpu.h
> +++ b/xen/arch/arm/include/asm/arm32/mpu.h
> @@ -5,11 +5,31 @@
>
> #ifndef __ASSEMBLY__
>
> -/* MPU Protection Region */
> -typedef struct {
> - uint32_t prbar;
> - uint32_t prlar;
> -} pr_t;
> +#define MPU_REGION_RES0 0x0
The name of the macro does not make a lot of sense in AArch32 context
and can create a confusion for the reader.
> +
> +/* Hypervisor Protection Region Base Address Register */
> +typedef union {
> + struct {
> + unsigned int xn:1; /* Execute-Never */
> + unsigned int ap_0:1; /* Acess Permission */
If you write AP[1] below, I would expect here AP[0]
Also s/acess/access/
> + unsigned long ro:1; /* Access Permission AP[1] */
> + unsigned int sh:2; /* Sharebility */
> + unsigned int res0:1; /* Reserved as 0 */
Here your comment for RES0 (which IMO could be just RES0) does not match...
> + unsigned int base:26; /* Base Address */
> + } reg;
> + uint32_t bits;
> +} prbar_t;
> +
> +/* Hypervisor Protection Region Limit Address Register */
> +typedef union {
> + struct {
> + unsigned int en:1; /* Region enable */
> + unsigned int ai:3; /* Memory Attribute Index */
> + unsigned int res0:2; /* Reserved 0 by hardware */
this one. I don't think you need to explain what RES0 means.
~Michal
Hi Michal,
On 05/06/2025 08:06, Orzel, Michal wrote:
>
> On 04/06/2025 19:43, Ayan Kumar Halder wrote:
>> Introduce pr_t typedef which is a structure having the prbar and prlar members,
>> each being structured as the registers of the AArch32 Armv8-R architecture.
>>
>> Also, define MPU_REGION_RES0 to 0 as there are no reserved 0 bits beyond the
>> BASE or LIMIT bitfields in prbar or prlar respectively.
>>
>> Move pr_t definition to common code.
>> Also, enclose xn_0 within ARM64 as it is not present for ARM32.
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>> xen/arch/arm/include/asm/arm32/mpu.h | 30 +++++++++++++++++++++++-----
>> xen/arch/arm/include/asm/arm64/mpu.h | 6 ------
>> xen/arch/arm/include/asm/mpu.h | 6 ++++++
>> xen/arch/arm/mpu/mm.c | 2 ++
>> 4 files changed, 33 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/arm32/mpu.h b/xen/arch/arm/include/asm/arm32/mpu.h
>> index f0d4d4055c..ae3b661fde 100644
>> --- a/xen/arch/arm/include/asm/arm32/mpu.h
>> +++ b/xen/arch/arm/include/asm/arm32/mpu.h
>> @@ -5,11 +5,31 @@
>>
>> #ifndef __ASSEMBLY__
>>
>> -/* MPU Protection Region */
>> -typedef struct {
>> - uint32_t prbar;
>> - uint32_t prlar;
>> -} pr_t;
>> +#define MPU_REGION_RES0 0x0
> The name of the macro does not make a lot of sense in AArch32 context
> and can create a confusion for the reader.
I know, but I want to avoid introducing ifdef or have separate
implementation (for arm32 and arm64) for the following
Refer xen/arch/arm/include/asm/mpu.h
static inline void pr_set_base(pr_t *pr, paddr_t base)
{
pr->prbar.reg.base = ((base & ~MPU_REGION_RES0) >> MPU_REGION_SHIFT);
}
Let me know your preference.
>
>> +
>> +/* Hypervisor Protection Region Base Address Register */
>> +typedef union {
>> + struct {
>> + unsigned int xn:1; /* Execute-Never */
>> + unsigned int ap_0:1; /* Acess Permission */
> If you write AP[1] below, I would expect here AP[0]
Again same reason as before, let me know if you want to have additional
ifdef in pr_of_addr() or separate functions for arm32 and arm64
pr_t pr_of_addr(...)
{...
prbar = (prbar_t) {
.reg = {
#ifdef CONFIG_ARM64
.xn_0 = 0,
#endif
.xn = PAGE_XN_MASK(flags),
.ap_0 = 0,
.ro = PAGE_RO_MASK(flags)
}};
...
}
> Also s/acess/access/
Ack.
>
>> + unsigned long ro:1; /* Access Permission AP[1] */
>> + unsigned int sh:2; /* Sharebility */
>> + unsigned int res0:1; /* Reserved as 0 */
> Here your comment for RES0 (which IMO could be just RES0) does not match...
I will remove the comment here and ..
>
>> + unsigned int base:26; /* Base Address */
>> + } reg;
>> + uint32_t bits;
>> +} prbar_t;
>> +
>> +/* Hypervisor Protection Region Limit Address Register */
>> +typedef union {
>> + struct {
>> + unsigned int en:1; /* Region enable */
>> + unsigned int ai:3; /* Memory Attribute Index */
>> + unsigned int res0:2; /* Reserved 0 by hardware */
> this one. I don't think you need to explain what RES0 means.
here.
- Ayan
>
> ~Michal
>
On 05/06/2025 15:39, Ayan Kumar Halder wrote:
> Hi Michal,
>
> On 05/06/2025 08:06, Orzel, Michal wrote:
>>
>> On 04/06/2025 19:43, Ayan Kumar Halder wrote:
>>> Introduce pr_t typedef which is a structure having the prbar and prlar members,
>>> each being structured as the registers of the AArch32 Armv8-R architecture.
>>>
>>> Also, define MPU_REGION_RES0 to 0 as there are no reserved 0 bits beyond the
>>> BASE or LIMIT bitfields in prbar or prlar respectively.
>>>
>>> Move pr_t definition to common code.
>>> Also, enclose xn_0 within ARM64 as it is not present for ARM32.
>>>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>> ---
>>> xen/arch/arm/include/asm/arm32/mpu.h | 30 +++++++++++++++++++++++-----
>>> xen/arch/arm/include/asm/arm64/mpu.h | 6 ------
>>> xen/arch/arm/include/asm/mpu.h | 6 ++++++
>>> xen/arch/arm/mpu/mm.c | 2 ++
>>> 4 files changed, 33 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/include/asm/arm32/mpu.h b/xen/arch/arm/include/asm/arm32/mpu.h
>>> index f0d4d4055c..ae3b661fde 100644
>>> --- a/xen/arch/arm/include/asm/arm32/mpu.h
>>> +++ b/xen/arch/arm/include/asm/arm32/mpu.h
>>> @@ -5,11 +5,31 @@
>>>
>>> #ifndef __ASSEMBLY__
>>>
>>> -/* MPU Protection Region */
>>> -typedef struct {
>>> - uint32_t prbar;
>>> - uint32_t prlar;
>>> -} pr_t;
>>> +#define MPU_REGION_RES0 0x0
>> The name of the macro does not make a lot of sense in AArch32 context
>> and can create a confusion for the reader.
>
> I know, but I want to avoid introducing ifdef or have separate
> implementation (for arm32 and arm64) for the following
>
> Refer xen/arch/arm/include/asm/mpu.h
>
> static inline void pr_set_base(pr_t *pr, paddr_t base)
> {
> pr->prbar.reg.base = ((base & ~MPU_REGION_RES0) >> MPU_REGION_SHIFT);
> }
>
> Let me know your preference.
I did not mean #ifdef-ing. I was more like suggesting to use a different macro
name that would be more meaningful than this one.
>
>>
>>> +
>>> +/* Hypervisor Protection Region Base Address Register */
>>> +typedef union {
>>> + struct {
>>> + unsigned int xn:1; /* Execute-Never */
>>> + unsigned int ap_0:1; /* Acess Permission */
>> If you write AP[1] below, I would expect here AP[0]
>
> Again same reason as before, let me know if you want to have additional
> ifdef in pr_of_addr() or separate functions for arm32 and arm64
I don't understand. My comment was only about changing comment to say /* Access
Permission AP[0] */ because below you have a comment with AP[1].
~Michal
Hi Michal,
On 06/06/2025 11:13, Orzel, Michal wrote:
>
> On 05/06/2025 15:39, Ayan Kumar Halder wrote:
>> Hi Michal,
>>
>> On 05/06/2025 08:06, Orzel, Michal wrote:
>>> On 04/06/2025 19:43, Ayan Kumar Halder wrote:
>>>> Introduce pr_t typedef which is a structure having the prbar and prlar members,
>>>> each being structured as the registers of the AArch32 Armv8-R architecture.
>>>>
>>>> Also, define MPU_REGION_RES0 to 0 as there are no reserved 0 bits beyond the
>>>> BASE or LIMIT bitfields in prbar or prlar respectively.
>>>>
>>>> Move pr_t definition to common code.
>>>> Also, enclose xn_0 within ARM64 as it is not present for ARM32.
>>>>
>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>>> ---
>>>> xen/arch/arm/include/asm/arm32/mpu.h | 30 +++++++++++++++++++++++-----
>>>> xen/arch/arm/include/asm/arm64/mpu.h | 6 ------
>>>> xen/arch/arm/include/asm/mpu.h | 6 ++++++
>>>> xen/arch/arm/mpu/mm.c | 2 ++
>>>> 4 files changed, 33 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/include/asm/arm32/mpu.h b/xen/arch/arm/include/asm/arm32/mpu.h
>>>> index f0d4d4055c..ae3b661fde 100644
>>>> --- a/xen/arch/arm/include/asm/arm32/mpu.h
>>>> +++ b/xen/arch/arm/include/asm/arm32/mpu.h
>>>> @@ -5,11 +5,31 @@
>>>>
>>>> #ifndef __ASSEMBLY__
>>>>
>>>> -/* MPU Protection Region */
>>>> -typedef struct {
>>>> - uint32_t prbar;
>>>> - uint32_t prlar;
>>>> -} pr_t;
>>>> +#define MPU_REGION_RES0 0x0
>>> The name of the macro does not make a lot of sense in AArch32 context
>>> and can create a confusion for the reader.
>> I know, but I want to avoid introducing ifdef or have separate
>> implementation (for arm32 and arm64) for the following
>>
>> Refer xen/arch/arm/include/asm/mpu.h
>>
>> static inline void pr_set_base(pr_t *pr, paddr_t base)
>> {
>> pr->prbar.reg.base = ((base & ~MPU_REGION_RES0) >> MPU_REGION_SHIFT);
>> }
>>
>> Let me know your preference.
> I did not mean #ifdef-ing. I was more like suggesting to use a different macro
> name that would be more meaningful than this one.
Now I understand you. However, I can't thing of a better name to make it
more meaningful.
I have added a comment on top. Is this helpful ?
/* Unlike arm64, there are no reserved 0 bits beyond base in prbar
register */
>
>>>> +
>>>> +/* Hypervisor Protection Region Base Address Register */
>>>> +typedef union {
>>>> + struct {
>>>> + unsigned int xn:1; /* Execute-Never */
>>>> + unsigned int ap_0:1; /* Acess Permission */
>>> If you write AP[1] below, I would expect here AP[0]
>> Again same reason as before, let me know if you want to have additional
>> ifdef in pr_of_addr() or separate functions for arm32 and arm64
> I don't understand. My comment was only about changing comment to say /* Access
> Permission AP[0] */ because below you have a comment with AP[1].
Ah makes sense now.
- Ayan
>
> ~Michal
>
On 06/06/2025 13:43, Ayan Kumar Halder wrote:
> Hi Michal,
>
> On 06/06/2025 11:13, Orzel, Michal wrote:
>>
>> On 05/06/2025 15:39, Ayan Kumar Halder wrote:
>>> Hi Michal,
>>>
>>> On 05/06/2025 08:06, Orzel, Michal wrote:
>>>> On 04/06/2025 19:43, Ayan Kumar Halder wrote:
>>>>> Introduce pr_t typedef which is a structure having the prbar and
>>>>> prlar members,
>>>>> each being structured as the registers of the AArch32 Armv8-R
>>>>> architecture.
>>>>>
>>>>> Also, define MPU_REGION_RES0 to 0 as there are no reserved 0 bits
>>>>> beyond the
>>>>> BASE or LIMIT bitfields in prbar or prlar respectively.
>>>>>
>>>>> Move pr_t definition to common code.
>>>>> Also, enclose xn_0 within ARM64 as it is not present for ARM32.
>>>>>
>>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>>>> ---
>>>>> xen/arch/arm/include/asm/arm32/mpu.h | 30
>>>>> +++++++++++++++++++++++-----
>>>>> xen/arch/arm/include/asm/arm64/mpu.h | 6 ------
>>>>> xen/arch/arm/include/asm/mpu.h | 6 ++++++
>>>>> xen/arch/arm/mpu/mm.c | 2 ++
>>>>> 4 files changed, 33 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/arm/include/asm/arm32/mpu.h
>>>>> b/xen/arch/arm/include/asm/arm32/mpu.h
>>>>> index f0d4d4055c..ae3b661fde 100644
>>>>> --- a/xen/arch/arm/include/asm/arm32/mpu.h
>>>>> +++ b/xen/arch/arm/include/asm/arm32/mpu.h
>>>>> @@ -5,11 +5,31 @@
>>>>> #ifndef __ASSEMBLY__
>>>>> -/* MPU Protection Region */
>>>>> -typedef struct {
>>>>> - uint32_t prbar;
>>>>> - uint32_t prlar;
>>>>> -} pr_t;
>>>>> +#define MPU_REGION_RES0 0x0
>>>> The name of the macro does not make a lot of sense in AArch32 context
>>>> and can create a confusion for the reader.
>>> I know, but I want to avoid introducing ifdef or have separate
>>> implementation (for arm32 and arm64) for the following
>>>
>>> Refer xen/arch/arm/include/asm/mpu.h
>>>
>>> static inline void pr_set_base(pr_t *pr, paddr_t base)
>>> {
>>> pr->prbar.reg.base = ((base & ~MPU_REGION_RES0) >>
>>> MPU_REGION_SHIFT);
>>> }
>>>
>>> Let me know your preference.
>> I did not mean #ifdef-ing. I was more like suggesting to use a
>> different macro
>> name that would be more meaningful than this one.
>
> Now I understand you. However, I can't thing of a better name to make
> it more meaningful.
>
> I have added a comment on top. Is this helpful ?
>
> /* Unlike arm64, there are no reserved 0 bits beyond base in prbar
> register */
/*
* Unlike arm64, there are no reserved 0 bits beyond base and limit
bitfield in
* prbar and prlar registers respectively.
*/
- Ayan
Hi Ayan,
> On 4 Jun 2025, at 18:43, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote:
>
> Introduce pr_t typedef which is a structure having the prbar and prlar members,
> each being structured as the registers of the AArch32 Armv8-R architecture.
>
> Also, define MPU_REGION_RES0 to 0 as there are no reserved 0 bits beyond the
> BASE or LIMIT bitfields in prbar or prlar respectively.
>
> Move pr_t definition to common code.
> Also, enclose xn_0 within ARM64 as it is not present for ARM32.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> xen/arch/arm/include/asm/arm32/mpu.h | 30 +++++++++++++++++++++++-----
> xen/arch/arm/include/asm/arm64/mpu.h | 6 ------
> xen/arch/arm/include/asm/mpu.h | 6 ++++++
> xen/arch/arm/mpu/mm.c | 2 ++
> 4 files changed, 33 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/arm32/mpu.h b/xen/arch/arm/include/asm/arm32/mpu.h
> index f0d4d4055c..ae3b661fde 100644
> --- a/xen/arch/arm/include/asm/arm32/mpu.h
> +++ b/xen/arch/arm/include/asm/arm32/mpu.h
> @@ -5,11 +5,31 @@
>
> #ifndef __ASSEMBLY__
>
> -/* MPU Protection Region */
> -typedef struct {
> - uint32_t prbar;
> - uint32_t prlar;
> -} pr_t;
> +#define MPU_REGION_RES0 0x0
> +
> +/* Hypervisor Protection Region Base Address Register */
> +typedef union {
> + struct {
> + unsigned int xn:1; /* Execute-Never */
> + unsigned int ap_0:1; /* Acess Permission */
> + unsigned long ro:1; /* Access Permission AP[1] */
> + unsigned int sh:2; /* Sharebility */
Typo: Sharebility -> Shareability.
Also, from the patch the comments feels not aligned, is that the case?
> + unsigned int res0:1; /* Reserved as 0 */
> + unsigned int base:26; /* Base Address */
> + } reg;
> + uint32_t bits;
> +} prbar_t;
> +
> +/* Hypervisor Protection Region Limit Address Register */
> +typedef union {
> + struct {
> + unsigned int en:1; /* Region enable */
> + unsigned int ai:3; /* Memory Attribute Index */
> + unsigned int res0:2; /* Reserved 0 by hardware */
> + unsigned int limit:26; /* Limit Address */
> + } reg;
> + uint32_t bits;
> +} prlar_t;
>
> #endif /* __ASSEMBLY__ */
>
> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/arm64/mpu.h
> index f0ce344e78..df46774dcb 100644
> --- a/xen/arch/arm/include/asm/arm64/mpu.h
> +++ b/xen/arch/arm/include/asm/arm64/mpu.h
> @@ -34,12 +34,6 @@ typedef union {
> uint64_t bits;
> } prlar_t;
>
> -/* MPU Protection Region */
> -typedef struct {
> - prbar_t prbar;
> - prlar_t prlar;
> -} pr_t;
I’m not sure I would do this, at some point there will be the transient flags and p2m type
and we know arm32 will need to store them in pr_t as additional members, instead
arm64 will store them as part of prbar/prlar using the not used space (without writing them
in the HW registers)
> -
> #endif /* __ASSEMBLY__ */
>
> #endif /* ARM_ARM64_MPU_H */
> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
> index 8f06ddac0f..c8573a5980 100644
> --- a/xen/arch/arm/include/asm/mpu.h
> +++ b/xen/arch/arm/include/asm/mpu.h
> @@ -25,6 +25,12 @@
>
> #ifndef __ASSEMBLY__
>
> +/* MPU Protection Region */
> +typedef struct {
> + prbar_t prbar;
> + prlar_t prlar;
> +} pr_t;
> +
> #ifdef CONFIG_ARM_64
> /*
> * Set base address of MPU protection region.
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index 86fbe105af..2fb6b822c6 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -167,7 +167,9 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags)
> /* Build up value for PRBAR_EL2. */
> prbar = (prbar_t) {
> .reg = {
> +#ifdef CONFIG_ARM64
> .xn_0 = 0,
> +#endif
> .xn = PAGE_XN_MASK(flags),
> .ap_0 = 0,
> .ro = PAGE_RO_MASK(flags)
> --
> 2.25.1
>
Cheers,
Luca
On 04/06/2025 20:19, Luca Fancellu wrote:
> Hi Ayan,
Hi Luca,
>
>> On 4 Jun 2025, at 18:43, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote:
>>
>> Introduce pr_t typedef which is a structure having the prbar and prlar members,
>> each being structured as the registers of the AArch32 Armv8-R architecture.
>>
>> Also, define MPU_REGION_RES0 to 0 as there are no reserved 0 bits beyond the
>> BASE or LIMIT bitfields in prbar or prlar respectively.
>>
>> Move pr_t definition to common code.
>> Also, enclose xn_0 within ARM64 as it is not present for ARM32.
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>> xen/arch/arm/include/asm/arm32/mpu.h | 30 +++++++++++++++++++++++-----
>> xen/arch/arm/include/asm/arm64/mpu.h | 6 ------
>> xen/arch/arm/include/asm/mpu.h | 6 ++++++
>> xen/arch/arm/mpu/mm.c | 2 ++
>> 4 files changed, 33 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/arm32/mpu.h b/xen/arch/arm/include/asm/arm32/mpu.h
>> index f0d4d4055c..ae3b661fde 100644
>> --- a/xen/arch/arm/include/asm/arm32/mpu.h
>> +++ b/xen/arch/arm/include/asm/arm32/mpu.h
>> @@ -5,11 +5,31 @@
>>
>> #ifndef __ASSEMBLY__
>>
>> -/* MPU Protection Region */
>> -typedef struct {
>> - uint32_t prbar;
>> - uint32_t prlar;
>> -} pr_t;
>> +#define MPU_REGION_RES0 0x0
>> +
>> +/* Hypervisor Protection Region Base Address Register */
>> +typedef union {
>> + struct {
>> + unsigned int xn:1; /* Execute-Never */
>> + unsigned int ap_0:1; /* Acess Permission */
>> + unsigned long ro:1; /* Access Permission AP[1] */
>> + unsigned int sh:2; /* Sharebility */
> Typo: Sharebility -> Shareability.
Ack
>
> Also, from the patch the comments feels not aligned, is that the case?
When you apply the patch, the comments look aligned (in the codebase).
Does it look the same for you ?
>
>> + unsigned int res0:1; /* Reserved as 0 */
>> + unsigned int base:26; /* Base Address */
>> + } reg;
>> + uint32_t bits;
>> +} prbar_t;
>> +
>> +/* Hypervisor Protection Region Limit Address Register */
>> +typedef union {
>> + struct {
>> + unsigned int en:1; /* Region enable */
>> + unsigned int ai:3; /* Memory Attribute Index */
>> + unsigned int res0:2; /* Reserved 0 by hardware */
>> + unsigned int limit:26; /* Limit Address */
>> + } reg;
>> + uint32_t bits;
>> +} prlar_t;
>>
>> #endif /* __ASSEMBLY__ */
>>
>> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/arm64/mpu.h
>> index f0ce344e78..df46774dcb 100644
>> --- a/xen/arch/arm/include/asm/arm64/mpu.h
>> +++ b/xen/arch/arm/include/asm/arm64/mpu.h
>> @@ -34,12 +34,6 @@ typedef union {
>> uint64_t bits;
>> } prlar_t;
>>
>> -/* MPU Protection Region */
>> -typedef struct {
>> - prbar_t prbar;
>> - prlar_t prlar;
>> -} pr_t;
> I’m not sure I would do this, at some point there will be the transient flags and p2m type
> and we know arm32 will need to store them in pr_t as additional members, instead
> arm64 will store them as part of prbar/prlar using the not used space (without writing them
> in the HW registers)
Ah I see. Ok, I will keep them separate as it is.
- Ayan
>
>> -
>> #endif /* __ASSEMBLY__ */
>>
>> #endif /* ARM_ARM64_MPU_H */
>> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
>> index 8f06ddac0f..c8573a5980 100644
>> --- a/xen/arch/arm/include/asm/mpu.h
>> +++ b/xen/arch/arm/include/asm/mpu.h
>> @@ -25,6 +25,12 @@
>>
>> #ifndef __ASSEMBLY__
>>
>> +/* MPU Protection Region */
>> +typedef struct {
>> + prbar_t prbar;
>> + prlar_t prlar;
>> +} pr_t;
>> +
>> #ifdef CONFIG_ARM_64
>> /*
>> * Set base address of MPU protection region.
>> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
>> index 86fbe105af..2fb6b822c6 100644
>> --- a/xen/arch/arm/mpu/mm.c
>> +++ b/xen/arch/arm/mpu/mm.c
>> @@ -167,7 +167,9 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags)
>> /* Build up value for PRBAR_EL2. */
>> prbar = (prbar_t) {
>> .reg = {
>> +#ifdef CONFIG_ARM64
>> .xn_0 = 0,
>> +#endif
>> .xn = PAGE_XN_MASK(flags),
>> .ap_0 = 0,
>> .ro = PAGE_RO_MASK(flags)
>> --
>> 2.25.1
>>
> Cheers,
> Luca
>
>
Hi Ayan,
>>> +/* Hypervisor Protection Region Base Address Register */
>>> +typedef union {
>>> + struct {
>>> + unsigned int xn:1; /* Execute-Never */
>>> + unsigned int ap_0:1; /* Acess Permission */
>>> + unsigned long ro:1; /* Access Permission AP[1] */
>>> + unsigned int sh:2; /* Sharebility */
>> Typo: Sharebility -> Shareability.
> Ack
>>
>> Also, from the patch the comments feels not aligned, is that the case?
> When you apply the patch, the comments look aligned (in the codebase). Does it look the same for you ?
I didn’t apply the patch, ok then it’s just in my client I see it not aligned, sorry for the noise!
Cheers,
Luca
© 2016 - 2025 Red Hat, Inc.