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).
>>> +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.
Jan
