Re: [Xen-devel] [PATCH for-next 1/9] xen/arm: Use mfn_to_pdx instead of pfn_to_pdx when possible

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 15 Apr 2019, Julien Grall wrote:
> Hi,
> 
> On 4/15/19 10:55 PM, Stefano Stabellini wrote:
> > On Mon, 18 Feb 2019, Julien Grall wrote:
> > > mfn_to_pdx adds more safety than pfn_to_pdx. Replace all but on place in
> > > the Arm code to use the former.
> > 
> > This is good but maybe we can go even further.
> > 
> > You should also be able to replace one call site of pfn_to_pdx in
> > mfn_valid and the one in maddr_to_virt. Something like this:
> > 
> > 
> > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> > index eafa26f..b3455ea 100644
> > --- a/xen/include/asm-arm/mm.h
> > +++ b/xen/include/asm-arm/mm.h
> > @@ -209,7 +209,7 @@ static inline void __iomem *ioremap_wc(paddr_t start,
> > size_t len)
> >   /* XXX -- account for base */
> >   #define mfn_valid(mfn)        ({
> > \
> >       unsigned long __m_f_n = mfn_x(mfn);
> > \
> > -    likely(pfn_to_pdx(__m_f_n) >= frametable_base_pdx &&
> > __mfn_valid(__m_f_n)); \
> > +    likely(mfn_to_pdx(mfn) >= frametable_base_pdx && __mfn_valid(__m_f_n));
> > \
> 
> This is quite undesirable, you will end up to evaluate mfn twice here.

Yes you are right


> The other solution is to turn _m_f_n to an mfn_t but then it does make much
> difference as you would need to use a mfn_x(mfn) in the code.

I think that would be better


> >   })
> >     /* Convert between machine frame numbers and page-info structures. */
> > @@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma)
> >   #else
> >   static inline void *maddr_to_virt(paddr_t ma)
> >   {
> > -    ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
> > +    ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
> >       return (void *)(XENHEAP_VIRT_START -
> >                       mfn_to_maddr(xenheap_mfn_start) +
> >                       ((ma & ma_va_bottom_mask) |
> > 
> 
> I fail to see what this chunk adds compare to the existing one...
> 
> > > @@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma)
> > >   #else
> > >   static inline void *maddr_to_virt(paddr_t ma)
> > >   {
> > > -    ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >>
> > > PAGE_SHIFT));
> > > +    ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >>
> > > PAGE_SHIFT));
> > >       return (void *)(XENHEAP_VIRT_START -
> > >                       mfn_to_maddr(xenheap_mfn_start) +
> > >                       ((ma & ma_va_bottom_mask) |
> ... here.

That's weird. I think `patch' didn't apply completely the patch to my
tree. Great you already have it.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel