Re: [Xen-devel] [PATCH] xen/arm: bug report: mfn_valid checking

[Jaeyong Yoo <jaeyong.yoo@xxxxxxxxxxx>]

> -----Original Message-----
> From: xen-devel-bounces@xxxxxxxxxxxxx [mailto:xen-devel-
> bounces@xxxxxxxxxxxxx] On Behalf Of Ian Campbell
> Sent: Friday, August 23, 2013 5:17 PM
> To: Jaeyong Yoo
> Cc: xen-devel@xxxxxxxxxxxxx
> 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@xxxxxxxxxxx>
> > ---
> >  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) ?

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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel