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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/18/25 6:08 PM, Jan Beulich wrote:

> On 10.06.2025 15:05, Oleksii Kurochko wrote:
> > Introduce the following things:
> > - Update p2m_domain structure, which describe per p2m-table state, with:
> >   - lock to protect updates to p2m.
> >   - pool with pages used to construct p2m.
> >   - clean_pte which indicate if it is requires to clean the cache when
> >     writing an entry.
> >   - radix tree to store p2m type as PTE doesn't have enough free bits to
> >     store type.
> >   - default_access to store p2m access type for each page in the domain.
> >   - back pointer to domain structure.
> > - p2m_init() to initalize members introduced in p2m_domain structure.
> > - Introudce p2m_write_lock() and p2m_is_write_locked().
> >       
> What about the reader variant? If you don't need that, why not use a simple
> spin lock?
>     
It will be introduced later in "xen/riscv: add support of page lookup by GFN"
of this patch series where it is really used.

But I can move it here.

> > @@ -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.

> > +    /*
> > +     * 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.

> > --- 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.

> > @@ -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.
> > +    /*
> > +     * 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? It is shared by implementation now.

> > +#else
> > +    p2m->clean_pte = false;
> >       
> I hope the struct starts out zero-filled, in which case you wouldn't need
> this.
>
> > +#endif
> > +
> > +    /*
> > +     * "Trivial" initialisation is now complete.  Set the backpointer so the
> > +     * users of p2m could get an access to domain structure.
> > +     */
> > +    p2m->domain = d;
> >       
> Better set this about the very first thing?
>     
It makes sense. I will move it up.

Thanks.

~ Oleksii