Re: [Xen-devel] [RFC v2 3/7] xen/passthrough/arm: Introduce iommu_fwspec

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi,

On 21/09/17 01:37, Sameer Goel wrote:
> Introduce a common structure to hold the fw (ACPI or DT) defined
> configuration for SMMU hw. The current use case is for arm SMMUs. So,
> making this architecture specific.
>
> Based on Linux kernel commit 57f98d2f61e1: iommu: Introduce iommu_fwspec
> Signed-off-by: Sameer Goel <sgoel@xxxxxxxxxxxxxx>
> ---
>   xen/drivers/passthrough/arm/iommu.c | 66 +++++++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/device.h        |  1 +
>   xen/include/xen/iommu.h             | 29 ++++++++++++++++
>   3 files changed, 96 insertions(+)
>
> diff --git a/xen/drivers/passthrough/arm/iommu.c 
> b/xen/drivers/passthrough/arm/iommu.c
> index 95b1abb..41c6497 100644
> --- a/xen/drivers/passthrough/arm/iommu.c
> +++ b/xen/drivers/passthrough/arm/iommu.c
> @@ -73,3 +73,69 @@ int arch_iommu_populate_page_table(struct domain *d)
>       /* The IOMMU shares the p2m with the CPU */
>       return -ENOSYS;
>   }
> +
> +const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
> +{
> +    return iommu_get_ops();
Can you please add a comment explain why you always return iommu_get_ops()?

Would it be possible that the device is not behind an IOMMU?

> +}
> +
> +int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
> +        const struct iommu_ops *ops)
> +{
> +    struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +
> +    if ( fwspec )
> +        return ops == fwspec->ops ? 0 : -EINVAL;
> +
> +    fwspec = _xzalloc(sizeof(struct iommu_fwspec), sizeof(void *));
On the previous version this was xzalloc(struct iommu_fwspec), why?

I also don't understand the align on sizeof(void *).

> +    if ( !fwspec )
> +        return -ENOMEM;
> +
> +    fwspec->iommu_fwnode = iommu_fwnode;
> +    fwspec->ops = ops;
> +    dev->iommu_fwspec = fwspec;
> +
> +    return 0;
> +}
> +
> +void iommu_fwspec_free(struct device *dev)
> +{
> +    struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +
> +    if ( fwspec )
> +    {
Linux is dropping the reference on the iommu_fwnode. Are we never 
expecting to take reference on the it in Xen?
> +        xfree(fwspec);
> +        dev->iommu_fwspec = NULL;
> +    }
> +}
> +
> +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
> +{
> +    struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +    struct iommu_fwspec *fwspec_n = NULL;
> +    size_t size, size_n;
> +    int i;
> +
> +    if ( !fwspec )
> +        return -EINVAL;
> +
> +    size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids]);
> +    size_n = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + num_ids]);
> +    if ( size_n > size )
> +    { > +        fwspec_n = _xzalloc(size_n, sizeof(void *));
Same question about _xzalloc() here.

> +        if ( !fwspec_n )
> +            return -ENOMEM;
> +
> +        memcpy(fwspec_n, fwspec, size);
> +        xfree(fwspec);
> +    }
> +
> +    for (i = 0; i < num_ids; i++)
> +        fwspec_n->ids[fwspec_n->num_ids + i] = ids[i];
> +
> +    fwspec_n->num_ids += num_ids;
> +    dev->iommu_fwspec = fwspec_n;
> +
> +    return 0;
> +}
> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
> index 78c38fe..5027c87 100644
> --- a/xen/include/asm-arm/device.h
> +++ b/xen/include/asm-arm/device.h
> @@ -21,6 +21,7 @@ struct device
>       struct dt_device_node *of_node; /* Used by drivers imported from Linux */
>   #endif
>       struct fwnode_handle *fwnode; /*fw device node identifier */
> +    struct iommu_fwspec *iommu_fwspec;
>       struct dev_archdata archdata;
>   };
>   
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 0dac4f3..34e8d68 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -208,4 +208,33 @@ DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
>   extern struct spinlock iommu_pt_cleanup_lock;
>   extern struct page_list_head iommu_pt_cleanup_list;
>   
> +/**
> + * Following block was ported from Linux to help with the implementation of
> + * arm64 iommu devices. Hence the architecture specific compile
> + */
> +
> +#if defined(CONFIG_ARM)
If it is Arm only, then it should be moved in asm-arm/iommu.h.

> +/**
> + * struct iommu_fwspec - per-device IOMMU instance data
> + * @ops: ops for this device's IOMMU
> + * @iommu_fwnode: firmware handle for this device's IOMMU
> + * @iommu_priv: IOMMU driver private data for this device
> + * @num_ids: number of associated device IDs
> + * @ids: IDs which this device may present to the IOMMU
> + */
> +struct iommu_fwspec {
> +    const struct iommu_ops *ops;
> +    struct fwnode_handle   *iommu_fwnode;
> +    void                   *iommu_priv;
> +    unsigned int           num_ids;
> +    u32                    ids[1];
> +};
> +
> +int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
> +                     const struct iommu_ops *ops);
> +void iommu_fwspec_free(struct device *dev);
> +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
> +const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode);
> +
> +#endif
>   #endif /* _IOMMU_H_ */
Cheers,

--
Julien Grall

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