Re: [Xen-devel] [PATCH 1 of 2] Use memops for mem paging, sharing, and access, instead of domctls

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2012-02-02 at 23:09 +0000, Andres Lagar-Cavilla wrote:
> > On Thu, 2012-02-02 at 20:23 +0000, Andres Lagar-Cavilla wrote:
> >> > On Thu, 2012-02-02 at 14:25 +0000, Andres Lagar-Cavilla wrote:
> >> >> > On Wed, 2012-02-01 at 20:09 +0000, Andres Lagar-Cavilla wrote:
> >> >> >>
> >> >> >>
> >> >> >> +int xc_mem_event_memop(xc_interface *xch, domid_t domain_id,
> >> >> >> +                        unsigned int op, unsigned int mode,
> >> >> >> +                        uint64_t gfn, void *buffer)
> >> >> >> +{
> >> >> >> +    xen_mem_event_op_t meo;
> >> >> >> +
> >> >> >> +    memset(&meo, 0, sizeof(meo));
> >> >> >> +
> >> >> >> +    meo.op      = op;
> >> >> >> +    meo.domain  = domain_id;
> >> >> >> +    meo.gfn     = gfn;
> >> >> >> +    meo.buffer  = (unsigned long) buffer;
> >> >> >
> >> >> > Hypercall arguments need to use the xc_hypercall_buffer stuff in
> >> order
> >> >> > to ensure that the memory is safe to access from a hypercall (in
> >> >> > particular that it is mlocked or similar)-- I don't see where that
> >> >> > happens for this buffer.
> >> >> >
> >> >> > meo itself is bounced by do_memory_op so that is OK.
> >> >> >
> >> >> > I was also a little suspicious of ring_addr and shared_addr in
> >> >> > xc_mem_event_control in this regard.
> >> >> >
> >> >> > Or does the mem sharing code make it's own arrangements for these
> >> >> sorts
> >> >> > of things somewhere?
> >> >>
> >> >> It's rather unclean. They do not get mlock'ed (and there is no sanity
> >> >> check to ensure they're page-aligned). They should follow the pattern
> >> >> established in xc_mem_paging_load. I'll get on it, probably a
> >> separate
> >> >> patch.
> >> >
> >> > If you are reworking this It Would Be Nice If (tm) you could make it
> >> use
> >> > xc_hypercall_buffer_alloc and friends to allocate and manage the
> >> buffer
> >> > while you are there e.g. the void *buffer above would become struct
> >> > xc_hypercall_buffer *buffer -- see xc_perfc_query for example.
> >>
> >> Took me a while to grok those hypercall buffer macros. Conclusion: that
> >> would need to change callers of paging_load (xenpaging in tree).
> >
> > Yes, in this case you probably want to bubble their use right up to the
> > top level rather than bouncing in the internals like we do for smaller
> > and.or more transient data.
> >
> >> Can we keep this separate? domctl interface also suffers from all these
> >> problems. domctl->memops switch is orthogonal to the buffer badness. I
> >> want to tackle one problem at a time.
> >
> > Sure, read "IWBNI" as "in the fullness of time" ;-)
> 
> Ok, trying to mb() what's needed now and what will take place in the
> fullness of time:
> - ring creation when initializing mem event is ugly, but cannot be fixed
> short of a kernel driver. I will however produce a patch now that at least
> mlock's these guys.

Sounds good.

mb() goes here ;-)

> - All libxc consumers of these interfaces should be updated to init these
> magic pages (and the paging load buffer) using xc_hypercall_buffer_alloc
> in their own code.

Correct, this can be done whenever though.

Ian.



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