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

[Julien Grall <julien.grall@xxxxxxx>]

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.
Cheers,

--
Julien Grall

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