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 9:06 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hi Oleksandr,
Hi Julien

>
>
> On 11/05/17 15:00, Oleksandr Tyshchenko wrote:
>>
>> 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.
>
>
> Looking at your patch #8, you always allocate the page table for hardware
> domain, so this is very similar to set iommu_enable in
> xen_arch_domain_config (see config. in start_xen).
Indeed.

I allocate page table for hw_domain only if need_iommu flag is set.
The need_iommu flag depends on iommu_dom0_strict.
We force iommu_dom0_strict to true.
This all means that we always use iommu for hw_domain)

So, you proposed to avoid new callback and allocate page table
directly in "init" callback?
This also means we have to add extra need_iommu argument to "init" callback.

>
> So this does not hold to me. Maybe Jan (IOMMU maintainer) has a different
> view on it.
>
> Cheers,
>
> --
> Julien Grall



-- 
Regards,

Oleksandr Tyshchenko

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