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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.07.2025 13:54, Oleksii Kurochko wrote:
> 
> 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?

Imo that seemingly redundant function (i.e. if it's really needed) would want
to be named guest_physmap_<whatever>().

Jan