Re: [Xen-devel] [RFC v4 8/8] drivers/passthrough/arm: Refactor code for arm smmu drivers

[Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>]

On 01/16/2018 07:10 PM, Julien Grall wrote:
> Hi Manish,
>
> On 16/01/18 13:27, Manish Jaggi wrote:
> > On 01/16/2018 06:44 PM, Julien Grall wrote:
> > > On 16/01/18 12:40, Manish Jaggi wrote:
> > > > Hi Julien,
> > > Hi,
> > >
> > > > On 01/16/2018 02:11 AM, Julien Grall wrote:
> > > > > On 01/03/2018 05:34 AM, Manish Jaggi wrote:
> > > > > > Hi Sameer,
> > > > > Hi Manish,
> > > > >
> > > > > > > +    unsigned int type;
> > > > > > > +
> > > > > > > +    /* Dummy compatibility defines */
> > > > > > > +    unsigned long pgsize_bitmap;
> > > > > > > +    struct iommu_domain_geometry geometry;
> > > > > > > +
> > > > > > > +    atomic_t ref;
> > > > > > > +    /* Used to link iommu_domain contexts for a same domain.
> > > > > > > +     * There is at least one per-SMMU to used by the domain.
> > > > > > > +     */
> > > > > > > +    struct list_head        list;
> > > > > > > +};
> > > > > > > +
> > > > > > > +/* Xen: Describes information required for a Xen domain */
> > > > > > > +struct arm_smmu_xen_domain {
> > > > > > > +    spinlock_t          lock;
> > > > > > > +    /* List of iommu domains associated to this domain */
> > > > > > > +    struct list_head        contexts;
> > > > > > Could we use a more verbose name, How about 
> > > > > > %s/contexts/iommu_domain_contexts/g ?
> > > > > How about a much more verbose name...? This name is 21 letters and 
> > > > > that's only for the field. Just imagine with the variable name 
> > > > > before and a couple of indentation.
> > > > How about io_context? anything which makes it more verbose is ok 
> > > > with me.
> > > I much prefer to stick with "contexts".
> > I am not able to understand why to use contexts for a list which has 
> > iommu_domain pointers.
> Because the comment should have been read "iommu_domain context". As 
> it is in other description.
> > If you are ok with this logic, we can rename all iommu_domain pointer 
> > variables in this file as context.
> Why do you keep asking renaming when I clearly said multiple time that 
> we *should not* rename any code coming from Linux. This is breaking 
> the idea of keeping Xen and Linux SMMU driver close.
> If you don't want to get SMMUv3 close to Linux. Then fine, but it 
> means you have to use Xen coding style and dropping all necessary code.
> But I am afraid this is not the solution I (and AFAIK Stefano) prefer 
> because it adds burden on maintenance on Xen community.
I agree with the point that code imported from linux should be touched 
minimally, but If you look closely the problem is in naming of xen 
specific code that is added later.
Not in code imported from linux. So why we cant fix xen specific code 
naming?
Below are the few examples
a. static struct iommu_domain *arm_smmu_get_domain(struct domain *d,...)
This function use of domain is confusing.
This is not linux function,

a function named arm_smmu_get_domain takes a parameter of domain which 
represents a virtual machine and returns an iommu_domain pointer.
while the file still has a structure arm_smmu_domain.
In what way this function naming explains itself?

If you want to take a cue from linux see this code below
structarm_smmu_domain *smmu_domain = to_smmu_domain(domain); => The 
naming is quite clear and not confusing.
Also If you see there are few functions in xen specifc smmu code which 
are named correctly
Look at arm_smmu_iommu_domain_init(struct domain *d)

But arm_smmu_iommu_domain_init(struct domain *d) and arm_smmu_get_domain 
naming dont match,  they are both referring to iommu_domain
So my _two cents_ on the current use of structures / functions and 
variable names which use domain in xen specific code.
b. Similarly contexts in arm_smmu_xen_domain structure is not coming 
from linux code
c. arm_smmu_assign_dev is a xen specific function so it can be changed
d. Why id arm_smmu_xen_domain named the way it is. Does linux code 
include _linux_ in any structure name?
> Cheers,

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