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.hal...@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
