From mboxrd@z Thu Jan 1 00:00:00 1970 From: Manish Jaggi Subject: Re: [PATCH v6 07/19] xen/passthrough: Introduce iommu_construct Date: Wed, 29 Apr 2015 05:37:50 +0530 Message-ID: <554020D6.3070102@caviumnetworks.com> References: <1430231563-25648-1-git-send-email-julien.grall@citrix.com> <1430231563-25648-8-git-send-email-julien.grall@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YnFY0-0000hs-DG for xen-devel@lists.xenproject.org; Wed, 29 Apr 2015 00:08:16 +0000 In-Reply-To: <1430231563-25648-8-git-send-email-julien.grall@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall , xen-devel@lists.xenproject.org Cc: tim@xen.org, Julien Grall , stefano.stabellini@citrix.com, ian.campbell@citrix.com, Jan Beulich List-Id: xen-devel@lists.xenproject.org On 28/04/15 8:02 pm, Julien Grall wrote: > From: Julien Grall > > This new function will correctly initialize the IOMMU page table for the > current domain. > > Also use it in iommu_assign_dt_device even though the current IOMMU > implementation on ARM shares P2M with the processor. > > Signed-off-by: Julien Grall > Acked-by: Jan Beulich > Acked-by: Ian Campbell > > --- > Changes in v5: > - Limit the scope of rc > - Add Jan's and Ian's ack > > Changes in v4: > - Move memory_type_changed in iommu_construct. Added by commit > 06ed8cc "x86: avoid needless EPT table ajustment and cache > flush" > - And an ASSERT and a comment in iommu_assign_dt_device to > explain why the call is safe for DOM0 > > Changes in v3: > - The ASSERT in iommu_construct was redundant with the if () > - Remove d->need_iommu = 1 in assign_device has it's already > done by iommu_construct. > - Simplify the code in the caller of iommu_construct > > Changes in v2: > - Add missing Signed-off-by > - Rename iommu_buildup to iommu_construct > --- > xen/drivers/passthrough/arm/iommu.c | 6 ++++++ > xen/drivers/passthrough/device_tree.c | 12 ++++++++++++ > xen/drivers/passthrough/iommu.c | 26 ++++++++++++++++++++++++++ > xen/drivers/passthrough/pci.c | 22 ++++------------------ > xen/include/xen/iommu.h | 2 ++ > 5 files changed, 50 insertions(+), 18 deletions(-) > > diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c > index 3007b99..9234657 100644 > --- a/xen/drivers/passthrough/arm/iommu.c > +++ b/xen/drivers/passthrough/arm/iommu.c > @@ -68,3 +68,9 @@ void arch_iommu_domain_destroy(struct domain *d) > { > iommu_dt_domain_destroy(d); > } > + > +int arch_iommu_populate_page_table(struct domain *d) > +{ > + /* The IOMMU shares the p2m with the CPU */ > + return -ENOSYS; > +} > diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c > index 377d41d..4d82a09 100644 > --- a/xen/drivers/passthrough/device_tree.c > +++ b/xen/drivers/passthrough/device_tree.c > @@ -41,6 +41,18 @@ int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev) > if ( !list_empty(&dev->domain_list) ) > goto fail; > > + if ( need_iommu(d) <= 0 ) > + { > + /* > + * The hwdom is forced to use IOMMU for protecting assigned > + * device. Therefore the IOMMU data is already set up. > + */ > + ASSERT(!is_hardware_domain(d)); > + rc = iommu_construct(d); > + if ( rc ) > + goto fail; > + } > + > rc = hd->platform_ops->assign_device(d, 0, dt_to_dev(dev)); > > if ( rc ) > diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c > index 92ea26f..ae42aae 100644 > --- a/xen/drivers/passthrough/iommu.c > +++ b/xen/drivers/passthrough/iommu.c > @@ -187,6 +187,32 @@ void iommu_teardown(struct domain *d) > tasklet_schedule(&iommu_pt_cleanup_tasklet); > } > > +int iommu_construct(struct domain *d) > + What is the meaning of construct here. How it is different from init ? Also iommu refers to iommu_domain or there is some other datastructure which is being updated ? Please write function header comments or use similar naming prefix in code. > + if ( need_iommu(d) > 0 ) > + return 0; > + > + if ( !iommu_use_hap_pt(d) ) > + { > + int rc; > + > + rc = arch_iommu_populate_page_table(d); > + if ( rc ) > + return rc; > + } > + > + d->need_iommu = 1; > + /* > + * There may be dirty cache lines when a device is assigned > + * and before need_iommu(d) becoming true, this will cause > + * memory_type_changed lose effect if memory type changes. > + * Call memory_type_changed here to amend this. > + */ > + memory_type_changed(d); > + > + return 0; > +} > + > void iommu_domain_destroy(struct domain *d) > { > struct hvm_iommu *hd = domain_hvm_iommu(d); > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > index 9f3413c..af26423 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -1358,25 +1358,11 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn) > if ( !spin_trylock(&pcidevs_lock) ) > return -ERESTART; > > - if ( need_iommu(d) <= 0 ) > + rc = iommu_construct(d); > + if ( rc ) > { > - if ( !iommu_use_hap_pt(d) ) > - { > - rc = arch_iommu_populate_page_table(d); > - if ( rc ) > - { > - spin_unlock(&pcidevs_lock); > - return rc; > - } > - } > - d->need_iommu = 1; > - /* > - * There may be dirty cache lines when a device is assigned > - * and before need_iommu(d) becoming true, this will cause > - * memory_type_changed lose effect if memory type changes. > - * Call memory_type_changed here to amend this. > - */ > - memory_type_changed(d); > + spin_unlock(&pcidevs_lock); > + return rc; > } > > pdev = pci_get_pdev_by_domain(hardware_domain, seg, bus, devfn); > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h > index bf4aff0..e9d2d5c 100644 > --- a/xen/include/xen/iommu.h > +++ b/xen/include/xen/iommu.h > @@ -65,6 +65,8 @@ int arch_iommu_domain_init(struct domain *d); > int arch_iommu_populate_page_table(struct domain *d); > void arch_iommu_check_autotranslated_hwdom(struct domain *d); > > +int iommu_construct(struct domain *d); > + > /* Function used internally, use iommu_domain_destroy */ > void iommu_teardown(struct domain *d); >