Xen devel - Dec 2008 - Re: [PATCH] fix ia64 breakage with PHYSDEVOP_pirq_eoi_mfn (was Re: [PATCH 2/2] linux/x86: use shared page indicating the need for an EOI notification)

>>> Isaku Yamahata <yamahata@valinux.co.jp> 03.12.08 10:20
>>>
>Yes, you''re correct. In fact I had the patch which you suggested,
>but I was hesitated to change the x86 implementation so that
>I had changed it to use virt_to_bus() on x86.
>
>
>
>evtchn, physdev: fix pirq_eoi_mfn for IA64 support.
>
>On ia64, global variables aren''t in identity mapping area (i.e.
kaddr)
>so that there is no relationship between its virtual address and
>its physical address. Thus virt_to_bus() can''t be applied to them.
>So introduce arbitrary_virt_to_bus() to wrap arch dependent function
>and make use of it.
I needed to look into this again, because the use of arbitary_virt_to_machine()
in drivers/xen/core/evtchn.c fails to build for me (and I can''t see how
the
2.6.18 tree would build for x86 either, as I can''t see how
asm/pgtable.h
would get included: it doesn''t get included in any of my 2.6.16,
2.6.22,
2.6.25, and 2.6.27 based trees). Perhaps there''s a configuration
dependency, but that would then be wrong. And I''m hesitant to include
asm/pgtable.h explicitly in that file, as it really shouldn''t need it.

Looking at how ia64 defines virt_to_machine() at present I would be
inclined to say that all current users (tpmfront, blktap, and gntdev) of
that macro don''t get what they expect, and the implementation you
added for arbitary_virt_to_machine() really ought to be the one for
virt_to_machine(), given your description above.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Mon, Dec 08, 2008 at 01:36:06PM +0000, Jan Beulich
wrote:
> >>> Isaku Yamahata <yamahata@valinux.co.jp> 03.12.08 10:20
>>>
> >Yes, you''re correct. In fact I had the patch which you
suggested,
> >but I was hesitated to change the x86 implementation so that
> >I had changed it to use virt_to_bus() on x86.
> >
> >
> >
> >evtchn, physdev: fix pirq_eoi_mfn for IA64 support.
> >
> >On ia64, global variables aren''t in identity mapping area
(i.e. kaddr)
> >so that there is no relationship between its virtual address and
> >its physical address. Thus virt_to_bus() can''t be applied to
them.
> >So introduce arbitrary_virt_to_bus() to wrap arch dependent function
> >and make use of it.
> 
> I needed to look into this again, because the use of
arbitary_virt_to_machine()
> in drivers/xen/core/evtchn.c fails to build for me (and I can''t
see how the
> 2.6.18 tree would build for x86 either, as I can''t see how
asm/pgtable.h
> would get included: it doesn''t get included in any of my 2.6.16,
2.6.22,
> 2.6.25, and 2.6.27 based trees). Perhaps there''s a configuration
> dependency, but that would then be wrong. And I''m hesitant to
include
> asm/pgtable.h explicitly in that file, as it really shouldn''t need
it.
Sorry for breakage.
How about moving arbitrary_virt_to_machine() from pgtable.h to maddr.h?
(Yes, this is a work around. and you wouldn''t like it...)

> Looking at how ia64 defines virt_to_machine() at present I would be
> inclined to say that all current users (tpmfront, blktap, and gntdev) of
> that macro don''t get what they expect, and the implementation you
> added for arbitary_virt_to_machine() really ought to be the one for
> virt_to_machine(), given your description above.
Looking the x86 virt_to_machine definition, virt_to_machine()
assumes the passed address in the straight mapping area.
So the ia64 assumption is same to x86.
Hmm, ia64 and x86_64 have nothing to do with highmem,
but x86_32 has to deal with highmem. So x86_32 with highmem
seems to have the issue you described above.
If ptep which is passed to virt_to_machine is highmem,
I don''t see how it works. So all virt_to_machine() shouldn''t
be changed to arbitrary_virt_to_machine()?

-- 
yamahata

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> Isaku Yamahata <yamahata@valinux.co.jp> 09.12.08 04:40
>>>
>Sorry for breakage.
>How about moving arbitrary_virt_to_machine() from pgtable.h to maddr.h?
>(Yes, this is a work around. and you wouldn''t like it...)
No, that wouldn''t work for x86 either, because the macro uses
lookup_address(), which in turn is also only declared in pgtable.h. I
wouldn''t really mind moving arbitrary_virt_to_machine(), but it would
then require duplicating (not moving) the lookup_address() declaration.
>> Looking at how ia64 defines virt_to_machine() at present I would be
>> inclined to say that all current users (tpmfront, blktap, and gntdev)
of
>> that macro don''t get what they expect, and the implementation
you
>> added for arbitary_virt_to_machine() really ought to be the one for
>> virt_to_machine(), given your description above.
>
>Looking the x86 virt_to_machine definition, virt_to_machine()
>assumes the passed address in the straight mapping area.
>So the ia64 assumption is same to x86.
Not exactly: Addresses of kernel objects *can* be passed to
virt_to_machine() on x86 (minus a supposed compiler issue demanding
the special __pa_symbol() to be used on x86-64 - I''m trying to find out
how relevant this still is), but they can''t be on ia64. This is what
seemed
wrong to me. But otoh as I understand it you can''t pass kernel
addresses through __pa() either, but (to my surprise) ia64 apparently
has no problem with this wrt architecture independent code (but making
necessary work-arounds like paddr_vmcoreinfo_note()).
>Hmm, ia64 and x86_64 have nothing to do with highmem,
>but x86_32 has to deal with highmem. So x86_32 with highmem
>seems to have the issue you described above.
>If ptep which is passed to virt_to_machine is highmem,
>I don''t see how it works. So all virt_to_machine()
shouldn''t
>be changed to arbitrary_virt_to_machine()?
Actually, looking at it a second time, tpmfront uses the result of the result
of __get_free_page() here, so the address is always in the 1:1 mapping.

But I think you''re quite right about the HIGHPTE implications on blktap
and
gntdev - these ought to be fixed, perhaps indeed by using
arbitrary_virt_to_machine() there (but I''d want to make this
conditional
upon the HIGHPTE config option, so to not affect performance of other
configurations: possibly this ought to be an architecture-defined macro
like ptep_virt_to_machine(), as I wouldn''t want to place an
x86-specific
conditional in there that would risk breaking any future supported
architecture without explicit notice).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Tue, Dec 09, 2008 at 10:04:55AM +0000, Jan Beulich
wrote:
> >>> Isaku Yamahata <yamahata@valinux.co.jp> 09.12.08 04:40
>>>
> >Sorry for breakage.
> >How about moving arbitrary_virt_to_machine() from pgtable.h to maddr.h?
> >(Yes, this is a work around. and you wouldn''t like it...)
> 
> No, that wouldn''t work for x86 either, because the macro uses
> lookup_address(), which in turn is also only declared in pgtable.h. I
> wouldn''t really mind moving arbitrary_virt_to_machine(), but it
would
> then require duplicating (not moving) the lookup_address() declaration.
> 
> >> Looking at how ia64 defines virt_to_machine() at present I would
be
> >> inclined to say that all current users (tpmfront, blktap, and
gntdev) of
> >> that macro don''t get what they expect, and the
implementation you
> >> added for arbitary_virt_to_machine() really ought to be the one
for
> >> virt_to_machine(), given your description above.
> >
> >Looking the x86 virt_to_machine definition, virt_to_machine()
> >assumes the passed address in the straight mapping area.
> >So the ia64 assumption is same to x86.
> 
> Not exactly: Addresses of kernel objects *can* be passed to
> virt_to_machine() on x86 (minus a supposed compiler issue demanding
> the special __pa_symbol() to be used on x86-64 - I''m trying to
find out
> how relevant this still is), but they can''t be on ia64. This is
what seemed
> wrong to me. But otoh as I understand it you can''t pass kernel
> addresses through __pa() either, but (to my surprise) ia64 apparently
> has no problem with this wrt architecture independent code (but making
> necessary work-arounds like paddr_vmcoreinfo_note()).
You are the first person to pass the kernel symbol address
to virt_to_machine() in arch independent code.
Is there any necessity to allocate pirq_needs_eoi statically?
(except it did before)
If no, can we allocate the pages for them dynamically?
Then the issue will go away.

> >Hmm, ia64 and x86_64 have nothing to do with highmem,
> >but x86_32 has to deal with highmem. So x86_32 with highmem
> >seems to have the issue you described above.
> >If ptep which is passed to virt_to_machine is highmem,
> >I don''t see how it works. So all virt_to_machine()
shouldn''t
> >be changed to arbitrary_virt_to_machine()?
> 
> Actually, looking at it a second time, tpmfront uses the result of the
result
> of __get_free_page() here, so the address is always in the 1:1 mapping.
> 
> But I think you''re quite right about the HIGHPTE implications on
blktap and
> gntdev - these ought to be fixed, perhaps indeed by using
> arbitrary_virt_to_machine() there (but I''d want to make this
conditional
> upon the HIGHPTE config option, so to not affect performance of other
> configurations: possibly this ought to be an architecture-defined macro
> like ptep_virt_to_machine(), as I wouldn''t want to place an
x86-specific
> conditional in there that would risk breaking any future supported
> architecture without explicit notice).
Introduce ptep_to_machine() or something like that?

-- 
yamahata

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> Isaku Yamahata <yamahata@valinux.co.jp> 09.12.08 11:43
>>>
>You are the first person to pass the kernel symbol address
>to virt_to_machine() in arch independent code.
>Is there any necessity to allocate pirq_needs_eoi statically?
>(except it did before)
Perhaps not - avoiding the possible allocation failure (-> BUG()) and the
extra indirection were the main reasons I kept it allocated statically.
>If no, can we allocate the pages for them dynamically?
>Then the issue will go away.
Indeed.
>Introduce ptep_to_machine() or something like that?
Yes, if that name isn''t ambiguous in some way.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 09/12/2008 10:54, "Jan Beulich" <jbeulich@novell.com> wrote:
>>>> Isaku Yamahata <yamahata@valinux.co.jp> 09.12.08 11:43
>>>
>> You are the first person to pass the kernel symbol address
>> to virt_to_machine() in arch independent code.
>> Is there any necessity to allocate pirq_needs_eoi statically?
>> (except it did before)
> 
> Perhaps not - avoiding the possible allocation failure (-> BUG()) and
the
> extra indirection were the main reasons I kept it allocated statically.
> 
>> If no, can we allocate the pages for them dynamically?
>> Then the issue will go away.
> 
> Indeed.
Yes please. Just do this and be done. It''s a one-off start-of-day
allocation
which, if it fails, means you''re screwed anyway.

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Tue, Dec 09, 2008 at 11:06:58AM +0000, Keir Fraser
wrote:
> On 09/12/2008 10:54, "Jan Beulich" <jbeulich@novell.com>
wrote:
> 
> >>>> Isaku Yamahata <yamahata@valinux.co.jp> 09.12.08
11:43 >>>
> >> You are the first person to pass the kernel symbol address
> >> to virt_to_machine() in arch independent code.
> >> Is there any necessity to allocate pirq_needs_eoi statically?
> >> (except it did before)
> > 
> > Perhaps not - avoiding the possible allocation failure (-> BUG())
and the
> > extra indirection were the main reasons I kept it allocated
statically.
> > 
> >> If no, can we allocate the pages for them dynamically?
> >> Then the issue will go away.
> > 
> > Indeed.
> 
> Yes please. Just do this and be done. It''s a one-off start-of-day
allocation
> which, if it fails, means you''re screwed anyway.
Here is.

evtchn: allocate pirq_needs_eoi bitmap dynamically.

allocating pirq_needs_eoi statically causes an address conversion
issue between ia64 and x86 because ia64 kernel symbol address
doesn''t lay in 1:1 mapping area which can''t be apssed to
virt_to_machine().
So allocate it dynamically to work around it.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

diff --git a/drivers/xen/core/evtchn.c b/drivers/xen/core/evtchn.c
--- a/drivers/xen/core/evtchn.c
+++ b/drivers/xen/core/evtchn.c
@@ -31,6 +31,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/bootmem.h>
 #include <linux/irq.h>
 #include <linux/interrupt.h>
 #include <linux/sched.h>
@@ -755,8 +756,7 @@ static struct hw_interrupt_type dynirq_t
 
 /* Bitmap indicating which PIRQs require Xen to be notified on unmask. */
 static int pirq_eoi_does_unmask;
-static DECLARE_BITMAP(pirq_needs_eoi, ALIGN(NR_PIRQS, PAGE_SIZE * 8))
-	__attribute__ ((__section__(".bss.page_aligned"),
__aligned__(PAGE_SIZE)));
+static unsigned long *pirq_needs_eoi;
 
 static void pirq_unmask_and_notify(unsigned int evtchn, unsigned int irq)
 {
@@ -1041,8 +1041,7 @@ void irq_resume(void)
 	if (pirq_eoi_does_unmask) {
 		struct physdev_pirq_eoi_gmfn eoi_gmfn;
 
-		eoi_gmfn.gmfn = arbitrary_virt_to_machine(pirq_needs_eoi)
-			>> PAGE_SHIFT;
+		eoi_gmfn.gmfn = virt_to_machine(pirq_needs_eoi) >> PAGE_SHIFT;
 		if (HYPERVISOR_physdev_op(PHYSDEVOP_pirq_eoi_gmfn, &eoi_gmfn))
 			BUG();
 	}
@@ -1137,9 +1136,12 @@ void __init xen_init_IRQ(void)
 
 	init_evtchn_cpu_bindings();
 
+	pirq_needs_eoi = alloc_bootmem_pages(PAGE_SIZE);
+	memset(pirq_needs_eoi, 0, PAGE_SIZE);
+	if (pirq_needs_eoi == NULL)
+		panic("failed to allocate a page for event channel.");
 	BUG_ON(!bitmap_empty(pirq_needs_eoi, PAGE_SIZE * 8));
-	eoi_gmfn.gmfn = arbitrary_virt_to_machine(pirq_needs_eoi)
-		>> PAGE_SHIFT;
+	eoi_gmfn.gmfn = virt_to_machine(pirq_needs_eoi) >> PAGE_SHIFT;
 	if (HYPERVISOR_physdev_op(PHYSDEVOP_pirq_eoi_gmfn, &eoi_gmfn) == 0)
 		pirq_eoi_does_unmask = 1;
 


-- 
yamahata

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Tue, Dec 09, 2008 at 10:54:57AM +0000, Jan Beulich
wrote:
> >Introduce ptep_to_machine() or something like that?
> Yes, if that name isn''t ambiguous in some way.
Like this?
To be honest, I haven''t tested the patch with highpte yet.

blktap, gntdev: fix highpte handling.

In case of highpte, virt_to_machine() can''t be used.
Introduce ptep_to_machine() and use it.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

diff --git a/drivers/xen/blktap/blktap.c b/drivers/xen/blktap/blktap.c
--- a/drivers/xen/blktap/blktap.c
+++ b/drivers/xen/blktap/blktap.c
@@ -54,6 +54,7 @@
 #include <linux/poll.h>
 #include <linux/delay.h>
 #include <asm/tlbflush.h>
+#include <asm/pgtable.h>
 
 #define MAX_TAP_DEV 256     /*the maximum number of tapdisk ring devices    */
 #define MAX_DEV_NAME 100    /*the max tapdisk ring device name e.g. blktap0 */
@@ -364,7 +365,7 @@ static pte_t blktap_clear_pte(struct vm_
 		BUG_ON(xen_feature(XENFEAT_auto_translated_physmap));
 
 		copy = *ptep;
-		gnttab_set_unmap_op(&unmap[count], virt_to_machine(ptep), 
+		gnttab_set_unmap_op(&unmap[count], ptep_to_machine(ptep), 
 				    GNTMAP_host_map 
 				    | GNTMAP_application_map 
 				    | GNTMAP_contains_pte,
diff --git a/drivers/xen/gntdev/gntdev.c b/drivers/xen/gntdev/gntdev.c
--- a/drivers/xen/gntdev/gntdev.c
+++ b/drivers/xen/gntdev/gntdev.c
@@ -34,7 +34,7 @@
 
 #include <linux/types.h>
 #include <xen/public/gntdev.h>
-
+#include <asm/pgtable.h>
 
 #define DRIVER_AUTHOR "Derek G. Murray
<Derek.Murray@cl.cam.ac.uk>"
 #define DRIVER_DESC   "User-space granted page access driver"
@@ -769,7 +769,7 @@ static pte_t gntdev_clear_pte(struct vm_
 		    GNTDEV_INVALID_HANDLE && 
 		    !xen_feature(XENFEAT_auto_translated_physmap)) {
 			/* NOT USING SHADOW PAGE TABLES. */
-			gnttab_set_unmap_op(&op, virt_to_machine(ptep), 
+			gnttab_set_unmap_op(&op, ptep_to_machine(ptep), 
 					    GNTMAP_contains_pte,
 					    private_data->grants[slot_index]
 					    .u.valid.user_handle);
diff --git a/include/asm-i386/mach-xen/asm/pgtable.h
b/include/asm-i386/mach-xen/asm/pgtable.h
--- a/include/asm-i386/mach-xen/asm/pgtable.h
+++ b/include/asm-i386/mach-xen/asm/pgtable.h
@@ -488,6 +488,17 @@ void make_pages_writable(void *va, unsig
 	(((maddr_t)pte_mfn(*virt_to_ptep(va)) << PAGE_SHIFT)		\
 	 | ((unsigned long)(va) & (PAGE_SIZE - 1)))
 
+#ifdef CONFIG_HIGHPTE
+#define ptep_to_machine(ptep)						\
+({									\
+	(unsigned long)(ptep) >= (unsigned long)high_memory?		\
+		arbitrary_virt_to_machine(ptep) :			\
+		virt_to_machine(ptep);					\
+})
+#else
+#define ptep_to_machine(ptep)	virt_to_machine(ptep)
+#endif
+
 #endif /* !__ASSEMBLY__ */
 
 #ifdef CONFIG_FLATMEM
diff --git a/include/asm-ia64/maddr.h b/include/asm-ia64/maddr.h
--- a/include/asm-ia64/maddr.h
+++ b/include/asm-ia64/maddr.h
@@ -100,6 +100,7 @@ mfn_to_local_pfn(unsigned long mfn)
 #define virt_to_mfn(virt) (__pa(virt) >> PAGE_SHIFT)
 #define virt_to_machine(virt) __pa(virt) /* for tpmfront.c */
 #define arbitrary_virt_to_machine(virt) virt_to_machine(ia64_imva(virt))
+#define ptep_to_machine(virt) virt_to_machine(virt)
 
 #define set_phys_to_machine(pfn, mfn) do { } while (0)
 
diff --git a/include/asm-x86_64/mach-xen/asm/pgtable.h
b/include/asm-x86_64/mach-xen/asm/pgtable.h
--- a/include/asm-x86_64/mach-xen/asm/pgtable.h
+++ b/include/asm-x86_64/mach-xen/asm/pgtable.h
@@ -30,6 +30,8 @@ extern pte_t *lookup_address(unsigned lo
 #define arbitrary_virt_to_machine(va)					\
 	(((maddr_t)pte_mfn(*virt_to_ptep(va)) << PAGE_SHIFT)		\
 	 | ((unsigned long)(va) & (PAGE_SIZE - 1)))
+
+#define ptep_to_machine(ptep)	virt_to_machine(ptep)
 #endif
 
 extern pud_t level3_kernel_pgt[512];


-- 
yamahata

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Sorry I sent out the old one. Here is the corrected one.
NULL check must be before its use.

evtchn: allocate pirq_needs_eoi bitmap dynamically.

allocating pirq_needs_eoi statically causes an address conversion
issue between ia64 and x86 because ia64 kernel symbol address
doesn''t lay in 1:1 mapping area which can''t be apssed to
virt_to_machine().
So allocate it dynamically to work around it.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

diff --git a/drivers/xen/core/evtchn.c b/drivers/xen/core/evtchn.c
--- a/drivers/xen/core/evtchn.c
+++ b/drivers/xen/core/evtchn.c
@@ -31,6 +31,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/bootmem.h>
 #include <linux/irq.h>
 #include <linux/interrupt.h>
 #include <linux/sched.h>
@@ -755,8 +756,7 @@ static struct hw_interrupt_type dynirq_t
 
 /* Bitmap indicating which PIRQs require Xen to be notified on unmask. */
 static int pirq_eoi_does_unmask;
-static DECLARE_BITMAP(pirq_needs_eoi, ALIGN(NR_PIRQS, PAGE_SIZE * 8))
-	__attribute__ ((__section__(".bss.page_aligned"),
__aligned__(PAGE_SIZE)));
+static unsigned long *pirq_needs_eoi;
 
 static void pirq_unmask_and_notify(unsigned int evtchn, unsigned int irq)
 {
@@ -1041,8 +1041,7 @@ void irq_resume(void)
 	if (pirq_eoi_does_unmask) {
 		struct physdev_pirq_eoi_gmfn eoi_gmfn;
 
-		eoi_gmfn.gmfn = arbitrary_virt_to_machine(pirq_needs_eoi)
-			>> PAGE_SHIFT;
+		eoi_gmfn.gmfn = virt_to_machine(pirq_needs_eoi) >> PAGE_SHIFT;
 		if (HYPERVISOR_physdev_op(PHYSDEVOP_pirq_eoi_gmfn, &eoi_gmfn))
 			BUG();
 	}
@@ -1137,9 +1136,12 @@ void __init xen_init_IRQ(void)
 
 	init_evtchn_cpu_bindings();
 
+	pirq_needs_eoi = alloc_bootmem_pages(PAGE_SIZE);
+	if (pirq_needs_eoi == NULL)
+		panic("failed to allocate a page for event channel.");
+	memset(pirq_needs_eoi, 0, PAGE_SIZE);
 	BUG_ON(!bitmap_empty(pirq_needs_eoi, PAGE_SIZE * 8));
-	eoi_gmfn.gmfn = arbitrary_virt_to_machine(pirq_needs_eoi)
-		>> PAGE_SHIFT;
+	eoi_gmfn.gmfn = virt_to_machine(pirq_needs_eoi) >> PAGE_SHIFT;
 	if (HYPERVISOR_physdev_op(PHYSDEVOP_pirq_eoi_gmfn, &eoi_gmfn) == 0)
 		pirq_eoi_does_unmask = 1;
 


-- 
yamahata

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> Isaku Yamahata <yamahata@valinux.co.jp> 10.12.08 05:09
>>>
>Like this?
Almost.
>--- a/include/asm-i386/mach-xen/asm/pgtable.h
>+++ b/include/asm-i386/mach-xen/asm/pgtable.h
>@@ -488,6 +488,17 @@ void make_pages_writable(void *va, unsig
> 	(((maddr_t)pte_mfn(*virt_to_ptep(va)) << PAGE_SHIFT)		\
> 	 | ((unsigned long)(va) & (PAGE_SIZE - 1)))
> 
>+#ifdef CONFIG_HIGHPTE
>+#define ptep_to_machine(ptep)						\
>+({									\
>+	(unsigned long)(ptep) >= (unsigned long)high_memory?		\
>+		arbitrary_virt_to_machine(ptep) :			\
>+		virt_to_machine(ptep);					\
>+})
I was intending to make use of kmap_atomic_to_page() here. Due to the
check at the beginning of that function, it would even be possible to get
away without a conditional in the highpte case of the macro:

#define ptep_to_machine(ptep) page_to_phys(kmap_atomic_to_page(ptep))

page_to_phys() and page_to_bus() could be used here interchangeably due
to them being identical on x86.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> Isaku Yamahata <yamahata@valinux.co.jp> 10.12.08 05:16
>>>
>Sorry I sent out the old one. Here is the corrected one.
>NULL check must be before its use.
Not really - alloc_bootmem() etc panic for themselves unless you use the
_nopanic variants.
Also, alloc_bootmem() etc zero the allocated memory, so no need for
memset(), and the subsequent BUG_ON() can obviously go away.
And finally, PAGE_SIZE isn''t correct, you should use the size
originally
used, just slightly modified:

BITS_TO_LONGS(ALIGN(NR_PIRQS, PAGE_SIZE * 8))

I was about to put together a patch for this myself...

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 10/12/2008 09:21, "Jan Beulich" <jbeulich@novell.com> wrote:
>>>> Isaku Yamahata <yamahata@valinux.co.jp> 10.12.08 05:16
>>>
>> Sorry I sent out the old one. Here is the corrected one.
>> NULL check must be before its use.
> 
> Not really - alloc_bootmem() etc panic for themselves unless you use the
> _nopanic variants.
> Also, alloc_bootmem() etc zero the allocated memory, so no need for
> memset(), and the subsequent BUG_ON() can obviously go away.
> And finally, PAGE_SIZE isn''t correct, you should use the size
originally
> used, just slightly modified:
> 
> BITS_TO_LONGS(ALIGN(NR_PIRQS, PAGE_SIZE * 8))
> 
> I was about to put together a patch for this myself...
Can you just fix up Isaku''s patch and then we''ll collect a
fresh sign-off
from him too?

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> Keir Fraser <keir.fraser@eu.citrix.com> 10.12.08 11:07
>>>
>On 10/12/2008 09:21, "Jan Beulich" <jbeulich@novell.com>
wrote:
>
>>>>> Isaku Yamahata <yamahata@valinux.co.jp> 10.12.08
05:16 >>>
>>> Sorry I sent out the old one. Here is the corrected one.
>>> NULL check must be before its use.
>> 
>> Not really - alloc_bootmem() etc panic for themselves unless you use
the
>> _nopanic variants.
>> Also, alloc_bootmem() etc zero the allocated memory, so no need for
>> memset(), and the subsequent BUG_ON() can obviously go away.
>> And finally, PAGE_SIZE isn''t correct, you should use the size
originally
>> used, just slightly modified:
>> 
>> BITS_TO_LONGS(ALIGN(NR_PIRQS, PAGE_SIZE * 8))
>> 
>> I was about to put together a patch for this myself...
>
>Can you just fix up Isaku''s patch and then we''ll collect a
fresh sign-off
>from him too?
Yes, I''m in the process of doing this (here and for the other one) - my
suggestions actually needed some further refinement, and getting the
HIGHPTE case of the other patch to build is actually non-trivial.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel