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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> @@ -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? And is this tree really about
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).

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

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

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

> +#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?

Jan