On 03.07.2025 15:28, Oleksii Kurochko wrote:
>
> On 7/3/25 3:09 PM, Jan Beulich wrote:
>> 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>().
>
> If it is redundant what is expected to be used instead to map_regions_p2mt()
> to map MMIO,
> for example, to guest: guest_physmap_add_page()? Based on the comment above
> the definition
> of this function it is for RAM: /* Untyped version for RAM only, for
> compatibility */
But we're talking about guest_physmap_add_entry().
Jan
