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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Tue, Sep 25, 2012 at 06:28:48PM -0700, Mukesh Rathor wrote:
> On Mon, 24 Sep 2012 15:24:16 +0100
> Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx> 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?
> 

Make sure you have a comment explaining it in here. In 3 months
neither you or me are going to recall why this is there.

Can the 1 become a #define?

> 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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel