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

[Julien Grall <julien.grall@xxxxxxx>]

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.
> 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?
> +
>   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