Re: [PATCH v6 07/19] xen/passthrough: Introduce iommu_construct

[Manish Jaggi <mjaggi@caviumnetworks.com>]

On 28/04/15 8:02 pm, Julien Grall wrote:
> From: Julien Grall <julien.grall@linaro.org>
>
> 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 <julien.grall@linaro.org>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> ---
>      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);
>