Re: [PATCH 1/4] x86: extend update_intpte() to support atomic get-and-update
Jan Beulich <[email protected]>
| Newsgroups | gmane.comp.emulators.xen.devel |
|---|---|
| Message-ID | <[email protected]> |
On 27.07.2026 17:06, Kevin Lampis wrote:
> --- a/xen/arch/x86/pv/mm.h
> +++ b/xen/arch/x86/pv/mm.h
> @@ -66,13 +66,14 @@ static inline intpte_t paging_cmpxchg_guest_entry(
> * How to write an entry to the guest pagetables.
> * Returns false for failure (pointer not valid), true for success.
> */
> -static inline bool update_intpte(intpte_t *p, intpte_t old, intpte_t new,
> - mfn_t mfn, struct vcpu *v, bool preserve_ad)
> +static inline bool update_intpte(intpte_t *p, intpte_t *old, intpte_t new,
> + mfn_t mfn, struct vcpu *v, bool preserve_ad,
> + bool use_cmpxchg)
No 2nd boolean parameter, please. (use_cmpxchg also doesn't look to be an
overly good name; "swap" maybe?) As you switch old to being a pointer, and
as ...
> {
> bool rv = true;
>
> #ifndef PTE_UPDATE_WITH_CMPXCHG
> - if ( !preserve_ad )
> + if ( !preserve_ad && !use_cmpxchg )
> paging_write_guest_entry(v, p, new, mfn);
... *old isn't used here, having callers pass in NULL in that case may be
one option.
(In any event, old becoming a pointer imo needs commenting upon, as otherwise
one might expect this to be only an output.)
Alternatively I have an old patch lying around which looks to apply cleanly,
and which may be useful here; see at the bottom.
> @@ -82,30 +83,36 @@ static inline bool update_intpte(intpte_t *p, intpte_t old, intpte_t new,
> intpte_t _new = new, t;
>
> if ( preserve_ad )
> - _new |= old & (_PAGE_ACCESSED | _PAGE_DIRTY);
> + _new |= *old & (_PAGE_ACCESSED | _PAGE_DIRTY);
>
> - t = paging_cmpxchg_guest_entry(v, p, old, _new, mfn);
> + t = paging_cmpxchg_guest_entry(v, p, *old, _new, mfn);
>
> - if ( t == old )
> + if ( t == *old )
> break;
>
> /* Allowed to change in Accessed/Dirty flags only. */
> - BUG_ON((t ^ old) & ~(intpte_t)(_PAGE_ACCESSED|_PAGE_DIRTY));
> + BUG_ON((t ^ *old) & ~(intpte_t)(_PAGE_ACCESSED|_PAGE_DIRTY));
>
> - old = t;
> + *old = t;
> }
> }
> return rv;
> }
>
> +static inline bool _update_intpte(intpte_t *p, intpte_t old, intpte_t new,
> + mfn_t mfn, struct vcpu *v, bool preserve_ad)
> +{
> + return update_intpte(p, &old, new, mfn, v, preserve_ad, false);
> +}
> +
> /*
> * 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,_ad) \
> - update_intpte(&_t ## e_get_intpte(*(_p)), \
> - _t ## e_get_intpte(_o), _t ## e_get_intpte(_n), \
> - (_m), (_v), (_ad))
> + _update_intpte(&_t ## e_get_intpte(*(_p)), \
> + _t ## e_get_intpte(_o), _t ## e_get_intpte(_n), \
> + (_m), (_v), (_ad))
Like the patch below does - if already this last line needs touching, I
think the excess parentheses then also want dropping.
Tangentially: We will want to consider dropping the function's return
value, since as of 1bc30c076a7f ("x86/mm:
{paging, sh}_{cmpxchg, write}_guest_entry() cannot fault") it only ever
returns true.
Jan
x86: make UPDATE_ENTRY() allow for multiple operation flags
Signed-off-by: Jan Beulich <[email protected]>
Reviewed-by: George Dunlap <[email protected]>
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2149,10 +2149,9 @@ 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 cmd,
+ mfn_t gl1mfn, unsigned int update_flags,
struct vcpu *pt_vcpu, struct domain *pg_dom)
{
- bool preserve_ad = (cmd == MMU_PT_UPDATE_PRESERVE_AD);
l1_pgentry_t ol1e = l1e_read(pl1e);
struct domain *pt_dom = pt_vcpu->domain;
int rc = 0;
@@ -2172,7 +2171,7 @@ static int mod_l1_entry(l1_pgentry_t *pl
}
/* Translate foreign guest address. */
- if ( cmd != MMU_PT_UPDATE_NO_TRANSLATE &&
+ if ( !(update_flags & PTE_UPDATE_NO_TRANSLATE) &&
paging_mode_translate(pg_dom) )
{
p2m_type_t p2mt;
@@ -2213,7 +2212,7 @@ static int mod_l1_entry(l1_pgentry_t *pl
if ( !l1e_has_changed(ol1e, nl1e, ~FASTPATH_FLAG_WHITELIST) )
{
rc = UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu,
- preserve_ad);
+ update_flags);
if ( page )
put_page(page);
return rc ? 0 : -EBUSY;
@@ -2237,7 +2236,7 @@ static int mod_l1_entry(l1_pgentry_t *pl
put_page(page);
if ( unlikely(!UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu,
- preserve_ad)) )
+ update_flags)) )
{
ol1e = nl1e;
rc = -EBUSY;
@@ -2246,7 +2245,7 @@ static int mod_l1_entry(l1_pgentry_t *pl
else if ( pv_l1tf_check_l1e(pt_dom, nl1e) )
return -ERESTART;
else if ( unlikely(!UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu,
- preserve_ad)) )
+ update_flags)) )
{
return -EBUSY;
}
@@ -2260,7 +2259,7 @@ static int mod_l1_entry(l1_pgentry_t *pl
static int mod_l2_entry(l2_pgentry_t *pl2e,
l2_pgentry_t nl2e,
mfn_t mfn,
- int preserve_ad,
+ unsigned int update_flags,
struct vcpu *vcpu)
{
l2_pgentry_t ol2e;
@@ -2292,7 +2291,7 @@ 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) )
{
- if ( UPDATE_ENTRY(l2, pl2e, ol2e, nl2e, mfn, vcpu, preserve_ad) )
+ if ( UPDATE_ENTRY(l2, pl2e, ol2e, nl2e, mfn, vcpu, update_flags) )
return 0;
return -EBUSY;
}
@@ -2301,7 +2300,7 @@ static int mod_l2_entry(l2_pgentry_t *pl
return rc;
if ( unlikely(!UPDATE_ENTRY(l2, pl2e, ol2e, nl2e, mfn, vcpu,
- preserve_ad)) )
+ update_flags)) )
{
ol2e = nl2e;
rc = -EBUSY;
@@ -2310,7 +2309,7 @@ static int mod_l2_entry(l2_pgentry_t *pl
else if ( pv_l1tf_check_l2e(d, nl2e) )
return -ERESTART;
else if ( unlikely(!UPDATE_ENTRY(l2, pl2e, ol2e, nl2e, mfn, vcpu,
- preserve_ad)) )
+ update_flags)) )
{
return -EBUSY;
}
@@ -2324,7 +2323,7 @@ static int mod_l2_entry(l2_pgentry_t *pl
static int mod_l3_entry(l3_pgentry_t *pl3e,
l3_pgentry_t nl3e,
mfn_t mfn,
- int preserve_ad,
+ unsigned int update_flags,
struct vcpu *vcpu)
{
l3_pgentry_t ol3e;
@@ -2354,7 +2353,7 @@ 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) )
{
- rc = UPDATE_ENTRY(l3, pl3e, ol3e, nl3e, mfn, vcpu, preserve_ad);
+ rc = UPDATE_ENTRY(l3, pl3e, ol3e, nl3e, mfn, vcpu, update_flags);
return rc ? 0 : -EFAULT;
}
@@ -2364,7 +2363,7 @@ static int mod_l3_entry(l3_pgentry_t *pl
rc = 0;
if ( unlikely(!UPDATE_ENTRY(l3, pl3e, ol3e, nl3e, mfn, vcpu,
- preserve_ad)) )
+ update_flags)) )
{
ol3e = nl3e;
rc = -EFAULT;
@@ -2373,7 +2372,7 @@ static int mod_l3_entry(l3_pgentry_t *pl
else if ( pv_l1tf_check_l3e(d, nl3e) )
return -ERESTART;
else if ( unlikely(!UPDATE_ENTRY(l3, pl3e, ol3e, nl3e, mfn, vcpu,
- preserve_ad)) )
+ update_flags)) )
{
return -EFAULT;
}
@@ -2386,7 +2385,7 @@ static int mod_l3_entry(l3_pgentry_t *pl
static int mod_l4_entry(l4_pgentry_t *pl4e,
l4_pgentry_t nl4e,
mfn_t mfn,
- int preserve_ad,
+ unsigned int update_flags,
struct vcpu *vcpu)
{
struct domain *d = vcpu->domain;
@@ -2416,7 +2415,7 @@ 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) )
{
- rc = UPDATE_ENTRY(l4, pl4e, ol4e, nl4e, mfn, vcpu, preserve_ad);
+ rc = UPDATE_ENTRY(l4, pl4e, ol4e, nl4e, mfn, vcpu, update_flags);
return rc ? 0 : -EFAULT;
}
@@ -2426,7 +2425,7 @@ static int mod_l4_entry(l4_pgentry_t *pl
rc = 0;
if ( unlikely(!UPDATE_ENTRY(l4, pl4e, ol4e, nl4e, mfn, vcpu,
- preserve_ad)) )
+ update_flags)) )
{
ol4e = nl4e;
rc = -EFAULT;
@@ -2435,7 +2434,7 @@ static int mod_l4_entry(l4_pgentry_t *pl
else if ( pv_l1tf_check_l4e(d, nl4e) )
return -ERESTART;
else if ( unlikely(!UPDATE_ENTRY(l4, pl4e, ol4e, nl4e, mfn, vcpu,
- preserve_ad)) )
+ update_flags)) )
{
return -EFAULT;
}
@@ -4135,18 +4134,23 @@ long do_mmu_update(
if ( page_lock(page) )
{
+ unsigned int update_flags = (cmd == MMU_PT_UPDATE_PRESERVE_AD)
+ ? PTE_UPDATE_PRESERVE_AD
+ : (cmd == MMU_PT_UPDATE_NO_TRANSLATE)
+ ? PTE_UPDATE_NO_TRANSLATE : 0;
+
switch ( page->u.inuse.type_info & PGT_type_mask )
{
case PGT_l1_page_table:
rc = mod_l1_entry(va, l1e_from_intpte(req.val), mfn,
- cmd, v, pg_owner);
+ update_flags, v, pg_owner);
break;
case PGT_l2_page_table:
if ( unlikely(pg_owner != pt_owner) )
break;
rc = mod_l2_entry(va, l2e_from_intpte(req.val), mfn,
- cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
+ update_flags, v);
if ( !rc )
flush_linear_pt = true;
break;
@@ -4155,7 +4159,7 @@ long do_mmu_update(
if ( unlikely(pg_owner != pt_owner) )
break;
rc = mod_l3_entry(va, l3e_from_intpte(req.val), mfn,
- cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
+ update_flags, v);
if ( !rc )
flush_linear_pt = true;
break;
@@ -4164,7 +4168,7 @@ long do_mmu_update(
if ( unlikely(pg_owner != pt_owner) )
break;
rc = mod_l4_entry(va, l4e_from_intpte(req.val), mfn,
- cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
+ update_flags, v);
if ( !rc )
flush_linear_pt = true;
if ( !rc && pt_owner->arch.pv.xpti )
--- a/xen/arch/x86/pv/mm.h
+++ b/xen/arch/x86/pv/mm.h
@@ -62,17 +62,20 @@ static inline intpte_t paging_cmpxchg_gu
#undef PTE_UPDATE_WITH_CMPXCHG
#endif
+#define PTE_UPDATE_PRESERVE_AD (1u << 0)
+#define PTE_UPDATE_NO_TRANSLATE (1u << 1)
+
/*
* How to write an entry to the guest pagetables.
* Returns false for failure (pointer not valid), true for success.
*/
static inline bool update_intpte(intpte_t *p, intpte_t old, intpte_t new,
- mfn_t mfn, struct vcpu *v, bool preserve_ad)
+ mfn_t mfn, struct vcpu *v, unsigned int flags)
{
bool rv = true;
#ifndef PTE_UPDATE_WITH_CMPXCHG
- if ( !preserve_ad )
+ if ( !(flags & PTE_UPDATE_PRESERVE_AD) )
paging_write_guest_entry(v, p, new, mfn);
else
#endif
@@ -81,7 +84,7 @@ static inline bool update_intpte(intpte_
{
intpte_t _new = new, t;
- if ( preserve_ad )
+ if ( flags & PTE_UPDATE_PRESERVE_AD )
_new |= old & (_PAGE_ACCESSED | _PAGE_DIRTY);
t = paging_cmpxchg_guest_entry(v, p, old, _new, mfn);
@@ -102,10 +105,10 @@ static inline bool update_intpte(intpte_
* 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,_ad) \
+#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), (_ad))
+ _m, _v, fl)
static always_inline l1_pgentry_t adjust_guest_l1e(l1_pgentry_t l1e,
const struct domain *d)