> On 25 Apr 2025, at 14:32, Julien Grall <jul...@xen.org> wrote:
>
> Hi Ayan
>
> On 25/04/2025 13:00, Ayan Kumar Halder wrote:
>>>> + unsigned int ns:1; /* Reserved 0 by hardware */
>>>> + unsigned int res0:1; /* Reserved 0 by hardware */
>> Then, we can change this on Arm32 as well.
>>>> + unsigned int limit:26; /* Limit Address */
>>>
>>> NIT: Can we align the comments?
>> Ack.
>>>
>>>> + } reg;
>>>> + uint32_t bits;
>>>> +} prlar_t;
>>>> +
>>>> +/* Protection Region */
>>>> +typedef struct {
>>>> + prbar_t prbar;
>>>> + prlar_t prlar;
>>>> + uint64_t p2m_type; /* Used to store p2m types. */
>>>> +} pr_t;
>>>
>>> This looks to be common with arm64. If so, I would prefer if the structure
>>> is in a common header.
>> No, in arm64 this is
>> typedef struct {
>> prbar_t prbar;
>> prlar_t prlar;
>> } pr_t;
>> The reason being Arm64 uses unused bits (ie 'pad') to store the type.
>
> Hmmm... They are bits from the system register (prbar), right? If so, you
> can't use them for storing the p2m_type as AFAICT they are RES0.
>
> You could possibly mask the bits before writing them.
This was the approach taken in the Arm64, I think the reason was because in
this way the overall maximum structure was less than a page
and it was easier to manage something, I’ll see if I can find what, I think it
was related to p2m...
> But then, it will become risky if the fields are defined.
>
> Also, the number of MPU regions is fairly small. So, I don't think it is
> worth it to store p2m_type.
>
> Regardless that, I think it would be better to defer the introduction of
> p2m_type until guest support is added. So it will be easier to understand how
> this is going to be used and the size (for instance, I doubt 64-bit is
> necessary...).
>
> Cheers,
>
> --
> Julien Grall
>
