Hello,
mfn_valid is comparing the mfn with max_page (i.e., maximum number
of pages). When frametable_base_mfn is non-zero, it does not give
right results. Should be comparing the mfn starting from the
frametable_base_mfn. Unless, mfn_valid does not check correctly,
and may lead to page fault in some places (especially, when
initializing heap at booting time).
Jaeyong
Signed-off-by: Jaeyong Yoo <jaeyong.yoo@samsung.com>
---
xen/include/asm-arm/mm.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 27284d0..c5b8367 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -189,7 +189,7 @@ static inline void __iomem *ioremap_wc(paddr_t start, size_t
len)
}
#define mfn_valid(mfn) ({ \
- unsigned long __m_f_n = (mfn); \
+ unsigned long __m_f_n = (mfn - frametable_base_mfn); \
likely(__m_f_n < max_page);
\
})
--
1.8.1.2
On Fri, 2013-08-23 at 13:29 +0900, Jaeyong Yoo wrote:> Hello, > > mfn_valid is comparing the mfn with max_page (i.e., maximum number > of pages). When frametable_base_mfn is non-zero, it does not give > right results. Should be comparing the mfn starting from the > frametable_base_mfn. Unless, mfn_valid does not check correctly, > and may lead to page fault in some places (especially, when > initializing heap at booting time). > > Jaeyong > > Signed-off-by: Jaeyong Yoo <jaeyong.yoo@samsung.com> > --- > xen/include/asm-arm/mm.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h > index 27284d0..c5b8367 100644 > --- a/xen/include/asm-arm/mm.h > +++ b/xen/include/asm-arm/mm.h > @@ -189,7 +189,7 @@ static inline void __iomem *ioremap_wc(paddr_t start, size_t len) > } > > #define mfn_valid(mfn) ({ \ > - unsigned long __m_f_n = (mfn); \ > + unsigned long __m_f_n = (mfn - frametable_base_mfn); \Does this do the right thing for invalid mfns which are below frametable_base_mfn? I suppose it will underflow to a value above max_page, which is wrong (and even if right it would be too subtle!)> likely(__m_f_n < max_page); \max_page is the PFN of the highest RAM address, not its offset into the frametable, whereas with your change __m_f_n is the offset. Is the right fix: unsigned long __m_f_n = (mfn); likely(__m_f_n >= frametable_base_mfn && __m_f_n < max_page) ? Ian.
> -----Original Message----- > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel- > bounces@lists.xen.org] On Behalf Of Ian Campbell > Sent: Friday, August 23, 2013 5:17 PM > To: Jaeyong Yoo > Cc: xen-devel@lists.xen.org > Subject: Re: [Xen-devel] [PATCH] xen/arm: bug report: mfn_valid checking > > On Fri, 2013-08-23 at 13:29 +0900, Jaeyong Yoo wrote: > > Hello, > > > > mfn_valid is comparing the mfn with max_page (i.e., maximum number of > > pages). When frametable_base_mfn is non-zero, it does not give right > > results. Should be comparing the mfn starting from the > > frametable_base_mfn. Unless, mfn_valid does not check correctly, and > > may lead to page fault in some places (especially, when initializing > > heap at booting time). > > > > Jaeyong > > > > Signed-off-by: Jaeyong Yoo <jaeyong.yoo@samsung.com> > > --- > > xen/include/asm-arm/mm.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h index > > 27284d0..c5b8367 100644 > > --- a/xen/include/asm-arm/mm.h > > +++ b/xen/include/asm-arm/mm.h > > @@ -189,7 +189,7 @@ static inline void __iomem *ioremap_wc(paddr_t > > start, size_t len) } > > > > #define mfn_valid(mfn) ({\> > - unsigned long __m_f_n = (mfn);\> > + unsigned long __m_f_n = (mfn - frametable_base_mfn); > \ > > Does this do the right thing for invalid mfns which are below > frametable_base_mfn? I suppose it will underflow to a value abovemax_page,> which is wrong (and even if right it would be too subtle!) > > > likely(__m_f_n < max_page);\> > max_page is the PFN of the highest RAM address, not its offset into the > frametable, whereas with your change __m_f_n is the offset. > > Is the right fix: > unsigned long __m_f_n = (mfn); > likely(__m_f_n >= frametable_base_mfn && __m_f_n < max_page) ?Yes, it is the right one. I thought max_page is the maximum number of pages, but it''s not. Max_page is the pfn of the RAM end. Jaeyong> > Ian. > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel