Re: [PATCH v3 10/13] x86: add iommu_op to enable modification of IOMMU mappings

[Wei Liu <wei.liu2@citrix.com>]

On Tue, Jul 17, 2018 at 02:38:13PM +0100, Paul Durrant wrote:
> +static int iommu_op_enable_modification(void)
> +{
> +    struct domain *currd = current->domain;
> +    struct domain_iommu *iommu = dom_iommu(currd);
> +    const struct iommu_ops *ops = iommu->platform_ops;
> +
> +    /* Has modification already been enabled? */
> +    if ( iommu->iommu_op_ranges )
> +        return 0;
> +
> +    /*
> +     * The IOMMU mappings cannot be modified if:
> +     * - the IOMMU is not enabled or,
> +     * - the current domain is dom0 and tranlsation is disabled or,
> +     * - HAP is enabled and the IOMMU shares the mappings.
> +     */
> +    if ( !iommu_enabled ||
> +         (is_hardware_domain(currd) && iommu_passthrough) ||
> +         iommu_use_hap_pt(currd) )
> +        return -EACCES;
> +
> +    /*
> +     * The IOMMU implementation must provide the lookup method if
> +     * modification of the mappings is to be supported.
> +     */
> +    if ( !ops->lookup_page )
> +        return -EOPNOTSUPP;
> +
> +    iommu->iommu_op_ranges = rangeset_new(currd, NULL, 0);
> +    if ( !iommu->iommu_op_ranges )
> +        return -ENOMEM;
> +
> +    currd->need_iommu = 0; /* Disable identity GFN mapping */

I think this flag currently serves two purposes: one is to indicate
whether there are resources allocated within xen, the other it indicate
whether it is in use (as suggested in the comment).

Suppose a domain gets all its devices deassigned after this call,
original iommu_teardown would have been called, but now it won't. The
iommu will still be torn down when it is destroyed, but this is a very
subtle change in behaviour.

IMHO we should clearly separate these two cases if possible.

> +    return 0;
> +}
> +
[...]
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index fb9d0e1848..c517428621 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -26,7 +26,6 @@ static void iommu_dump_p2m_table(unsigned char key);
>  
>  unsigned int __read_mostly iommu_dev_iotlb_timeout = 1000;
>  integer_param("iommu_dev_iotlb_timeout", iommu_dev_iotlb_timeout);
> -

Please keep the blank line here.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel