Re: [Xen-devel] [PATCH v1 07/10] iommu/arm: Add alloc_page_table platform callback

[Julien Grall <julien.grall@xxxxxxx>]

Hi Oleksandr,

On 10/05/17 15:03, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>
> The alloc_page_table callback is a mandatory thing
> for the IOMMUs that don't share page table with the CPU on ARM.
> The non-shared IOMMUs have to perform all required actions here
> to be ready to handle IOMMU mapping updates right after completing it.
>
> The arch_iommu_populate_page_table() seems an appropriate place
> to call newly created callback.
> Since we will only be here for the non-shared IOMMUs always
> return error if the callback wasn't implemented.
Why do you need a specific callback and not doing it directly in 
iommu_domain_init?
My take here is in the unshare case, we may want to have multiple set of 
page tables (e.g one per device). So this should be left at the 
discretion of the IOMMU itself.
Am I missing something?

> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
>
> ---
>    Changes in V1:
>       - Wrap callback in #ifdef CONFIG_ARM.
> ---
>  xen/drivers/passthrough/arm/iommu.c | 5 +++--
>  xen/include/xen/iommu.h             | 3 +++
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/xen/drivers/passthrough/arm/iommu.c 
> b/xen/drivers/passthrough/arm/iommu.c
> index 95b1abb..f132032 100644
> --- a/xen/drivers/passthrough/arm/iommu.c
> +++ b/xen/drivers/passthrough/arm/iommu.c
> @@ -70,6 +70,7 @@ void arch_iommu_domain_destroy(struct domain *d)
>
>  int arch_iommu_populate_page_table(struct domain *d)
>  {
> -    /* The IOMMU shares the p2m with the CPU */
> -    return -ENOSYS;
> +    const struct iommu_ops *ops = iommu_get_ops();
> +
> +    return ops->alloc_page_table ? ops->alloc_page_table(d) : -ENOSYS;
>  }
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 3afbc3b..f5914db 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -175,6 +175,9 @@ struct iommu_ops {
>                                    unsigned int flags);
>      int __must_check (*unmap_pages)(struct domain *d, unsigned long gfn,
>                                      unsigned int order);
> +#ifdef CONFIG_ARM
> +    int (*alloc_page_table)(struct domain *d);
> +#endif /* CONFIG_ARM */
>      void (*free_page_table)(struct page_info *);
>  #ifdef CONFIG_X86
>      void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, unsigned 
> int value);
--
Julien Grall

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