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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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.

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