Re: [Xen-devel] [RFC 4/6] xen/passthrough/arm: Introduce iommu_fwspec

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 08/06/2017 20:30, 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 | 57 +++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/device.h        |  1 +
>  xen/include/xen/iommu.h             | 28 ++++++++++++++++++
>  3 files changed, 86 insertions(+)
>
> diff --git a/xen/drivers/passthrough/arm/iommu.c 
> b/xen/drivers/passthrough/arm/iommu.c
> index 95b1abb..edf70c2 100644
> --- a/xen/drivers/passthrough/arm/iommu.c
> +++ b/xen/drivers/passthrough/arm/iommu.c
> @@ -73,3 +73,60 @@ int arch_iommu_populate_page_table(struct domain *d)
>      /* The IOMMU shares the p2m with the CPU */
>      return -ENOSYS;
>  }
> +
> +int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
> +        const struct iommu_ops *ops)
If you put code in iommu.c then you need to respect the coding style of 
the file. So this should be correctly indented.
> +{
> +    struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +
> +    if (fwspec)
Coding style.

> +        return ops == fwspec->ops ? 0 : -EINVAL;
> +
> +    fwspec = xzalloc(struct iommu_fwspec);
> +    if (!fwspec)
Ditto.

> +        return -ENOMEM;
> +
> +    /* Ref counting for the dt device node is not needed */
> +
> +    /*of_node_get(to_of_node(iommu_fwnode));*/
I am a bit confused. You put this in a comment here and drop it in patch 
#5. So you probably want to drop here.
> +
> +    fwspec->iommu_fwnode = iommu_fwnode;
> +    fwspec->ops = ops;
> +    dev->iommu_fwspec = fwspec;
Newline here please.

> +    return 0;
> +}
> +
> +void iommu_fwspec_free(struct device *dev)
> +{
> +    struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +
> +    if (fwspec) {
Coding style.

> +        /*fwnode_handle_put(fwspec->iommu_fwnode);*/
And ditto for the comment.

> +        xfree(fwspec);
> +        dev->iommu_fwspec = NULL;
> +    }
> +}
> +
> +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
As you don't respect the Linux coding style (you are using soft-tab). I 
see little point to stay close to Linux for this code as it would be 
difficult to port anyway.
So s/u32/uint32_t/

> +{
> +    struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +    size_t size;
> +    int i;
> +
> +    if (!fwspec)
Coding style.

> +        return -EINVAL;
> +
> +    size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + num_ids]);
> +    if (size > sizeof(*fwspec)) {
Coding style.

> +        fwspec = _xrealloc(dev->iommu_fwspec, size, sizeof(void *));
If you introduce _xrealloc (I will leave the REST maintainers decide 
whether they want it), then please introduce xrealloc macro.
> +        if (!fwspec)
Coding style.

> +            return -ENOMEM;
> +    }
> +
> +    for (i = 0; i < num_ids; i++)
Coding style.

> +        fwspec->ids[fwspec->num_ids + i] = ids[i];
> +
> +    fwspec->num_ids += num_ids;
> +    dev->iommu_fwspec = fwspec;
newline.

> +    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 5803e3f..7ef9b93 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -224,4 +224,32 @@ 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_64) || defined(CONFIG_ARM)
No ifdef ARCH in xen/* headers. If this is only used by ARM, then it 
should go 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);
> +
> +#endif
>  #endif /* _IOMMU_H_ */
Cheers,

--
Julien Grall

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