Implement map_regions_p2mt() to map a region in the guest p2m with
a specific p2m type. The memory attributes will be derived from the
p2m type. This function is going to be called from dom0less common
code.
To implement it, introduce:
- p2m_write_(un)lock() to ensure safe concurrent updates to the P2M.
As part of this change, introduce p2m_tlb_flush_sync() and
p2m_force_tlb_flush_sync().
- A stub for p2m_set_range() to map a range of GFNs to MFNs.
- p2m_insert_mapping().
- p2m_is_write_locked().
Drop guest_physmap_add_entry() and call map_regions_p2mt() directly
from guest_physmap_add_page(), making guest_physmap_add_entry()
unnecessary.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v3:
- Introudce p2m_write_lock() and p2m_is_write_locked().
- Introduce p2m_force_tlb_flush_sync() and p2m_flush_tlb() to flush TLBs
after p2m table update.
- Change an argument of p2m_insert_mapping() from struct domain *d to
p2m_domain *p2m.
- Drop guest_physmap_add_entry() and use map_regions_p2mt() to define
guest_physmap_add_page().
- Add declaration of map_regions_p2mt() to asm/p2m.h.
- Rewrite commit message and subject.
- Drop p2m_access_t related stuff.
- Add defintion of p2m_is_write_locked().
---
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 | 31 ++++++++++-----
xen/arch/riscv/p2m.c | 65 ++++++++++++++++++++++++++++++++
2 files changed, 87 insertions(+), 9 deletions(-)
diff --git a/xen/arch/riscv/include/asm/p2m.h b/xen/arch/riscv/include/asm/p2m.h
index 5f253da1dd..ada3c398b4 100644
--- a/xen/arch/riscv/include/asm/p2m.h
+++ b/xen/arch/riscv/include/asm/p2m.h
@@ -121,21 +121,22 @@ 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;
-}
+/*
+ * Map a region in the guest p2m with a specific p2m type.
+ * The memory attributes will be derived from the p2m type.
+ */
+int map_regions_p2mt(struct domain *d,
+ gfn_t gfn,
+ unsigned long nr,
+ mfn_t mfn,
+ p2m_type_t p2mt);
/* Untyped version for RAM only, for compatibility */
static inline int __must_check
guest_physmap_add_page(struct domain *d, gfn_t gfn, mfn_t mfn,
unsigned int page_order)
{
- return guest_physmap_add_entry(d, gfn, mfn, page_order, p2m_ram_rw);
+ return map_regions_p2mt(d, gfn, BIT(page_order, UL), mfn, p2m_ram_rw);
}
static inline mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn)
@@ -159,6 +160,18 @@ static inline void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
/* Not supported on RISCV. */
}
+static inline void p2m_write_lock(struct p2m_domain *p2m)
+{
+ write_lock(&p2m->lock);
+}
+
+void p2m_write_unlock(struct p2m_domain *p2m);
+
+static inline int p2m_is_write_locked(struct p2m_domain *p2m)
+{
+ return rw_is_write_locked(&p2m->lock);
+}
+
unsigned long construct_hgatp(struct p2m_domain *p2m, uint16_t vmid);
#endif /* ASM__RISCV__P2M_H */
diff --git a/xen/arch/riscv/p2m.c b/xen/arch/riscv/p2m.c
index cac07c51c9..7cfcf76f24 100644
--- a/xen/arch/riscv/p2m.c
+++ b/xen/arch/riscv/p2m.c
@@ -9,6 +9,41 @@
unsigned int __read_mostly p2m_root_order;
+/*
+ * Force a synchronous P2M TLB flush.
+ *
+ * Must be called with the p2m lock held.
+ */
+static void p2m_force_tlb_flush_sync(struct p2m_domain *p2m)
+{
+ struct domain *d = p2m->domain;
+
+ ASSERT(p2m_is_write_locked(p2m));
+
+ sbi_remote_hfence_gvma(d->dirty_cpumask, 0, 0);
+
+ p2m->need_flush = false;
+}
+
+void p2m_tlb_flush_sync(struct p2m_domain *p2m)
+{
+ if ( p2m->need_flush )
+ p2m_force_tlb_flush_sync(p2m);
+}
+
+/* Unlock the flush and do a P2M TLB flush if necessary */
+void p2m_write_unlock(struct p2m_domain *p2m)
+{
+ /*
+ * The final flush is done with the P2M write lock taken to avoid
+ * someone else modifying the P2M wbefore the TLB invalidation has
+ * completed.
+ */
+ p2m_tlb_flush_sync(p2m);
+
+ write_unlock(&p2m->lock);
+}
+
static void clear_and_clean_page(struct page_info *page)
{
clear_domain_page(page_to_mfn(page));
@@ -139,3 +174,33 @@ int p2m_set_allocation(struct domain *d, unsigned long pages, bool *preempted)
return 0;
}
+
+static int p2m_set_range(struct p2m_domain *p2m,
+ gfn_t sgfn,
+ unsigned long nr,
+ mfn_t smfn,
+ p2m_type_t t)
+{
+ return -EOPNOTSUPP;
+}
+
+static int p2m_insert_mapping(struct p2m_domain *p2m, gfn_t start_gfn,
+ unsigned long nr, mfn_t mfn, p2m_type_t t)
+{
+ int rc;
+
+ p2m_write_lock(p2m);
+ rc = p2m_set_range(p2m, start_gfn, nr, mfn, t);
+ 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(p2m_get_hostp2m(d), gfn, nr, mfn, p2mt);
+}
--
2.50.1On 31.07.2025 17:58, Oleksii Kurochko wrote:
> Implement map_regions_p2mt() to map a region in the guest p2m with
> a specific p2m type. The memory attributes will be derived from the
> p2m type. This function is going to be called from dom0less common
> code.
s/is going to be/is/ ? Such a call exists already, after all.
> --- a/xen/arch/riscv/include/asm/p2m.h
> +++ b/xen/arch/riscv/include/asm/p2m.h
> @@ -121,21 +121,22 @@ 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;
> -}
> +/*
> + * Map a region in the guest p2m with a specific p2m type.
What is "the guest p2m"? In your answer, please consider the possible
(and at some point likely necessary) existence of altp2m and nestedp2m.
In patch 04 you introduce p2m_get_hostp2m(), and I expect it's that
what you mean here.
> --- a/xen/arch/riscv/p2m.c
> +++ b/xen/arch/riscv/p2m.c
> @@ -9,6 +9,41 @@
>
> unsigned int __read_mostly p2m_root_order;
>
> +/*
> + * Force a synchronous P2M TLB flush.
> + *
> + * Must be called with the p2m lock held.
> + */
> +static void p2m_force_tlb_flush_sync(struct p2m_domain *p2m)
> +{
> + struct domain *d = p2m->domain;
Pointer-to-const please. Personally, given the implementation of this
function (and also ...
> + ASSERT(p2m_is_write_locked(p2m));
> +
> + sbi_remote_hfence_gvma(d->dirty_cpumask, 0, 0);
> +
> + p2m->need_flush = false;
> +}
> +
> +void p2m_tlb_flush_sync(struct p2m_domain *p2m)
> +{
> + if ( p2m->need_flush )
> + p2m_force_tlb_flush_sync(p2m);
> +}
... this one) I'd further ask for the function parameters to also be
pointer-to-const, but Andrew may object to that. Andrew - it continues to
be unclear to me under what conditions you agree with adding const, and
under what conditions you would object to me asking for such. Please can
you take the time to clarify this?
> +/* Unlock the flush and do a P2M TLB flush if necessary */
> +void p2m_write_unlock(struct p2m_domain *p2m)
> +{
> + /*
> + * The final flush is done with the P2M write lock taken to avoid
> + * someone else modifying the P2M wbefore the TLB invalidation has
Nit: Stray 'w'.
> + * completed.
> + */
> + p2m_tlb_flush_sync(p2m);
Wasn't the plan to have this be conditional?
> @@ -139,3 +174,33 @@ int p2m_set_allocation(struct domain *d, unsigned long pages, bool *preempted)
>
> return 0;
> }
> +
> +static int p2m_set_range(struct p2m_domain *p2m,
> + gfn_t sgfn,
> + unsigned long nr,
> + mfn_t smfn,
> + p2m_type_t t)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> +static int p2m_insert_mapping(struct p2m_domain *p2m, gfn_t start_gfn,
> + unsigned long nr, mfn_t mfn, p2m_type_t t)
> +{
> + int rc;
> +
> + p2m_write_lock(p2m);
> + rc = p2m_set_range(p2m, start_gfn, nr, mfn, t);
> + 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(p2m_get_hostp2m(d), gfn, nr, mfn, p2mt);
> +}
And eventually both helper functions will gain further callers? Otherwise
it's a little hard to see why they would both need to be separate functions.
Jan
On 8/5/25 5:20 PM, Jan Beulich wrote:
> On 31.07.2025 17:58, Oleksii Kurochko wrote:
>> Implement map_regions_p2mt() to map a region in the guest p2m with
>> a specific p2m type. The memory attributes will be derived from the
>> p2m type. This function is going to be called from dom0less common
>> code.
> s/is going to be/is/ ? Such a call exists already, after all.
>
>> --- a/xen/arch/riscv/include/asm/p2m.h
>> +++ b/xen/arch/riscv/include/asm/p2m.h
>> @@ -121,21 +121,22 @@ 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;
>> -}
>> +/*
>> + * Map a region in the guest p2m with a specific p2m type.
> What is "the guest p2m"? In your answer, please consider the possible
> (and at some point likely necessary) existence of altp2m and nestedp2m.
> In patch 04 you introduce p2m_get_hostp2m(), and I expect it's that
> what you mean here.
In the current one context it is host p2m. I can update the comment with:
"guest's hostp2m".
>
>> --- a/xen/arch/riscv/p2m.c
>> +++ b/xen/arch/riscv/p2m.c
>> @@ -9,6 +9,41 @@
>>
>> unsigned int __read_mostly p2m_root_order;
>>
>> +/*
>> + * Force a synchronous P2M TLB flush.
>> + *
>> + * Must be called with the p2m lock held.
>> + */
>> +static void p2m_force_tlb_flush_sync(struct p2m_domain *p2m)
>> +{
>> + struct domain *d = p2m->domain;
> Pointer-to-const please. Personally, given the implementation of this
> function (and also ...
>
>> + ASSERT(p2m_is_write_locked(p2m));
>> +
>> + sbi_remote_hfence_gvma(d->dirty_cpumask, 0, 0);
>> +
>> + p2m->need_flush = false;
>> +}
>> +
>> +void p2m_tlb_flush_sync(struct p2m_domain *p2m)
>> +{
>> + if ( p2m->need_flush )
>> + p2m_force_tlb_flush_sync(p2m);
>> +}
> ... this one) I'd further ask for the function parameters to also be
> pointer-to-const, but Andrew may object to that. Andrew - it continues to
> be unclear to me under what conditions you agree with adding const, and
> under what conditions you would object to me asking for such. Please can
> you take the time to clarify this?
>
>> +/* Unlock the flush and do a P2M TLB flush if necessary */
>> +void p2m_write_unlock(struct p2m_domain *p2m)
>> +{
>> + /*
>> + * The final flush is done with the P2M write lock taken to avoid
>> + * someone else modifying the P2M wbefore the TLB invalidation has
> Nit: Stray 'w'.
>
>> + * completed.
>> + */
>> + p2m_tlb_flush_sync(p2m);
> Wasn't the plan to have this be conditional?
Not really, probably, I misunderstood you before.
Previously, I only had|p2m_force_tlb_flush_sync()| here, instead of
|p2m_tlb_flush_sync()|, and the latter includes a condition check on
|p2m->need_flush|.
>
>> @@ -139,3 +174,33 @@ int p2m_set_allocation(struct domain *d, unsigned long pages, bool *preempted)
>>
>> return 0;
>> }
>> +
>> +static int p2m_set_range(struct p2m_domain *p2m,
>> + gfn_t sgfn,
>> + unsigned long nr,
>> + mfn_t smfn,
>> + p2m_type_t t)
>> +{
>> + return -EOPNOTSUPP;
>> +}
>> +
>> +static int p2m_insert_mapping(struct p2m_domain *p2m, gfn_t start_gfn,
>> + unsigned long nr, mfn_t mfn, p2m_type_t t)
>> +{
>> + int rc;
>> +
>> + p2m_write_lock(p2m);
>> + rc = p2m_set_range(p2m, start_gfn, nr, mfn, t);
>> + 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(p2m_get_hostp2m(d), gfn, nr, mfn, p2mt);
>> +}
> And eventually both helper functions will gain further callers? Otherwise
> it's a little hard to see why they would both need to be separate functions.
Good point.
Actually, I think that it is enough to have map_regions_p2mt() as it is used
for dom0less common code, and re-use it every where potentially p2m_insert_mapping()
will be needed.
~ Oleksii
On 08.08.2025 15:46, Oleksii Kurochko wrote:
> On 8/5/25 5:20 PM, Jan Beulich wrote:
>> On 31.07.2025 17:58, Oleksii Kurochko wrote:
>>> +/* Unlock the flush and do a P2M TLB flush if necessary */
>>> +void p2m_write_unlock(struct p2m_domain *p2m)
>>> +{
>>> + /*
>>> + * The final flush is done with the P2M write lock taken to avoid
>>> + * someone else modifying the P2M wbefore the TLB invalidation has
>> Nit: Stray 'w'.
>>
>>> + * completed.
>>> + */
>>> + p2m_tlb_flush_sync(p2m);
>> Wasn't the plan to have this be conditional?
>
> Not really, probably, I misunderstood you before.
>
> Previously, I only had|p2m_force_tlb_flush_sync()| here, instead of
> |p2m_tlb_flush_sync()|, and the latter includes a condition check on
> |p2m->need_flush|.
Just to re-iterate my point: Not every unlock will require a flush. Hence
why I expect the flush to be conditional upon there being an indication
that some change was done that requires flushing.
Jan
On 8/11/25 9:28 AM, Jan Beulich wrote:
> On 08.08.2025 15:46, Oleksii Kurochko wrote:
>> On 8/5/25 5:20 PM, Jan Beulich wrote:
>>> On 31.07.2025 17:58, Oleksii Kurochko wrote:
>>>> +/* Unlock the flush and do a P2M TLB flush if necessary */
>>>> +void p2m_write_unlock(struct p2m_domain *p2m)
>>>> +{
>>>> + /*
>>>> + * The final flush is done with the P2M write lock taken to avoid
>>>> + * someone else modifying the P2M wbefore the TLB invalidation has
>>> Nit: Stray 'w'.
>>>
>>>> + * completed.
>>>> + */
>>>> + p2m_tlb_flush_sync(p2m);
>>> Wasn't the plan to have this be conditional?
>> Not really, probably, I misunderstood you before.
>>
>> Previously, I only had|p2m_force_tlb_flush_sync()| here, instead of
>> |p2m_tlb_flush_sync()|, and the latter includes a condition check on
>> |p2m->need_flush|.
> Just to re-iterate my point: Not every unlock will require a flush. Hence
> why I expect the flush to be conditional upon there being an indication
> that some change was done that requires flushing.
>
The flush is actually conditional; the condition is inside
|p2m_tlb_flush_sync()|:
void p2m_tlb_flush_sync(struct p2m_domain *p2m)
{
if ( p2m->need_flush )
p2m_force_tlb_flush_sync(p2m);
}
~ Oleksii
On 11.08.2025 11:29, Oleksii Kurochko wrote:
>
> On 8/11/25 9:28 AM, Jan Beulich wrote:
>> On 08.08.2025 15:46, Oleksii Kurochko wrote:
>>> On 8/5/25 5:20 PM, Jan Beulich wrote:
>>>> On 31.07.2025 17:58, Oleksii Kurochko wrote:
>>>>> +/* Unlock the flush and do a P2M TLB flush if necessary */
>>>>> +void p2m_write_unlock(struct p2m_domain *p2m)
>>>>> +{
>>>>> + /*
>>>>> + * The final flush is done with the P2M write lock taken to avoid
>>>>> + * someone else modifying the P2M wbefore the TLB invalidation has
>>>> Nit: Stray 'w'.
>>>>
>>>>> + * completed.
>>>>> + */
>>>>> + p2m_tlb_flush_sync(p2m);
>>>> Wasn't the plan to have this be conditional?
>>> Not really, probably, I misunderstood you before.
>>>
>>> Previously, I only had|p2m_force_tlb_flush_sync()| here, instead of
>>> |p2m_tlb_flush_sync()|, and the latter includes a condition check on
>>> |p2m->need_flush|.
>> Just to re-iterate my point: Not every unlock will require a flush. Hence
>> why I expect the flush to be conditional upon there being an indication
>> that some change was done that requires flushing.
>>
> The flush is actually conditional; the condition is inside
> |p2m_tlb_flush_sync()|:
> void p2m_tlb_flush_sync(struct p2m_domain *p2m)
> {
> if ( p2m->need_flush )
> p2m_force_tlb_flush_sync(p2m);
> }
Hmm, I'd consider this misleading function naming then. Especially with
"force" and "sync" being kind of redundant with one another already anyway.
See x86'es naming.
Jan
© 2016 - 2025 Red Hat, Inc.