Re: [PATCH v2 04/17] xen/riscv: construct the P2M pages pool for guests

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.06.2025 16:48, Oleksii Kurochko wrote:
> 
> On 6/18/25 5:53 PM, Jan Beulich wrote:
>> On 10.06.2025 15:05, Oleksii Kurochko wrote:
>>> @@ -18,10 +20,20 @@ struct arch_vcpu_io {
>>>   struct arch_vcpu {
>>>   };
>>>   
>>> +struct paging_domain {
>>> +    spinlock_t lock;
>>> +    /* Free P2M pages from the pre-allocated P2M pool */
>>> +    struct page_list_head p2m_freelist;
>>> +    /* Number of pages from the pre-allocated P2M pool */
>>> +    unsigned long p2m_total_pages;
>>> +};
>>> +
>>>   struct arch_domain {
>>>       struct hvm_domain hvm;
>>>   
>>>       struct p2m_domain p2m;
>>> +
>>> +    struct paging_domain paging;
>> With the separate structures, do you have plans to implement e.g. shadow 
>> paging?
>> Or some other paging mode beyond the basic one based on the H extension?
> 
> No, there is no such plans.
> 
>>   If the
>> structures are to remain separate, may I suggest that you keep things 
>> properly
>> separated (no matter how e.g. Arm may have it) in terms of naming? I.e. no
>> single "p2m" inside struct paging_domain.
> 
> Arm doesn't implement shadow paging too (AFAIK) and probably this approach was
> copied from x86, and then to RISC-V.
> I thought that a reason for that was just to have two separate entities: one 
> which
> covers page tables and which covers the full available guest memory.
> And if the only idea of that was to have shadow paging then I don't how it 
> should
> be done better. As p2m code is based on Arm's, perhaps, it makes sense to have
> this stuff separated, so easier porting will be.
> 
>>> @@ -105,6 +106,9 @@ int p2m_init(struct domain *d)
>>>       struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>>       int rc;
>>>   
>>> +    spin_lock_init(&d->arch.paging.lock);
>>> +    INIT_PAGE_LIST_HEAD(&d->arch.paging.p2m_freelist);
>> If you want p2m and paging to be separate, you will want to put these in a 
>> new
>> paging_init().
> 
> I am not really understand what is wrong to have it here, but likely it is 
> because
> I don't really get an initial purpose of having p2m and paging separately.
> It seems like p2m and paging are connected between each other, so it is fine
> to init them together.

If you want to retain the separation, imo you want to follow what x86 has:
paging_domain_init() calling p2m_init(). And d->arch.paging.* would then
be initialized in paging_domain_init(), like x86 has it.

Jan