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
