Re: [Xen-devel] [PATCH v2] CONFIG_XEN_PV breaks xen_create_contiguous_region on ARM

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 26 Oct 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 10/26/18 7:04 PM, Stefano Stabellini wrote:
> > From: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > 
> > xen_create_contiguous_region has now only an implementation if
> > CONFIG_XEN_PV is defined. However, on ARM we never set CONFIG_XEN_PV but
> > we do have an implementation of xen_create_contiguous_region which is
> > required for swiotlb-xen to work correctly (although it just sets
> > *dma_handle).
> > 
> > Add a stub implementation of xen_remap_pfn: it should never be called on
> > ARM but it is necessary for linking.
> > 
> > This fixes a bug introduced by 16624390816c4c40df3d777b34665d3fd01e693d.
> 
> Again, this should contain a tag "Fixes: sha1 ("commit title")" so it can be
> picked for backporting automatically.

Yeah, I forgot about it, it should be definitely added.


> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > CC: Jeff.Kubascik@xxxxxxxxxxxxxxx
> > CC: Jarvis.Roach@xxxxxxxxxxxxxxx
> > CC: Nathan.Studer@xxxxxxxxxxxxxxx
> > CC: vkuznets@xxxxxxxxxx
> > CC: boris.ostrovsky@xxxxxxxxxx
> > CC: jgross@xxxxxxxx
> > ---
> >   arch/arm/xen/mm.c     | 8 ++++++++
> >   include/xen/xen-ops.h | 2 +-
> >   2 files changed, 9 insertions(+), 1 deletion(-)
> > ---
> > Changes in v2:
> > - improve commit message
> > - add xen_remap_pfn stub implementation
> > - use if defined()
> > diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> > index 785d2a5..7230e22 100644
> > --- a/arch/arm/xen/mm.c
> > +++ b/arch/arm/xen/mm.c
> > @@ -182,6 +182,14 @@ void xen_destroy_contiguous_region(phys_addr_t pstart,
> > unsigned int order)
> >   }
> >   EXPORT_SYMBOL_GPL(xen_destroy_contiguous_region);
> >   +int xen_remap_pfn(struct vm_area_struct *vma, unsigned long addr,
> > +             xen_pfn_t *pfn, int nr, int *err_ptr, pgprot_t prot,
> > +             unsigned int domid, bool no_translate, struct page **pages)
> > +{
> > +   return -EOPNOTSUPP;
> > +}
> > +EXPORT_SYMBOL_GPL(xen_remap_pfn);
> 
> This does not match the "unimplemented" version in xen-ops.h.
> 
> But I find this solution quite ugly. Why don't you split the #ifdef in two
> below?

I am happy to follow the maintainers' opinion on this. I would keep
those those definition together but I don't mind either way.


Juergen, Boris,

What do you prefer?


> > +
> >   const struct dma_map_ops *xen_dma_ops;
> >   EXPORT_SYMBOL(xen_dma_ops);
> >   diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> > index 18803ff..765dd05 100644
> > --- a/include/xen/xen-ops.h
> > +++ b/include/xen/xen-ops.h
> > @@ -42,7 +42,7 @@ int xen_setup_shutdown_event(void);
> >     extern unsigned long *xen_contiguous_bitmap;
> >   -#ifdef CONFIG_XEN_PV
> > +#if defined(CONFIG_XEN_PV) || defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> >   int xen_create_contiguous_region(phys_addr_t pstart, unsigned int order,
> >                             unsigned int address_bits,
> >                             dma_addr_t *dma_handle);
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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