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

["Goel, Sameer" <sgoel@xxxxxxxxxxxxxx>]

On 10/12/2017 7:05 AM, Julien Grall wrote:
> 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?
That is true. I will fix this.
> 
>> +}
>> +
>> +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 *).
Forgot why I did this. I will change it back. I just copied the alignment value 
from xzalloc.
> 
>> +    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?
I will fix this.
> 
>> +        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.
> 
See above.
>> +        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.
Will do.
> 
>> +/**
>> + * 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,
> 

-- 
 Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, 
Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

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