[PATCH] I386: Deactivate the test for the dead CONFIG_DEBUG_PAGE_TYPE variable.

[PATCH] I386: Deactivate the test for the dead CONFIG_DEBUG_PAGE_TYPE variable.

[Robert P. J. Day]

Signed-off-by: Robert P. J. Day <rpjday@mindspring.com>

---

diff --git a/arch/i386/kernel/vmi.c b/arch/i386/kernel/vmi.c
index c12720d..e3ce5c8 100644
--- a/arch/i386/kernel/vmi.c
+++ b/arch/i386/kernel/vmi.c
@@ -235,7 +235,7 @@ static void vmi_nop(void)
 {
 }

-#ifdef CONFIG_DEBUG_PAGE_TYPE
+#if 0 /* debug page type */

 #ifdef CONFIG_X86_PAE
 #define MAX_BOOT_PTS (2048+4+1)
@@ -336,7 +336,7 @@ static void vmi_check_page_type(u32 pfn, int type)
 #else
 #define vmi_set_page_type(p,t) do { } while (0)
 #define vmi_check_page_type(p,t) do { } while (0)
-#endif
+#endif /* debug page type */

 #ifdef CONFIG_HIGHPTE
 static void *vmi_kmap_atomic_pte(struct page *page, enum km_type type)
-- 
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================

Re: [PATCH] I386: Deactivate the test for the dead CONFIG_DEBUG_PAGE_TYPE variable.

[Stefan Richter]

Robert P. J. Day wrote:
> Signed-off-by: Robert P. J. Day <rpjday@mindspring.com>
> 
> ---
> 
> diff --git a/arch/i386/kernel/vmi.c b/arch/i386/kernel/vmi.c

Maintainers are apparently those under "PARAVIRT_OPS INTERFACE".
CCs added.

> index c12720d..e3ce5c8 100644
> --- a/arch/i386/kernel/vmi.c
> +++ b/arch/i386/kernel/vmi.c
> @@ -235,7 +235,7 @@ static void vmi_nop(void)
>  {
>  }
> 
> -#ifdef CONFIG_DEBUG_PAGE_TYPE
> +#if 0 /* debug page type */
> 
>  #ifdef CONFIG_X86_PAE
>  #define MAX_BOOT_PTS (2048+4+1)
> @@ -336,7 +336,7 @@ static void vmi_check_page_type(u32 pfn, int type)
>  #else
>  #define vmi_set_page_type(p,t) do { } while (0)
>  #define vmi_check_page_type(p,t) do { } while (0)
> -#endif
> +#endif /* debug page type */
> 
>  #ifdef CONFIG_HIGHPTE
>  static void *vmi_kmap_atomic_pte(struct page *page, enum km_type type)

This misnamed CONFIG_DEBUG_PAGE_TYPE (it's not a Kconfig variable) has
about 120 lines debug code dangling on it.  So, replacing it by #if 0
will hopefully motivate a kind janitor to send a removal patch for that
debug code eventually.  I don't do so just now because that code went in
between 2.6.20 and 2.6.21-rc1, i.e. not so long ago.
-- 
Stefan Richter
-=====-=-=== -=== --==-
http://arcgraph.de/sr/

Re: [PATCH] I386: Deactivate the test for the dead CONFIG_DEBUG_PAGE_TYPE variable.

[Zachary Amsden]

Stefan Richter wrote:
> Robert P. J. Day wrote:
>   
>> Signed-off-by: Robert P. J. Day <rpjday@mindspring.com>
>>
>> ---
>>
>> diff --git a/arch/i386/kernel/vmi.c b/arch/i386/kernel/vmi.c
>>     
>
> Maintainers are apparently those under "PARAVIRT_OPS INTERFACE".
> CCs added.
>
>   
>> index c12720d..e3ce5c8 100644
>> --- a/arch/i386/kernel/vmi.c
>> +++ b/arch/i386/kernel/vmi.c
>> @@ -235,7 +235,7 @@ static void vmi_nop(void)
>>  {
>>  }
>>
>> -#ifdef CONFIG_DEBUG_PAGE_TYPE
>> +#if 0 /* debug page type */
>>
>>  #ifdef CONFIG_X86_PAE
>>  #define MAX_BOOT_PTS (2048+4+1)
>> @@ -336,7 +336,7 @@ static void vmi_check_page_type(u32 pfn, int type)
>>  #else
>>  #define vmi_set_page_type(p,t) do { } while (0)
>>  #define vmi_check_page_type(p,t) do { } while (0)
>> -#endif
>> +#endif /* debug page type */
>>
>>  #ifdef CONFIG_HIGHPTE
>>  static void *vmi_kmap_atomic_pte(struct page *page, enum km_type type)
>>     
>
> This misnamed CONFIG_DEBUG_PAGE_TYPE (it's not a Kconfig variable) has
> about 120 lines debug code dangling on it.  So, replacing it by #if 0
> will hopefully motivate a kind janitor to send a removal patch for that
> debug code eventually.  I don't do so just now because that code went in
> between 2.6.20 and 2.6.21-rc1, i.e. not so long ago.
>   

Nack.  This code was very tricky to get right and found some very 
obscure bugs.  I want to keep it around even in broken form for as long 
as possible.

Also, it is not misnamed.  I didn't add the rest of the code upstream 
because it steals bits from struct page.

Zach

[PATCH] VMI: remove CONFIG_DEBUG_PAGE_TYPE and associated bitrotted code

[Chris Wright]

* Stefan Richter (stefanr@s5r6.in-berlin.de) wrote:
> > -#ifdef CONFIG_DEBUG_PAGE_TYPE
> > +#if 0 /* debug page type */
<snip>
> This misnamed CONFIG_DEBUG_PAGE_TYPE (it's not a Kconfig variable) has
> about 120 lines debug code dangling on it.  So, replacing it by #if 0
> will hopefully motivate a kind janitor to send a removal patch for that
> debug code eventually.  I don't do so just now because that code went in
> between 2.6.20 and 2.6.21-rc1, i.e. not so long ago.

This is Zach's code, his final call.  I know it was pretty useful early
on, and used to be an actual Kconfig option for VMI.  However, it's
completely disconnected; the setup call to vmi_apply_boot_page_allocations
isn't merged and the page->type field isn't either (no surprise on that),
and some of the VMI_PAGE_ constants have changed names.  Clearly, it is
ripe for bitrot (already has !CONFIG_NEED_MULTIPLE_NODES dependency,
dunno if VMI has the same limitation).  It definitely should not have
a misleading Kconfig name.  I'd nuke it all rather than #if 0.

thanks,
-chris
--

Subject: [PATCH] VMI: remove CONFIG_DEBUG_PAGE_TYPE and associated bitrotted code

From: Chris Wright <chrisw@sous-sol.org>

Remove the poorly named (non Kconfig) CONFIG_DEBUG_PAGE_TYPE compile
time conditional as well as the associated bitrotted debug code.

Cc: "Robert P. J. Day" <rpjday@mindspring.com>
Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: Zachary Amsden <zach@vmware.com>
Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---

diff --git a/arch/i386/kernel/vmi.c b/arch/i386/kernel/vmi.c
index c12720d..45c55e4 100644
--- a/arch/i386/kernel/vmi.c
+++ b/arch/i386/kernel/vmi.c
@@ -235,109 +235,6 @@ static void vmi_nop(void)
 {
 }
 
-#ifdef CONFIG_DEBUG_PAGE_TYPE
-
-#ifdef CONFIG_X86_PAE
-#define MAX_BOOT_PTS (2048+4+1)
-#else
-#define MAX_BOOT_PTS (1024+1)
-#endif
-
-/*
- * During boot, mem_map is not yet available in paging_init, so stash
- * all the boot page allocations here.
- */
-static struct {
-	u32 pfn;
-	int type;
-} boot_page_allocations[MAX_BOOT_PTS];
-static int num_boot_page_allocations;
-static int boot_allocations_applied;
-
-void vmi_apply_boot_page_allocations(void)
-{
-	int i;
-	BUG_ON(!mem_map);
-	for (i = 0; i < num_boot_page_allocations; i++) {
-		struct page *page = pfn_to_page(boot_page_allocations[i].pfn);
-		page->type = boot_page_allocations[i].type;
-		page->type = boot_page_allocations[i].type &
-				~(VMI_PAGE_ZEROED | VMI_PAGE_CLONE);
-	}
-	boot_allocations_applied = 1;
-}
-
-static void record_page_type(u32 pfn, int type)
-{
-	BUG_ON(num_boot_page_allocations >= MAX_BOOT_PTS);
-	boot_page_allocations[num_boot_page_allocations].pfn = pfn;
-	boot_page_allocations[num_boot_page_allocations].type = type;
-	num_boot_page_allocations++;
-}
-
-static void check_zeroed_page(u32 pfn, int type, struct page *page)
-{
-	u32 *ptr;
-	int i;
-	int limit = PAGE_SIZE / sizeof(int);
-
-	if (page_address(page))
-		ptr = (u32 *)page_address(page);
-	else
-		ptr = (u32 *)__va(pfn << PAGE_SHIFT);
-	/*
-	 * When cloning the root in non-PAE mode, only the userspace
-	 * pdes need to be zeroed.
-	 */
-	if (type & VMI_PAGE_CLONE)
-		limit = USER_PTRS_PER_PGD;
-	for (i = 0; i < limit; i++)
-		BUG_ON(ptr[i]);
-}
-
-/*
- * We stash the page type into struct page so we can verify the page
- * types are used properly.
- */
-static void vmi_set_page_type(u32 pfn, int type)
-{
-	/* PAE can have multiple roots per page - don't track */
-	if (PTRS_PER_PMD > 1 && (type & VMI_PAGE_PDP))
-		return;
-
-	if (boot_allocations_applied) {
-		struct page *page = pfn_to_page(pfn);
-		if (type != VMI_PAGE_NORMAL)
-			BUG_ON(page->type);
-		else
-			BUG_ON(page->type == VMI_PAGE_NORMAL);
-		page->type = type & ~(VMI_PAGE_ZEROED | VMI_PAGE_CLONE);
-		if (type & VMI_PAGE_ZEROED)
-			check_zeroed_page(pfn, type, page);
-	} else {
-		record_page_type(pfn, type);
-	}
-}
-
-static void vmi_check_page_type(u32 pfn, int type)
-{
-	/* PAE can have multiple roots per page - skip checks */
-	if (PTRS_PER_PMD > 1 && (type & VMI_PAGE_PDP))
-		return;
-
-	type &= ~(VMI_PAGE_ZEROED | VMI_PAGE_CLONE);
-	if (boot_allocations_applied) {
-		struct page *page = pfn_to_page(pfn);
-		BUG_ON((page->type ^ type) & VMI_PAGE_PAE);
-		BUG_ON(type == VMI_PAGE_NORMAL && page->type);
-		BUG_ON((type & page->type) == 0);
-	}
-}
-#else
-#define vmi_set_page_type(p,t) do { } while (0)
-#define vmi_check_page_type(p,t) do { } while (0)
-#endif
-
 #ifdef CONFIG_HIGHPTE
 static void *vmi_kmap_atomic_pte(struct page *page, enum km_type type)
 {
@@ -364,7 +261,6 @@ static void *vmi_kmap_atomic_pte(struct page *page, enum km_type type)
 
 static void vmi_allocate_pt(u32 pfn)
 {
-	vmi_set_page_type(pfn, VMI_PAGE_L1);
 	vmi_ops.allocate_page(pfn, VMI_PAGE_L1, 0, 0, 0);
 }
 
@@ -375,27 +271,22 @@ static void vmi_allocate_pd(u32 pfn)
 	 * It is called only for swapper_pg_dir, which already has
 	 * data on it.
 	 */
- 	vmi_set_page_type(pfn, VMI_PAGE_L2);
 	vmi_ops.allocate_page(pfn, VMI_PAGE_L2, 0, 0, 0);
 }
 
 static void vmi_allocate_pd_clone(u32 pfn, u32 clonepfn, u32 start, u32 count)
 {
- 	vmi_set_page_type(pfn, VMI_PAGE_L2 | VMI_PAGE_CLONE);
-	vmi_check_page_type(clonepfn, VMI_PAGE_L2);
 	vmi_ops.allocate_page(pfn, VMI_PAGE_L2 | VMI_PAGE_CLONE, clonepfn, start, count);
 }
 
 static void vmi_release_pt(u32 pfn)
 {
 	vmi_ops.release_page(pfn, VMI_PAGE_L1);
-	vmi_set_page_type(pfn, VMI_PAGE_NORMAL);
 }
 
 static void vmi_release_pd(u32 pfn)
 {
 	vmi_ops.release_page(pfn, VMI_PAGE_L2);
-	vmi_set_page_type(pfn, VMI_PAGE_NORMAL);
 }
 
 /*
@@ -419,26 +310,22 @@ static void vmi_release_pd(u32 pfn)
 
 static void vmi_update_pte(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
 {
-	vmi_check_page_type(__pa(ptep) >> PAGE_SHIFT, VMI_PAGE_PTE);
 	vmi_ops.update_pte(ptep, vmi_flags_addr(mm, addr, VMI_PAGE_PT, 0));
 }
 
 static void vmi_update_pte_defer(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
 {
-	vmi_check_page_type(__pa(ptep) >> PAGE_SHIFT, VMI_PAGE_PTE);
 	vmi_ops.update_pte(ptep, vmi_flags_addr_defer(mm, addr, VMI_PAGE_PT, 0));
 }
 
 static void vmi_set_pte(pte_t *ptep, pte_t pte)
 {
 	/* XXX because of set_pmd_pte, this can be called on PT or PD layers */
-	vmi_check_page_type(__pa(ptep) >> PAGE_SHIFT, VMI_PAGE_PTE | VMI_PAGE_PD);
 	vmi_ops.set_pte(pte, ptep, VMI_PAGE_PT);
 }
 
 static void vmi_set_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep, pte_t pte)
 {
-	vmi_check_page_type(__pa(ptep) >> PAGE_SHIFT, VMI_PAGE_PTE);
 	vmi_ops.set_pte(pte, ptep, vmi_flags_addr(mm, addr, VMI_PAGE_PT, 0));
 }
 
@@ -446,10 +333,8 @@ static void vmi_set_pmd(pmd_t *pmdp, pmd_t pmdval)
 {
 #ifdef CONFIG_X86_PAE
 	const pte_t pte = { pmdval.pmd, pmdval.pmd >> 32 };
-	vmi_check_page_type(__pa(pmdp) >> PAGE_SHIFT, VMI_PAGE_PMD);
 #else
 	const pte_t pte = { pmdval.pud.pgd.pgd };
-	vmi_check_page_type(__pa(pmdp) >> PAGE_SHIFT, VMI_PAGE_PGD);
 #endif
 	vmi_ops.set_pte(pte, (pte_t *)pmdp, VMI_PAGE_PD);
 }
@@ -471,7 +356,6 @@ static void vmi_set_pte_atomic(pte_t *ptep, pte_t pteval)
 
 static void vmi_set_pte_present(struct mm_struct *mm, unsigned long addr, pte_t *ptep, pte_t pte)
 {
-	vmi_check_page_type(__pa(ptep) >> PAGE_SHIFT, VMI_PAGE_PTE);
 	vmi_ops.set_pte(pte, ptep, vmi_flags_addr_defer(mm, addr, VMI_PAGE_PT, 1));
 }
 
@@ -479,21 +363,18 @@ static void vmi_set_pud(pud_t *pudp, pud_t pudval)
 {
 	/* Um, eww */
 	const pte_t pte = { pudval.pgd.pgd, pudval.pgd.pgd >> 32 };
-	vmi_check_page_type(__pa(pudp) >> PAGE_SHIFT, VMI_PAGE_PGD);
 	vmi_ops.set_pte(pte, (pte_t *)pudp, VMI_PAGE_PDP);
 }
 
 static void vmi_pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
 {
 	const pte_t pte = { 0 };
-	vmi_check_page_type(__pa(ptep) >> PAGE_SHIFT, VMI_PAGE_PTE);
 	vmi_ops.set_pte(pte, ptep, vmi_flags_addr(mm, addr, VMI_PAGE_PT, 0));
 }
 
 static void vmi_pmd_clear(pmd_t *pmd)
 {
 	const pte_t pte = { 0 };
-	vmi_check_page_type(__pa(pmd) >> PAGE_SHIFT, VMI_PAGE_PMD);
 	vmi_ops.set_pte(pte, (pte_t *)pmd, VMI_PAGE_PD);
 }
 #endif
diff --git a/include/asm-i386/vmi.h b/include/asm-i386/vmi.h
index eb8bd89..70d7d6f 100644
--- a/include/asm-i386/vmi.h
+++ b/include/asm-i386/vmi.h
@@ -225,7 +225,6 @@ struct pci_header {
 /* Function prototypes for bootstrapping */
 extern void vmi_init(void);
 extern void vmi_bringup(void);
-extern void vmi_apply_boot_page_allocations(void);
 
 /* State needed to start an application processor in an SMP system. */
 struct vmi_ap_state {

Re: [PATCH] VMI: remove CONFIG_DEBUG_PAGE_TYPE and associated bitrotted code

[Zachary Amsden]

Chris Wright wrote:
> * Stefan Richter (stefanr@s5r6.in-berlin.de) wrote:
>   
>>> -#ifdef CONFIG_DEBUG_PAGE_TYPE
>>> +#if 0 /* debug page type */
>>>       
> <snip>
>   
>> This misnamed CONFIG_DEBUG_PAGE_TYPE (it's not a Kconfig variable) has
>> about 120 lines debug code dangling on it.  So, replacing it by #if 0
>> will hopefully motivate a kind janitor to send a removal patch for that
>> debug code eventually.  I don't do so just now because that code went in
>> between 2.6.20 and 2.6.21-rc1, i.e. not so long ago.
>>     
>
> This is Zach's code, his final call.  I know it was pretty useful early
> on, and used to be an actual Kconfig option for VMI.  However, it's
> completely disconnected; the setup call to vmi_apply_boot_page_allocations
> isn't merged and the page->type field isn't either (no surprise on that),
> and some of the VMI_PAGE_ constants have changed names.  Clearly, it is
> ripe for bitrot (already has !CONFIG_NEED_MULTIPLE_NODES dependency,
> dunno if VMI has the same limitation).  It definitely should not have
> a misleading Kconfig name.  I'd nuke it all rather than #if 0.
>
> thanks,
> -chris
>   

I'd rather keep it, even with bitrot - it was non-trivial to get 
correct, and found many surprises in the code; most notably, it can detect

1) PTE writes to pages not declared as page tables
2) Failure to allocate or de-allocate page tables using the paravirt-ops API
3) PTE writes using the wrong level operations

These are most useful properties; in fact, I would like to extend the 
code for 64-bit paravirt-ops and 4-level paging, so rather not kill it 
until then.

I never merged the whole bit upstream because it added a field to struct 
page.

Zach

Re: [PATCH] VMI: remove CONFIG_DEBUG_PAGE_TYPE and associated bitrotted code

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> I'd rather keep it, even with bitrot - it was non-trivial to get 
> correct, and found many surprises in the code; most notably, it can 
> detect
>
> 1) PTE writes to pages not declared as page tables
> 2) Failure to allocate or de-allocate page tables using the 
> paravirt-ops API
> 3) PTE writes using the wrong level operations
>
> These are most useful properties; in fact, I would like to extend the 
> code for 64-bit paravirt-ops and 4-level paging, so rather not kill it 
> until then.
>
> I never merged the whole bit upstream because it added a field to 
> struct page. 

Hm, is that a big problem?  It would be OK for a debug config option, 
wouldn't it?  Also, it doesn't seem particularly vmi-specific.  Could it 
be made part of the pvops infrastructure?

    J

Re: [PATCH] VMI: remove CONFIG_DEBUG_PAGE_TYPE and associated bitrotted code

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
>>
>> I never merged the whole bit upstream because it added a field to 
>> struct page. 
>
> Hm, is that a big problem?  It would be OK for a debug config option, 
> wouldn't it?  Also, it doesn't seem particularly vmi-specific.  Could 
> it be made part of the pvops infrastructure?

I though about it, but it gets really ugly.  You need wrappers for all 
the MMU ops in pvops generic code, which means either another layer of 
wrappers or a bunch of CONFIG_DEBUG_PARAVIRT only things that are easy 
to break because they also depend on PAE vs. non-PAE.

It's doable, though, and might even be extensible to s390 for CMM page 
type debugging, as well as descriptor type tracking and enforcement of 
page isolation of GDTs.

Page state tracking could track -

PAGE_ZERO, PAGE_UNUSED, PAGE_STABLE, PAGE_VOLATILE, 
PAGE_POTENTIALLY_VOLATILE, PAGE_L1{2/3/4}, PAGE_LDT, PAGE_GDT,

actually, no this seems silly, since we'd just be duplicating bits for 
the page types, so the only debug benefit is ensuring the intersection 
of volatile and L{1/2/3/4} is nil, which is already trivially verifiable 
by inspection.

Zach

Re: [PATCH] VMI: remove CONFIG_DEBUG_PAGE_TYPE and associated bitrotted code

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> I though about it, but it gets really ugly.  You need wrappers for all 
> the MMU ops in pvops generic code, which means either another layer of 
> wrappers or a bunch of CONFIG_DEBUG_PARAVIRT

Oh, yes, more wrappers please!  We could do it at the paravirt_ops 
level: set up your pv_ops, then call paravirt_debug_mmuops(), which 
would save away your ops and replace them with wrappers.  That basic 
structure would lend itself to all kinds of paravirt-level debugging tools.

It would be a bit more elegant if we had mmu_ops.  Maybe we should do 
that splitup before 64bit?

> only things that are easy to break because they also depend on PAE vs. 
> non-PAE.

Hm, would they?  Would they need to inspect the content of the pte_t 
(etc), or just look at the struct page for the page being updated?  (pmd 
operations being a bit more awkward, of course.)

> It's doable, though, and might even be extensible to s390 for CMM page 
> type debugging, as well as descriptor type tracking and enforcement of 
> page isolation of GDTs.
>
> Page state tracking could track -
>
> PAGE_ZERO, PAGE_UNUSED, PAGE_STABLE, PAGE_VOLATILE, 
> PAGE_POTENTIALLY_VOLATILE, PAGE_L1{2/3/4}, PAGE_LDT, PAGE_GDT,
>
> actually, no this seems silly, since we'd just be duplicating bits for 
> the page types, so the only debug benefit is ensuring the intersection 
> of volatile and L{1/2/3/4} is nil, which is already trivially 
> verifiable by inspection. 

Well, I have to say that keeping the hypervisor hints in sync with the 
actual kernel-level page state worries me, so any debug tools which 
could help there would be good.  This seems like it should be the right 
place to do it, but I can't say I've thought about it in any detail.

Of course, if it *is* helpful to the page hinting patches, then it 
suggests that it's a facility with wider scope than pv-ops.

    J

Re: [PATCH] VMI: remove CONFIG_DEBUG_PAGE_TYPE and associated bitrotted code

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
> Zachary Amsden wrote:
>> I though about it, but it gets really ugly.  You need wrappers for 
>> all the MMU ops in pvops generic code, which means either another 
>> layer of wrappers or a bunch of CONFIG_DEBUG_PARAVIRT
>
> Oh, yes, more wrappers please!  We could do it at the paravirt_ops 
> level: set up your pv_ops, then call paravirt_debug_mmuops(), which 
> would save away your ops and replace them with wrappers.  That basic 
> structure would lend itself to all kinds of paravirt-level debugging 
> tools.
>
> It would be a bit more elegant if we had mmu_ops.  Maybe we should do 
> that splitup before 64bit?

zamsden strongly agrees jsgf.

>> only things that are easy to break because they also depend on PAE 
>> vs. non-PAE.
>
> Hm, would they?  Would they need to inspect the content of the pte_t 
> (etc), or just look at the struct page for the page being updated?  
> (pmd operations being a bit more awkward, of course.)

No, but there are different sets of operations and different sets of 
validation rules (for example, multiple PGDs per page for PAE, depending 
on pgd allocation size).

> Well, I have to say that keeping the hypervisor hints in sync with the 
> actual kernel-level page state worries me, so any debug tools which 
> could help there would be good.  This seems like it should be the 
> right place to do it, but I can't say I've thought about it in any 
> detail.

Yes, these things are subtle and easy to break.  In fact, still broken 
(although in a different way) - check out this naked write to a pte:

arch/i386/mm/init.c:            pte->pte_high &= ~(1 << (_PAGE_BIT_NX - 
32));
arch/i386/mm/init.c:            pte->pte_high |= 1 << (_PAGE_BIT_NX - 32);

Not only has page typing created a dependence on the guest accurately 
representing page state to the hypervisor, it also requires the guest to 
use proper APIs for access to those pages.  That is much harder to 
enforce - perhaps static checking can get some of the way there.

The page type debugging is certainly a start in a good direction.

> Of course, if it *is* helpful to the page hinting patches, then it 
> suggests that it's a facility with wider scope than pv-ops.

I certainly think it's not wide enough to justify a new struct page 
field, but perhaps a debug-only field is commonly useful; those pg bits 
are getting quite crammed.

Zach

Re: [PATCH] VMI: remove CONFIG_DEBUG_PAGE_TYPE and associated bitrotted code

[Chris Wright]

* Jeremy Fitzhardinge (jeremy@goop.org) wrote:
> Zachary Amsden wrote:
> >I'd rather keep it, even with bitrot - it was non-trivial to get 
> >correct, and found many surprises in the code; most notably, it can 
> >detect
> >
> >1) PTE writes to pages not declared as page tables
> >2) Failure to allocate or de-allocate page tables using the 
> >paravirt-ops API
> >3) PTE writes using the wrong level operations
> >
> >These are most useful properties; in fact, I would like to extend the 
> >code for 64-bit paravirt-ops and 4-level paging, so rather not kill it 
> >until then.
> >
> >I never merged the whole bit upstream because it added a field to 
> >struct page. 
> 
> Hm, is that a big problem?  It would be OK for a debug config option, 
> wouldn't it?  Also, it doesn't seem particularly vmi-specific.  Could it 
> be made part of the pvops infrastructure?

I'm pretty sure lguest64 hit some of the problems Zach is trying to
catch, so should generalize well-enough.

thanks,
-chris