Re: [Xen-devel] [v2 4/6] xen/iommu: smmu-v3: Add Xen specific code to enable the ported driver

[Sameer Goel <sameer.goel@xxxxxxxxxx>]

On 5/31/2018 11:16 PM, Manish Jaggi wrote:
>
>
> On 05/31/2018 09:27 PM, Sameer Goel wrote:
>>
>> On 5/30/2018 10:13 PM, Manish Jaggi wrote:
>>>
>>> On 05/31/2018 04:31 AM, Sameer Goel wrote:
>>>>>>>> +
>>>>>>>> +static int arm_smmu_iommu_domain_init(struct domain *d)
>>>>>>> Where is iommu_domain initialized?
>>>>>>> The function does not use a iommu_domain * variable
>>>> Please check iommu.c 2 levels up.
>>> In this function do you see iommu_domain getting allocated or initialized?
>>> As per the name of function arm_smmu iommu_domain_init.
>>> Where is init of iommu_domain in this function?
>> Well without the xen_domain the iommu_domain is not initialized. It is just 
>> the default value. This generic iommu code makes an .init call to our code 
>> for whatever initialization is needed. So the name here seemed absolutely 
>> fine to me.
>>
>> Initialization does not always refer to allocation. In this case this is 
>> driver specific initialization. Since, the iommu code is making an init call 
>> to the smmu code hence the name arm_smmu_iommu_domain_init. So, again I 
>> agree with your comments on the domain variable names and I'm making these 
>> changes as they would make the code cleaner. This function name change 
>> probably will not do much but the move along the discussion, let me know 
>> what you were thinking.
> Sameer, few points
> a. all the functions are prefixed with arm_smmu_ , so what the function is 
> doing can be understood by the rest part of the name
> In this case it is iommu_domain_init.
>
> b. By the name it seems to suggest that you are  doing some kind of init for 
> iommu_domain
>
> c. But in this complete function, iommu_domain pointer is never used.
>
> If I take your point, the appropriate name of the function should be 
> arm_smmu_xen_domain_init().
Its not the iommu domain defined within the current driver.  I'll change the 
name to arm_smmu_xen_iommu_init().

arm_smmu for just keeping the prefixes as needed. Sounds good?

Thanks,
Sameer
>
> -Manish
>>> +static int arm_smmu_iommu_domain_init(struct domain *d)
>>> +{
>>> +    struct arm_smmu_xen_domain *xen_domain;
>>> +
>>> +    xen_domain = xzalloc(struct arm_smmu_xen_domain);
>>> +    if (!xen_domain)
>>> +        return -ENOMEM;
>>> +
>>> +    spin_lock_init(&xen_domain->lock);
>>> +    INIT_LIST_HEAD(&xen_domain->contexts);
>>> +
>>> +    dom_iommu(d)->arch.priv = xen_domain;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>
>>>
>>>
>>>> Thanks,
>>>> Sameer
>>>>> _______________________________________________
>>>>> Xen-devel mailing list
>>>>> Xen-devel@xxxxxxxxxxxxxxxxxxxx
>>>>> https://lists.xenproject.org/mailman/listinfo/xen-devel
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@xxxxxxxxxxxxxxxxxxxx
>>> https://lists.xenproject.org/mailman/listinfo/xen-devel
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel


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