unmap_page_range optimisation
Subsystems (4)
v4 cover unmap_page_range optimisation
https://lore.kernel.org/xen-devel/[email protected]/
Previous discussion:
RFC: unmap_page_range optimisation (avoiding emulation faults during VM migration)
https://lore.kernel.org/xen-devel/[email protected]/
This series adds a new MMU_PT_UPDATE_SWAP sub-op to the mmu_update hypercall which Linux can use from a new pte_get_and_clear pv-op to significantly improve the performance of unmap_page_range.
A microbenchmark[1] which allocates a large number of pages and then clears them shows a performance increase from 1100ms to 640ms using the new pte_get_and_clear pv-op.
Further profiling the microbenchmark with bpftrace[2] shows that Linux calls unmap_page_range() 23 times with an average execution time of 48ms dropping to 28ms when using the new operation.
Changes in v4:
- Review comments
Changes in v3:
- Extend swap to work with l2/l3/l4 PTEs and any value not just 0
- Other review comments
Changes in v2:
- Add a sub-op to mmu_update instead of new hypercall
- Add Linux patch
- Other review comments
[1] microbenchmark
#include <err.h>
#include <sys/mman.h>
#include <time.h>
#include <stdint.h>
#include <stdio.h>
#include <unistd.h>
static uint64_t nsec(void)
{
struct timespec ts;
clock_gettime(CLOCK_MONOTONIC, &ts);
return (uint64_t)ts.tv_sec * 1e9 + ts.tv_nsec;
}int main(int argc, char **argv)
{
const size_t len = 1024UL * 1024 * 1024 * 4;
const long pagesz = sysconf(_SC_PAGESIZE);
char *p = mmap(NULL, len,
PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANONYMOUS,
-1, 0);
if ( p == MAP_FAILED )
err(1, "mmap");
/* Fault every page in */
for (size_t i = 0; i < len; i += pagesz)
p[i] = 1;
uint64_t start = nsec();
if ( madvise(p, len, MADV_DONTNEED) )
err(1, "madvise");
uint64_t end = nsec();
printf("MADV_DONTNEED on %zu MB took %.3f ms\n",
len / 1024 / 1024,
(end - start) / 1e6);
munmap(p, len);
return 0;
}[2] bpftrace
bpftrace -e '
kprobe:unmap_page_range
/comm == "a.out"/
{
@start[tid] = nsecs;
}kretprobe:unmap_page_range
/@start[tid] && comm == "a.out"/
{
$delta = nsecs - @start[tid];
@count = count();
@total = sum($delta);
delete(@start[tid]);
}interval:s:10
{
printf("avg call time = %d ns (%d calls)\n",
(@total / @count), (uint64)@count);
exit();
}'Kevin Lampis (4):
x86: extend update_intpte() to support atomic get-and-update
x86: extend mod_l*_entry() to optionally return the old PTE value
x86: extend do_mmu_update() to support returning the old PTE value
x86: New feature flag XENFEAT_mmu_pt_update_swap.
xen/arch/x86/mm.c | 158 +++++++++++++++++++++++++++++----- xen/arch/x86/pv/mm.h | 14 ++- xen/common/kernel.c | 1 + xen/include/public/features.h | 3 + xen/include/public/xen.h | 1 + 5 files changed, 151 insertions(+), 26 deletions(-)
Signature · 1 line
2.52.0
PATCH v4 1/5 x86: extend update_intpte() to support atomic get-and-update
The update_intpte() now accepts a new swap flag and if set returns the old pte value.
No functional change for existing callers.
---
Changes in v4:
- Remove UPDATE_ENTRY_SWAP
- Don't move the backslashes in UPDATE_ENTRY
Changes in v3:
- Return 0 in the paging_write_guest_entry() case (update_intpte)
- Add UPDATE_ENTRY_SWAP to convert return value to correct type
Changes in v2:
- Add new PTE_UPDATE_SWAP flag instead of a bool argument
- Return the old pte value instead of turning old into an out pointer
--- xen/arch/x86/pv/mm.h | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
xen/arch/x86/pv/mm.h +11 -3
@@ -64,16 +64,22 @@ static inline intpte_t paging_cmpxchg_guest_entry(#define PTE_UPDATE_PRESERVE_AD (1u << 0)#define PTE_UPDATE_NO_TRANSLATE (1u << 1) +#define PTE_UPDATE_SWAP (1u << 2)/* * How to write an entry to the guest pagetables. +* If the SWAP flag is set then does an atomic swap and returns the old PTE +* value, otherwise returns 0. */ -static inline void update_intpte (intpte_t *p,intpte_t old,intpte_t new, -mfn_t mfn,struct vcpu *v,unsigned int flags) +static inline intpte_t update_intpte (intpte_t *p,intpte_t old,intpte_t new, +mfn_t mfn,struct vcpu *v,unsigned int flags) {#ifndef PTE_UPDATE_WITH_CMPXCHG -if ( !(flags &PTE_UPDATE_PRESERVE_AD ) ) +if ( !(flags & (PTE_UPDATE_PRESERVE_AD |PTE_UPDATE_SWAP )) ) + {paging_write_guest_entry (v, p, new, mfn); +return 0 ; + } else#endif {
@@ -95,6 +101,8 @@ static inline void update_intpte(intpte_t *p, intpte_t old, intpte_t new, old = t; } } + +return old; }/*
Signature · 1 line
2.52.0
Re: [PATCH v4 1/5] x86: extend update_intpte() to support atomic get-and-update
Quoted 27 lines · Kevin Lampis
The
update_intpte()now accepts a new swap flag and if set returns the old pte value.No functional change for existing callers.
Signed-off-by Kevin Lampis <[email protected]>---
Changes in v4:
- RemoveUPDATE_ENTRY_SWAP
- Don't move the backslashes inUPDATE_ENTRYChanges in v3:
- Return 0 in thepaging_write_guest_entry()case (update_intpte)
- AddUPDATE_ENTRY_SWAPto convert return value to correct typeChanges in v2:
- Add newPTE_UPDATE_SWAPflag instead of a bool argument
- Return the old pte value instead of turningoldinto an out pointer--- xen/arch/x86/pv/mm.h | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)xen/arch/x86/pv/mm.h +0
-0
xen/arch/x86/pv/mm.h static inline intpte_t paging_cmpxchg_guest_entry( · line 72
Quoted 9 lines
xen/arch/x86/pv/mm.h +3
-0@@ -64,16 +64,22 @@ static inline intpte_t paging_cmpxchg_guest_entry(#define PTE_UPDATE_PRESERVE_AD (1u << 0)#define PTE_UPDATE_NO_TRANSLATE (1u << 1) +#define PTE_UPDATE_SWAP (1u << 2)/* * How to write an entry to the guest pagetables. +* If the SWAP flag is set then does an atomic swap and returns the old PTE +* value, otherwise returns 0.
This isn't quite true. It will also return the old PTE for PRESERVE_AD, and unconditionally in debug builds which set PTE_UPDATE_WITH_CMPXCHG.
I'd be tempted to just finish the sentence at "value." SWAP is the only one we care about giving a guarantee of a return, and the other cases are far easier to see from looking at the function rather than attempting to describe in the comment.
I can fix this up on commit if you're happy.
~Andrew
Re: [PATCH v4 1/5] x86: extend update_intpte() to support atomic get-and-update
Quoted 1 line
I can fix this up on commit if you're happy.
Thank you. Please go ahead.
PATCH v4 2/5 x86: extend mod_l*_entry() to optionally return the old PTE value
In reply to Kevin LampisCalling mod_l*_entry() with a valid ol*e_out pointer will do atomic CMPXCHG to set the new pte value and return the old pte value through the ol*e_out pointer. If the ol*e_out pointer is NULL then old behavior is preserved.
---
Changes in v4:
- Refactor how UPDATE_ENTRY is used
- Update commit message to specify CMPXCHG instead of xchg
Changes in v3:
- Add swap to mod_l{2,3,4}_entry()
Changes in v2:
- UPDATE_ENTRY now returns the old pte value instead of using an out pointer
--- xen/arch/x86/mm.c | 106 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 84 insertions(+), 22 deletions(-)
xen/arch/x86/mm.c +84 -22
@@ -2193,7 +2193,8 @@ static void l3t_unlock(struct page_info *page)/* Update the L1 entry at pl1e to new value nl1e. */ static int mod_l1_entry (l1_pgentry_t *pl1e,l1_pgentry_t nl1e,mfn_t gl1mfn,unsigned int update_flags, -struct vcpu *pt_vcpu,struct domain *pg_dom) +struct vcpu *pt_vcpu,struct domain *pg_dom, +l1_pgentry_t *ol1e_out) {l1_pgentry_t ol1e =l1e_read (pl1e);struct domain *pt_dom = pt_vcpu->domain;
@@ -2254,7 +2255,11 @@ static int mod_l1_entry(l1_pgentry_t *pl1e, l1_pgentry_t nl1e,/* Fast path for sufficiently-similar mappings. */ if ( !l1e_has_changed (ol1e, nl1e, ~FASTPATH_FLAG_WHITELIST ) ) { -UPDATE_ENTRY (l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, update_flags); +intpte_t old =UPDATE_ENTRY (l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, + update_flags); +if ( ol1e_out ) + *ol1e_out =l1e_from_intpte (old); +if ( page )put_page (page);return 0 ;
@@ -2277,14 +2282,25 @@ static int mod_l1_entry(l1_pgentry_t *pl1e, l1_pgentry_t nl1e,if ( page )put_page (page); -UPDATE_ENTRY (l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, update_flags); + { +intpte_t old =UPDATE_ENTRY (l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, + update_flags); +if ( ol1e_out ) + *ol1e_out =l1e_from_intpte (old); + } }else if (pv_l1tf_check_l1e (pt_dom, nl1e) )return -ERESTART ;else -UPDATE_ENTRY (l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, update_flags); + { +intpte_t old =UPDATE_ENTRY (l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, + update_flags); +if ( ol1e_out ) + *ol1e_out =l1e_from_intpte (old); + }put_page_from_l1e (ol1e, pt_dom); +return rc; }
@@ -2294,7 +2310,8 @@ static int mod_l2_entry(l2_pgentry_t *pl2e,l2_pgentry_t nl2e, mfn_t mfn, unsigned int update_flags, - struct vcpu *vcpu) + struct vcpu *vcpu, + l2_pgentry_t *ol2e_out) { l2_pgentry_t ol2e;struct domain *d = vcpu->domain;
@@ -2325,19 +2342,33 @@ static int mod_l2_entry(l2_pgentry_t *pl2e,/* Fast path for sufficiently-similar mappings. */ if ( !l2e_has_changed (ol2e, nl2e, ~FASTPATH_PDE_FLAG_WHITELIST ) ) { -UPDATE_ENTRY (l2, pl2e, ol2e, nl2e, mfn, vcpu, update_flags); +intpte_t old =UPDATE_ENTRY (l2, pl2e, ol2e, nl2e, mfn, vcpu, + update_flags); +if ( ol2e_out ) + *ol2e_out =l2e_from_intpte (old); +return 0 ; }if (unlikely ((rc =get_page_from_l2e (nl2e, mfn, d,0 )) <0 ) )return rc; -UPDATE_ENTRY (l2, pl2e, ol2e, nl2e, mfn, vcpu, update_flags); + { +intpte_t old =UPDATE_ENTRY (l2, pl2e, ol2e, nl2e, mfn, vcpu, + update_flags); +if ( ol2e_out ) + *ol2e_out =l2e_from_intpte (old); + } }else if (pv_l1tf_check_l2e (d, nl2e) )return -ERESTART ;else -UPDATE_ENTRY (l2, pl2e, ol2e, nl2e, mfn, vcpu, update_flags); + { +intpte_t old =UPDATE_ENTRY (l2, pl2e, ol2e, nl2e, mfn, vcpu, + update_flags); +if ( ol2e_out ) + *ol2e_out =l2e_from_intpte (old); + }put_page_from_l2e (ol2e, mfn, PTF_defer);
@@ -2349,7 +2380,8 @@ static int mod_l3_entry(l3_pgentry_t *pl3e,l3_pgentry_t nl3e, mfn_t mfn, unsigned int update_flags, - struct vcpu *vcpu) + struct vcpu *vcpu, + l3_pgentry_t *ol3e_out) { l3_pgentry_t ol3e;struct domain *d = vcpu->domain;
@@ -2378,7 +2410,11 @@ static int mod_l3_entry(l3_pgentry_t *pl3e,/* Fast path for sufficiently-similar mappings. */ if ( !l3e_has_changed (ol3e, nl3e, ~FASTPATH_PDE_FLAG_WHITELIST ) ) { -UPDATE_ENTRY (l3, pl3e, ol3e, nl3e, mfn, vcpu, update_flags); +intpte_t old =UPDATE_ENTRY (l3, pl3e, ol3e, nl3e, mfn, vcpu, + update_flags); +if ( ol3e_out ) + *ol3e_out =l3e_from_intpte (old); +return 0 ; }
@@ -2387,12 +2423,22 @@ static int mod_l3_entry(l3_pgentry_t *pl3e,return rc; rc =0 ; -UPDATE_ENTRY (l3, pl3e, ol3e, nl3e, mfn, vcpu, update_flags); + { +intpte_t old =UPDATE_ENTRY (l3, pl3e, ol3e, nl3e, mfn, vcpu, + update_flags); +if ( ol3e_out ) + *ol3e_out =l3e_from_intpte (old); + } }else if (pv_l1tf_check_l3e (d, nl3e) )return -ERESTART ;else -UPDATE_ENTRY (l3, pl3e, ol3e, nl3e, mfn, vcpu, update_flags); + { +intpte_t old =UPDATE_ENTRY (l3, pl3e, ol3e, nl3e, mfn, vcpu, + update_flags); +if ( ol3e_out ) + *ol3e_out =l3e_from_intpte (old); + }put_page_from_l3e (ol3e, mfn, PTF_defer);return rc;
@@ -2403,7 +2449,8 @@ static int mod_l4_entry(l4_pgentry_t *pl4e,l4_pgentry_t nl4e, mfn_t mfn, unsigned int update_flags, - struct vcpu *vcpu) + struct vcpu *vcpu, + l4_pgentry_t *ol4e_out) { struct domain *d = vcpu->domain;l4_pgentry_t ol4e;
@@ -2432,7 +2479,11 @@ static int mod_l4_entry(l4_pgentry_t *pl4e,/* Fast path for sufficiently-similar mappings. */ if ( !l4e_has_changed (ol4e, nl4e, ~FASTPATH_PDE_FLAG_WHITELIST ) ) { -UPDATE_ENTRY (l4, pl4e, ol4e, nl4e, mfn, vcpu, update_flags); +intpte_t old =UPDATE_ENTRY (l4, pl4e, ol4e, nl4e, mfn, vcpu, + update_flags); +if ( ol4e_out ) + *ol4e_out =l4e_from_intpte (old); +return 0 ; }
@@ -2441,12 +2492,22 @@ static int mod_l4_entry(l4_pgentry_t *pl4e,return rc; rc =0 ; -UPDATE_ENTRY (l4, pl4e, ol4e, nl4e, mfn, vcpu, update_flags); + { +intpte_t old =UPDATE_ENTRY (l4, pl4e, ol4e, nl4e, mfn, vcpu, + update_flags); +if ( ol4e_out ) + *ol4e_out =l4e_from_intpte (old); + } }else if (pv_l1tf_check_l4e (d, nl4e) )return -ERESTART ;else -UPDATE_ENTRY (l4, pl4e, ol4e, nl4e, mfn, vcpu, update_flags); + { +intpte_t old =UPDATE_ENTRY (l4, pl4e, ol4e, nl4e, mfn, vcpu, + update_flags); +if ( ol4e_out ) + *ol4e_out =l4e_from_intpte (old); + }put_page_from_l4e (ol4e, mfn, PTF_defer);return rc;
@@ -3318,7 +3379,7 @@ int new_guest_cr3(mfn_t mfn)l4e_from_mfn (mfn, (_PAGE_PRESENT | _PAGE_RW | _PAGE_USER | _PAGE_ACCESSED)), - gt_mfn, 0, curr); + gt_mfn, 0, curr, NULL);unmap_domain_page (pl4e);switch ( rc ) {
@@ -4156,14 +4217,14 @@ long do_mmu_update( {case PGT_l1_page_table: rc =mod_l1_entry (va,l1e_from_intpte (req.val), mfn, - update_flags, v, pg_owner); + update_flags, v, pg_owner,NULL );break ;case PGT_l2_page_table:if (unlikely (pg_owner != pt_owner) )break ; rc =mod_l2_entry (va,l2e_from_intpte (req.val), mfn, - update_flags, v); + update_flags, v,NULL );if ( !rc ) flush_linear_pt = true;break ;
@@ -4172,7 +4233,7 @@ long do_mmu_update(if (unlikely (pg_owner != pt_owner) )break ; rc =mod_l3_entry (va,l3e_from_intpte (req.val), mfn, - update_flags, v); + update_flags, v,NULL );if ( !rc ) flush_linear_pt = true;break ;
@@ -4181,7 +4242,7 @@ long do_mmu_update(if (unlikely (pg_owner != pt_owner) )break ; rc =mod_l4_entry (va,l4e_from_intpte (req.val), mfn, - update_flags, v); + update_flags, v,NULL );if ( !rc ) flush_linear_pt = true;if ( !rc && pt_owner->arch.pv.xpti )
@@ -4521,7 +4582,8 @@ static int __do_update_va_mapping( goto out; } - rc =mod_l1_entry (pl1e, val, gl1mfn,MMU_NORMAL_PT_UPDATE , v, pg_owner); + rc =mod_l1_entry (pl1e, val, gl1mfn,MMU_NORMAL_PT_UPDATE , v, pg_owner, +NULL );page_unlock (gl1pg);put_page (gl1pg);
Signature · 1 line
2.52.0
Re: [PATCH v4 2/5] x86: extend mod_l*_entry() to optionally return the old PTE value
Quoted 36 lines · Kevin Lampis
Calling mod_l*_entry() with a valid ol*e_out pointer will do atomic CMPXCHG to set the new pte value and return the old pte value through the ol*e_out pointer. If the ol*e_out pointer is NULL then old behavior is preserved.
Signed-off-by Kevin Lampis <[email protected]>---
Changes in v4:
- Refactor howUPDATE_ENTRYis used
- Update commit message to specify CMPXCHG instead of xchgChanges in v3:
- Add swap to mod_l{2,3,4}_entry()Changes in v2:
-UPDATE_ENTRYnow returns the old pte value instead of using an out pointer--- xen/arch/x86/mm.c | 106 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 84 insertions(+), 22 deletions(-)xen/arch/x86/mm.c +2
-1@@ -2193,7 +2193,8 @@ static void l3t_unlock(struct page_info *page)/* Update the L1 entry at pl1e to new value nl1e. */ static int mod_l1_entry (l1_pgentry_t *pl1e,l1_pgentry_t nl1e,mfn_t gl1mfn,unsigned int update_flags, -struct vcpu *pt_vcpu,struct domain *pg_dom) +struct vcpu *pt_vcpu,struct domain *pg_dom, +l1_pgentry_t *ol1e_out) {l1_pgentry_t ol1e =l1e_read (pl1e);struct domain *pt_dom = pt_vcpu->domain;
xen/arch/x86/mm.c static int mod_l1_entry(l1_pgentry_t *pl1e, l1_pgentry_t nl1e, · line 2262
Quoted 9 lines
xen/arch/x86/mm.c +5
-1@@ -2254,7 +2255,11 @@ static int mod_l1_entry(l1_pgentry_t *pl1e, l1_pgentry_t nl1e,/* Fast path for sufficiently-similar mappings. */ if ( !l1e_has_changed (ol1e, nl1e, ~FASTPATH_FLAG_WHITELIST ) ) { -UPDATE_ENTRY (l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, update_flags); +intpte_t old =UPDATE_ENTRY (l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, + update_flags); +if ( ol1e_out ) + *ol1e_out =l1e_from_intpte (old); +
Same "intpte_t old = UPDATE_ENTRY(); if ( olNe_out ) ..." code block is now repeated three times in each of mod_l{1,2,3,4}_entry().
Alternatively, a single local variable per function, assigned at each call site and copied out once at the end. Does this avoid the duplication?
Re: [PATCH v4 2/5] x86: extend mod_l*_entry() to optionally return the old PTE value
Quoted 50 lines · On 20/10/2026 3:34 am, [email protected] wrote:
Quoted 36 lines · Kevin Lampis
Calling mod_l*_entry() with a valid ol*e_out pointer will do atomic CMPXCHG to set the new pte value and return the old pte value through the ol*e_out pointer. If the ol*e_out pointer is NULL then old behavior is preserved.
Signed-off-by Kevin Lampis <[email protected]>---
Changes in v4:
- Refactor howUPDATE_ENTRYis used
- Update commit message to specify CMPXCHG instead of xchgChanges in v3:
- Add swap to mod_l{2,3,4}_entry()Changes in v2:
-UPDATE_ENTRYnow returns the old pte value instead of using an out pointer--- xen/arch/x86/mm.c | 106 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 84 insertions(+), 22 deletions(-)xen/arch/x86/mm.c +2
-1@@ -2193,7 +2193,8 @@ static void l3t_unlock(struct page_info *page)/* Update the L1 entry at pl1e to new value nl1e. */ static int mod_l1_entry (l1_pgentry_t *pl1e,l1_pgentry_t nl1e,mfn_t gl1mfn,unsigned int update_flags, -struct vcpu *pt_vcpu,struct domain *pg_dom) +struct vcpu *pt_vcpu,struct domain *pg_dom, +l1_pgentry_t *ol1e_out) {l1_pgentry_t ol1e =l1e_read (pl1e);struct domain *pt_dom = pt_vcpu->domain;xen/arch/x86/mm.c static int mod_l1_entry(l1_pgentry_t *pl1e, l1_pgentry_t nl1e, · line 2262
Quoted 9 lines
xen/arch/x86/mm.c +5
-1@@ -2254,7 +2255,11 @@ static int mod_l1_entry(l1_pgentry_t *pl1e, l1_pgentry_t nl1e,/* Fast path for sufficiently-similar mappings. */ if ( !l1e_has_changed (ol1e, nl1e, ~FASTPATH_FLAG_WHITELIST ) ) { -UPDATE_ENTRY (l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, update_flags); +intpte_t old =UPDATE_ENTRY (l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, + update_flags); +if ( ol1e_out ) + *ol1e_out =l1e_from_intpte (old); +Same "
intpte_told =UPDATE_ENTRY(); if (olNe_out) ..." code block is now repeated three times in each of mod_l{1,2,3,4}_entry().
Quoted 2 lines
Alternatively, a single local variable per function, assigned at each call site and copied out once at the end. Does this avoid the duplication?
This is a bit repetitive, but there's no easy way to merge all 3 UPDATE_ENTRY() in each function.
Given this:
xen/arch/x86/mm.c +0 -0
@@ -2408,7 +2408,6 @@ static int mod_l4_entry(l4_pgentry_t *pl4e,
{
struct domain *d = vcpu->domain;
l4_pgentry_t ol4e;
- int rc = 0;
if ( unlikely(!is_guest_l4_slot(d, pgentry_ptr_to_slot(pl4e))) ) {
@@ -2421,6 +2420,8 @@ static int mod_l4_entry(l4_pgentry_t *pl4e,
if ( l4e_get_flags(nl4e) & _PAGE_PRESENT )
{
+ int rc;
+
if ( unlikely(l4e_get_flags(nl4e) & L4_DISALLOW_MASK) ) {
gdprintk(XENLOG_WARNING, "Bad L4 flags %x\n", @@ -2440,17 +2441,14 @@ static int mod_l4_entry(l4_pgentry_t *pl4e, rc = get_page_from_l4e(nl4e, mfn, d, 0);
if ( unlikely(rc < 0) )
return rc;
- rc = 0;
-
- UPDATE_ENTRY(l4, pl4e, ol4e, nl4e, mfn, vcpu, update_flags); }
else if ( pv_l1tf_check_l4e(d, nl4e) )
return -ERESTART;
- else
- UPDATE_ENTRY(l4, pl4e, ol4e, nl4e, mfn, vcpu, update_flags); +
+ UPDATE_ENTRY(l4, pl4e, ol4e, nl4e, mfn, vcpu, update_flags);
put_page_from_l4e(ol4e, mfn, PTF_defer);
- return rc;
+ return 0;
}
#endif /* CONFIG_PV */
we can at least turn 3 into 2, which bloat-o-meter says removes an alarming amount of code:
add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-165 (-165) Function old new delta mod_l4_entry.cold 39 31 -8 mod_l4_entry 917 760 -157
That in turn means this patch only doubles up.
I'm pretty certain that the get_page_from_l?e() calls can't return a positive number on these paths. The one case I've found returning a positive number is in iommu_legacy_{un,}map() but we don't pass IOMMUF_preempt in and only operate on single pages.
Removing the final duplicate would require something like a bool "dont_put" which is set in the !l4e_has_changed() path, but I think that makes things more complicated rather than less.
~Andrew
Re: [PATCH v4 2/5] x86: extend mod_l*_entry() to optionally return the old PTE value
Quoted 110 lines · Andrew Cooper
Quoted 50 lines · On 20/10/2026 3:34 am, [email protected] wrote:
Quoted 36 lines · Kevin Lampis
Calling mod_l*_entry() with a valid ol*e_out pointer will do atomic CMPXCHG to set the new pte value and return the old pte value through the ol*e_out pointer. If the ol*e_out pointer is NULL then old behavior is preserved.
Signed-off-by Kevin Lampis <[email protected]>---
Changes in v4:
- Refactor howUPDATE_ENTRYis used
- Update commit message to specify CMPXCHG instead of xchgChanges in v3:
- Add swap to mod_l{2,3,4}_entry()Changes in v2:
-UPDATE_ENTRYnow returns the old pte value instead of using an out pointer--- xen/arch/x86/mm.c | 106 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 84 insertions(+), 22 deletions(-)xen/arch/x86/mm.c +2
-1@@ -2193,7 +2193,8 @@ static void l3t_unlock(struct page_info *page)/* Update the L1 entry at pl1e to new value nl1e. */ static int mod_l1_entry (l1_pgentry_t *pl1e,l1_pgentry_t nl1e,mfn_t gl1mfn,unsigned int update_flags, -struct vcpu *pt_vcpu,struct domain *pg_dom) +struct vcpu *pt_vcpu,struct domain *pg_dom, +l1_pgentry_t *ol1e_out) {l1_pgentry_t ol1e =l1e_read (pl1e);struct domain *pt_dom = pt_vcpu->domain;xen/arch/x86/mm.c static int mod_l1_entry(l1_pgentry_t *pl1e, l1_pgentry_t nl1e, · line 2262
Quoted 9 lines
xen/arch/x86/mm.c +5
-1@@ -2254,7 +2255,11 @@ static int mod_l1_entry(l1_pgentry_t *pl1e, l1_pgentry_t nl1e,/* Fast path for sufficiently-similar mappings. */ if ( !l1e_has_changed (ol1e, nl1e, ~FASTPATH_FLAG_WHITELIST ) ) { -UPDATE_ENTRY (l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, update_flags); +intpte_t old =UPDATE_ENTRY (l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, + update_flags); +if ( ol1e_out ) + *ol1e_out =l1e_from_intpte (old); +Same "
intpte_told =UPDATE_ENTRY(); if (olNe_out) ..." code block is now repeated three times in each of mod_l{1,2,3,4}_entry().Quoted 2 lines
Alternatively, a single local variable per function, assigned at each call site and copied out once at the end. Does this avoid the duplication?
This is a bit repetitive, but there's no easy way to merge all 3
UPDATE_ENTRY()in each function.Given this:
xen/arch/x86/mm.c +0
-1@@ -2408,7 +2408,6 @@ static int mod_l4_entry(l4_pgentry_t *pl4e, { struct domain *d = vcpu->domain; l4_pgentry_t ol4e; - int rc = 0;
if (unlikely(!is_guest_l4_slot(d, pgentry_ptr_to_slot(pl4e)))) {xen/arch/x86/mm.c +0
-0@@ -2421,6 +2420,8 @@ static int mod_l4_entry(l4_pgentry_t *pl4e,
if (l4e_get_flags(nl4e)&_PAGE_PRESENT)
{
+ int rc;
+
if (unlikely(l4e_get_flags(nl4e) & L4_DISALLOW_MASK)) {
gdprintk(XENLOG_WARNING, "Bad L4 flags %x\n",xen/arch/x86/mm.c +2
-5@@ -2440,17 +2441,14 @@ static int mod_l4_entry(l4_pgentry_t *pl4e, rc = get_page_from_l4e(nl4e, mfn, d, 0); if ( unlikely(rc < 0) ) return rc; - rc = 0; - - UPDATE_ENTRY(l4, pl4e, ol4e, nl4e, mfn, vcpu, update_flags); } else if ( pv_l1tf_check_l4e(d, nl4e) ) return -ERESTART; - else - UPDATE_ENTRY(l4, pl4e, ol4e, nl4e, mfn, vcpu, update_flags); + + UPDATE_ENTRY(l4, pl4e, ol4e, nl4e, mfn, vcpu, update_flags);
put_page_from_l4e(ol4e, mfn,PTF_defer);
- return rc;
+ return 0;
}
#endif /*CONFIG_PV*/we can at least turn 3 into 2, which bloat-o-meter says removes an alarming amount of code:
add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-165 (-165) Function old new delta mod_l4_entry.cold 39 31 -8 mod_l4_entry 917 760 -157
Quoted 6 lines
That in turn means this patch only doubles up.
I'm pretty certain that the get_page_from_l?e() calls can't return a positive number on these paths. The one case I've found returning a positive number is in iommu_legacy_{un,}map() but we don't pass
IOMMUF_preemptin and only operate on single pages.
In the Gitlab run, I put in ASSERT()s to this effect, and nothing exploded during real testing.
https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/3046070649
Anyway, I think an intermediate patch rearranging each of mod_l?_entry() in the above way will be a good move.
~Andrew
PATCH v4 3/5 x86: extend do_mmu_update() to support returning the old PTE value
In reply to Kevin LampisA new sub-op MMU_PT_UPDATE_SWAP when used will do an atomic swap to set the new value and return the old PTE value through the req.val field.
- The old PTE value is passed back to the guest through the req.val field
If MMU_PT_UPDATE_SWAP is not set then the old behavior is preserved
do_mmu_update -> mod_l*_entry -> UPDATE_ENTRY -> paging_write_guest_entry
The new MMU_PT_UPDATE_SWAP call chain looks like this
do_mmu_update -> mod_l*_entry -> UPDATE_ENTRY -> paging_cmpxchg_guest_entry
---
Changes in v4:
- Refactor how mod_l*_entry() is used
Checking if (!rc) multiple times in a row is inelegant but reduces the diff size. Let me know if that's not the right tradeoff here.
Changes in v3:
- Do swap for l2, l3, l4
- Allow all values not just 0
Changes in v2:
- Add a sub-op to mmu_update instead of new hypercall
- Rename to "swap" instead of "clear", although only 0 is accepted
- Remove spurious curly brace from case PGT_l1_page_table:
- Restructure case PGT_l1_page_table: to avoid diff churn
- Add missing MMU_PT_UPDATE_SWAP check for the second PGT_writable_page block
--- xen/arch/x86/mm.c | 60 ++++++++++++++++++++++++++++++++++++---- xen/include/public/xen.h | 1 + 2 files changed, 56 insertions(+), 5 deletions(-)
xen/arch/x86/mm.c +55 -5
@@ -4151,6 +4151,7 @@ long do_mmu_update(case MMU_NORMAL_PT_UPDATE :case MMU_PT_UPDATE_PRESERVE_AD :case MMU_PT_UPDATE_NO_TRANSLATE : +case MMU_PT_UPDATE_SWAP : {p2m_type_t p2mt;
@@ -4212,37 +4213,80 @@ long do_mmu_update( update_flags =PTE_UPDATE_PRESERVE_AD ; else if ( cmd == MMU_PT_UPDATE_NO_TRANSLATE ) update_flags =PTE_UPDATE_NO_TRANSLATE ; + else if ( cmd == MMU_PT_UPDATE_SWAP ) + update_flags =PTE_UPDATE_SWAP ;switch ( page->u.inuse.type_info & PGT_type_mask ) {case PGT_l1_page_table: +l1_pgentry_t ol1e = {}; rc =mod_l1_entry (va,l1e_from_intpte (req.val), mfn, - update_flags, v, pg_owner,NULL ); + update_flags, v, pg_owner, &ol1e); + +if ( !rc && cmd ==MMU_PT_UPDATE_SWAP ) + { + req.val = ol1e.l1; +if (unlikely (copy_to_guest (ureqs, &req,1 )) ) + rc = -EFAULT ; + }break ;case PGT_l2_page_table: +l2_pgentry_t ol2e = {}; +if (unlikely (pg_owner != pt_owner) )break ; + rc =mod_l2_entry (va,l2e_from_intpte (req.val), mfn, - update_flags, v,NULL ); + update_flags, v, &ol2e); + +if ( !rc && cmd ==MMU_PT_UPDATE_SWAP ) + { + req.val = ol2e.l2; +if (unlikely (copy_to_guest (ureqs, &req,1 )) ) + rc = -EFAULT ; + } +if ( !rc ) flush_linear_pt = true;break ;case PGT_l3_page_table: +l3_pgentry_t ol3e = {}; +if (unlikely (pg_owner != pt_owner) )break ; + rc =mod_l3_entry (va,l3e_from_intpte (req.val), mfn, - update_flags, v,NULL ); + update_flags, v, &ol3e); + +if ( !rc && cmd ==MMU_PT_UPDATE_SWAP ) + { + req.val = ol3e.l3; +if (unlikely (copy_to_guest (ureqs, &req,1 )) ) + rc = -EFAULT ; + } +if ( !rc ) flush_linear_pt = true;break ;case PGT_l4_page_table: +l4_pgentry_t ol4e = {}; +if (unlikely (pg_owner != pt_owner) )break ; + rc =mod_l4_entry (va,l4e_from_intpte (req.val), mfn, - update_flags, v,NULL ); + update_flags, v, &ol4e); + +if ( !rc && cmd ==MMU_PT_UPDATE_SWAP ) + { + req.val = ol4e.l4; +if (unlikely (copy_to_guest (ureqs, &req,1 )) ) + rc = -EFAULT ; + } +if ( !rc ) flush_linear_pt = true;if ( !rc && pt_owner->arch.pv.xpti )
@@ -4270,6 +4314,11 @@ long do_mmu_update(break ;case PGT_writable_page: +if (unlikely (cmd ==MMU_PT_UPDATE_SWAP ) ) + { + rc = -EINVAL ; +break ; + }perfc_incr (writable_mmu_updates);paging_write_guest_entry (v, va, req.val, mfn); rc =0 ;
@@ -4279,7 +4328,8 @@ long do_mmu_update(if ( rc == -EINTR ) rc = -ERESTART ; } -else if (get_page_type (page, PGT_writable_page) ) +else if (likely (cmd !=MMU_PT_UPDATE_SWAP ) && +get_page_type (page, PGT_writable_page) ) {perfc_incr (writable_mmu_updates);paging_write_guest_entry (v, va, req.val, mfn);
xen/include/public/xen.h +1 -0
@@ -349,6 +349,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);#define MMU_PT_UPDATE_PRESERVE_AD 2/* atomically: *ptr = val | (*ptr&(A|D)) */ #define MMU_PT_UPDATE_NO_TRANSLATE 3/* checked '*ptr = val'. ptr is MA. */ /* val never translated. */ +#define MMU_PT_UPDATE_SWAP 4/* Update and return the old value. */ /* * MMU EXTENDED OPERATIONS
Signature · 1 line
2.52.0
Re: [PATCH v4 3/5] x86: extend do_mmu_update() to support returning the old PTE value
Quoted 5 lines · Kevin Lampis
xen/arch/x86/mm.c +0
-0
xen/arch/x86/mm.c long do_mmu_update( · line 4221
Quoted 9 lines
xen/arch/x86/mm.c +2
-0@@ -4212,37 +4213,80 @@ long do_mmu_update( update_flags =PTE_UPDATE_PRESERVE_AD ; else if ( cmd == MMU_PT_UPDATE_NO_TRANSLATE ) update_flags =PTE_UPDATE_NO_TRANSLATE ; + else if ( cmd == MMU_PT_UPDATE_SWAP ) + update_flags =PTE_UPDATE_SWAP ;switch ( page->u.inuse.type_info & PGT_type_mask ) {case PGT_l1_page_table:
{
xen/arch/x86/mm.c long do_mmu_update( · line 4231
Quoted 11 lines
xen/arch/x86/mm.c +9
-1+l1_pgentry_t ol1e = {}; rc =mod_l1_entry (va,l1e_from_intpte (req.val), mfn, - update_flags, v, pg_owner,NULL ); + update_flags, v, pg_owner, &ol1e); + +if ( !rc && cmd ==MMU_PT_UPDATE_SWAP ) + { + req.val = ol1e.l1; +if (unlikely (copy_to_guest (ureqs, &req,1 )) ) + rc = -EFAULT ; + }
}
You need braces, otherwise:
arch/x86/mm.c:4227:21: error: label followed by a declaration is a C23 extension [-Werror,-Wc23-extensions] 4227 | l1_pgentry_t ol1e = {};
| ^
It's also a MISRA violation.
https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/3046070649
Quoted 11 lines
xen/arch/x86/mm.c +4
-1break ;case PGT_l2_page_table: +l2_pgentry_t ol2e = {}; +if (unlikely (pg_owner != pt_owner) )break ; + rc =mod_l2_entry (va,l2e_from_intpte (req.val), mfn, - update_flags, v,NULL ); + update_flags, v, &ol2e);
xen/arch/x86/mm.c long do_mmu_update( · line 4251
Quoted 10 lines
xen/arch/x86/mm.c +8
-0+ +if ( !rc && cmd ==MMU_PT_UPDATE_SWAP ) + { + req.val = ol2e.l2; +if (unlikely (copy_to_guest (ureqs, &req,1 )) ) + rc = -EFAULT ; + } +if ( !rc ) flush_linear_pt = true;
You must set flush_linear_pt based on the success of mod_l2_entry() before potentially clobbering rc with -EFAULT.
As this logic stands, it's an easy privilege escalation into Xen, by causing Xen to skip a safety TLB flush.
Other than these two, everything looks in order.
~Andrew
Re: [PATCH v4 3/5] x86: extend do_mmu_update() to support returning the old PTE value
In reply to Kevin LampisQuoted 10 lines · Kevin Lampis
xen/arch/x86/mm.c +2
-0@@ -4212,37 +4213,80 @@ long do_mmu_update( update_flags =PTE_UPDATE_PRESERVE_AD ; else if ( cmd == MMU_PT_UPDATE_NO_TRANSLATE ) update_flags =PTE_UPDATE_NO_TRANSLATE ; + else if ( cmd == MMU_PT_UPDATE_SWAP ) + update_flags =PTE_UPDATE_SWAP ;switch ( page->u.inuse.type_info & PGT_type_mask ) {case PGT_l1_page_table:
xen/arch/x86/mm.c long do_mmu_update( · line 4230
Quoted 10 lines
xen/arch/x86/mm.c +8
-1+l1_pgentry_t ol1e = {}; rc =mod_l1_entry (va,l1e_from_intpte (req.val), mfn, - update_flags, v, pg_owner,NULL ); + update_flags, v, pg_owner, &ol1e); + +if ( !rc && cmd ==MMU_PT_UPDATE_SWAP ) + { + req.val = ol1e.l1; +if (unlikely (copy_to_guest (ureqs, &req,1 )) ) + rc = -EFAULT ;
Because of the earlier copy_from_guest(), __copy_to_guest() will do here. Also you only mean to copy back .val, so __copy_field_to_guest() is all you need. (Same further down, obviously.)
Jan
PATCH v4 4/5 x86: New feature flag XENFEAT_mmu_pt_update_swap.
In reply to Kevin Lampis--- xen/common/kernel.c | 1 + xen/include/public/features.h | 3 +++ 2 files changed, 4 insertions(+)
xen/common/kernel.c +1 -0
@@ -690,6 +690,7 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)if (is_pv_domain (d) ) { fi.submap |= (1U << XENFEAT_mmu_pt_update_preserve_ad) | + (1U << XENFEAT_mmu_pt_update_swap) | (1U << XENFEAT_highmem_assist) | (1U << XENFEAT_gnttab_map_avail_bits);if (IS_ENABLED (CONFIG_PV32 ) &&VM_ASSIST (d, pae_extended_cr3) )
xen/include/public/features.h +3 -0
@@ -128,6 +128,9 @@*/ #define XENFEAT_dm_msix_all_writes 20 +/* x86: Does this Xen host support the MMU_PT_UPDATE_SWAP hypercall? */ +#define XENFEAT_mmu_pt_update_swap 21 +#define XENFEAT_NR_SUBMAPS 1#endif /* __XEN_PUBLIC_FEATURES_H__ */
Signature · 1 line
2.52.0
Re: [PATCH v4 4/5] x86: New feature flag XENFEAT_mmu_pt_update_swap.
Quoted 1 line · Kevin Lampis
Signed-off-by Kevin Lampis <[email protected]>
This wants folding into the previous patch. Otherwise there's a boundary where the hypercall exists but isn't reported.
The prior patch is more simple than earlier revisions, so merging will be fine.
~Andrew
PATCH v4 5/5 xen: Add new Xen pv-op pte_get_and_clear
In reply to Kevin LampisIf Xen supports XENFEAT_mmu_pt_update_swap, use the new MMU_PT_UPDATE_SWAP instead of native_ptep_get_and_clear(). This avoids the pagetable write trapping for emulation.
---
Changes in v4:
- Reword the commit message
Changes in v3:
- Add spacing to xen_pte_get_and_clear
- BUG_ON if hypercall fails
- Remove extra blank line
- Add documentation for MMU_PT_UPDATE_SWAP
Changes in v2:
- New patch
--- arch/x86/include/asm/paravirt.h | 5 +++++ arch/x86/include/asm/paravirt_types.h | 1 + arch/x86/include/asm/pgtable.h | 3 ++- arch/x86/kernel/paravirt.c | 1 + arch/x86/xen/mmu_pv.c | 17 +++++++++++++++++ include/xen/interface/features.h | 2 ++ include/xen/interface/xen.h | 1 + 7 files changed, 29 insertions(+), 1 deletion(-)
arch/x86/include/asm/paravirt.h +5 -0
@@ -373,6 +373,11 @@ static inline void set_pmd(pmd_t *pmdp, pmd_t pmd)PVOP_VCALL2 (pv_ops, mmu.set_pmd, pmdp,native_pmd_val (pmd)); } +static inline pte_t pte_get_and_clear (pte_t *ptep) +{ +return (pte_t ){PVOP_CALL1 (pte_t, mmu.pte_get_and_clear, ptep)}; +} + static inline pmd_t __pmd(pmdval_t val) { return (pmd_t) { PVOP_ALT_CALLEE1(pmdval_t, pv_ops, mmu.make_pmd, val,
arch/x86/include/asm/paravirt_types.h +1 -0
@@ -135,6 +135,7 @@ struct pv_mmu_ops {/* Pagetable manipulation functions */ void (*set_pte)(pte_t *ptep,pte_t pteval);void (*set_pmd)(pmd_t *pmdp,pmd_t pmdval); +pte_t (*pte_get_and_clear)(pte_t *ptep);pte_t (*ptep_modify_prot_start)(struct vm_area_struct *vma,unsigned long addr,pte_t *ptep);
arch/x86/include/asm/pgtable.h +2 -1
@@ -58,6 +58,7 @@ extern pmdval_t early_pmd_flags;#include <asm/paravirt.h> #else /* !CONFIG_PARAVIRT_XXL */ #define set_pte (ptep, pte) native_set_pte(ptep, pte) +#define pte_get_and_clear (ptep) native_ptep_get_and_clear(ptep)#define set_pte_atomic (ptep, pte) \ native_set_pte_atomic(ptep, pte)
@@ -1243,7 +1244,7 @@ bool ptep_clear_flush_young(struct vm_area_struct *vma,static inline pte_t ptep_get_and_clear (struct mm_struct *mm,unsigned long addr,pte_t *ptep) { -pte_t pte =native_ptep_get_and_clear (ptep); +pte_t pte =pte_get_and_clear (ptep);page_table_check_pte_clear (mm, addr, pte);return pte; }
arch/x86/kernel/paravirt.c +1 -0
@@ -178,6 +178,7 @@ struct paravirt_patch_template pv_ops = { .mmu.set_pte = native_set_pte, .mmu.set_pmd = native_set_pmd, + .mmu.pte_get_and_clear = native_ptep_get_and_clear, .mmu.ptep_modify_prot_start = __ptep_modify_prot_start, .mmu.ptep_modify_prot_commit = __ptep_modify_prot_commit,
arch/x86/xen/mmu_pv.c +17 -0
@@ -359,6 +359,18 @@ static void xen_set_pte(pte_t *ptep, pte_t pteval)__xen_set_pte (ptep, pteval); } +static pte_t xen_pte_get_and_clear (pte_t *ptep) +{ +struct mmu_update u = { + .ptr =virt_to_machine (ptep).maddr |MMU_PT_UPDATE_SWAP , + .val =pte_val_ma (native_make_pte (0 )) + }; + +BUG_ON (HYPERVISOR_mmu_update (&u,1 ,NULL ,DOMID_SELF ) !=0 ); + +return native_make_pte (u.val); +} +static pte_t xen_ptep_modify_prot_start (struct vm_area_struct *vma,unsigned long addr,pte_t *ptep) {
@@ -2165,6 +2177,11 @@ static void __init xen_post_allocator_init(void) pv_ops.mmu.set_pud = xen_set_pud; pv_ops.mmu.set_p4d = xen_set_p4d; +if (xen_feature (XENFEAT_mmu_pt_update_swap)) + pv_ops.mmu.pte_get_and_clear = xen_pte_get_and_clear; +else + pv_ops.mmu.pte_get_and_clear = native_ptep_get_and_clear; +/* This will work as long as patching hasn't happened yet (which it hasn't) */ pv_ops.mmu.alloc_pte = xen_alloc_pte;
include/xen/interface/features.h +2 -0
@@ -97,6 +97,8 @@#define XENFEAT_not_direct_mapped 16#define XENFEAT_direct_mapped 17 +#define XENFEAT_mmu_pt_update_swap 21 +#define XENFEAT_NR_SUBMAPS 1#endif /* __XEN_PUBLIC_FEATURES_H__ */
include/xen/interface/xen.h +1 -0
@@ -252,6 +252,7 @@#define MMU_MACHPHYS_UPDATE 1/* ptr = MA of frame to modify entry for */ #define MMU_PT_UPDATE_PRESERVE_AD 2/* atomically: *ptr = val | (*ptr&(A|D)) */ #define MMU_PT_UPDATE_NO_TRANSLATE 3/* checked '*ptr = val'. ptr is MA. */ +#define MMU_PT_UPDATE_SWAP 4/* Update and return the old value. */ /* * MMU EXTENDED OPERATIONS
Signature · 1 line
2.52.0
Re: [PATCH Linux v4 5/5] xen: Add new Xen pv-op pte_get_and_clear
Quoted 5 lines · Kevin Lampis
If Xen supports
XENFEAT_mmu_pt_update_swap, use the newMMU_PT_UPDATE_SWAPinstead ofnative_ptep_get_and_clear(). This avoids the pagetable write trapping for emulation.Signed-off-by Kevin Lampis <[email protected]>
I'll take this after the Xen-side public header related patch has been taken.
Juergen