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

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On Thu, May 11, 2017 at 2:38 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hi Oleksandr,
Hi, Julien

>
> 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?
I was thinking about extra need_iommu argument for init platform
callback as I had done for iommu_domain_init API.
But I had doubts regarding hw_domain. During iommu_domain_init
execution we haven't known yet is the IOMMU expected for domain 0
or not.

Taking into account that I needed to:
- populate page table followed by setting need_iommu flag.
- implement arch_iommu_populate_page_table() on ARM because of
!iommu_use_hap_pt(d).
- find a solution for hw_domain.

I decided to use iommu_construct() and implement alloc_page_table
callback to be called for populating page table.
I thought that it would allow us to keep all required actions in a
single place rather than spreading.

>
>
>>
>> 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



-- 
Regards,

Oleksandr Tyshchenko

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