[PATCH 4/9] Vmi fix highpte

[PATCH 4/9] Vmi fix highpte

[Zachary Amsden]

Provide a PT map hook for HIGHPTE kernels to designate where they are mapping
page tables.  This information is required so the physical address of PTE
updates can be determined; otherwise, the mm layer would have to carry the
physical address all the way to each PTE modification callsite, which is
even more hideous that the macros required to provide the proper hooks.

So lets not mess up arch neutral code to achieve this, but keep the horror
in an #ifdef HIGHPTE in include/asm-i386/pgtable.h.  I had to use macros
here because some types are not yet defined in all the include paths for
this header.

This patch is absolutely required for HIGHPTE kernels to operate properly
with VMI.

Signed-off-by: Zachary Amsden <zach@vmware.com>

diff -r 87bf6b2d338d arch/i386/kernel/paravirt.c
--- a/arch/i386/kernel/paravirt.c	Tue Feb 27 14:14:34 2007 -0800
+++ b/arch/i386/kernel/paravirt.c	Tue Feb 27 14:14:36 2007 -0800
@@ -553,6 +553,8 @@ struct paravirt_ops paravirt_ops = {
 	.flush_tlb_kernel = native_flush_tlb_global,
 	.flush_tlb_single = native_flush_tlb_single,
 
+	.map_pt_hook = (void *)native_nop,
+
 	.alloc_pt = (void *)native_nop,
 	.alloc_pd = (void *)native_nop,
 	.alloc_pd_clone = (void *)native_nop,
diff -r 87bf6b2d338d arch/i386/kernel/vmi.c
--- a/arch/i386/kernel/vmi.c	Tue Feb 27 14:14:34 2007 -0800
+++ b/arch/i386/kernel/vmi.c	Tue Feb 27 16:23:37 2007 -0800
@@ -370,6 +370,24 @@ static void vmi_check_page_type(u32 pfn,
 #define vmi_check_page_type(p,t) do { } while (0)
 #endif
 
+static void vmi_map_pt_hook(int type, pte_t *va, u32 pfn)
+{
+	/*
+	 * Internally, the VMI ROM must map virtual addresses to physical
+	 * addresses for processing MMU updates.  By the time MMU updates
+	 * are issued, this information is typically already lost.
+	 * Fortunately, the VMI provides a cache of mapping slots for active
+	 * page tables.
+	 *
+	 * We use slot zero for the linear mapping of physical memory, and
+	 * in HIGHPTE kernels, slot 1 and 2 for KM_PTE0 and KM_PTE1.
+	 * 
+	 *  args:                 SLOT                 VA    COUNT PFN
+	 */
+	BUG_ON(type != KM_PTE0 && type != KM_PTE1);
+	vmi_ops.set_linear_mapping((type - KM_PTE0)+1, (u32)va, 1, pfn);
+}
+
 static void vmi_allocate_pt(u32 pfn)
 {
 	vmi_set_page_type(pfn, VMI_PAGE_L1);
@@ -813,6 +831,7 @@ static inline int __init activate_vmi(vo
 	vmi_ops.allocate_page = vmi_get_function(VMI_CALL_AllocatePage);
 	vmi_ops.release_page = vmi_get_function(VMI_CALL_ReleasePage);
 
+	paravirt_ops.map_pt_hook = vmi_map_pt_hook;
 	paravirt_ops.alloc_pt = vmi_allocate_pt;
 	paravirt_ops.alloc_pd = vmi_allocate_pd;
 	paravirt_ops.alloc_pd_clone = vmi_allocate_pd_clone;
diff -r 87bf6b2d338d include/asm-i386/paravirt.h
--- a/include/asm-i386/paravirt.h	Tue Feb 27 14:14:34 2007 -0800
+++ b/include/asm-i386/paravirt.h	Tue Feb 27 16:21:22 2007 -0800
@@ -131,6 +131,8 @@ struct paravirt_ops
 	void (*flush_tlb_kernel)(void);
 	void (*flush_tlb_single)(u32 addr);
 
+	void (fastcall *map_pt_hook)(int type, pte_t *va, u32 pfn);
+
 	void (*alloc_pt)(u32 pfn);
 	void (*alloc_pd)(u32 pfn);
 	void (*alloc_pd_clone)(u32 pfn, u32 clonepfn, u32 start, u32 count);
@@ -354,6 +356,8 @@ static inline void startup_ipi_hook(int 
 #define __flush_tlb_global() paravirt_ops.flush_tlb_kernel()
 #define __flush_tlb_single(addr) paravirt_ops.flush_tlb_single(addr)
 
+#define paravirt_map_pt_hook(type, va, pfn) paravirt_ops.map_pt_hook(type, va, pfn)
+
 #define paravirt_alloc_pt(pfn) paravirt_ops.alloc_pt(pfn)
 #define paravirt_release_pt(pfn) paravirt_ops.release_pt(pfn)
 
diff -r 87bf6b2d338d include/asm-i386/pgtable.h
--- a/include/asm-i386/pgtable.h	Tue Feb 27 14:14:34 2007 -0800
+++ b/include/asm-i386/pgtable.h	Tue Feb 27 16:19:54 2007 -0800
@@ -263,6 +263,7 @@ static inline pte_t pte_mkhuge(pte_t pte
  */
 #define pte_update(mm, addr, ptep)		do { } while (0)
 #define pte_update_defer(mm, addr, ptep)	do { } while (0)
+#define paravirt_map_pt_hook(slot, va, pfn)	do { } while (0)
 #endif
 
 /*
@@ -469,10 +470,24 @@ extern pte_t *lookup_address(unsigned lo
 #endif
 
 #if defined(CONFIG_HIGHPTE)
-#define pte_offset_map(dir, address) \
-	((pte_t *)kmap_atomic(pmd_page(*(dir)),KM_PTE0) + pte_index(address))
-#define pte_offset_map_nested(dir, address) \
-	((pte_t *)kmap_atomic(pmd_page(*(dir)),KM_PTE1) + pte_index(address))
+#define pte_offset_map(dir, address)				\
+({								\
+	pte_t *__ptep;						\
+	unsigned pfn = pmd_val(*(dir)) >> PAGE_SHIFT;	   	\
+	__ptep = (pte_t *)kmap_atomic(pfn_to_page(pfn),KM_PTE0);\
+	paravirt_map_pt_hook(KM_PTE0,__ptep, pfn);		\
+	__ptep = __ptep + pte_index(address);			\
+	__ptep;							\
+})
+#define pte_offset_map_nested(dir, address)			\
+({								\
+	pte_t *__ptep;						\
+	unsigned pfn = pmd_val(*(dir)) >> PAGE_SHIFT;	   	\
+	__ptep = (pte_t *)kmap_atomic(pfn_to_page(pfn),KM_PTE1);\
+	paravirt_map_pt_hook(KM_PTE1,__ptep, pfn);		\
+	__ptep = __ptep + pte_index(address);			\
+	__ptep;							\
+})
 #define pte_unmap(pte) kunmap_atomic(pte, KM_PTE0)
 #define pte_unmap_nested(pte) kunmap_atomic(pte, KM_PTE1)
 #else

Re: [PATCH 4/9] Vmi fix highpte

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> Provide a PT map hook for HIGHPTE kernels to designate where they are mapping
> page tables.  This information is required so the physical address of PTE
> updates can be determined; otherwise, the mm layer would have to carry the
> physical address all the way to each PTE modification callsite, which is
> even more hideous that the macros required to provide the proper hooks.
>
> So lets not mess up arch neutral code to achieve this, but keep the horror
> in an #ifdef HIGHPTE in include/asm-i386/pgtable.h.  I had to use macros
> here because some types are not yet defined in all the include paths for
> this header.
>
> This patch is absolutely required for HIGHPTE kernels to operate properly
> with VMI.
>   

Hm, I don't think this interface will work for Xen.  In Xen, whenever a
pagetable page gets mapped, it must be mapped RO.  map_pt_hook gets
called after the mapping has already been created, so its too late for Xen.

I was planning on adding kmap_atomic_pte() for use in pte_offset_map*(),
which would be wired through to paravirt_ops to allow Xen to make this a
RO mapping.  Would this be sufficient for you to do your vmi thing?

    J

Re: [PATCH 4/9] Vmi fix highpte

[Jeremy Fitzhardinge]

Jeremy Fitzhardinge wrote:
> Hm, I don't think this interface will work for Xen.  In Xen, whenever a
> pagetable page gets mapped, it must be mapped RO.  map_pt_hook gets
> called after the mapping has already been created, so its too late for Xen.
>
> I was planning on adding kmap_atomic_pte() for use in pte_offset_map*(),
> which would be wired through to paravirt_ops to allow Xen to make this a
> RO mapping.  Would this be sufficient for you to do your vmi thing?
>   

Something like this (compiled, untested).

    J

diff -r 972e84c265cf arch/i386/kernel/paravirt.c
--- a/arch/i386/kernel/paravirt.c	Thu Mar 01 19:12:49 2007 -0800
+++ b/arch/i386/kernel/paravirt.c	Thu Mar 01 19:38:42 2007 -0800
@@ -32,6 +32,7 @@
 #include <asm/fixmap.h>
 #include <asm/apic.h>
 #include <asm/tlbflush.h>
+#include <asm/highmem.h>
 
 /* nop stub */
 void _paravirt_nop(void)
@@ -605,6 +606,8 @@ struct paravirt_ops paravirt_ops = {
 
 	.kpte_clear_flush = native_kpte_clear_flush,
 
+	.kmap_atomic_pte = native_kmap_atomic_pte,
+
 #ifdef CONFIG_X86_PAE
 	.set_pte_atomic = native_set_pte_atomic,
 	.set_pte_present = native_set_pte_present,
diff -r 972e84c265cf arch/i386/mm/highmem.c
--- a/arch/i386/mm/highmem.c	Thu Mar 01 19:12:49 2007 -0800
+++ b/arch/i386/mm/highmem.c	Thu Mar 01 19:38:42 2007 -0800
@@ -26,7 +26,7 @@ void kunmap(struct page *page)
  * However when holding an atomic kmap is is not legal to sleep, so atomic
  * kmaps are appropriate for short, tight code paths only.
  */
-void *kmap_atomic(struct page *page, enum km_type type)
+void *_kmap_atomic(struct page *page, enum km_type type, pgprot_t prot)
 {
 	enum fixed_addresses idx;
 	unsigned long vaddr;
@@ -41,9 +41,14 @@ void *kmap_atomic(struct page *page, enu
 		return page_address(page);
 
 	vaddr = __fix_to_virt(FIX_KMAP_BEGIN + idx);
-	set_pte(kmap_pte-idx, mk_pte(page, kmap_prot));
+	set_pte(kmap_pte-idx, mk_pte(page, prot));
 
 	return (void*) vaddr;
+}
+
+void *kmap_atomic(struct page *page, enum km_type type)
+{
+	return _kmap_atomic(page, type, kmap_prot);
 }
 
 void kunmap_atomic(void *kvaddr, enum km_type type)
diff -r 972e84c265cf arch/i386/xen/enlighten.c
--- a/arch/i386/xen/enlighten.c	Thu Mar 01 19:12:49 2007 -0800
+++ b/arch/i386/xen/enlighten.c	Thu Mar 01 19:38:42 2007 -0800
@@ -24,6 +24,7 @@
 #include <asm/setup.h>
 #include <asm/desc.h>
 #include <asm/pgtable.h>
+#include <asm/highmem.h>
 
 #include "xen-ops.h"
 #include "mmu.h"
@@ -499,6 +500,11 @@ static void xen_release_pt(u32 pfn)
 		ClearPagePinned(page);
 		make_lowmem_page_readwrite(__va(PFN_PHYS(pfn)));
 	}
+}
+
+static void *xen_kmap_atomic_pte(struct page *page, enum km_type type)
+{
+	return _kmap_atomic(page, type, PAGE_KERNEL_RO);
 }
 
 static __init void xen_pagetable_setup_start(pgd_t *base)
@@ -688,6 +694,8 @@ static const struct paravirt_ops xen_par
 
 	.kpte_clear_flush = xen_kpte_clear_flush,
 
+	.kmap_atomic_pte = xen_kmap_atomic_pte,
+
 	.pte_val = xen_pte_val,
 	.pgd_val = xen_pgd_val,
 
diff -r 972e84c265cf include/asm-i386/highmem.h
--- a/include/asm-i386/highmem.h	Thu Mar 01 19:12:49 2007 -0800
+++ b/include/asm-i386/highmem.h	Thu Mar 01 19:38:42 2007 -0800
@@ -24,6 +24,7 @@
 #include <linux/threads.h>
 #include <asm/kmap_types.h>
 #include <asm/tlbflush.h>
+#include <asm/paravirt.h>
 
 /* declarations for highmem.c */
 extern unsigned long highstart_pfn, highend_pfn;
@@ -67,10 +68,20 @@ extern void FASTCALL(kunmap_high(struct 
 
 void *kmap(struct page *page);
 void kunmap(struct page *page);
+void *_kmap_atomic(struct page *page, enum km_type type, pgprot_t prot);
 void *kmap_atomic(struct page *page, enum km_type type);
 void kunmap_atomic(void *kvaddr, enum km_type type);
 void *kmap_atomic_pfn(unsigned long pfn, enum km_type type);
 struct page *kmap_atomic_to_page(void *ptr);
+
+static inline void *native_kmap_atomic_pte(struct page *page, enum km_type type)
+{
+	return kmap_atomic(page, type);
+}
+
+#ifndef CONFIG_PARAVIRT
+#define kmap_atomic_pte(page, type)	native_kmap_atomic_pte(page, type)
+#endif
 
 #define flush_cache_kmaps()	do { } while (0)
 
diff -r 972e84c265cf include/asm-i386/paravirt.h
--- a/include/asm-i386/paravirt.h	Thu Mar 01 19:12:49 2007 -0800
+++ b/include/asm-i386/paravirt.h	Thu Mar 01 19:38:42 2007 -0800
@@ -15,6 +15,9 @@
 
 #ifndef __ASSEMBLY__
 #include <linux/types.h>
+#include <asm/kmap_types.h>
+
+struct page;
 
 #define paravirt_type(type)		[paravirt_typenum] "i" (type)
 #define paravirt_clobber(clobber)	[paravirt_clobber] "i" (clobber)
@@ -372,6 +375,8 @@ struct paravirt_ops
 
  	pte_t (*ptep_get_and_clear)(pte_t *ptep);
 
+	void *(*kmap_atomic_pte)(struct page *page, enum km_type type);
+
 #ifdef CONFIG_X86_PAE
 	void (*set_pte_atomic)(pte_t *ptep, pte_t pteval);
  	void (*set_pte_present)(struct mm_struct *mm, unsigned long addr, pte_t *ptep, pte_t pte);
@@ -695,6 +700,13 @@ static inline void paravirt_init_pda(str
 #define paravirt_alloc_pd_clone(pfn, clonepfn, start, count) \
 	PVOP_VCALL4(alloc_pd_clone, pfn, clonepfn, start, count)
 #define paravirt_release_pd(pfn) PVOP_VCALL1(release_pd, pfn)
+
+static inline void *kmap_atomic_pte(struct page *page, enum km_type type)
+{
+	unsigned long ret;
+	ret = PVOP_CALL2(unsigned long, kmap_atomic_pte, page, type);
+	return (void *)ret;
+}
 
 static inline void kpte_clear_flush(pte_t *ptep, unsigned long vaddr)
 {
diff -r 972e84c265cf include/asm-i386/pgtable.h
--- a/include/asm-i386/pgtable.h	Thu Mar 01 19:12:49 2007 -0800
+++ b/include/asm-i386/pgtable.h	Thu Mar 01 19:38:42 2007 -0800
@@ -479,9 +479,9 @@ extern pte_t *lookup_address(unsigned lo
 
 #if defined(CONFIG_HIGHPTE)
 #define pte_offset_map(dir, address) \
-	((pte_t *)kmap_atomic(pmd_page(*(dir)),KM_PTE0) + pte_index(address))
+	((pte_t *)kmap_atomic_pte(pmd_page(*(dir)),KM_PTE0) + pte_index(address))
 #define pte_offset_map_nested(dir, address) \
-	((pte_t *)kmap_atomic(pmd_page(*(dir)),KM_PTE1) + pte_index(address))
+	((pte_t *)kmap_atomic_pte(pmd_page(*(dir)),KM_PTE1) + pte_index(address))
 #define pte_unmap(pte) kunmap_atomic(pte, KM_PTE0)
 #define pte_unmap_nested(pte) kunmap_atomic(pte, KM_PTE1)
 #else

Re: [PATCH 4/9] Vmi fix highpte

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
> Jeremy Fitzhardinge wrote:
>   
>> Hm, I don't think this interface will work for Xen.  In Xen, whenever a
>> pagetable page gets mapped, it must be mapped RO.  map_pt_hook gets
>> called after the mapping has already been created, so its too late for Xen.
>>
>> I was planning on adding kmap_atomic_pte() for use in pte_offset_map*(),
>> which would be wired through to paravirt_ops to allow Xen to make this a
>> RO mapping.  Would this be sufficient for you to do your vmi thing?
>>   
>>     
>
> Something like this (compiled, untested).
>
>     J
>
> diff -r 972e84c265cf arch/i386/kernel/paravirt.c
> --- a/arch/i386/kernel/paravirt.c	Thu Mar 01 19:12:49 2007 -0800
> +++ b/arch/i386/kernel/paravirt.c	Thu Mar 01 19:38:42 2007 -0800
> @@ -32,6 +32,7 @@
>  #include <asm/fixmap.h>
>  #include <asm/apic.h>
>  #include <asm/tlbflush.h>
> +#include <asm/highmem.h>
>  
>  /* nop stub */
>  void _paravirt_nop(void)
> @@ -605,6 +606,8 @@ struct paravirt_ops paravirt_ops = {
>  
>  	.kpte_clear_flush = native_kpte_clear_flush,
>  
> +	.kmap_atomic_pte = native_kmap_atomic_pte,
> +
>   

That doesn't quite work, since we need to know which of the two - 
KM_PTE0 or KM_PTE1 is being mapped.  But it could be moved to before the 
mapping, as you need, and take this as a parameter.

Zach

Re: [PATCH 4/9] Vmi fix highpte

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> That doesn't quite work, since we need to know which of the two -
> KM_PTE0 or KM_PTE1 is being mapped.  But it could be moved to before
> the mapping, as you need, and take this as a parameter. 

Err, kmap_atomic_pte gets passed the type - KM_PTE0 or KM_PTE1.

    J

Re: [PATCH 4/9] Vmi fix highpte

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
> Jeremy Fitzhardinge wrote:
>   
>> Hm, I don't think this interface will work for Xen.  In Xen, whenever a
>> pagetable page gets mapped, it must be mapped RO.  map_pt_hook gets
>> called after the mapping has already been created, so its too late for Xen.
>>
>> I was planning on adding kmap_atomic_pte() for use in pte_offset_map*(),
>> which would be wired through to paravirt_ops to allow Xen to make this a
>> RO mapping.  Would this be sufficient for you to do your vmi thing?
>>   
>>     
>
> Something like this (compiled, untested).
>
>     J
>
> diff -r 972e84c265cf arch/i386/kernel/paravirt.c
> --- a/arch/i386/kernel/paravirt.c	Thu Mar 01 19:12:49 2007 -0800
> +++ b/arch/i386/kernel/paravirt.c	Thu Mar 01 19:38:42 2007 -0800
> @@ -32,6 +32,7 @@
>  #include <asm/fixmap.h>
>  #include <asm/apic.h>
>  #include <asm/tlbflush.h>
> +#include <asm/highmem.h>
>  
>  /* nop stub */
>  void _paravirt_nop(void)
> @@ -605,6 +606,8 @@ struct paravirt_ops paravirt_ops = {
>  
>  	.kpte_clear_flush = native_kpte_clear_flush,
>  
> +	.kmap_atomic_pte = native_kmap_atomic_pte,
> +
>  #ifdef CONFIG_X86_PAE
>  	.set_pte_atomic = native_set_pte_atomic,
>  	.set_pte_present = native_set_pte_present,
> diff -r 972e84c265cf arch/i386/mm/highmem.c
> --- a/arch/i386/mm/highmem.c	Thu Mar 01 19:12:49 2007 -0800
> +++ b/arch/i386/mm/highmem.c	Thu Mar 01 19:38:42 2007 -0800
> @@ -26,7 +26,7 @@ void kunmap(struct page *page)
>   * However when holding an atomic kmap is is not legal to sleep, so atomic
>   * kmaps are appropriate for short, tight code paths only.
>   */
> -void *kmap_atomic(struct page *page, enum km_type type)
> +void *_kmap_atomic(struct page *page, enum km_type type, pgprot_t prot)
>  {
>  	enum fixed_addresses idx;
>  	unsigned long vaddr;
> @@ -41,9 +41,14 @@ void *kmap_atomic(struct page *page, enu
>  		return page_address(page);
>  
>  	vaddr = __fix_to_virt(FIX_KMAP_BEGIN + idx);
> -	set_pte(kmap_pte-idx, mk_pte(page, kmap_prot));
> +	set_pte(kmap_pte-idx, mk_pte(page, prot));
>  
>  	return (void*) vaddr;
> +}
> +
> +void *kmap_atomic(struct page *page, enum km_type type)
> +{
> +	return _kmap_atomic(page, type, kmap_prot);
>  }
>   

Yeah, actually that does work, since you pass the km_type, we can use 
that.  But I would rather not respin this for 2.6.21; getting this 100% 
right can be tricky, and we've already done a good deal of testing on 
this patch the way it is.  Do you have any objection to me creating a 
patch for -mm tree that implements kmap_atomic_pte the way you have 
described above and attaching it to the Xen patch series, but leaving 
the current patch as is for now?

Thanks, (and thanks for the suggestion - I was a little worried about 
how it would play with Xen when HIGHPTE support came around, but it 
looks like it will work for both of us with just one paravirt-op).

Zach

Re: [PATCH 4/9] Vmi fix highpte

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> Yeah, actually that does work, since you pass the km_type, we can use
> that.  But I would rather not respin this for 2.6.21; getting this
> 100% right can be tricky, and we've already done a good deal of
> testing on this patch the way it is.

It seems fairly low risk to me; its basically the same structure with
the same calls happening in the same order, but just slightly rearranged
in the source.  Of course, if I'd seen this patch earlier I could have
given you earlier feedback...

>   Do you have any objection to me creating a patch for -mm tree that
> implements kmap_atomic_pte the way you have described above and
> attaching it to the Xen patch series, but leaving the current patch as
> is for now?

Not particularly, but it seems odd to put something in knowing its going
to be immediately replaced.  What's the urgency?

> Thanks, (and thanks for the suggestion - I was a little worried about
> how it would play with Xen when HIGHPTE support came around, but it
> looks like it will work for both of us with just one paravirt-op).

Yeah, the kpte_clear_flush change helped as well.  I have a patch to
make that into a pvop as well, since its useful to do the clear+flush in
a single call.

    J

Re: [PATCH 4/9] Vmi fix highpte

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
> Zachary Amsden wrote:
>   
>> Yeah, actually that does work, since you pass the km_type, we can use
>> that.  But I would rather not respin this for 2.6.21; getting this
>> 100% right can be tricky, and we've already done a good deal of
>> testing on this patch the way it is.
>>     
>
> It seems fairly low risk to me; its basically the same structure with
> the same calls happening in the same order, but just slightly rearranged
> in the source.  Of course, if I'd seen this patch earlier I could have
> given you earlier feedback...
>   

I've been sending out this particular patch or a variant of it for a 
long time.  It did get lost for a while during the paravirt-ops 
conversion, however.  You're the first to give any feedback on it.

>>   Do you have any objection to me creating a patch for -mm tree that
>> implements kmap_atomic_pte the way you have described above and
>> attaching it to the Xen patch series, but leaving the current patch as
>> is for now?
>>     
>
> Not particularly, but it seems odd to put something in knowing its going
> to be immediately replaced.  What's the urgency?
>   

Better to keep what is known working for now, even if it is going to be 
replaced later... code is easy to change in development cycles, less 
easy to fix when nearing release.  It really is easy to mess up one of 
the pte conversions by, say, shift the wrong value or calculate wrong or 
PAE dependent PTE offset.  Plus it is harder to test, since you need > 
896 M memory for your VM before the code even gets run.

Not that I expect anything to go wrong with it, but I would prefer to 
err on the side of caution for now, that's all.

Zach

Re: [PATCH 4/9] Vmi fix highpte

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> I've been sending out this particular patch or a variant of it for a
> long time.  It did get lost for a while during the paravirt-ops
> conversion, however.  You're the first to give any feedback on it.

Oops, I guess so.  I've been doing a lot more Xen pagetable work for the
last couple of weeks, so it caught my eye this time.

>>>   Do you have any objection to me creating a patch for -mm tree that
>>> implements kmap_atomic_pte the way you have described above and
>>> attaching it to the Xen patch series, but leaving the current patch as
>>> is for now?
>>>     
>>
>> Not particularly, but it seems odd to put something in knowing its going
>> to be immediately replaced.  What's the urgency?
>>   
>
> Better to keep what is known working for now, even if it is going to
> be replaced later... code is easy to change in development cycles,
> less easy to fix when nearing release.  It really is easy to mess up
> one of the pte conversions by, say, shift the wrong value or calculate
> wrong or PAE dependent PTE offset.

Those are bugs that can occur, but they don't apply in this case.  The
vmi implementation of kmap_atomic_pte() would be:

static void *vmi_kmap_atomic_pte(struct page *page, enum km_type type)
{
	void *ptep = kmap_atomic(page, type);
	vmi_map_pt_hook(type, ptep, page_to_pfn(page));
	return ptep;
}

Right?  Which is functionally identical to the code in your patch,
except wrapped up in a new function.

    J

Re: [PATCH 4/9] Vmi fix highpte

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
> Those are bugs that can occur, but they don't apply in this case.  The
> vmi implementation of kmap_atomic_pte() would be:
>
> static void *vmi_kmap_atomic_pte(struct page *page, enum km_type type)
> {
> 	void *ptep = kmap_atomic(page, type);
> 	vmi_map_pt_hook(type, ptep, page_to_pfn(page));
> 	return ptep;
> }
>
> Right?  Which is functionally identical to the code in your patch,
> except wrapped up in a new function.
>   

Yes, but the hook point now happens before the page table mapping.  Not 
that it should cause a problem.  But we've been testing things the 
original way for over a year now, and if we want to get the fix upstream 
for 2.6.21, it seems better to upstream a more tested fix rather than a 
new way of doing things, even if it is identical in theory.

That said, I have no problems with the approach you propose going 
forward.  I just don't think it is appropriate for an -rc release, 
because it provides no tangible benefit for any of the in-kernel code, 
and causes a lot of retesting.  I still believe there is almost zero 
risk to doing things the way you propose.  But I am also a firm believer 
in shipping what is tested and working unless there is a compelling 
reason to do otherwise.  And if Xen is not going to be in 2.6.21, the 
compelling reason becomes getting the code working for both of us for 
2.6.22 - so let's do that, and keep the patches from Andrew's -mm tree 
around to make sure that we have a suitable patch base that can be 
applied to 2.6.21 for any distros that are willing to pick up the Xen 
paravirt-ops.

Sound reasonable?

Zach

Re: [PATCH 4/9] Vmi fix highpte

[Jeremy Fitzhardinge]

Zachary Amsden wrote:
> Yes, but the hook point now happens before the page table mapping.

Did you change that since you posted the patch?  Sounds like a larger
change than the one I'm proposing.

>   Not that it should cause a problem.  But we've been testing things
> the original way for over a year now, and if we want to get the fix
> upstream for 2.6.21, it seems better to upstream a more tested fix
> rather than a new way of doing things, even if it is identical in theory.

Sure.

> That said, I have no problems with the approach you propose going
> forward.  I just don't think it is appropriate for an -rc release,
> because it provides no tangible benefit for any of the in-kernel code,
> and causes a lot of retesting.  I still believe there is almost zero
> risk to doing things the way you propose.  But I am also a firm
> believer in shipping what is tested and working unless there is a
> compelling reason to do otherwise.  And if Xen is not going to be in
> 2.6.21, the compelling reason becomes getting the code working for
> both of us for 2.6.22 - so let's do that, and keep the patches from
> Andrew's -mm tree around to make sure that we have a suitable patch
> base that can be applied to 2.6.21 for any distros that are willing to
> pick up the Xen paravirt-ops.
>
> Sound reasonable?

I can deal with the change going into -git, but it does seem awkward
knowing that it is the wrong change and it will be replaced by something
else almost immediately.

My main concern is that the Xen patch queue is complex enough as-is, and
I've been trying hard to remove dependencies on other uncommitted
patches.   It's doubly complex because I'm not really sure if I'm
targeting Andrew or Andi's tree as a base, though at the moment -git
seems to work either way.

    J

Re: [PATCH 4/9] Vmi fix highpte

[Zachary Amsden]

Jeremy Fitzhardinge wrote:
>
> I can deal with the change going into -git, but it does seem awkward
> knowing that it is the wrong change and it will be replaced by something
> else almost immediately.
>   

Well, it is not quite wrong - it is appropriate for -git.  That it will 
be replaced soon is a minor thing, as long as we can work together to 
make sure your patches are unaffected.

> My main concern is that the Xen patch queue is complex enough as-is, and
> I've been trying hard to remove dependencies on other uncommitted
> patches.   It's doubly complex because I'm not really sure if I'm
> targeting Andrew or Andi's tree as a base, though at the moment -git
> seems to work either way.
>
>     J
>   

Believe me, I understand the complexities of dealing with -mm, -git, and 
-i386 trees.  It's not just doubly complex, its triply so.  I believe 
the best approach is to make it easiest on the maintainers.  So the 
patch stream I am uploading through -mm, -i386, to -git does not 
conflict with the Xen patch queue as long as we make sure the next 
bundle to those same trees applies clean.  Again, I broke the Xen patch 
queue in -mm, and for that I am sorry.  I will help you re-integrate 
against the VMI patches I sent out, as I broke your code, I will help 
fix it.  And we both have a tree to work off from, which makes it rather 
easy to work together ;)

Cheers,

Zach

----

Sutra I.2 - interruptah chittavritti nirodhah