Re: [Xen-devel] [PATCH 10/10] xen: arm: Use a direct mapping of RAM on arm64

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Thu, 2013-06-20 at 17:43 +0100, Stefano Stabellini wrote:
> On Tue, 18 Jun 2013, Ian Campbell wrote:
> > On Tue, 2013-06-18 at 15:05 +0100, Julien Grall wrote:
> > 
> > Please could you trim your quotes.
> > 
> > > On 06/18/2013 02:26 PM, Ian Campbell wrote:
> > > [...]
> > > > +#ifdef CONFIG_ARM_32
> > > >  static inline void *maddr_to_virt(paddr_t ma)
> > > >  {
> > > >      ASSERT(is_xen_heap_mfn(ma >> PAGE_SHIFT));
> > > >      ma -= pfn_to_paddr(xenheap_mfn_start);
> > > >      return (void *)(unsigned long) ma + XENHEAP_VIRT_START;
> > > >  }
> > > > +#else
> > > > +static inline void *maddr_to_virt(paddr_t ma)
> > > > +{
> > > > +    ASSERT((ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
> > > > +    ma -= pfn_to_paddr(xenheap_mfn_start);
> > > > +    return (void *)(unsigned long) ma + DIRECTMAP_VIRT_START;
> > > > +}
> > > > +#endif
> > > 
> > > 
> > > I'm curious, this is not specific to this patch, why don't you split the
> > > file in 3 parts (common, arm32, arm64)?
> > 
> > In this specific instance I figured it was better to have this one
> > function which differed alongside all the other foo_to_bar
> > (virt_to_page, gvirt_to_maddr etc) which don't.
> > 
> > > From what I've seen, we use the both way on Xen. I think it's clearer to
> > > divide the code in different headers/sources files. Sometimes defines
> > > are hard to follow.
> > 
> > I'm not sure that putting them in different files makes them easier to
> > follow.
> > 
> > I'm not really sure what is best. Obviously if something is totally
> > different on the two different architectures (e.g. sysregs, tlbs etc)
> > then splitting them up makes sense. When it's just one function among
> > many which differs then I'm not so sure splitting is useful.
> > 
> > I don't have a strong opinion though.
> 
> From a quick look it seems to me that most functions have #ifdefs now.
> The code would probably improve by having separate arm32 and
> arm64 functions.

Are we still talking about xen/include/asm-arm/mm.h? Because there's
only around 20 lines under two ifdefs in that 350+ line file.

is_xen_heap_page, is_xen_heap_mfn and maddr_to_virt are ifdeffed but all
the other stuff is common. I'd rather keep maddr_to_virt next to all the
other foo_to_bar functions.

Ian.


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