Re: [PATCH 2/8] xen/arm: Implement p2m_get_entry for MPU systems

["Halder, Ayan Kumar" <ayankuma@xxxxxxx>]

Hi Luca,

On 20/04/2026 15:25, Luca Fancellu wrote:
> From: Penny Zheng <Penny.Zheng@xxxxxxx>
>
> Implement the function p2m_get_entry for MPU systems, which is
> responsible for looking up an entry in the p2m table.
>
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> Signed-off-by: Hari Limaye <hari.limaye@xxxxxxx>
> ---
>   xen/arch/arm/include/asm/mpu/p2m.h |  5 ++
>   xen/arch/arm/mpu/p2m.c             | 76 ++++++++++++++++++++++++++++--
>   2 files changed, 77 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/mpu/p2m.h 
> b/xen/arch/arm/include/asm/mpu/p2m.h
> index b9c7be2d9dcc..d0ec8a77a15a 100644
> --- a/xen/arch/arm/include/asm/mpu/p2m.h
> +++ b/xen/arch/arm/include/asm/mpu/p2m.h
> @@ -31,6 +31,11 @@ static inline p2m_type_t region_get_p2m(pr_t *pr)
>       return pr->p2m_type;
>   }
>   
> +static inline bool region_is_p2m_valid(pr_t *pr)
const pr_t *

As this is just accessed.

> +{
> +    return (pr->p2m_type != p2m_invalid);
> +}
> +
>   #endif /* __ARM_MPU_P2M_H__ */
>   
>   /*
> diff --git a/xen/arch/arm/mpu/p2m.c b/xen/arch/arm/mpu/p2m.c
> index 4a8595b1b25e..681717eacf37 100644
> --- a/xen/arch/arm/mpu/p2m.c
> +++ b/xen/arch/arm/mpu/p2m.c
> @@ -135,11 +135,79 @@ int p2m_set_entry(struct p2m_domain *p2m, gfn_t sgfn, 
> unsigned long nr,
>       return __p2m_set_entry(p2m, sgfn, nr, smfn, t, a);
>   }
>   
> -mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn, p2m_type_t *t,
> -                    p2m_access_t *a, unsigned int *page_order, bool *valid)
> +/*
> + * Get the details of guest MPU memory region [gfn, gfn + nr_gfns).
> + *
> + * If the region is mapped, `t` is set to the p2m_type of the region and the
> + * starting MFN is returned.
> + * If the region is not mapped, INVALID_MFN is returned.
> + */
> +static mfn_t p2m_get_mpu_region(struct p2m_domain *p2m, gfn_t gfn,
> +                                unsigned long nr_gfns, p2m_type_t *type,
> +                                bool *valid)
>   {
> -    BUG_ON("unimplemented");
> -    return INVALID_MFN;
> +    pr_t *table = NULL;
> +    pr_t *region = NULL;
> +    uint8_t idx = INVALID_REGION_IDX;
> +    gfn_t egfn = gfn_add(gfn, nr_gfns);
> +    p2m_type_t optional_type = p2m_invalid;
> +    bool optional_valid = false;
> +    mfn_t ret = INVALID_MFN;
> +
> +    ASSERT(p2m_is_locked(p2m));
> +
> +    /*
> +     * Check if the ending gfn is higher than the highest the p2m map
> +     * currently holds, or the starting gfn lower than the lowest it holds.
> +     */
> +    if ( (gfn_x(egfn) > gfn_x(p2m->max_mapped_gfn)) ||
> +         (gfn_x(gfn) < gfn_x(p2m->lowest_mapped_gfn)) )
> +        goto out;
> +
> +    table = (pr_t *)page_to_virt(p2m->root);
> +    /* The table should always be non-NULL and is always present. */
> +    if ( !table )
> +        ASSERT_UNREACHABLE();
> +
> +    if ( is_gfns_mapped(table, p2m->nr_regions, gfn, nr_gfns, &idx) )
> +        goto out;
ASSERT_UNREACHABLE() is a no-op in a release build, so on a NULL table you
fall straight through and dereference it in is_gfns_mapped(). Add a
goto out (as __p2m_set_entry already does with its return -EINVAL).

> +
> +    region = &table[idx];
> +    if ( region_is_p2m_valid(region) )
> +    {
> +        optional_type = region_get_p2m(region);
> +        optional_valid = region_is_valid(region);
> +    }
> +
> +    /* Always GFN == MFN on MPU systems. */
> +    ret = _mfn(gfn_x(gfn));
> +
> +out:
> +    if ( type )
> +        *type = optional_type;
> +
> +    if ( valid )
> +        *valid = optional_valid;
> +
> +    return ret;
> +}
> +
> +/*
> + * Get the details of a given gfn.
> + *
> + * If the entry is present, the associated MFN will be returned and the
> + * p2m type gets filled up.
> + * If the entry is not present, INVALID_MFN will be returned
> + *
> + * The page_order is meaningless in MPU system, and we keep it here
> + * to be compatible with MMU system.
> + */
> +mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
> +                    p2m_type_t *t, p2m_access_t *a,
> +                    unsigned int *page_order,
> +                    bool *valid)
> +{
> +    return p2m_get_mpu_region(p2m, gfn, 1, t, valid);
The a (p2m_access_t *) out-parameter is never written. A caller that 
passes a
non-NULL a reads stale stack. Either fill it or document why access is not
tracked on MPU.

>   }
>   
>   void p2m_dump_info(struct domain *d)
- Ayan