|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v5 3/6] x86: extend mod_l*_entry() to optionally return the old PTE value
On 12.11.2026 17:36, Andrew Cooper wrote:
> On 12/11/2026 1:50 pm, Jan Beulich wrote:
>> On 12.11.2026 11:00, Kevin Lampis wrote:
>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -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;
>>> @@ -2280,7 +2285,12 @@ static int mod_l1_entry(l1_pgentry_t *pl1e,
>>> l1_pgentry_t nl1e,
>>> else if ( pv_l1tf_check_l1e(pt_dom, nl1e) )
>>> return -ERESTART;
>>>
>>> - 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 0;
>> This (recurring) pattern still adds quite a bit of redundancy. Was it ruled
>> out
>> in an earlier version to pass ol1e_out into UPDATE_ENTRY() (and perhaps
>> through
>> to update_intpte()), to have the updating done there? If so, why?
>
> I can't spot a nice way of doing this.
See below. That's still retaining type safety as per your reply to patch 4
and as per ...
> The pattern is quite nasty at all levels. We have pl?e which is the
> live PTE in guest memory. We read that (once) to get ol?e, then pass it
> down to a CMPXCHG which can return it modified by A/D.
>
> The issue is it's modify_l?_entry() which reads pl?e into ol?e, not the
> caller, so you can't just plumb mod_l1_entry()'s parameter all the way down.
>
> Also, mod_l?_entry() wants to be typesafe, whereas UPDATE_ENTRY() wants
> to be not.
... what you say here. Things may get yet a bit easier if we sacrificed
that (albeit, as indicated there, more in patch 4 than here). With some
count_args() trickery it may also be possible to avoid the need to touch
pv/grant_table.c and pv/ro-page-fault.c. Not sure whether that would be
worth it.
Jan
x86: extend mod_l*_entry() to optionally return the old PTE value
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 <kevin.lampis@xxxxxxxxxx>
Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
---
Changes in v6:
- Write of the output value moved into UPDATE_ENTRY().
Changes in v5:
- Refactoring the mod_l*_entry() functions was split out
into a patch before this one
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
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2194,7 +2194,8 @@ static void l3t_unlock(struct page_info
/* 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;
@@ -2255,7 +2256,8 @@ static int mod_l1_entry(l1_pgentry_t *pl
/* 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);
+ UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, update_flags,
+ ol1e_out);
if ( page )
put_page(page);
return 0;
@@ -2281,7 +2283,7 @@ static int mod_l1_entry(l1_pgentry_t *pl
else if ( pv_l1tf_check_l1e(pt_dom, nl1e) )
return -ERESTART;
- UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, update_flags);
+ UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu, update_flags,
ol1e_out);
put_page_from_l1e(ol1e, pt_dom);
return 0;
@@ -2293,7 +2295,8 @@ static int mod_l2_entry(l2_pgentry_t *pl
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,7 +2328,8 @@ static int mod_l2_entry(l2_pgentry_t *pl
/* 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);
+ UPDATE_ENTRY(l2, pl2e, ol2e, nl2e, mfn, vcpu, update_flags,
+ ol2e_out);
return 0;
}
@@ -2335,7 +2339,7 @@ static int mod_l2_entry(l2_pgentry_t *pl
else if ( pv_l1tf_check_l2e(d, nl2e) )
return -ERESTART;
- UPDATE_ENTRY(l2, pl2e, ol2e, nl2e, mfn, vcpu, update_flags);
+ UPDATE_ENTRY(l2, pl2e, ol2e, nl2e, mfn, vcpu, update_flags, ol2e_out);
put_page_from_l2e(ol2e, mfn, PTF_defer);
@@ -2347,7 +2351,8 @@ static int mod_l3_entry(l3_pgentry_t *pl
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;
@@ -2377,7 +2382,8 @@ static int mod_l3_entry(l3_pgentry_t *pl
/* 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);
+ UPDATE_ENTRY(l3, pl3e, ol3e, nl3e, mfn, vcpu, update_flags,
+ ol3e_out);
return 0;
}
@@ -2388,7 +2394,7 @@ static int mod_l3_entry(l3_pgentry_t *pl
else if ( pv_l1tf_check_l3e(d, nl3e) )
return -ERESTART;
- UPDATE_ENTRY(l3, pl3e, ol3e, nl3e, mfn, vcpu, update_flags);
+ UPDATE_ENTRY(l3, pl3e, ol3e, nl3e, mfn, vcpu, update_flags, ol3e_out);
put_page_from_l3e(ol3e, mfn, PTF_defer);
return 0;
@@ -2399,7 +2405,8 @@ static int mod_l4_entry(l4_pgentry_t *pl
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;
@@ -2429,7 +2436,8 @@ static int mod_l4_entry(l4_pgentry_t *pl
/* 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);
+ UPDATE_ENTRY(l4, pl4e, ol4e, nl4e, mfn, vcpu, update_flags,
+ ol4e_out);
return 0;
}
@@ -2440,7 +2448,7 @@ static int mod_l4_entry(l4_pgentry_t *pl
else if ( pv_l1tf_check_l4e(d, nl4e) )
return -ERESTART;
- UPDATE_ENTRY(l4, pl4e, ol4e, nl4e, mfn, vcpu, update_flags);
+ UPDATE_ENTRY(l4, pl4e, ol4e, nl4e, mfn, vcpu, update_flags, ol4e_out);
put_page_from_l4e(ol4e, mfn, PTF_defer);
return 0;
@@ -3318,7 +3326,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 +4164,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 +4180,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 +4189,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 +4529,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);
--- a/xen/arch/x86/pv/grant_table.c
+++ b/xen/arch/x86/pv/grant_table.c
@@ -98,7 +98,7 @@ int create_grant_pv_mapping(uint64_t add
goto out_unlock;
ol1e = *pl1e;
- UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, curr, 0);
+ UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, curr, 0, null_pl1e);
/*
* We always create mappings in this path. However, our caller,
* map_grant_ref(), only passes potentially non-zero cache_flags for
@@ -163,7 +163,7 @@ static bool steal_linear_address(unsigne
goto out_unlock;
ol1e = *pl1e;
- UPDATE_ENTRY(l1, pl1e, ol1e, l1e_empty(), gl1mfn, curr, 0);
+ UPDATE_ENTRY(l1, pl1e, ol1e, l1e_empty(), gl1mfn, curr, 0, null_pl1e);
*out = ol1e;
okay = true;
@@ -290,7 +290,7 @@ int replace_grant_pv_mapping(uint64_t ad
"PTE flags %x for %"PRIx64" don't match grant (%x)\n",
l1e_get_flags(ol1e), addr, grant_pte_flags);
- UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, curr, 0);
+ UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, curr, 0, null_pl1e);
/*
* Generally, replace_grant_pv_mapping() is used to destroy mappings
* (n1le = l1e_empty()), but it can be a present mapping on the
--- a/xen/arch/x86/pv/mm.h
+++ b/xen/arch/x86/pv/mm.h
@@ -109,10 +109,17 @@ static inline intpte_t update_intpte(int
* Macro that wraps the appropriate type-changes around update_intpte().
* Arguments are: type, ptr, old, new, mfn, vcpu
*/
-#define UPDATE_ENTRY(_t ,_p ,_o ,_n ,_m ,_v , fl) \
- update_intpte(&_t ## e_get_intpte(*(_p)), \
- _t ## e_get_intpte(_o), _t ## e_get_intpte(_n), \
- _m, _v, fl)
+#define UPDATE_ENTRY(_t ,_p ,_o ,_n ,_m ,_v , fl, out) do { \
+ intpte_t old_ = update_intpte(&_t ## e_get_intpte(*(_p)), \
+ _t ## e_get_intpte(_o), \
+ _t ## e_get_intpte(_n), \
+ _m, _v, fl); \
+ if ( out ) \
+ *(out) = _t ## e_from_intpte(old_); \
+} while ( false )
+
+/* Use this instead of NULL for UPDATE_ENTRY()'s last argument. */
+static l1_pgentry_t *const null_pl1e = NULL;
static always_inline l1_pgentry_t adjust_guest_l1e(l1_pgentry_t l1e,
const struct domain *d)
--- a/xen/arch/x86/pv/ro-page-fault.c
+++ b/xen/arch/x86/pv/ro-page-fault.c
@@ -200,7 +200,7 @@ static int ptwr_emulated_update(unsigned
else
{
ol1e = *pl1e;
- UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, mfn, v, 0);
+ UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, mfn, v, 0, null_pl1e);
}
trace_ptwr_emulation(addr, nl1e);
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |