| Message ID | 1494424994-26232-8-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Oleksandr, On 10/05/17 15:03, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > 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@epam.com> > CC: Jan Beulich <jbeulich@suse.com> > > --- > 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); >
On Thu, May 11, 2017 at 2:38 PM, Julien Grall <julien.grall@arm.com> wrote: > Hi Oleksandr, Hi, Julien > > On 10/05/17 15:03, Oleksandr Tyshchenko wrote: >> >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> 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@epam.com> >> CC: Jan Beulich <jbeulich@suse.com> >> >> --- >> 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
Hi Oleksandr, On 11/05/17 15:00, Oleksandr Tyshchenko wrote: > On Thu, May 11, 2017 at 2:38 PM, Julien Grall <julien.grall@arm.com> wrote: >> Hi Oleksandr, > Hi, Julien > >> >> On 10/05/17 15:03, Oleksandr Tyshchenko wrote: >>> >>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> >>> 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,
On Thu, May 11, 2017 at 9:06 PM, Julien Grall <julien.grall@arm.com> 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@arm.com> >> wrote: >>> >>> Hi Oleksandr, >> >> Hi, Julien >> >>> >>> On 10/05/17 15:03, Oleksandr Tyshchenko wrote: >>>> >>>> >>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> >>>> 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
>>> On 11.05.17 at 20:06, <julien.grall@arm.com> 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@arm.com> wrote: >>> Hi Oleksandr, >> Hi, Julien >> >>> >>> On 10/05/17 15:03, Oleksandr Tyshchenko wrote: >>>> >>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> >>>> 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
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);