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

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

On 11-May-26 12:39, Luca Fancellu wrote:
> Hi Michal,
> 
> 
>>> 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
> 
> right, I’ll include
> 
>>
>>> +#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.
> 
> I did it on purpose since it’s a compiler extension, but if you say it’s ok 
> I’ll change it to void.
Yes, this is a documented extension in our MISRA language toolchain and as you
can see, we use it in our code base.

BTW, please also ...

> 
>>> +    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;
... drop this redundant assignment.

~Michal