Re: [PARAVIRT/x86] BUGFIX: Put a missing paravirt_release_pmd in pgd_dtor

[Alok Kataria <akataria@vmware.com>]

On Thu, 2009-02-05 at 18:30 -0800, Jeremy Fitzhardinge wrote: 
> >  
> > +	if (PAGETABLE_LEVELS == 2 ||
> > +            (PAGETABLE_LEVELS == 3 && SHARED_KERNEL_PMD) ||
> > +            PAGETABLE_LEVELS == 4) {
> > +		paravirt_release_pmd(__pa(pgd) >> PAGE_SHIFT);
> > +	}
> >   
> 
> Ah, you want release_pmd to be called on pgds as well...  Why?  Do you 
> track the page type for pgds? 

VMI didn't/still doesn't differentiate between a *release* of pgd or
pmd. 
>  Why not just make a copy of the entries 
> on cr3 reload like the real hardware does?
> 
> Alternatively you could hook pv_mmu_ops.exit_mmap to get a call when the 
> last reference to the pagetable has been dropped.

> 
> Or, if you really must, introduce paravirt_release_pgd and hook that.

As it affects only VMI, instead of adding another callback, i have
hooked on the paravirt_pgd_free call for vmi to release the pgd page.
Below is the patch. I will run some overnight tests with this patch and
get back if there are any errors.

> 
> But either way, calling release_pmd here is wrong, since its only meant 
> to be applied to pmds,
Maybe i misunderstand, but that's how it used to work before that
commit, we had a single call to release_*pd*, no ?
>  and it would break the Xen code.

i see xen doesn't define the alloc_pmd_clone call. 

Thanks,
Alok

-- 
[VMI/x86] Release the pgd page in pgd_dtor.

From: Alok N Kataria <akataria@vmware.com>

The commit...
-----------------------------
commit 6194ba6ff6ccf8d5c54c857600843c67aa82c407
Author: Jeremy Fitzhardinge <jeremy@goop.org>
Date:   Wed Jan 30 13:34:11 2008 +0100

    x86: don't special-case pmd allocations as much
------------------------------
...made changes to the way we handle pmd allocations, and while doing that
it dropped a call to  paravirt_release_pd on the pgd page from the pgd_dtor
code path.

As a result of this missing release, the hypervisor is now unaware of the
pgd page being freed, and as a result it ends up tracking this page as a
page table page.
After this the guest may start using the same page for other purposes, and
depending on what use the page is put to, it may result in various performance
and/or functional issues ( hangs, reboots).

Since this bug affects only VMI, instead of adding another pgd_release hook,
I now release the pgd page in the (vmi)_pgd_free hook.

Patch on top of 2.6.29-rc3 (mainline head).

Signed-off-by: Alok N Kataria <akataria@vmware.com>
Cc: Rohit Jain <rjain@vmware.com>
Cc: Zachary Amsden <zach@vmware.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: stable@kernel.org
---

 arch/x86/kernel/vmi_32.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)


diff --git a/arch/x86/kernel/vmi_32.c b/arch/x86/kernel/vmi_32.c
index 1d3302c..dfed18f 100644
--- a/arch/x86/kernel/vmi_32.c
+++ b/arch/x86/kernel/vmi_32.c
@@ -321,6 +321,16 @@ static void vmi_release_pmd(unsigned long pfn)
 }
 
 /*
+ * We hack the pgd_free hook for releasing the pgd page.
+ */
+static void vmi_pgd_free(struct mm_struct *mm, pgd_t *pgd)
+{
+	unsigned long pfn = __pa(pgd) >> PAGE_SHIFT;
+
+	vmi_ops.release_page(pfn, VMI_PAGE_L2);
+}
+
+/*
  * Helper macros for MMU update flags.  We can defer updates until a flush
  * or page invalidation only if the update is to the current address space
  * (otherwise, there is no flush).  We must check against init_mm, since
@@ -762,6 +772,7 @@ static inline int __init activate_vmi(void)
 	if (vmi_ops.release_page) {
 		pv_mmu_ops.release_pte = vmi_release_pte;
 		pv_mmu_ops.release_pmd = vmi_release_pmd;
+		pv_mmu_ops.pgd_free = vmi_pgd_free;
 	}
 
 	/* Set linear is needed in all cases */