Re: [PATCH v3 11/20] xen/riscv: implement function to map memory in guest p2m

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.07.2025 17:58, Oleksii Kurochko wrote:
> Implement map_regions_p2mt() to map a region in the guest p2m with
> a specific p2m type. The memory attributes will be derived from the
> p2m type. This function is going to be called from dom0less common
> code.

s/is going to be/is/ ? Such a call exists already, after all.

> --- a/xen/arch/riscv/include/asm/p2m.h
> +++ b/xen/arch/riscv/include/asm/p2m.h
> @@ -121,21 +121,22 @@ static inline int 
> guest_physmap_mark_populate_on_demand(struct domain *d,
>      return -EOPNOTSUPP;
>  }
>  
> -static inline int guest_physmap_add_entry(struct domain *d,
> -                                          gfn_t gfn, mfn_t mfn,
> -                                          unsigned long page_order,
> -                                          p2m_type_t t)
> -{
> -    BUG_ON("unimplemented");
> -    return -EINVAL;
> -}
> +/*
> + * Map a region in the guest p2m with a specific p2m type.

What is "the guest p2m"? In your answer, please consider the possible
(and at some point likely necessary) existence of altp2m and nestedp2m.
In patch 04 you introduce p2m_get_hostp2m(), and I expect it's that
what you mean here.

> --- a/xen/arch/riscv/p2m.c
> +++ b/xen/arch/riscv/p2m.c
> @@ -9,6 +9,41 @@
>  
>  unsigned int __read_mostly p2m_root_order;
>  
> +/*
> + * 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;

Pointer-to-const please. Personally, given the implementation of this
function (and also ...

> +    ASSERT(p2m_is_write_locked(p2m));
> +
> +    sbi_remote_hfence_gvma(d->dirty_cpumask, 0, 0);
> +
> +    p2m->need_flush = false;
> +}
> +
> +void p2m_tlb_flush_sync(struct p2m_domain *p2m)
> +{
> +    if ( p2m->need_flush )
> +        p2m_force_tlb_flush_sync(p2m);
> +}

... this one) I'd further ask for the function parameters to also be
pointer-to-const, but Andrew may object to that. Andrew - it continues to
be unclear to me under what conditions you agree with adding const, and
under what conditions you would object to me asking for such. Please can
you take the time to clarify this?

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

Nit: Stray 'w'.

> +     * completed.
> +     */
> +    p2m_tlb_flush_sync(p2m);

Wasn't the plan to have this be conditional?

> @@ -139,3 +174,33 @@ int p2m_set_allocation(struct domain *d, unsigned long 
> pages, bool *preempted)
>  
>      return 0;
>  }
> +
> +static int p2m_set_range(struct p2m_domain *p2m,
> +                         gfn_t sgfn,
> +                         unsigned long nr,
> +                         mfn_t smfn,
> +                         p2m_type_t t)
> +{
> +    return -EOPNOTSUPP;
> +}
> +
> +static int p2m_insert_mapping(struct p2m_domain *p2m, gfn_t start_gfn,
> +                              unsigned long nr, mfn_t mfn, p2m_type_t t)
> +{
> +    int rc;
> +
> +    p2m_write_lock(p2m);
> +    rc = p2m_set_range(p2m, start_gfn, nr, mfn, t);
> +    p2m_write_unlock(p2m);
> +
> +    return rc;
> +}
> +
> +int map_regions_p2mt(struct domain *d,
> +                     gfn_t gfn,
> +                     unsigned long nr,
> +                     mfn_t mfn,
> +                     p2m_type_t p2mt)
> +{
> +    return p2m_insert_mapping(p2m_get_hostp2m(d), gfn, nr, mfn, p2mt);
> +}

And eventually both helper functions will gain further callers? Otherwise
it's a little hard to see why they would both need to be separate functions.

Jan