Re: [PATCH v7 8/8] xen/arm: mmu: move MMU specific P2M code to mmu/p2m.{c,h}

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> On Oct 14, 2023, at 02:22, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Henry,
> 
> On 09/10/2023 02:03, Henry Wang wrote:
>> From: Penny Zheng <penny.zheng@xxxxxxx>
>> Current P2M implementation is designed for MMU system only.
>> We move the MMU-specific codes into mmu/p2m.c, and only keep generic
>> codes in p2m.c, like VMID allocator, etc. We also move MMU-specific
>> definitions and declarations to mmu/p2m.h, such as p2m_tlb_flush_sync().
>> Also expose previously static functions p2m_vmid_allocator_init(),
>> p2m_alloc_vmid(), and setup_virt_paging_one() for further MPU usage.
>> With the code movement, global variable max_vmid is used in multiple
>> files instead of a single file (and will be used in MPU P2M
>> implementation), declare it in the header and remove the "static" of
>> this variable.
>> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
>> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
>> Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
> 
> Some remarks about some of the code not moved:
> * struct p2m_domain: The bulk of the fields seems to be MMU specific. So 
> depending on the number of common fields we either want to split or move the 
> structure to p2m_domain. I would be ok to wait until the MPU code is present.
> * p2m_type_t: It is not yet clear how this will apply to the MPU. I am ok to 
> wait before moving it.

Agree with both here, let’s continue the discussion in the actual MPU patch for 
P2M
then, but I am then a bit confused about...

> * p2m_cache_flush_range(): I expect the code will need some change because 
> you may get large chunk of memory for the MPU.
> * p2m_set_way_flush()/p2m_toggle_cache(): This was a giant hack to support 
> cache flush operations via set/way. To make it efficient, we track the pages 
> that have been touched and only flush them. For the MPU, this would not work. 
> Can we attempt to not emulate the instructions?

…these two remarks here, do you expect me to do some changes with these three
functions in this patch? Or we can defer the required changes to the MPU patch 
for
P2M? 

I think I am highly likely to make a mistake here but I took a look at the MPU
implementation [1] and it looks like the MPU code can use these tree functions
without changes - probably because these functions are simply used by
(1) domctl and we only have dom0less DomUs on MPU
(2) trap handlers
which means these functions are simply not called…

So maybe moving these three functions to mmu/p2m.c would be a good idea for
this patch?

> 
>> ---
>> diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
>> index 940495d42b..a9622dac9a 100644
>> --- a/xen/arch/arm/include/asm/p2m.h
>> +++ b/xen/arch/arm/include/asm/p2m.h
>> @@ -19,6 +19,22 @@ extern unsigned int p2m_root_level;
>>  #define P2M_ROOT_ORDER    p2m_root_order
> 
> You seem to use P2M_ROOT_ORDER to allocate p2m->root in arm/p2m.c. However, 
> as I mentioned before, I don't think the defintion of p2m->root is suitable 
> for the MPU. I think the two functions using p2m->root should be moved in 
> mmu/p2m.c and P2M_ROOT_ORDER should be moved in mmu/p2m.h.

Sure, will follow that, sorry for missing this part.

> 
>>  #define P2M_ROOT_LEVEL p2m_root_level
> 
> P2M_ROOT_LEVEL doesn't seem to make sense for the MPU. The only use in 
> arch/arm/p2m.c seems to be in dump_p2m_lookup() which is calling an MMU 
> specific function. So I think this wants to be moved in the MMU code.

Will do in v8.

> 
>> +#define P2M_ROOT_PAGES    (1<<P2M_ROOT_ORDER)
> 
> See above.
> 
> Also NIT, I would suggest to take the opportunity to use 1U and add space 
> before/after <<.

Sounds good, will do in v8.

> 
>> +
>>  struct domain;
>>    extern void memory_type_changed(struct domain *);
>> @@ -156,6 +172,10 @@ typedef enum {
>>  #endif
>>  #include <xen/p2m-common.h>
>>  +#ifdef CONFIG_MMU
>> +#include <asm/mmu/p2m.h>
>> +#endif
>> +
>>  static inline bool arch_acquire_resource_check(struct domain *d)
>>  {
>>      /*
>> @@ -180,7 +200,11 @@ void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
>>   */
>>  void p2m_restrict_ipa_bits(unsigned int ipa_bits);
>>  +void p2m_vmid_allocator_init(void);
>> +int p2m_alloc_vmid(struct domain *d);
>> +
>>  /* Second stage paging setup, to be called on all CPUs */
>> +void setup_virt_paging_one(void *data);
> 
> I don't much like the idea to export setup_virt_paging_one(). Could we 
> instead move cpu_virt_paging_callback() & co to mmu/p2m.c?

Yeah good idea, thank you for this suggestion :) will do in v8.

[1] 
https://gitlab.com/xen-project/people/henryw/xen/-/commits/mpu_v5/?ref_type=heads

Kind regards,
Henry

> 
> Cheers,
> 
> -- 
> Julien Grall