Xen devel - Jan 2009 - [PATCH 2/2] x86: add pte_set_flags/clear_flags for pte flag manipulation

It''s not necessary to deconstruct and reconstruct a pte every time its
flags are being updated.  Introduce pte_set_flags and pte_clear_flags
to set and clear flags in a pte.  This allows the flag manipulation
code to be inlined, and avoids calls via paravirt-ops.

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
---
 arch/x86/include/asm/pgtable.h |   38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

==================================================================---
a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -240,64 +240,78 @@
 		(_PAGE_PSE | _PAGE_PRESENT);
 }
 
+static inline pte_t pte_set_flags(pte_t pte, pteval_t set)
+{
+	pteval_t v = native_pte_val(pte);
+
+	return native_make_pte(v | set);
+}
+
+static inline pte_t pte_clear_flags(pte_t pte, pteval_t clear)
+{
+	pteval_t v = native_pte_val(pte);
+
+	return native_make_pte(v & ~clear);
+}
+
 static inline pte_t pte_mkclean(pte_t pte)
 {
-	return __pte(pte_val(pte) & ~_PAGE_DIRTY);
+	return pte_clear_flags(pte, _PAGE_DIRTY);
 }
 
 static inline pte_t pte_mkold(pte_t pte)
 {
-	return __pte(pte_val(pte) & ~_PAGE_ACCESSED);
+	return pte_clear_flags(pte, _PAGE_ACCESSED);
 }
 
 static inline pte_t pte_wrprotect(pte_t pte)
 {
-	return __pte(pte_val(pte) & ~_PAGE_RW);
+	return pte_clear_flags(pte, _PAGE_RW);
 }
 
 static inline pte_t pte_mkexec(pte_t pte)
 {
-	return __pte(pte_val(pte) & ~_PAGE_NX);
+	return pte_clear_flags(pte, _PAGE_NX);
 }
 
 static inline pte_t pte_mkdirty(pte_t pte)
 {
-	return __pte(pte_val(pte) | _PAGE_DIRTY);
+	return pte_set_flags(pte, _PAGE_DIRTY);
 }
 
 static inline pte_t pte_mkyoung(pte_t pte)
 {
-	return __pte(pte_val(pte) | _PAGE_ACCESSED);
+	return pte_set_flags(pte, _PAGE_ACCESSED);
 }
 
 static inline pte_t pte_mkwrite(pte_t pte)
 {
-	return __pte(pte_val(pte) | _PAGE_RW);
+	return pte_set_flags(pte, _PAGE_RW);
 }
 
 static inline pte_t pte_mkhuge(pte_t pte)
 {
-	return __pte(pte_val(pte) | _PAGE_PSE);
+	return pte_set_flags(pte, _PAGE_PSE);
 }
 
 static inline pte_t pte_clrhuge(pte_t pte)
 {
-	return __pte(pte_val(pte) & ~_PAGE_PSE);
+	return pte_clear_flags(pte, _PAGE_PSE);
 }
 
 static inline pte_t pte_mkglobal(pte_t pte)
 {
-	return __pte(pte_val(pte) | _PAGE_GLOBAL);
+	return pte_set_flags(pte, _PAGE_GLOBAL);
 }
 
 static inline pte_t pte_clrglobal(pte_t pte)
 {
-	return __pte(pte_val(pte) & ~_PAGE_GLOBAL);
+	return pte_clear_flags(pte, _PAGE_GLOBAL);
 }
 
 static inline pte_t pte_mkspecial(pte_t pte)
 {
-	return __pte(pte_val(pte) | _PAGE_SPECIAL);
+	return pte_set_flags(pte, _PAGE_SPECIAL);
 }
 
 extern pteval_t __supported_pte_mask;



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> Jeremy Fitzhardinge <jeremy@goop.org> 22.01.09 23:24
>>>
>+static inline pte_t pte_set_flags(pte_t pte, pteval_t set)
>+{
>+	pteval_t v = native_pte_val(pte);
>+
>+	return native_make_pte(v | set);
>+}
>+
>+static inline pte_t pte_clear_flags(pte_t pte, pteval_t clear)
>+{
>+	pteval_t v = native_pte_val(pte);
>+
>+	return native_make_pte(v & ~clear);
>+}
I think a comment (or event a BUG_ON()) should be added here to make
clear that this absolutely must not be used to toggle the present bit. I
even view toggling _PAGE_PSE as dangerous this way.

And alternative would be to make these macros and #undef them (or keep
them inline functions but add destructive #define-s) after all their users.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich wrote:
> I think a comment (or event a BUG_ON()) should be added here to make
> clear that this absolutely must not be used to toggle the present bit. I
> even view toggling _PAGE_PSE as dangerous this way.
>
> And alternative would be to make these macros and #undef them (or keep
> them inline functions but add destructive #define-s) after all their users.
>   
I don''t see any particular problem with changing PSE or even Present 
with these functions; they don''t operate on live in-memory ptes, so its
not like they could ever be used to modify a pte unless followed with 
some kind of set_pte operation.  It is unwise to change any pte flag 
without knowing what you''re doing (though P or PSE would probably have 
less subtle effects than some of the others).

But it probably wouldn''t hurt to have a __ prefix to indicate
they''re
"internal" and not for general use.

    J

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> Jeremy Fitzhardinge <jeremy@goop.org> 23.01.09 22:05
>>>
>Jan Beulich wrote:
>> I think a comment (or event a BUG_ON()) should be added here to make
>> clear that this absolutely must not be used to toggle the present bit.
I
>> even view toggling _PAGE_PSE as dangerous this way.
>>
>> And alternative would be to make these macros and #undef them (or keep
>> them inline functions but add destructive #define-s) after all their
users.
>>   
>
>I don''t see any particular problem with changing PSE or even
Present
>with these functions; they don''t operate on live in-memory ptes, so
its
>not like they could ever be used to modify a pte unless followed with 
>some kind of set_pte operation.  It is unwise to change any pte flag 
>without knowing what you''re doing (though P or PSE would probably
have
>less subtle effects than some of the others).
Whether a pte is live doesn''t matter here: If you change P on Xen, the
frame number representation *must* change from/to PFN to/from MFN.
In no case (other than iomem pages) is it allowed to flip just this bit.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich wrote:
> Whether a pte is live doesn''t matter here: If you change P on Xen,
the
> frame number representation *must* change from/to PFN to/from MFN.
> In no case (other than iomem pages) is it allowed to flip just this bit.
Yes, true. Same with _PAGE_SPECIAL, which is actually flipped by code 
here.  I guess a warning here might be useful, but really the calling 
code needs to be properly disciplined; after all setting P on a swap 
entry is no fun either.

    J

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel