Hi Julien,

> On 14 Apr 2025, at 12:48, Julien Grall <jul...@xen.org> wrote:
> 
> Hi Luca,
> 
> On 11/04/2025 23:56, Luca Fancellu wrote:
>> Introduce the MPU memory mapping flags in asm/page.h.
>> Signed-off-by: Luca Fancellu <luca.fance...@arm.com>
>> ---
>>  xen/arch/arm/include/asm/page.h | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>> diff --git a/xen/arch/arm/include/asm/page.h 
>> b/xen/arch/arm/include/asm/page.h
>> index 69f817d1e68a..22f7d2c6cb30 100644
>> --- a/xen/arch/arm/include/asm/page.h
>> +++ b/xen/arch/arm/include/asm/page.h
>> @@ -62,6 +62,7 @@
>>    #define MAIRVAL (MAIR1VAL << 32 | MAIR0VAL)
>>  +#ifdef CONFIG_MMU
>>  /*
>>   * Layout of the flags used for updating the hypervisor page tables
>>   *
>> @@ -90,6 +91,30 @@
>>  #define _PAGE_CONTIG_BIT    8
>>  #define _PAGE_CONTIG        (1U << _PAGE_CONTIG_BIT)
>>  +#else /* !CONFIG_MMU */
>> +
>> +/*
>> + * Layout of the flags used for updating MPU memory region attributes
>> + * [0:2] Memory attribute Index
>> + * [3:4] Execute Never
>> + * [5:6] Access Permission
> 
> I am rather confused why we are splitting Execute Never from the Access 
> Permission. I guess you tried to match the HW, but it also means we need to 
> duplicate a lot of define between the MMU and MPU code.
> 
> Instead, I would rather try to re-use the existing ones and ignore the ones 
> we don't need (e.g. BLOCK_BIT and CONTIG).

I’m having a bit of trouble understanding the MMU part:

/*
* Layout of the flags used for updating the hypervisor page tables
*
* [0:2] Memory Attribute Index
* [3:4] Permission flags
* [5] Page present
* [6] Only populate page tables
* [7] Superpage mappings is allowed
* [8] Set contiguous bit (internal flag)
*/
#define PAGE_AI_MASK(x) ((x) & 0x7U)

#define _PAGE_XN_BIT 3
#define _PAGE_RO_BIT 4
#define _PAGE_XN (1U << _PAGE_XN_BIT)
#define _PAGE_RO (1U << _PAGE_RO_BIT)
#define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x1U)
#define PAGE_RO_MASK(x) (((x) >> _PAGE_RO_BIT) & 0x1U)

I can see on the MMU basically AP[1] means RO or not, AP[0] means XN or not, 
from the arm spec
(verison L.a, D8.4.2.1.1 Stage 2 data accesses using Direct permissions) I can 
see stage 2 AP[1:0] is:
 - 00: no access
 - 01: RO
 - 10: WO
 - 11: RW

So:
 - 00: read-only is zero and execution is allowed
 - 01: read-only is zero and execution is not allowed
 - 10: read-only is one (??), execution is allowed
 - 11: read-only is one (??), execution is not allowed

I’m clearly missing something because everything works, so could you help me to 
understand this part?

Reply via email to