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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 8/5/25 5:20 PM, Jan Beulich wrote:

> 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.
>     
In the current one context it is host p2m. I can update the comment with:
"guest's hostp2m".

> > --- 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?
>     
Not really, probably, I misunderstood you before.

Previously, I only had p2m_force_tlb_flush_sync() here, instead of
p2m_tlb_flush_sync(), and the latter includes a condition check on
p2m->need_flush.

> > @@ -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.
>     
Good point.
Actually, I think that it is enough to have map_regions_p2mt() as it is used
for dom0less common code, and re-use it every where potentially p2m_insert_mapping()
will be needed.
~ Oleksii