Re: [PATCH v1] xen/arm: AArch32-V8R: Add MPU register definitions

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Ayan,

>>>>> +        /*
>>>>> +         * There is no actual ns bit in hardware. It is used here for
>>>>> +         * compatibility with Armr64 code. Thus, we are reusing a res0 
>>>>> bit for ns.
>>>> typo: Arm64.
>>> Ack
>>>>> +         */
>>>> Hmmmm, this would mean someone may mistakenly set the bit to 0 by mistake. 
>>>> If the field is always meant to be 0 on arm64, then I would consider to 
>>>> name is res0 on arm64 with an explanation.
>>>> 
>>>> This would make clear the bit is not supposed to have a value other than 0.
>>> On Arm64, ns == 0 as it can only support secure mode.
>>> 
>>> So we can change this on Arm64 as well :-
>>> 
>>> unsigned int res0:2; /* ns == 0 as only secure mode is supported */
>>> 
>>> @Luca to clarify
>> From the specifications: "Non-secure bit. Specifies whether the output 
>> address is in the Secure or Non-secure memory”, I’m not sure
>> that we should remove it from Arm64, so I don’t think you should have 
>> something only for compatibility, maybe the code accessing .ns
>> can be compiled out for Arm32 or we can have arch-specific implementation. I 
>> think you are referring to pr_of_xenaddr when you say
>> “compatibility with Arm64 code"
> 
> Yes, that is correct. So are you saying that we should have an "ifdef" in the 
> function.
> 
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -221,7 +221,9 @@ pr_t pr_of_xenaddr(paddr_t base, paddr_t limit, unsigned 
> attr)
>      /* 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,
>              .en = 1,        /* Region enabled */
>          }};
> 
> I am ok with this. I just want to know if you and Julien are aligned as well.

this is my proposal, yes

>>>>> 
>>>> NIT: Is there any way we could generate the values using macros?
>>> This looks tricky. So I will prefer to keep this as it is.
>>>>> +
>>>>>   /* Aliases of AArch64 names for use in common code */
>>>>>   #ifdef CONFIG_ARM_32
>>>>>   /* Alphabetically... */
>>>>>   #define MPUIR_EL2       HMPUIR
>>>>>   #define PRBAR_EL2       HPRBAR
>>>>> +#define PRBAR0_EL2      HPRBAR0
>>>> AFAIU, the alias will be mainly used in the macro generate
>>>> the switch. Rather than open-coding all the PR*AR_EL2, can we
>>>> provide two macros PR{B, L}AR_N that will be implemented as
>>>> HPR{B,L}AR##n for arm32 and PR{B,L}AR##n for arm64?
>>> Yes , we can have
>>> 
>>> #define PR{B,L}AR_EL2_(n)          HPR{B,L}AR##n for arm32
>>> 
>>> #define PR{B,L}AR_EL2_(n)          PR{B,L}AR##n##_EL2
>> we could have them in mm.c, I see in your v2 you’ve put them in cpregs.h,
>> but since they are only used by the generator, I would put them there.
> 
> You mean the above two macros should be moved to mm.c. I am ok with that.
> 
> Just 2 more things to be aligned :-
> 
> 1. Are we ok to use PRBAR_EL2_(num) and PRLAR_EL2_(num) in the common code 
> (ie mpu/mm.c) ?
> 
> 2. Are you ok to introduce ifdef in prepare_selector() ?
> 
> Please have a look at my v2 for reference.

I will change my implementation to introduce on Arm64 PR{B,L}AR_EL2_(num) -> 
PR{B,L}AR##num##_EL2,
I will then protect everything with CONFIG_ARM_64, this will ensure proper 
compilation on both architecture.

When you will introduce your changes, you will have only to revert the #ifdef 
CONFIG_ARM_64 that protects
the common code and implement the changes to build on Arm32.

I think this is the best way to ensure we are not blocked by each other while 
keeping the churn as low as possible.

Please @julien and/or other maintainers let me know your opinion on this.

Cheers,
Luca