Re: [PATCH v2 06/17] xen/riscv: add root page table allocation

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8472 bytes --]


On 7/1/25 12:27 PM, Jan Beulich wrote:
> On 01.07.2025 11:44, Oleksii Kurochko wrote:
>> On 7/1/25 8:29 AM, Jan Beulich wrote:
>>> On 30.06.2025 18:18, Oleksii Kurochko wrote:
>>>> On 6/30/25 5:22 PM, Jan Beulich wrote:
>>>>> On 10.06.2025 15:05, Oleksii Kurochko wrote:
>>>>>> --- a/xen/arch/riscv/include/asm/p2m.h
>>>>>> +++ b/xen/arch/riscv/include/asm/p2m.h
>>>>>> @@ -26,6 +26,12 @@ struct p2m_domain {
>>>>>>         /* Pages used to construct the p2m */
>>>>>>         struct page_list_head pages;
>>>>>>     
>>>>>> +    /* The root of the p2m tree. May be concatenated */
>>>>>> +    struct page_info *root;
>>>>>> +
>>>>>> +    /* Address Translation Table for the p2m */
>>>>>> +    paddr_t hgatp;
>>>>> Does this really need holding in a struct field? Can't is be re-created at
>>>>> any time from "root" above?
>>>> Yes, with the current one implementation, I agree it would be enough only
>>>> root. But as you noticed below...
>>>>
>>>>> And such re-creation is apparently infrequent,
>>>>> if happening at all after initial allocation. (But of course I don't know
>>>>> what future patches of yours will bring.) This is even more so if ...
>>>>>
>>>>>> --- a/xen/arch/riscv/include/asm/riscv_encoding.h
>>>>>> +++ b/xen/arch/riscv/include/asm/riscv_encoding.h
>>>>>> @@ -133,11 +133,13 @@
>>>>>>     #define HGATP_MODE_SV48X4		_UL(9)
>>>>>>     
>>>>>>     #define HGATP32_MODE_SHIFT		31
>>>>>> +#define HGATP32_MODE_MASK		_UL(0x80000000)
>>>>>>     #define HGATP32_VMID_SHIFT		22
>>>>>>     #define HGATP32_VMID_MASK		_UL(0x1FC00000)
>>>>>>     #define HGATP32_PPN			_UL(0x003FFFFF)
>>>>>>     
>>>>>>     #define HGATP64_MODE_SHIFT		60
>>>>>> +#define HGATP64_MODE_MASK		_ULL(0xF000000000000000)
>>>>>>     #define HGATP64_VMID_SHIFT		44
>>>>>>     #define HGATP64_VMID_MASK		_ULL(0x03FFF00000000000)
>>>>> ... VMID management is going to change as previously discussed, at which
>>>>> point the value to put in hgatp will need (partly) re-calculating at certain
>>>>> points anyway.
>>>> ... after VMID management will changed to per-CPU base then it will be needed
>>>> to update re-calculate hgatp each time vCPU on pCPU is changed.
>>>> In this case I prefer to have partially calculated 'hgatp'.
>>> But why, when you need to do some recalculation anyway?
>> Less operations will be needed to do.
> Right; I wonder how big the savings would be.

Probably not big.

>
>> If we have partially prepared 'hgatp' then we have to only update VMID bits
>> instead of getting ppn for page, then calculate hgatp_mode each time.
>> But if you think it isn't really needed I can add vmid argument for hgatp_from_page()
>> and just call this function when an update of hgatp is needed.
> I think it'll need to be struct p2m_domain * that you (also?) pass in. In the
> longer run I think you will want to support all three permitted modes, with
> smaller guests using fewer page table levels.

Yes, but these modes will be const for a domain, I guess. I mean that once a mode has
been set, it isn't going to be changed. But VMID is going to be changed each time vCPU
gives control to another vCPU.
Anyway, I am okay to make update of hgatp more generic and just update it fully each
time it is needed.

>
> As to "also" - maybe it's better to change the name of the function, and pass
> in just (const if possible) struct p2m_domain *.
>
>>>>>> --- a/xen/arch/riscv/p2m.c
>>>>>> +++ b/xen/arch/riscv/p2m.c
>>>>>> @@ -41,6 +41,91 @@ void p2m_write_unlock(struct p2m_domain *p2m)
>>>>>>         write_unlock(&p2m->lock);
>>>>>>     }
>>>>>>     
>>>>>> +static void clear_and_clean_page(struct page_info *page)
>>>>>> +{
>>>>>> +    clean_dcache_va_range(page, PAGE_SIZE);
>>>>>> +    clear_domain_page(page_to_mfn(page));
>>>>>> +}
>>>>> A function of this name can, imo, only clear and then clean. Question is why
>>>>> it's the other way around, and what the underlying requirement is for the
>>>>> cleaning part to be there in the first place. Maybe that's obvious for a
>>>>> RISC-V person, but it's entirely non-obvious to me (Arm being different in
>>>>> this regard because of running with caches disabled at certain points in
>>>>> time).
>>>> You're right, the current name|clear_and_clean_page()| implies that clearing
>>>> should come before cleaning, which contradicts the current implementation.
>>>> The intent here is to ensure that the page contents are consistent in RAM
>>>> (not just in cache) before use by other entities (guests or devices).
>>>>
>>>> The clean must follow the clear — so yes, the order needs to be reversed.
>>> What you don't address though - why's the cleaning needed in the first place?
>> If we clean the data cache first, we flush the d-cache and then use the page to
>> perform the clear operation. As a result, the "cleared" value will be written into
>> the d-cache. To avoid polluting the d-cache with the "cleared" value, the correct
>> sequence is to clear the page first, then clean the data cache.
> If you want to avoid cache pollution, I think you'd need to use a form of stores
> which simply bypass the cache. Yet then - why would this matter here, but not
> elsewhere? Wouldn't you better leave such to the hardware, unless you can prove
> a (meaningful) performance gain?

I thought about a case when IOMMU doesn't support coherent walks and p2m tables are
shared between CPU and IOMMU. Then my understanding is:
- clear_page(p) just zero-ing a page in a CPU's cache.
- But IOMMU can see old data or uninitialized, if they still in cache.
- So, it is need to do clean_cache() to writeback data from cache to RAM, before a
   page will be used as a part of page table for IOMMU.

>
>>>>>> +    unsigned int nr_pages = _AC(1,U) << order;
>>>>> Nit (style): Missing blank after comma.
>>>> I've changed that to BIT(order, U)
>>>>
>>>>>> +    /* Return back nr_pages necessary for p2m root table. */
>>>>>> +
>>>>>> +    if ( ACCESS_ONCE(d->arch.paging.p2m_total_pages) < nr_pages )
>>>>>> +        panic("Specify more xen,domain-p2m-mem-mb\n");
>>>>> You shouldn't panic() in anything involved in domain creation. You want to
>>>>> return NULL in this case.
>>>> It makes sense in this case just to return NULL.
>>>>
>>>>> Further, to me the use of "more" looks misleading here. Do you perhaps mean
>>>>> "larger" or "bigger"?
>>>>>
>>>>> This also looks to be happening without any lock held. If that's intentional,
>>>>> I think the "why" wants clarifying in a code comment.
>>>> Agree, returning back pages necessary for p2m root table should be done under
>>>> spin_lock(&d->arch.paging.lock).
>>> Which should be acquired at the paging_*() layer then, not at the p2m_*() layer.
>>> (As long as you mean to have that separation, that is. See the earlier discussion
>>> on that matter.)
>> Then partly p2m_set_allocation() should be moved to paging_*() too.
> Not exactly sure what you mean. On x86 at least the paging layer part of
> the function is pretty slim.

I meant that part of code which is spin_lock(&d->arch.paging.lock); ... spin_unlock(&d->arch.paging.lock)
in function p2m_set_allocation() should be moved somewhere to paging_*() layer for the same logic as you
suggested to move part of  p2m_allocate_root()'s code which is guarded by d->arch.paging.lock to
paging_*() layer.

Or I just misunderstood your initial idea with this paging_*() layer and its necessity.

>
>>>>>> +    for ( unsigned int i = 0; i < nr_pages; i++ )
>>>>>> +    {
>>>>>> +        /* Return memory to domheap. */
>>>>>> +        page = page_list_remove_head(&d->arch.paging.p2m_freelist);
>>>>>> +        if( page )
>>>>>> +        {
>>>>>> +            ACCESS_ONCE(d->arch.paging.p2m_total_pages)--;
>>>>>> +            free_domheap_page(page);
>>>>>> +        }
>>>>>> +        else
>>>>>> +        {
>>>>>> +            printk(XENLOG_ERR
>>>>>> +                   "Failed to free P2M pages, P2M freelist is empty.\n");
>>>>>> +            return NULL;
>>>>>> +        }
>>>>>> +    }
>>>>> The reason for doing this may also want to be put in a comment.
>>>> I thought it would be enough the comment above: /* Return back nr_pages necessary for p2m root table. */
>>> That describes what the code does, but not why.
>> I will add to the comment: "... to get the memory accounting right".
> I'm sorry to be picky, but what is "right"? You want assure the root table
> memory is also accounted against the P2M pool of the domain. Can't you say
> exactly that?

It can be said in this way.

Thanks.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 12478 bytes --]