Re: [PATCH v2 05/17] xen/riscv: introduce things necessary for p2m initialization

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.06.2025 17:31, Oleksii Kurochko wrote:
> On 6/18/25 6:08 PM, Jan Beulich wrote:
>> On 10.06.2025 15:05, Oleksii Kurochko wrote:
>>> @@ -14,6 +18,29 @@
>>>   
>>>   /* Per-p2m-table state */
>>>   struct p2m_domain {
>>> +    /*
>>> +     * Lock that protects updates to the p2m.
>>> +     */
>>> +    rwlock_t lock;
>>> +
>>> +    /* Pages used to construct the p2m */
>>> +    struct page_list_head pages;
>>> +
>>> +    /* Indicate if it is required to clean the cache when writing an entry 
>>> */
>>> +    bool clean_pte;
>>> +
>>> +    struct radix_tree_root p2m_type;
>> A field with a p2m_ prefix in a p2m struct?
> 
> p2m_ prefix could be really dropped.
> 
>>   And is this tree really about
>> just a single "type"?
> 
> Yes, we don't have enough bits in PTE so we need some extra storage to store 
> type.

My question wasn't about that, though. My question was whether in the name
"type" (singular) is appropriate. I didn't think you need a tree to store just
a single type.

>>> +    /*
>>> +     * Default P2M access type for each page in the the domain: new pages,
>>> +     * swapped in pages, cleared pages, and pages that are ambiguously
>>> +     * retyped get this access type.  See definition of p2m_access_t.
>>> +     */
>>> +    p2m_access_t default_access;
>>> +
>>> +    /* Back pointer to domain */
>>> +    struct domain *domain;
>> This you may want to introduce earlier, to prefer passing around struct
>> p2m_domain * in / to P2M functions (which would benefit earlier patches
>> already, I think).
> 
> But nothing uses it earlier.

If you do as suggested and pass around struct p2m_domain * for p2m_*()
functions, you'll quickly find it used, I think.

>>> --- a/xen/arch/riscv/p2m.c
>>> +++ b/xen/arch/riscv/p2m.c
>>> @@ -1,13 +1,46 @@
>>>   #include <xen/bitops.h>
>>> +#include <xen/domain_page.h>
>>>   #include <xen/event.h>
>>> +#include <xen/iommu.h>
>>>   #include <xen/lib.h>
>>> +#include <xen/mm.h>
>>> +#include <xen/pfn.h>
>>> +#include <xen/rwlock.h>
>>>   #include <xen/sched.h>
>>>   #include <xen/spinlock.h>
>>>   #include <xen/xvmalloc.h>
>>>   
>>> +#include <asm/page.h>
>>>   #include <asm/p2m.h>
>>>   #include <asm/sbi.h>
>>>   
>>> +/*
>>> + * Force a synchronous P2M TLB flush.
>>> + *
>>> + * Must be called with the p2m lock held.
>>> + */
>>> +static void p2m_force_tlb_flush_sync(struct p2m_domain *p2m)
>>> +{
>>> +    struct domain *d = p2m->domain;
>>> +
>>> +    ASSERT(p2m_is_write_locked(p2m));
>>> +
>>> +    sbi_remote_hfence_gvma_vmid(d->dirty_cpumask, 0, 0, p2m->vmid);
>>> +}
>>> +
>>> +/* Unlock the flush and do a P2M TLB flush if necessary */
>>> +void p2m_write_unlock(struct p2m_domain *p2m)
>>> +{
>>> +    /*
>>> +     * The final flush is done with the P2M write lock taken to avoid
>>> +     * someone else modifying the P2M wbefore the TLB invalidation has
>>> +     * completed.
>>> +     */
>>> +    p2m_force_tlb_flush_sync(p2m);
>> The comment ahead of the function says "if necessary". Yet there's no
>> conditional here. I also question the need for a global flush in all
>> cases.
> 
> Stale comment.
> 
> But if p2m page table was modified that it is needed to do a flush for CPUs
> in d->dirty_cpumask.

Right, but is that true for each and every case where you acquire the
lock in write mode? There may e.g. be early-out path which end up doing
nothing, yet you would then still flush the TLB.

>>> @@ -109,8 +142,33 @@ int p2m_init(struct domain *d)
>>>       spin_lock_init(&d->arch.paging.lock);
>>>       INIT_PAGE_LIST_HEAD(&d->arch.paging.p2m_freelist);
>>>   
>>> +    rwlock_init(&p2m->lock);
>>> +    INIT_PAGE_LIST_HEAD(&p2m->pages);
>>> +
>>>       p2m->vmid = INVALID_VMID;
>>>   
>>> +    p2m->default_access = p2m_access_rwx;
>>> +
>>> +    radix_tree_init(&p2m->p2m_type);
>>> +
>>> +#ifdef CONFIG_HAS_PASSTHROUGH
>> Do you expect this to be conditionally selected on RISC-V?
> 
> No, once it will be implemented it will be just selected once by config 
> RISC-V.
> And it was done so because iommu_has_feature() isn't implemented now as IOMMU
> isn't supported now and depends on CONFIG_HAS_PASSTHROUGH.

If the selection isn't going to be conditional, then I see no reason to have
such conditionals in RISC-V-specific code. The piece of code presently inside
that #ifdef may simply need adding later, once there's enough infrastructure
to allow that code to compile. Or maybe it would even compile fine already now?

>>> +    /*
>>> +     * Some IOMMUs don't support coherent PT walk. When the p2m is
>>> +     * shared with the CPU, Xen has to make sure that the PT changes have
>>> +     * reached the memory
>>> +     */
>>> +    p2m->clean_pte = is_iommu_enabled(d) &&
>>> +        !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);
>> The comment talks about shared page tables, yet you don't check whether
>> page table sharing is actually enabled for the domain.
> 
> Do we have such function/macros?

We have iommu_hap_pt_share, and we have the per-domain hap_pt_share flag.

> It is shared by implementation now.

I don't understand. There's no IOMMU support yet for RISC-V. Hence it's in
neither state - not shared, but also not not shared.

Jan