paravirt: set_access_flags/set_wrprotect should use paravirt interface]

PATCH Jeremy Fitzhardinge 3 participants 5 messages Jan 30, 2008 – Jan 31, 2008 3 messages not in the archive KVM

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): 20080130223928.GA26561@dmt

Jeremy Fitzhardinge

author

2008-01-30 23:00 UTC #1 lore raw

Quoted 3 lines · Marcelo Tosatti

Forgot to copy you... Ideally all pte updates should be done via the paravirt interface.

Hm, are you sure?

Review

Quoted 7 lines

(quoted diff) +6 -0

+static inline void pte_clear_bit(unsigned int bit, pte_t *ptep)
+{
+	pte_t pte = *ptep;
+	clear_bit(bit, (unsigned long *)&pte.pte);
+	set_pte(ptep, pte);
+}

This is not a safe transformation. This will lose hardware A/D bit changes if the pte is part of an active pagetable. Aside from that, clear_bit is atomic and so is the wrong thing to use on a local variable; a plain bitmask would do the job.

clear_bit on a PTE is fine, since everyone has to support some level of trap'n'emulate on pte-level entries (ptep_get_and_clear is very hard to implement otherwise). Shadow pagetable implementations will do this naturally, and Xen must do it to allow atomic updates on ptes (also because it makes late-pinning/early-unpinning a performance win).

Quoted 18 lines

(quoted diff) +1 -0

+
 static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
 			      pte_t *ptep, pte_t pte)
 {

Index: linux-2.6-x86-kvm/include/asm-x86/pgtable.h
===================================================================

include/asm-x86/pgtable.h +2 -0

@@ -227,6 +227,8 @@ void native_pagetable_setup_done(pgd_t *
 #define pte_update(mm, addr, ptep)              do { } while (0)
 #define pte_update_defer(mm, addr, ptep)        do { } while (0)
 
+#define pte_clear_bit(bit, ptep)	clear_bit(bit, (unsigned long *)&ptep->pte)
+
 static inline void paravirt_pagetable_setup_start(pgd_t *base)
 {
 	native_pagetable_setup_start(base);

include/asm-x86/pgtable.h static inline void native_set_pte_at(str · line 308

Quoted 6 lines

include/asm-x86/pgtable.h +1 -1

@@ -302,7 +304,7 @@ static inline void native_set_pte_at(str
 ({									\
 	int __changed = !pte_same(*(ptep), entry);			\
 	if (__changed && dirty) {					\
-		*ptep = entry;						\
+		set_pte(ptep, entry);					\

This change is OK, but doesn't really do anything. All hypervisors allow direct access to ptes.

include/asm-x86/pgtable.h static inline pte_t ptep_get_and_clear_f · line 363

Quoted 10 lines

include/asm-x86/pgtable.h +0 -0

 		pte_update_defer((vma)->vm_mm, (address), (ptep));	\
 		flush_tlb_page(vma, address);				\

include/asm-x86/pgtable.h +1 -1

 	}								\

@@ -357,7 +359,7 @@ static inline pte_t ptep_get_and_clear_f
 #define __HAVE_ARCH_PTEP_SET_WRPROTECT
 static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
 {
-	clear_bit(_PAGE_BIT_RW, (unsigned long *)&ptep->pte);
+	pte_clear_bit(_PAGE_BIT_RW, ptep);

This is only valid if ptep_set_wrprotect is never used on active ptes, which I don't think is the case.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

A message that is not in the archive (sent off-list or not yet received): [email protected]

Marcelo Tosatti

2008-01-31 00:39 UTC #3 lore raw

Quoted 7 lines · Jeremy Fitzhardinge

Quoted 3 lines · Marcelo Tosatti
Forgot to copy you... Ideally all pte updates should be done via the paravirt interface.
Hm, are you sure?

Quoted 12 lines

Review
Quoted 7 lines
(quoted diff) +6 -0
+static inline void pte_clear_bit(unsigned int bit, pte_t *ptep)
+{
+	pte_t pte = *ptep;
+	clear_bit(bit, (unsigned long *)&pte.pte);
+	set_pte(ptep, pte);
+}
This is not a safe transformation. This will lose hardware A/D bit changes if the pte is part of an active pagetable. Aside from that, clear_bit is atomic and so is the wrong thing to use on a local variable; a plain bitmask would do the job.

True this is racy on baremetal.

Quoted 5 lines

clear_bit on a PTE is fine, since everyone has to support some level of trap'n'emulate on pte-level entries (ptep_get_and_clear is very hard to implement otherwise). Shadow pagetable implementations will do this naturally, and Xen must do it to allow atomic updates on ptes (also because it makes late-pinning/early-unpinning a performance win).

Sure clear_bit on a PTE is fine, but we wan't to virtualize for batching pte updates via hypercall.

Quoted 12 lines

Quoted 12 lines
+
static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
pte_t *ptep, pte_t pte)
{
Index: linux-2.6-x86-kvm/include/asm-x86/pgtable.h
===================================================================
include/asm-x86/pgtable.h +0 -0
@@ -227,6 +227,8 @@ void native_pagetable_setup_done(pgd_t *
 #define pte_update(mm, addr, ptep)              do { } while (0)
 #define pte_update_defer(mm, addr, ptep)        do { } while (0)
 

Quoted 16 lines

include/asm-x86/pgtable.h static inline void native_set_pte_at(str · line 308
Quoted 13 lines
include/asm-x86/pgtable.h +2 -0
+#define pte_clear_bit(bit, ptep)	clear_bit(bit, (unsigned long 
 *)&ptep->pte)
+
 static inline void paravirt_pagetable_setup_start(pgd_t *base)
 {
	native_pagetable_setup_start(base);
include/asm-x86/pgtable.h +1 -1
@@ -302,7 +304,7 @@ static inline void native_set_pte_at(str
 ({									\
 	int __changed = !pte_same(*(ptep), entry);			\
 	if (__changed && dirty) {					\
-		*ptep = entry;						\
+		set_pte(ptep, entry);					\
 
This change is OK, but doesn't really do anything. All hypervisors allow direct access to ptes.

We want the pte update to go through a hypercall instead of trap'n'emulate.

Quoted 15 lines

include/asm-x86/pgtable.h
Quoted 12 lines
include/asm-x86/pgtable.h +0 -0
 		pte_update_defer((vma)->vm_mm, (address), (ptep));	\
 		flush_tlb_page(vma, address);				\
	}								\
include/asm-x86/pgtable.h +0 -0
@@ -357,7 +359,7 @@ static inline pte_t ptep_get_and_clear_f
 #define __HAVE_ARCH_PTEP_SET_WRPROTECT
static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
{
include/asm-x86/pgtable.h +1 -1
-	clear_bit(_PAGE_BIT_RW, (unsigned long *)&ptep->pte);
+	pte_clear_bit(_PAGE_BIT_RW, ptep);
This is only valid if ptep_set_wrprotect is never used on active ptes, which I don't think is the case.

Ok, pte_clear_bit() like above is broken. But the point is that we want to batch the write protection of the source pte along with the creation of the write protected destination pte on COW using the paravirt_enter_lazy/paravirt_leave_lazy.

So a pv_mmu_ops->pte_clear_bit() which does native clear_bit() on baremetal and a hypercall based variant on KVM (or Xen) seems an appropriate thing to do. I'll come up with a decent patch.

Thanks

Jeremy Fitzhardinge

author

2008-01-31 01:44 UTC #4 lore raw

Quoted 25 lines · Marcelo Tosatti

Quoted 9 lines · Jeremy Fitzhardinge
Quoted 4 lines · Marcelo Tosatti
Forgot to copy you... Ideally all pte updates should be done via the paravirt interface.
Hm, are you sure?
Quoted 13 lines
Review
Quoted 8 lines
(quoted diff) +6 -0
+static inline void pte_clear_bit(unsigned int bit, pte_t *ptep)
+{
+	pte_t pte = *ptep;
+	clear_bit(bit, (unsigned long *)&pte.pte);
+	set_pte(ptep, pte);
+}
This is not a safe transformation. This will lose hardware A/D bit changes if the pte is part of an active pagetable. Aside from that, clear_bit is atomic and so is the wrong thing to use on a local variable; a plain bitmask would do the job.

Quoted 2 lines

True this is racy on baremetal.

And under Xen (since the pagetable is the real hardware pagetable).

Quoted 10 lines

Quoted 6 lines
clear_bit on a PTE is fine, since everyone has to support some level of trap'n'emulate on pte-level entries (ptep_get_and_clear is very hard to implement otherwise). Shadow pagetable implementations will do this naturally, and Xen must do it to allow atomic updates on ptes (also because it makes late-pinning/early-unpinning a performance win).
Sure clear_bit on a PTE is fine, but we wan't to virtualize for batching pte updates via hypercall.

Yes, mprotect needs batching, but that probably needs special handling.

Quoted 31 lines

Quoted 13 lines
Quoted 13 lines
+
static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
pte_t *ptep, pte_t pte)
{
Index: linux-2.6-x86-kvm/include/asm-x86/pgtable.h
===================================================================
include/asm-x86/pgtable.h +0 -0
@@ -227,6 +227,8 @@ void native_pagetable_setup_done(pgd_t *
 #define pte_update(mm, addr, ptep)              do { } while (0)
 #define pte_update_defer(mm, addr, ptep)        do { } while (0)
 
Quoted 17 lines
Quoted 7 lines
include/asm-x86/pgtable.h +2 -0
+#define pte_clear_bit(bit, ptep)	clear_bit(bit, (unsigned long 
 *)&ptep->pte)
+
 static inline void paravirt_pagetable_setup_start(pgd_t *base)
 {
	native_pagetable_setup_start(base);
include/asm-x86/pgtable.h static inline void native_set_pte_at(str · line 309
Quoted 7 lines
include/asm-x86/pgtable.h +1 -1
@@ -302,7 +304,7 @@ static inline void native_set_pte_at(str
 ({									\
 	int __changed = !pte_same(*(ptep), entry);			\
 	if (__changed && dirty) {					\
-		*ptep = entry;						\
+		set_pte(ptep, entry);					\
 
 
This change is OK, but doesn't really do anything. All hypervisors allow direct access to ptes.

Quoted 2 lines

We want the pte update to go through a hypercall instead of trap'n'emulate.

ptep_set_access_flags is only used in fault handling, so there's no scope for batching.

Quoted 17 lines

Quoted 16 lines
include/asm-x86/pgtable.h
Quoted 13 lines
include/asm-x86/pgtable.h +0 -0
 		pte_update_defer((vma)->vm_mm, (address), (ptep));	\
		flush_tlb_page(vma, address);				\
	}								\
include/asm-x86/pgtable.h +0 -0
@@ -357,7 +359,7 @@ static inline pte_t ptep_get_and_clear_f
 #define __HAVE_ARCH_PTEP_SET_WRPROTECT
static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
{
include/asm-x86/pgtable.h +1 -1
-	clear_bit(_PAGE_BIT_RW, (unsigned long *)&ptep->pte);
+	pte_clear_bit(_PAGE_BIT_RW, ptep);
This is only valid if ptep_set_wrprotect is never used on active ptes, which I don't think is the case.

Quoted 5 lines

Ok, pte_clear_bit() like above is broken. But the point is that we want to batch the write protection of the source pte along with the creation of the write protected destination pte on COW using the paravirt_enter_lazy/paravirt_leave_lazy.

Which "we" are we talking about here? In Xen, the destination pte is unpinned during a fork(), so a plain memory write will work without any hypervisor interaction. That's what I mean about the "late pin/early unpin" optimisation. Almost all fork/exit pagetable manipulation happens on unpinned pagetables which are plain RW pages. It's only updates on pinned pagetables which need hypervisor interaction. The only significant batchable update is mprotect.

I don't know what the issues for KVM/VMI are.

Avi Kivity

2008-01-31 07:18 UTC #5 lore raw

In reply to a message not in the archive

Quoted 10 lines · Jeremy Fitzhardinge

Quoted 4 lines · Marcelo Tosatti
Forgot to copy you... Ideally all pte updates should be done via the paravirt interface.
Hm, are you sure?

It has the advantage of not falsely triggering any unshadowing heuristics, and of avoiding the lovely x86 emulator. Not sure how big that advantage is, though.

Signature · 7 lines

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

A message that is not in the archive (sent off-list or not yet received): 47A17637.4050800-atKUWr5tajBWk0Htik3J/[email protected]

Jeremy Fitzhardinge

author

2008-01-31 18:28 UTC #7 lore raw

Quoted 3 lines · Avi Kivity

It has the advantage of not falsely triggering any unshadowing heuristics, and of avoiding the lovely x86 emulator. Not sure how big that advantage is, though.

Fair enough. But doing partial pte updates needs an update-with-mask hypercall (or something) to prevent the hardware race.