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

[Julien Grall <julien.grall@linaro.org>]

Hi,

On 12/10/17 14:05, 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@codeaurora.org>
>> ---
>>   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.

Also, please see the comment I just made on "[RFC 3/6] Introduce _xrealloc".

I would prefer to explore the possibility of a generic helper rather 
than open-coding it. I think we have enough information in hand to get 
the size of the old region.

Cheers,

-- 
Julien Grall

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