Re: [PATCH v1 2/6] xen/riscv: introduce things necessary for p2m initialization

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.05.2025 17:57, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/domain.h
> +++ b/xen/arch/riscv/include/asm/domain.h
> @@ -5,6 +5,8 @@
>  #include <xen/xmalloc.h>
>  #include <public/hvm/params.h>
>  
> +#include <asm/p2m.h>
> +
>  struct hvm_domain
>  {
>      uint64_t              params[HVM_NR_PARAMS];
> @@ -16,8 +18,12 @@ struct arch_vcpu_io {
>  struct arch_vcpu {
>  };
>  
> +

Nit: As before, no double blank lines please.

>  struct arch_domain {
>      struct hvm_domain hvm;
> +
> +    struct p2m_domain p2m;
> +
>  };

Similarly, no blank lines please at the end of a struct/union/enum.

> --- a/xen/arch/riscv/include/asm/mm.h
> +++ b/xen/arch/riscv/include/asm/mm.h
> @@ -149,6 +149,10 @@ extern struct page_info *frametable_virt_start;
>  #define mfn_to_page(mfn)    (frametable_virt_start + mfn_x(mfn))
>  #define page_to_mfn(pg)     _mfn((pg) - frametable_virt_start)
>  
> +/* Convert between machine addresses and page-info structures. */
> +#define maddr_to_page(ma) mfn_to_page(maddr_to_mfn(ma))
> +#define page_to_maddr(pg) (mfn_to_maddr(page_to_mfn(pg)))

Nit: The outermost parentheses aren't really needed here. Or if you really
want them, then please be consistent and also add them for the other macro
you add.

> --- /dev/null
> +++ b/xen/arch/riscv/p2m.c
> @@ -0,0 +1,168 @@
> +#include <xen/domain_page.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 <asm/page.h>
> +#include <asm/p2m.h>
> +
> +/*
> + * Force a synchronous P2M TLB flush.
> + *
> + * Must be called with the p2m lock held.
> + *
> + * TODO: add support of flushing TLB connected to VMID.
> + */
> +static void p2m_force_tlb_flush_sync(struct p2m_domain *p2m)
> +{
> +    ASSERT(p2m_is_write_locked(p2m));
> +
> +    /*
> +     * TODO: shouldn't be this flush done for each physical CPU?
> +     *       If yes, then SBI call sbi_remote_hfence_gvma() could
> +     *       be used for that.
> +     */
> +#if defined(__riscv_hh) || defined(__riscv_h)

This is a compiler capability check (which would be applicable if you
used a built-in function below).

> +    asm volatile ( "hfence.gvma" ::: "memory" );

Whereas here you use a feature in the assembler, at least for the GNU
toolchain.

> +static void clear_and_clean_page(struct page_info *page)
> +{
> +    void *p = __map_domain_page(page);
> +
> +    clear_page(p);
> +    unmap_domain_page(p);
> +}

What's the "clean" about in the function name? The "clear" is referring
to the clear_page() call afaict. Also aren't you largely open-coding
clear_domain_page() here?

> +static struct page_info *p2m_get_clean_page(struct domain *d)
> +{
> +    struct page_info *page;
> +
> +    /*
> +     * As mentioned in the Priviliged Architecture Spec (version 20240411)
> +     * As explained in Section 18.5.1, for the paged virtual-memory schemes
> +     * (Sv32x4, Sv39x4, Sv48x4, and Sv57x4), the root page table is 16 KiB
> +     * and must be aligned to a 16-KiB boundary.
> +     */
> +    page = alloc_domheap_pages(NULL, 2, 0);

Shouldn't this allocation come from the domain's P2M pool (which is yet
to be introduced)? Also hard-coding 2 here as order effectively builds
in an assumption that PAGE_SIZE will only ever be 4k. I think to wants
properly calculating instead.

> +    if ( page == NULL )
> +        return NULL;
> +
> +    clear_and_clean_page(page);
> +
> +    return page;
> +}

Contrary to the function name you obtained 4 pages here, which is suitable
for ...

> +static struct page_info *p2m_allocate_root(struct domain *d)
> +{
> +    return p2m_get_clean_page(d);
> +}

... this but - I expect - no anywhere else.

> +static unsigned long hgatp_from_page_info(struct page_info *page_info)

Except for the struct name please drop _info; we don#t use such anywhere
else.

> +{
> +    unsigned long ppn;
> +    unsigned long hgatp_mode;
> +
> +    ppn = PFN_DOWN(page_to_maddr(page_info)) & HGATP_PPN;
> +
> +    /* ASID (VMID) not supported yet */
> +
> +#if RV_STAGE1_MODE == SATP_MODE_SV39
> +    hgatp_mode = HGATP_MODE_SV39X4;
> +#elif RV_STAGE1_MODE == SATP_MODE_SV48
> +    hgatp_mode = HGATP_MODE_SV48X4;
> +#else
> +    #error "add HGATP_MODE"

As before, please have the # of pre-processor directives in the first column.

> +#endif
> +
> +    return ppn | (hgatp_mode << HGATP_MODE_SHIFT);

Use MASK_INSR()?

> +}
> +
> +static int p2m_alloc_table(struct domain *d)
> +{
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +
> +    p2m->root = p2m_allocate_root(d);
> +    if ( !p2m->root )
> +        return -ENOMEM;
> +
> +    p2m->hgatp = hgatp_from_page_info(p2m->root);
> +
> +    /*
> +     * Make sure that all TLBs corresponding to the new VMID are flushed
> +     * before using it.
> +     */
> +    p2m_write_lock(p2m);
> +    p2m_force_tlb_flush_sync(p2m);
> +    p2m_write_unlock(p2m);

While Andrew directed you towards a better model in general, it won't be
usable here then, as the guest didn't run on any pCPU(s) yet. Imo you
want to do a single global flush e.g. when VMIDs wrap around. That'll be
fewer global flushes than one per VM creation.

> +int p2m_init(struct domain *d)
> +{
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    int rc;
> +
> +    rwlock_init(&p2m->lock);
> +    INIT_PAGE_LIST_HEAD(&p2m->pages);
> +
> +    p2m->max_mapped_gfn = _gfn(0);
> +    p2m->lowest_mapped_gfn = _gfn(ULONG_MAX);

You don't ever read these two fields. Likely better to introduce them when
they're actually needed. Same possibly for further things done in this
function.

> +    p2m->default_access = p2m_access_rwx;
> +
> +    radix_tree_init(&p2m->p2m_type);
> +
> +#ifdef 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);
> +#else
> +    p2m->clean_pte = true;

When there's no IOMMU (in use), doesn't this want to be "false"?

> +#endif
> +
> +    /*
> +     * "Trivial" initialisation is now complete.  Set the backpointer so
> +     * p2m_teardown() and friends know to do something.
> +     */
> +    p2m->domain = d;

And where is that p2m_teardown(), to cross-check the comment against?

Jan