From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [PATCH v1 7/8]: PVH privcmd changes Date: Tue, 25 Sep 2012 18:28:48 -0700 Message-ID: <20120925182848.52978702@mantra.us.oracle.com> References: <20120921122123.33489ce1@mantra.us.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: "Xen-devel@lists.xensource.com" , Ian Campbell , Konrad Rzeszutek Wilk List-Id: xen-devel@lists.xenproject.org On Mon, 24 Sep 2012 15:24:16 +0100 Stefano Stabellini 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.