Re: [PATCH v5 3/3] arm/mpu: Implement p2m tables

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 08-May-26 16:33, Luca Fancellu wrote:
> From: Harry Ramsey <harry.ramsey@xxxxxxx>
> 
> Implement `p2m_alloc_table`, `p2m_init` and `p2m_final_teardown` for MPU
> systems.
> 
> Signed-off-by: Harry Ramsey <harry.ramsey@xxxxxxx>
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
> v5:
>  - removed comment for P2M_ROOT_PAGES/P2M_ROOT_ORDER
>  - used MAX_MPU_REGION_NR instead of opencoding 255
>  - use 'table' in p2m_alloc_table to clear pages
>  - remove p2m_free_vmid() from error path in p2m_init()
>    which is already handled outside
> v4:
>  - no changes
> v3:
> - Check for alloc_xenheap_pages allocation
> - Clear additional page allocated for ARM64
> - Add check for INVALID_VCPU_ID
> - Remove unnecessary function generate_vsctlr
> - Code formatting fixes
> ---
>  xen/arch/arm/include/asm/arm32/mpu.h |  2 +
>  xen/arch/arm/include/asm/arm64/mpu.h |  2 +
>  xen/arch/arm/include/asm/mpu/p2m.h   |  7 +++
>  xen/arch/arm/include/asm/p2m.h       |  5 ++
>  xen/arch/arm/mpu/p2m.c               | 72 ++++++++++++++++++++++++++--
>  5 files changed, 85 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/arm32/mpu.h 
> b/xen/arch/arm/include/asm/arm32/mpu.h
> index 2cf0f8cbacae..d565230f84ee 100644
> --- a/xen/arch/arm/include/asm/arm32/mpu.h
> +++ b/xen/arch/arm/include/asm/arm32/mpu.h
> @@ -11,6 +11,8 @@
>   */
>  #define MPU_REGION_RES0       0x0
>  
> +#define VSCTLR_VMID_SHIFT     16
> +
>  /* Hypervisor Protection Region Base Address Register */
>  typedef union {
>      struct {
> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h 
> b/xen/arch/arm/include/asm/arm64/mpu.h
> index 4f694190a8a3..8b86a03fee44 100644
> --- a/xen/arch/arm/include/asm/arm64/mpu.h
> +++ b/xen/arch/arm/include/asm/arm64/mpu.h
> @@ -7,6 +7,8 @@
>  
>  #define MPU_REGION_RES0        (0xFFFFULL << 48)
>  
> +#define VSCTLR_VMID_SHIFT      48
> +
>  /* Protection Region Base Address Register */
>  typedef union {
>      struct __packed {
> diff --git a/xen/arch/arm/include/asm/mpu/p2m.h 
> b/xen/arch/arm/include/asm/mpu/p2m.h
> index e46d9e757a1d..0d4149c71987 100644
> --- a/xen/arch/arm/include/asm/mpu/p2m.h
> +++ b/xen/arch/arm/include/asm/mpu/p2m.h
> @@ -3,8 +3,15 @@
>  #ifndef __ARM_MPU_P2M_H__
>  #define __ARM_MPU_P2M_H__
>  
> +#include <xen/bitops.h>
> +#include <xen/macros.h>
> +#include <xen/page-size.h>
> +
>  struct p2m_domain;
>  
> +#define P2M_ROOT_PAGES DIV_ROUND_UP(MAX_MPU_REGION_NR * sizeof(pr_t), 
> PAGE_SIZE)
You should include mpu.h for MAX_MPU_REGION_NR definition

> +#define P2M_ROOT_ORDER get_count_order(P2M_ROOT_PAGES)
> +
>  static inline void p2m_clear_root_pages(struct p2m_domain *p2m) {}
>  
>  static inline void p2m_tlb_flush_sync(struct p2m_domain *p2m) {}
> diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
> index 010ce8c9ebbd..ed1b6dd40f40 100644
> --- a/xen/arch/arm/include/asm/p2m.h
> +++ b/xen/arch/arm/include/asm/p2m.h
> @@ -48,8 +48,13 @@ struct p2m_domain {
>      /* Current VMID in use */
>      uint16_t vmid;
>  
> +#ifdef CONFIG_MMU
>      /* Current Translation Table Base Register for the p2m */
>      uint64_t vttbr;
> +#else
> +    /* Current Virtualization System Control Register for the p2m */
> +    register_t vsctlr;
> +#endif
>  
>      /* Highest guest frame that's ever been mapped in the p2m */
>      gfn_t max_mapped_gfn;
> diff --git a/xen/arch/arm/mpu/p2m.c b/xen/arch/arm/mpu/p2m.c
> index f7fb58ab6aa8..8d5c65b1003f 100644
> --- a/xen/arch/arm/mpu/p2m.c
> +++ b/xen/arch/arm/mpu/p2m.c
> @@ -28,10 +28,63 @@ void p2m_dump_info(struct domain *d)
>      BUG_ON("unimplemented");
>  }
>  
> +static int p2m_alloc_table(struct domain *d)
> +{
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    char *table = alloc_xenheap_pages(P2M_ROOT_ORDER, 0);
Why char and not void? We use void and pointer arithmetic is done by treating
the size of a void as 1.
> +    unsigned int i;
> +
> +    if ( !table )
> +    {
> +        printk(XENLOG_G_ERR "%pd: p2m: unable to allocate P2M MPU mapping 
> table\n",
> +               d);
> +        return -ENOMEM;
> +    }
> +
> +    p2m->root = virt_to_page(table);
> +
> +    for ( i = 0; i < P2M_ROOT_PAGES; i++ )
> +        clear_page(table + (i * PAGE_SIZE));
> +
> +    return 0;
> +}
> +
>  int p2m_init(struct domain *d)
>  {
> -    BUG_ON("unimplemented");
> -    return -EINVAL;
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    int rc = 0;
> +    unsigned int cpu;
> +
> +    rwlock_init(&p2m->lock);
> +
> +    p2m->vmid = INVALID_VMID;
> +    p2m->max_mapped_gfn = _gfn(0);
> +    p2m->lowest_mapped_gfn = _gfn(ULONG_MAX);
> +
> +    p2m->default_access = p2m_access_rwx;
> +    /* mem_access is NOT supported in MPU system. */
s/in/on

With the above fixed:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal