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
