Introduce an initial implementation of guest_physmap_add_entry() on RISC-V
by adding a basic framework to insert guest physical memory mappings.
This allows mapping a range of GFNs to MFNs using a placeholder
p2m_set_entry() function, which currently returns -EOPNOTSUPP.
Changes included:
- Promoting guest_physmap_add_entry() from a stub to a functional
interface calling a new p2m_insert_mapping() helper.
- Adding map_regions_p2mt() for generic mapping purposes.
- Introducing p2m_insert_mapping() and a skeleton for p2m_set_entry() to
prepare for future support of actual page table manipulation.
- Enclosing the actual mapping logic within
p2m_write_lock() / p2m_write_unlock() to ensure safe concurrent
updates to the P2M.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v2:
- This changes were part of "xen/riscv: implement p2m mapping functionality".
No additional signigicant changes were done.
---
xen/arch/riscv/include/asm/p2m.h | 12 ++++------
xen/arch/riscv/p2m.c | 41 ++++++++++++++++++++++++++++++++
2 files changed, 45 insertions(+), 8 deletions(-)
diff --git a/xen/arch/riscv/include/asm/p2m.h b/xen/arch/riscv/include/asm/p2m.h
index 0c05b58992..af2025b9fd 100644
--- a/xen/arch/riscv/include/asm/p2m.h
+++ b/xen/arch/riscv/include/asm/p2m.h
@@ -118,14 +118,10 @@ 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;
-}
+int guest_physmap_add_entry(struct domain *d,
+ gfn_t gfn, mfn_t mfn,
+ unsigned long page_order,
+ p2m_type_t t);
/* Untyped version for RAM only, for compatibility */
static inline int __must_check
diff --git a/xen/arch/riscv/p2m.c b/xen/arch/riscv/p2m.c
index 2419a61d8c..cea37c8bda 100644
--- a/xen/arch/riscv/p2m.c
+++ b/xen/arch/riscv/p2m.c
@@ -324,3 +324,44 @@ int p2m_set_allocation(struct domain *d, unsigned long pages, bool *preempted)
return 0;
}
+
+static int p2m_set_entry(struct p2m_domain *p2m,
+ gfn_t sgfn,
+ unsigned long nr,
+ mfn_t smfn,
+ p2m_type_t t,
+ p2m_access_t a)
+{
+ return -EOPNOTSUPP;
+}
+
+static int p2m_insert_mapping(struct domain *d, gfn_t start_gfn,
+ 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);
+}
+
+int guest_physmap_add_entry(struct domain *d,
+ gfn_t gfn,
+ mfn_t mfn,
+ unsigned long page_order,
+ p2m_type_t t)
+{
+ return p2m_insert_mapping(d, gfn, (1 << page_order), mfn, t);
+}
--
2.49.0On 10.06.2025 15:05, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/p2m.c
> +++ b/xen/arch/riscv/p2m.c
> @@ -324,3 +324,44 @@ int p2m_set_allocation(struct domain *d, unsigned long pages, bool *preempted)
>
> return 0;
> }
> +
> +static int p2m_set_entry(struct p2m_domain *p2m,
> + gfn_t sgfn,
> + unsigned long nr,
> + mfn_t smfn,
> + p2m_type_t t,
> + p2m_access_t a)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> +static int p2m_insert_mapping(struct domain *d, gfn_t start_gfn,
This likely again wants to be struct p2m_domain *.
> + 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. ...
> +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.
> +{
> + return p2m_insert_mapping(d, gfn, (1 << page_order), mfn, t);
1UL please, while at the same time the parentheses could be omitted.
Jan
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.
>> +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.
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.
Currently, this functionality is not fully operational, as p2m_set_entry()
still returns -EOPNOTSUPP.
Additionally, introduce p2m_write_(un)lock() to protect modifications to
the p2m page tables, along with p2m TLB flush helpers to ensure proper
TLB invalidation (if necessary) when the p2m lock is released.
~ Oleksii
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
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
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
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 */
~ Oleksii
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
© 2016 - 2025 Red Hat, Inc.