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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.

Ian.


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