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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 4/15/19 11:25 PM, Stefano Stabellini wrote:
> 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
I should have probably said that I haven't done that in the patch 
because adding the mfn_x(...) is breached the 80-characters limit. So we 
would need to split the line.
I don't really think this will make easier to read the code in this context.

I am curious to know how this would make it better.

Cheers,

--
Julien Grall

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