Vmi fix highpte

PATCH In review Zachary Amsden 2 participants 12 messages Mar 2, 2007 – Mar 3, 2007

Subsystems (2)

virtio core PARAVIRT_OPS interface

lore

Share & tools

lore

Zachary Amsden

author

2007-03-02 02:54 UTC #0 lore raw

PATCH 4/9 Vmi fix highpte

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

arch/i386/kernel/paravirt.c +2 -0

diff -r 87bf6b2d338d arch/i386/kernel/paravirt.c
@@ -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,

arch/i386/kernel/vmi.c +19 -0

diff -r 87bf6b2d338d arch/i386/kernel/vmi.c
@@ -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;

include/asm-i386/paravirt.h +4 -0

diff -r 87bf6b2d338d include/asm-i386/paravirt.h
@@ -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)

include/asm-i386/pgtable.h +19 -4

diff -r 87bf6b2d338d include/asm-i386/pgtable.h
@@ -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

Jeremy Fitzhardinge

2007-03-02 03:08 UTC #1 lore raw

Quoted 6 lines · 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.

Quoted 8 lines

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?

Jeremy Fitzhardinge

2007-03-02 03:39 UTC #2 lore raw

Quoted 8 lines · Jeremy Fitzhardinge

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

arch/i386/kernel/paravirt.c +3 -0

diff -r 972e84c265cf arch/i386/kernel/paravirt.c
@@ -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,

arch/i386/mm/highmem.c +7 -2

diff -r 972e84c265cf arch/i386/mm/highmem.c
@@ -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)

arch/i386/xen/enlighten.c +8 -0

diff -r 972e84c265cf arch/i386/xen/enlighten.c
@@ -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,

include/asm-i386/highmem.h +11 -0

diff -r 972e84c265cf include/asm-i386/highmem.h
@@ -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)

include/asm-i386/paravirt.h +12 -0

diff -r 972e84c265cf include/asm-i386/paravirt.h
@@ -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)
 {

include/asm-i386/pgtable.h +2 -2

diff -r 972e84c265cf include/asm-i386/pgtable.h
@@ -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

Zachary Amsden

author

2007-03-02 06:24 UTC #3 lore raw

Quoted 28 lines · Jeremy Fitzhardinge

Quoted 9 lines · Jeremy Fitzhardinge
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
arch/i386/kernel/paravirt.c +1 -0
@@ -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)

arch/i386/kernel/paravirt.c struct paravirt_ops paravirt_ops = { · line 611

Quoted 6 lines

arch/i386/kernel/paravirt.c +2 -0

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

Jeremy Fitzhardinge

2007-03-02 06:29 UTC #4 lore raw

Quoted 3 lines · Zachary Amsden

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.

Zachary Amsden

author

2007-03-02 06:31 UTC #5 lore raw

In reply to Jeremy Fitzhardinge

Quoted 53 lines · Jeremy Fitzhardinge

Quoted 9 lines · Jeremy Fitzhardinge
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
arch/i386/kernel/paravirt.c +3 -0
@@ -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
arch/i386/mm/highmem.c +1 -2
@@ -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));

arch/i386/mm/highmem.c void *kmap_atomic(struct page *page, enu · line 53

Quoted 10 lines

arch/i386/mm/highmem.c +6 -0

+	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

Jeremy Fitzhardinge

2007-03-02 06:45 UTC #6 lore raw

Quoted 4 lines · Zachary Amsden

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

Quoted 4 lines

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?

Quoted 3 lines

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.

Zachary Amsden

author

2007-03-02 09:53 UTC #7 lore raw

Quoted 8 lines · Jeremy Fitzhardinge

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

Quoted 5 lines

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.

Quoted 9 lines

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

Jeremy Fitzhardinge

2007-03-02 16:55 UTC #8 lore raw

Quoted 3 lines · Zachary Amsden

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.

Quoted 10 lines

Quoted 9 lines
Quoted 5 lines
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?

Quoted 5 lines

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.

Zachary Amsden

author

2007-03-03 07:17 UTC #9 lore raw

Quoted 4 lines · Jeremy Fitzhardinge

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)

Quoted 9 lines

{
	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

Jeremy Fitzhardinge

2007-03-03 07:43 UTC #10 lore raw

Quoted 1 line · Zachary Amsden

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.

Quoted 4 lines

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.

Quoted 13 lines

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.

Quoted 1 line

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.

Zachary Amsden

author

2007-03-03 07:58 UTC #11 lore raw

Quoted 5 lines · Jeremy Fitzhardinge

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.

Quoted 8 lines

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