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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.05.17 at 20:06, <julien.grall@xxxxxxx> wrote:
> Hi Oleksandr,
> 
> 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).
> 
> So this does not hold to me. Maybe Jan (IOMMU maintainer) has a 
> different view on it.

Well, I have to admit that I don't really understand the need for
this new callback. But it looks like in a later reply Oleksandr moves
to agreeing with you to drop this new hook. As it's ARM-specific,
I'll leave this to you for the moment.

Jan



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