I386: Deactivate the test for the dead CONFIG_DEBUG_PAGE_TYPE variable.

PATCH In review Robert P. J. Day 5 participants 10 messages Jul 6, 2007 virtio core

lore

Share & tools

lore

Robert P. J. Day

author

2007-07-06 12:51 UTC #0 lore raw

PATCH 1/1 I386: Deactivate the test for the dead CONFIG_DEBUG_PAGE_TYPE variable.

Signed-off-by Robert P. J. Day <[email protected]>

---

arch/i386/kernel/vmi.c +2 -2

index c12720d..e3ce5c8 100644
@@ -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)

Signature · 7 lines

========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

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

Stefan Richter

2007-07-06 18:25 UTC #1 lore raw

arch/i386/kernel/vmi.c

Quoted 5 lines · Robert P. J. Day

Signed-off-by Robert P. J. Day <[email protected]>
---
arch/i386/kernel/vmi.c +0 -0

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

Quoted 13 lines

index c12720d..e3ce5c8 100644

arch/i386/kernel/vmi.c +1 -1

@@ -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)

arch/i386/kernel/vmi.c static void vmi_check_page_type(u32 pfn, int type) · line 342

Quoted 8 lines

arch/i386/kernel/vmi.c +1 -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.

Signature · 3 lines

Stefan Richter
-=====-=-=== -=== --==-
http://arcgraph.de/sr/

Zachary Amsden

2007-07-06 19:54 UTC #2 lore raw

Quoted 32 lines · Stefan Richter

arch/i386/kernel/vmi.c
Quoted 6 lines · Robert P. J. Day
Signed-off-by Robert P. J. Day <[email protected]>
---
arch/i386/kernel/vmi.c +0 -0
Maintainers are apparently those under "PARAVIRT_OPS INTERFACE". CCs added.
Quoted 19 lines
index c12720d..e3ce5c8 100644
arch/i386/kernel/vmi.c +2 -2
@@ -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 */
 

Quoted 10 lines

arch/i386/kernel/vmi.c static void vmi_check_page_type(u32 pfn, int type) · line 342
Quoted 3 lines
arch/i386/kernel/vmi.c +0 -0
 #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

Chris Wright

2007-07-06 20:16 UTC #3 lore raw

PATCH 1/1 VMI: remove CONFIG_DEBUG_PAGE_TYPE and associated bitrotted code

In reply to Stefan Richter

Quoted 2 lines · * Stefan Richter ([email protected]) wrote:

Quoted 2 lines
(quoted diff) +1 -1
-#ifdef CONFIG_DEBUG_PAGE_TYPE
+#if 0 /* debug page type */

<snip>

Quoted 5 lines

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 <[email protected]>

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" <[email protected]>

Cc Stefan Richter <[email protected]>

Cc Zachary Amsden <[email protected]>

Signed-off-by Chris Wright <[email protected]>

---

arch/i386/kernel/vmi.c +0 ~~-119~~

index c12720d..45c55e4 100644
@@ -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

include/asm-i386/vmi.h +0 -1

index eb8bd89..70d7d6f 100644
@@ -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 {

Zachary Amsden

2007-07-06 20:13 UTC #4 lore raw

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

Quoted 23 lines · Chris Wright

Quoted 3 lines · * Stefan Richter ([email protected]) wrote:
Quoted 3 lines
(quoted diff) +1 -1
-#ifdef CONFIG_DEBUG_PAGE_TYPE
+#if 0 /* debug page type */
<snip>
Quoted 6 lines
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.

Quoted 3 lines

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

Jeremy Fitzhardinge

2007-07-06 20:28 UTC #5 lore raw

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

Quoted 9 lines · Zachary Amsden

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?

Zachary Amsden

2007-07-06 20:34 UTC #6 lore raw

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

Quoted 4 lines · Jeremy Fitzhardinge

Quoted 2 lines
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

Jeremy Fitzhardinge

2007-07-06 21:01 UTC #7 lore raw

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

Quoted 1 line · Zachary Amsden

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?

Quoted 1 line

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.)

Quoted 7 lines

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.

Zachary Amsden

2007-07-06 21:17 UTC #8 lore raw

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

Quoted 6 lines · Jeremy Fitzhardinge

Quoted 1 line · Zachary Amsden
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.

Quoted 3 lines

Quoted 1 line
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).

Quoted 1 line

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.

Quoted 1 line

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

Chris Wright

2007-07-06 20:40 UTC #9 lore raw

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

In reply to Jeremy Fitzhardinge

Quoted 14 lines · * Jeremy Fitzhardinge ([email protected]) wrote:

Quoted 13 lines · Zachary Amsden
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.

Quoted 6 lines

Quoted 2 lines
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