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

[Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>]

On 06/02/2018 12:28 AM, Sameer Goel wrote:
> 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?
yes. thanks.
> 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

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