Re: [PATCH v2 10/17] xen/riscv: implement guest_physmap_add_entry() for mapping GFNs to MFNs

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 7/3/25 1:33 PM, Jan Beulich wrote:

> On 03.07.2025 13:02, Oleksii Kurochko wrote:
> > On 6/30/25 5:59 PM, Jan Beulich wrote:
> > > On 10.06.2025 15:05, Oleksii Kurochko wrote:
> > > > +                              unsigned long nr, mfn_t mfn, p2m_type_t t)
> > > > +{
> > > > +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > > > +    int rc;
> > > > +
> > > > +    p2m_write_lock(p2m);
> > > > +    rc = p2m_set_entry(p2m, start_gfn, nr, mfn, t, p2m->default_access);
> > > > +    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(d, gfn, nr, mfn, p2mt);
> > > > +}
> > > >           
> > > What is this function doing here? The description says "for generic mapping
> > > purposes", which really may mean anything. Plus, if and when you need it, it
> > > wants to come with a name that fits with e.g. ...
> > >         
> > These names are used across the common code and various architectures. Not all
> > architectures need to implement all of these functions.
> > I believe|guest_physmap_add_page()| (which internally calls|guest_physmap_add_entry()|)
> > is needed to be implemented for all architectures, while|map_regions_p2mt()| is used
> > by Arm and the common Dom0less-related code, and because of RISC-V is going to re-use
> > common Dom0less code it is implementing this function too.
> >       
> First, my comment was solely about this one function above. And then I didn't
> even know Arm had such a function. It's not used from common code (except again
> from dom0less code where it should have been better abstracted, imo). I'm also
> not surprised I wasn't aware of it since, as can be implied from the above,
> otherwise I would likely have complained about its name not fitting the general
> scheme (which isn't all that good either).
>     
If I'm right, there is nothing similar to map_regions_p2mt() in the common headers.

Anyway, I think we could follow up with a patch to rename this function to
something more appropriate.
I was thinking about adding something like map_regions_to_guest(), map_p2m_regions(),
or map_p2m_memory() to xen/mm.h, along with proper renaming in the Arm code.
Does that make sense?

> > > > +int guest_physmap_add_entry(struct domain *d,
> > > > +                            gfn_t gfn,
> > > > +                            mfn_t mfn,
> > > > +                            unsigned long page_order,
> > > > +                            p2m_type_t t)
> > > >           
> > > ... this one, to understand their relationship / difference.
> > >         
> > Basically, the difference is only in API and where they are expected to be used:
> > - guest_physmap_add_entry() to map and set a specific p2m type for a page.
> > - map_regions_p2mt() to map a region (mostly MMIO) in the guest p2m with
> >    a specific p2m type.
> >       
> Sorry, from this description they still look basically identical to me. The
> visible difference being that one takes a "nr" argument and the other a
> "page_order" one. Which still makes them largely redundant, and which still
> suggests that the earlier one's name doesn't really fit.
>
> > I added both of them here as they are implemented in a similar way.
> > I will re-word commit subject and message:
> >    xen/riscv: implement functions to map memory in guest p2m
> >
> >    Introduce guest_physmap_add_entry() to map a page and assign a specific
> >    p2m type, and map_regions_p2mt() to map a region (typically MMIO) in
> >    the guest p2m with a designated p2m type.
> >       
> I.e., as per above, two functions for basically the same purpose.
>     
Generally, I agree that the purpose is the same.

I will then just drop guest_physmap_add_entry() and use only
map_regions_p2mt() (or whatever name we decide on).
~ Oleksii