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 17, 2023, at 00:41, Julien Grall <julien@xxxxxxx> wrote:
> On 14/10/2023 02:26, Henry Wang wrote:
>> Hi Julien,
> 
> Hi Henry,
> 
>>> 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?
> 
> My original intention was to ask to move them right now. But if it is unclear 
> whether they would be used, then it would be best to defer until we have a 
> better understanding.

Sure, I think I will defer it then.

> 
>> 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…
> 
> I am not sure I fully understand why would the trap handlers not called. Is 
> this suggesting that a dom0less domUs can not use set/way instructions?

No I was wrong, sorry for the confusion, I somehow “mis-linked” the “trap” to 
“panic” :/

But as I saw your reply above, so I think probably we can defer the movement of 
these functions
to further discussions in MPU series.

Kind regards,
Henry


> 
> Cheers,
> 
> -- 
> Julien Grall