Re: [PATCH v1 7/8]: PVH privcmd changes

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

On Mon, 24 Sep 2012 15:24:16 +0100
Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:

> On Fri, 21 Sep 2012, Mukesh Rathor wrote:
> > ---
> >  drivers/xen/privcmd.c |   77
> > ++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed,
> > 70 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> > index ccee0f1..195d89f 100644
> > -				       st->vma->vm_page_prot,
> > st->domain) < 0) {
> > +	if (xen_remap_domain_mfn_range(vma, st->va & PAGE_MASK,
> > *mfnp, 1,
> > +				       vma->vm_page_prot,
> > st->domain, 
> > +				       pvhp) < 0) {
> >  		*mfnp |= 0xf0000000U;
> >  		st->err++;
> >  	}
> 
> I don't like that a parameter like "xen_pvh_pfn_info" has been added
> to a generic arch-agnostic function like xen_remap_domain_mfn_range.
> 
> If you need to pass more parameters to xen_remap_domain_mfn_range, it
> should be done in a cross-architecture way. In fact, keep in mind that
> privcmd.c compiles on ARM (and IA64?) as well.
> 
> I think that in this particular case you are using pvh to actually
> specify auto_translate_physmap, am I correct?
> Maybe we just need to rename xen_pvh_pfn_info to xen_xlat_pfn_info.

Ok, I renamed it. And for the API, as I mentioned in prev email,
I changed xen_remap_domain_mfn_range to have last parameter be 
called void *arch_specific_info.

> 
> > @@ -274,6 +284,40 @@ static int mmap_return_errors(void *data, void
> > +		return -ENOMEM;
> > +	}
> > +
> > +	rc = alloc_xenballooned_pages(numpgs, pvhp->pi_paga, 0);
> > +	if (rc != 0) {
> > +		pr_warn("%s Could not alloc %d pfns rc:%d\n",
> > __FUNCTION__, 
> > +			numpgs, rc);
> > +		kfree(pvhp->pi_paga);
> > +		kfree(pvhp);
> > +		return -ENOMEM;
> > +	}
> > +	pvhp->pi_num_pgs = numpgs;
> > +	BUG_ON(vma->vm_private_data != (void *)1);
> 
> what?

See privcmd_enforce_singleshot_mapping().

> >  
> > +	if (xen_pv_domain() &&
> > xen_feature(XENFEAT_auto_translated_physmap)) {
> > +		if ((ret=pvh_privcmd_resv_pfns(vma, m.num))) {
> 
> I would change this into:
> 
>     if ((ret = pvh_privcmd_resv_pfns(vma, m.num)) < 0) {
> 

OK.


> > +	count = xen_unmap_domain_mfn_range(vma, pvhp);
> > +	while (count--)
> > +		free_xenballooned_pages(1, &pvhp->pi_paga[count]);
> > +	kfree(pvhp->pi_paga);
> > +	kfree(pvhp);
> 
> So xen_remap_domain_mfn_range adds the mappings to the vma, while
> xen_unmap_domain_mfn_range does not remove them. I take that somehow
> this is done by the generic Linux code that calls into this function?

Correct. The kernel MMU seems to do all cleanup before calling
privcmd_close.