WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-ia64-devel] Re: [XenPPC] RFC: xencomm - linux side

Le Mercredi 23 Août 2006 18:35, Hollis Blanchard a écrit :
> On Wed, 2006-08-23 at 09:59 +0200, Tristan Gingold wrote:
> > Le Mardi 22 Août 2006 21:03, Hollis Blanchard a écrit :
> > > I don't understand the point of all these routines if they just call
> > > arch_foo anyways.
> >
> > Sorry I have not explained the principle.
> > xencomm_arch_hypercall_XXX are the raw hypercalls.  They must be defined
> > by architecture code. The xencomm_hypercall_XXX are the xencomm wrapper
> > and are shared.
>
> That much is clear. :) My question is what is being done in those "raw
> hypercalls" that can't be done here? You didn't include them in your
> patch, so I can't tell.
>
> It seems there are a few missing pieces to your patch. Next time please
> include the whole thing, including arch-specific parts, so we can see
> what's going on.
>
> > > > +       if (ret)
> > > > +               goto out; /* error mapping the nested pointer */
> > > > +
> > > > +       ret = xencomm_arch_hypercall_dom0_op (op_desc);
> > > > +
> > > > +       /* FIXME: should we restore the handle?  */
> > > > +       if (copy_to_user(user_op, &kern_op, sizeof(dom0_op_t)))
> > > > +               ret = -EFAULT;
> > > > +
> > > > +       if (desc)
> > > > +               xencomm_free(desc);
> > > > +out:
> > > > +       return ret;
> > > > +}
> > >
> > > You misplaced the out label; it needs to go before xencomm_free(desc);
> >
> > ??? This was copied from your work.
>
> You've made changes here, and that's what I'm pointing out.
>
> > The code branches to out iff xencomm allocation failed.  It is safe to
> > call xencomm_free but useless.
>
> There are multiple descriptors being created: one for the dom0_op
> top-level structure, and possibly one for a sub-structure. In fact, in
> your patch you never free 'op_desc' inside xencomm_privcmd_dom0_op().
> OK, reading closer, I don't like that at *all*. The trick is that
> xencomm_create_inline() doesn't actually create anything, and therefore
> you don't need to free it. That needs to change.
>
> My suggestion: have xencomm_create() test IS_KERNEL_ADDR() (in whatever
> way is best for portability) and if it is, do the "inline" stuff. On the
> free side, if the descriptor was inline, free can just return. That
> would also make me happy because it removes the need to think about
> whether callers can/should call "create_inline" or not; the code just
> does the right thing.
We definitly disagree here.  One whole point of xencomm_create_inline is it 
doesn't allocate memory and can't fail.  Because of that we don't need to 
worry about failure and freeing memory.  This makes the code a lot easier to 
write and to read.

Tristan.

_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ia64-devel