Re: [PATCH v1 2/8]: PVH mmu changes

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Thu, 4 Oct 2012 09:27:59 +0100
Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Wed, 2012-10-03 at 23:29 +0100, Mukesh Rathor wrote:
> > On Wed, 3 Oct 2012 16:42:43 +0100
> > Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > 
> > > 
> > > >  int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
> > > >  			       unsigned long addr,
> > > >  			       unsigned long mfn, int nr,
> > > > -			       pgprot_t prot, unsigned domid);
> > > > +			       pgprot_t prot, unsigned domid,
> > > > +			       struct xen_pvh_pfn_info *pvhp);
> > > > +int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
> > > > +			       struct xen_pvh_pfn_info *pvhp);
> > > > +
> > > > +struct xen_pvh_pfn_info {
> > > 
> > > Can we call this xen_remap_mfn_info or something? PVH is x86
> > > specific while this struct is also useful on ARM.
> > 
> > I already renamed it to: xen_xlat_pfn_info.
> 
> What does xlat refer to here? I don't think we translate anything with
> this struct.

paging mode xlate! See stefanno's prev email where he suggested changing
name to this.


> > > > +	struct page **pi_paga;		/* pfn info page
> > > > array */
> > > 
> > > can we just call this "pages"? paga is pretty meaningless.
> > 
> > page array! i can rename page_array or page_a.
> 
> What's wrong with pages? It's short (some of the lines using this
> stuff are necessarily pretty long) and obvious.

grep'ing pages would give thousands results. I can prefix with something
and use pages. 


> > > > +	int 	      pi_num_pgs;
> > > > +	int 	      pi_next_todo;
> > > 
> > > I don't think we need the pi_ prefix for any of these.
> > 
> > The prefix for fields in struct make it easy to find via cscope or
> > grep, otherwise, it's a nightmare to find common field names like
> > pages when reading code. I really get frustrated. I prefer prefixing
> > all field names.
> 
> It's not common practice in Linux to do so but fair enough.
> 
> While implementing the ARM version of this interface it occurred to me
> that the num_pgs and next_todo fields here are not really needed
> across the arch interface e.g. you already get pi_num_pgs from the nr
> argument to xen_remap_domain_mfn_range and pi_next_todo is really
> state which fits in struct pvh_remap_data. That would mean that
> remap_foo could take a struct page * directly and I think you would
> save an allocation.

Ok, let me explore this.

thanks
Mukesh