Xen devel - Feb 2006 - xencomm address space API

We were discussing buffer management for the xencomm address space that
we''d use to allocate memory to pass between userspace/kernel and
hypervisor. It''s needed in the kernel for hcalls made from the kernel
that currently contain pointers (in particular the balloon driver).

x86 wants to know about buffer (not memory) allocation and destruction,
since that''s when it needs to mlock/munlock. PPC wants to know about
buffer allocation so the buffer can be registered with the hypervisor.

Unfortunately, implementing a buffer registration API became rather
complicated.

Problem #1: concurrency. By decoupling buffer registration from the
actual hcall, we can introduce problems like these:

dom0 tool:		balloon driver:
register buffer
			register buffer
			hcall
			unregister buffer
hcall
unregister buffer

This means that the hypervisor must track multiple registered buffers
per domain. (In the general case this could be an arbitrary number, but
I guess it would need to be limited to prevent a domain from exhausting
the Xen heap.)

That also means that each hcall must somehow indicate which buffer
should be used with its arguments. I think that could be done by
encoding the buffer ID into the memory reference, necessitating an API
like this:
	bufid = alloc_buf(nr_pages)
		user: mmap anonymous page
		user: register page with kernel
		kernel: translate to phys and register with xen
		xen: returns bufid

	memref = alloc_mem(bufid, nr_bytes)
		user: ... some allocator code ...
		user: return bufid | memref

	struct.foo = memref

In Xen, copy_from_user(xenbuf, memref) would then decode memref to
figure out what buffer was being referred to. copy_from_user would then
need to understand the data structures used by userland to track the
memory references within the buffer.

Problem #2: Spanning pages is still really difficult. One possible
solution (different from above) would be to have the kernel reserve some
physically contiguous pages, and then export that area by having
userland mmap some device.

Problem #3: We need to know beforehand the maximum number of bytes
needed for the buffer.

Problem #4: The kernel must track the buffers that userland registered,
and unregister them when the process dies, since it may not have been
able to unregister them properly.

This mail isn''t comprehensive, but I think gives some idea of the
complexity involved. So a solution like replacing pointers with embedded
structures is far more attractive.

-- 
Hollis Blanchard
IBM Linux Technology Center


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

> This means that the hypervisor must track multiple registered buffers
> per domain. (In the general case this could be an arbitrary number, but
> I guess it would need to be limited to prevent a domain from exhausting
> the Xen heap.)
I would expect all registering to be done by the guest kernel. The 
guest kernel has to register pages of memory that belong to the guest, 
and specify a location in the hypercall address space to register them 
at. There is memory usage in Xen to store the translation from 
hypercall address to physical address, but the transfer memory itself 
is provided by the guest.
> That also means that each hcall must somehow indicate which buffer
> should be used with its arguments. I think that could be done by
> encoding the buffer ID into the memory reference, necessitating an API
> like this:
The alloc_buf() routine returns a handle. We provide a function for 
turning that into a pointer for the application/kernel to dereference. 
The handle is poked into hypercall structures/arguments where raw 
pointers would currently be passed.

By the way -- I mean that alloc_buf() is decoupled from registering 
hypercall memory. I expect the kernel to register a chunk of memory at 
start of day, and then run an allocator over that chunk. Only register 
more/bigger chunks when alloc_buf() cannot succeed.
> In Xen, copy_from_user(xenbuf, memref) would then decode memref to
> figure out what buffer was being referred to. copy_from_user would then
> need to understand the data structures used by userland to track the
> memory references within the buffer.
Callers of copy_from_user() already know where the user 
pointers/handles are that need special treatment.
> Problem #2: Spanning pages is still really difficult. One possible
> solution (different from above) would be to have the kernel reserve 
> some
> physically contiguous pages, and then export that area by having
> userland mmap some device.
Easy for application/kernel where the pages can be mapped contiguously. 
Only a problem for ppc xen which does not run with paging enabled. But 
page crossings can be hidden inside copy_to_user/copy_from_user.
> Problem #3: We need to know beforehand the maximum number of bytes
> needed for the buffer.
Nope I don''t think so.
> Problem #4: The kernel must track the buffers that userland registered,
> and unregister them when the process dies, since it may not have been
> able to unregister them properly.
Yes. Applications should get hypercall-capable memory by mmap()ing a 
device file (e.g., privcmd). That can then be resource tracked.
> This mail isn''t comprehensive, but I think gives some idea of the
> complexity involved. So a solution like replacing pointers with 
> embedded
> structures is far more attractive.
Not sure what you mean. Can you give an example?

  -- Keir
> -- 
> Hollis Blanchard
> IBM Linux Technology Center
>

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

On Tue, 2006-02-07 at 14:28 +0000, Keir Fraser wrote:
> 
> > This mail isn''t comprehensive, but I think gives some idea of
the
> > complexity involved. So a solution like replacing pointers with 
> > embedded structures is far more attractive.
> 
> Not sure what you mean. Can you give an example?
typedef struct dom0_setvcpucontext {
    /* IN variables. */
    domid_t               domain;
    uint32_t              vcpu;
    /* IN/OUT parameters */
    vcpu_guest_context_t ctxt;
} dom0_setvcpucontext_t;

Note that ''ctxt'' is no longer a pointer.

This is a far simpler solution, though it does introduce a size
limitation for memory-based hcalls; I went into some detail in my other
mail. Could you reply to that if there''s something unclear?

-- 
Hollis Blanchard
IBM Linux Technology Center


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

I think I''ve solved this problem to my satisfaction. The solution I
implemented was not registering buffers and running an allocator out of
it, since that got a lot too complicated for my taste.

The code I''ve just checked in to the PPC trees
(http://xenbits.xensource.com/ext/linux-ppc-2.6.hg and
http://xenbits.xensource.com/ext/xenppc-unstable.hg) basically creates a
scatter/gather list for communication, and requires no hcall-user
modifications (e.g. libxc). In summary, all pointers in the hcall data
structures are replaced with pointers to scatter/gather structures.
Xen''s copy_to/from_user now works only with these structures.

Userspace allocates memory for buffers and for the dom0_op itself from
anywhere it likes. When it calls into the kernel via privcmd_ioctl(),
the kernel records the virtually contiguous buffers with scatter/gather
structures containing physical addresses, and replaces all nested
virtual pointers with physical pointers to these structures.

Kernel code (e.g. the console driver) does basically the same thing,
except it''s done in hcall wrapper functions rather than the privcmd
code.

The kernel is the best place to create these structures for two reasons:
a) they contain physical addresses, and b) we don''t need to modify any
userspace tools. We all know that as time goes by, people would fail to
use a fancy allocator since they don''t need to on x86. We would need to
keep going back to patch and retest all those changes in common code.

Cons:
- the kernel must understand all data structures coming in via privcmd
	- adds complexity to the kernel
	- may fail poorly if data structures change without new opcodes
		- could be mitigated by something like Linux''s ksyms
	- using new dom0 ops requires kernel build and reboot
		- but we already had to reboot Xen anyways
			- unless migrating domains...
- get/put_user() can no longer be single-instruction accesses

Pros:
- should usually fail gracefully (new/unknown dom0 op returns ENOSYS)
- requires no userspace modifications
	- supports multipage buffers
	- supports allocation from arbitrary memory
- changes are localized to the PPC kernel
	- new interfaces can be fixed with an arch-specific patch, which won''t
risk breaking already-tested users

There''s one catch: get/put_user(). I haven''t implemented those
properly
yet, but that interface would need to change something like this:
--- a/xen/drivers/char/console.c        Mon Feb 13 10:35:24 2006
+++ b/xen/drivers/char/console.c        Tue Feb 14 09:24:07 2006
@@ -363,7 +363,7 @@
         while ( (serial_rx_cons != serial_rx_prod) && (rc < count) )
         {
             if ( put_user(serial_rx_ring[SERIAL_RX_MASK(serial_rx_cons)],
-                          &buffer[rc]) )
+                          buffer, rc) )
             {
                 rc = -EFAULT;
                 break;
In other words, provide the base buffer pointer and an offset into that
buffer. On x86,
#define put_user(value, base, offset) \
	x86_put_user(value, base + offset)

There are very few uses of get/put_user() in common code right now, and
those can be trivially converted. However, there are some in arch code
e.g. xen/arch/x86/domain.c) that cannot, and so the current put_user
interface would need to be preserved as some arch-specific macro. In
this case I''d call it "x86_put_user" to emphasize that only
x86 arch
code should be using it. Too many x86isms creep into places like
grant_table.c...

These changes are far less invasive and less complicated than the rework
that libxc would need, so I still believe this is the best solution.

Finally, I am happy to help adapt the code to work on other
architectures. In particular, once an hcall routine is properly
abstracted, most of linux/arch/powerpc/platforms/xen/hcall.c and
xen/arch/ppc/usercopy.c could be used by any architecture that needs it.

-- 
Hollis Blanchard
IBM Linux Technology Center


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

On 13 Feb 2006, at 22:56, Hollis Blanchard wrote:
> There are very few uses of get/put_user() in common code right now, and
> those can be trivially converted. However, there are some in arch code
> e.g. xen/arch/x86/domain.c) that cannot, and so the current put_user
> interface would need to be preserved as some arch-specific macro. In
> this case I''d call it "x86_put_user" to emphasize that
only x86 arch
> code should be using it. Too many x86isms creep into places like
> grant_table.c...
I expect copy_to/from_user would also need to change. There are places 
where an array of structs are passed to a hcall, a pointer into that 
array is passed around in Xen, and individual array entries are copied 
in/out.

However, this approach seems little different on the Xen side from the 
xencomm allocator approach -- the pointers are still opaque handles 
from the p.o.v. of generic Xen code. So some interface changes would be 
required even if we went down that road.

This opacity should probably be represented in the public header file 
definitions and in the prototypes of uaccess functions, instead of 
pretending the handles are really ''foo *'' pointers. That would
help
clean up all uses of the uaccess functions in Xen.

We''ll have to see how this all pans out....

  -- Keir


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

On Mon, 2006-02-13 at 23:45 +0000, Keir Fraser wrote:
> On 13 Feb 2006, at 22:56, Hollis Blanchard wrote:
> 
> > There are very few uses of get/put_user() in common code right now,
and
> > those can be trivially converted. However, there are some in arch code
> > e.g. xen/arch/x86/domain.c) that cannot, and so the current put_user
> > interface would need to be preserved as some arch-specific macro. In
> > this case I''d call it "x86_put_user" to emphasize
that only x86 arch
> > code should be using it. Too many x86isms creep into places like
> > grant_table.c...
> 
> I expect copy_to/from_user would also need to change. There are places 
> where an array of structs are passed to a hcall, a pointer into that 
> array is passed around in Xen, and individual array entries are copied 
> in/out.
Hmm, I didn''t run across that; can you point me to some code? At any
rate, changing "struct foo *entry" to "struct foo *array, int
index" in
the function parameter lists shouldn''t be a big deal.
> However, this approach seems little different on the Xen side from the 
> xencomm allocator approach -- the pointers are still opaque handles 
> from the p.o.v. of generic Xen code. So some interface changes would be 
> required even if we went down that road.
> 
> This opacity should probably be represented in the public header file 
> definitions and in the prototypes of uaccess functions, instead of 
> pretending the handles are really ''foo *'' pointers. That
would help
> clean up all uses of the uaccess functions in Xen.
Ok, so what I''m hearing is that copy_to/from_user() and get/put_user()
should take unsigned long and not void*.
> We''ll have to see how this all pans out....
This is my biggest issue and it''s blocking me; I am trying to solve it
now. If you are amenable to the get/put_user() changes I described, I
will go ahead and implement them today.

-- 
Hollis Blanchard
IBM Linux Technology Center


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

hollisb@us.ltcfwd.linux.ibm.com wrote on 02/13/2006 05:56:36 PM:
> I think I''ve solved this problem to my satisfaction. The solution
I
> implemented was not registering buffers and running an allocator out of
> it, since that got a lot too complicated for my taste.
> 
> The code I''ve just checked in to the PPC trees
> (http://xenbits.xensource.com/ext/linux-ppc-2.6.hg and
> http://xenbits.xensource.com/ext/xenppc-unstable.hg) basically creates a
> scatter/gather list for communication, and requires no hcall-user
> modifications (e.g. libxc). In summary, all pointers in the hcall data
> structures are replaced with pointers to scatter/gather structures.
> Xen''s copy_to/from_user now works only with these structures.
> 
> Userspace allocates memory for buffers and for the dom0_op itself from
> anywhere it likes. When it calls into the kernel via privcmd_ioctl(),
> the kernel records the virtually contiguous buffers with scatter/gather
> structures containing physical addresses, and replaces all nested
> virtual pointers with physical pointers to these structures.
Would the following be a simpler solution for pcc for the privcmd 
interface:

  - force copying of all structures into arrays allocated with 
__get_free_pages() using a wrapper function (for contig. memory to avoid 
scatter-gather)
    function returns the physical address

If the same code was to also be used for x86 to re-write the user-space 
arrays:
  - the wrapper function that is used to copying into contiguous memory on 
PPC results in a no-op on x86 and returns the address passed to it


The code could look something like this:

#ifdef PPC

unsigned long xencomm_copy(unsigned long addr, unsigned long len, struct 
collect *c) 
{
        struct collect *coll = malloc(*c);

        if (coll) {
                coll->order = get_order(len);

                coll->addr = __get_free_pages(order);
                copy_from_user(coll->addr, addr, len);

                coll->next = c->next;
                c->next = coll;
                return coll->addr;
        }
        return 0;
}

unsigned long xencomm_txcopy(unsigned long addr, unsigned long len, struct 
collect *c)
{
        return __pa(xencomm_copy(addr,len,c));
}

#else
unsigned long xencomm_txcopy(unsigned long addr, unsigned long len, struct 
collect *c)
{
        return addr;
}
unsigned long xencomm_copy(unsigned long addr, unsigned long len, struct 
collect *c)
{
        return __pa(xencomm_copy(addr,len,c));
}


#endif

xencomm_free(struct collect *c)
{
        struct collect *_c;
        while (c) {
                free_pages(c->dest, c->order);
                _c = c->next;
                free(c);
                c = _c;
        }
}


For the PPC-tree:


                 struct collection *coll;
                [...]

                 if (copy_from_user(&kern_op, user_op, sizeof(dom0_op_t)))
                                 return -EFAULT;

                 if (kern_op.interface_version != DOM0_INTERFACE_VERSION)
                                 return -EACCES;

                 switch (kern_op.cmd) {
                                 case DOM0_GETMEMLIST:
 kern_op.u.getmemlist.buffer = xencomm_txcopy(kern_op.u.getmemlist.buffer, 
)
                                 kern_op.u.getmemlist.max_pfns * 
sizeof(long),
 coll);
                                                 break;

                [...]

                 ret = plpar_hcall_norets(XEN_MARK(hypercall->op), 
__pa(&kern_op),
                                                 0, 0, 0, 0);

                 xencomm_free(coll);
 

This should not require changes to the copy_from_user function in the HV 
but does not get around copying of the arrays / rewriting of the data 
structures.

 Stefan



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

On Mon, 2006-02-13 at 21:23 -0500, Stefan Berger wrote:
>   - force copying of all structures into arrays allocated with
> __get_free_pages() using a wrapper function (for contig. memory to
> avoid scatter-gather) 
>     function returns the physical address 
The kernel''s physically-contiguous memory is not necessarily
machine-contiguous, so a scatter/gather approach would still be
required... and of course, since you need the scatter/gather logic
anyways, you might as well avoid the copy and directly reference the
original user pages.

-- 
Hollis Blanchard
IBM Linux Technology Center


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

On 14 Feb 2006, at 00:06, Hollis Blanchard wrote:
> Ok, so what I''m hearing is that copy_to/from_user() and
get/put_user()
> should take unsigned long and not void*.
I''d prefer a type that can''t be directly manipulated by
standard
operators. In fact, a family of types that would allow us to 
differentiate between references to different types (e.g., unsigned 
long * versus multicall_entry_t *).

Perhaps replace ''foo *name'' in public structures with 
''XENPARM_HANDLE(foo, name)''.

The uaccess macros then take xenparm handles plus an offset. The macros 
would need a bit of CPP magic to make them suitably polymorphic.

  -- Keir


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

On Mon, 2006-02-13 at 23:45 +0000, Keir Fraser wrote:
> 
> This opacity should probably be represented in the public header file 
> definitions and in the prototypes of uaccess functions, instead of 
> pretending the handles are really ''foo *'' pointers. That
would help
> clean up all uses of the uaccess functions in Xen.
One nice thing about leaving the types present is that some of the
macros can do some type-checking.

I''d be happy to help look at the uaccess users in general, but
that''s
not my immediate problem. This patch solves my immediate problem (though
obviously breaks x86* and ia64; I include it only for discussion). There
are other users of uaccess macros in common code, but due to other
problems (*ahem* grant_table.c) we don''t build those right now anyways.

I am also attaching my new asm-ppc/uaccess.h so you can see the full API
presented. (I want to remove all get/put_user calls entirely so the
compiler will catch xencomm-incompatible users.)

I don''t know what implications this could have for ia64, since they
suck
in so much Linux code... but perhaps that Linux code isn''t the stuff
that deals with userland, so might not have uaccess users.

While we''re at it, I would like to rename *_user to *_domain, but
that''s
a cosmetic thing that can be done at any time.

Keir, would you accept this patch?

--- a/xen/common/memory.c	Tue Feb 14 05:48:33 2006
+++ b/xen/common/memory.c	Tue Feb 14 06:22:09 2006
@@ -60,7 +60,7 @@
 
         /* Inform the domain of the new page''s machine address. */ 
         if ( (extent_list != NULL) &&
-             (__put_user(page_to_pfn(page), &extent_list[i]) != 0) )
+             (__put_user_array(page_to_pfn(page), extent_list, i) != 0) )
             return i;
     }
 
@@ -90,7 +90,7 @@
             return i;
         }
 
-        if ( unlikely(__get_user(mpfn, &extent_list[i]) != 0) )
+        if ( unlikely(__get_user_array(mpfn, extent_list, i) != 0) )
             return i;
 
         for ( j = 0; j < (1 << extent_order); j++ )
@@ -197,7 +197,7 @@
 
     case XENMEM_current_reservation:
     case XENMEM_maximum_reservation:
-        if ( get_user(domid, (domid_t *)arg) )
+        if ( get_user_offset(domid, (domid_t *)arg, 0) )
             return -EFAULT;
 
         if ( likely((domid = (unsigned long)arg) == DOMID_SELF) )
--- a/xen/drivers/char/console.c	Tue Feb 14 05:48:33 2006
+++ b/xen/drivers/char/console.c	Tue Feb 14 06:22:09 2006
@@ -230,7 +230,7 @@
 
     /* Start of buffer may get overwritten during copy. So copy backwards. */
     for ( p = conringp, q = count; (p > conringc) && (q > 0);
p--, q-- )
-        if ( put_user(conring[CONRING_IDX_MASK(p-1)], (char *)str+q-1) )
+        if ( put_user_array(conring[CONRING_IDX_MASK(p-1)], (char *)str, q-1) )
             return -EFAULT;
 
     if ( clear )
@@ -362,8 +362,8 @@
         rc = 0;
         while ( (serial_rx_cons != serial_rx_prod) && (rc < count) )
         {
-            if ( put_user(serial_rx_ring[SERIAL_RX_MASK(serial_rx_cons)],
-                          &buffer[rc]) )
+            if ( put_user_array(serial_rx_ring[SERIAL_RX_MASK(serial_rx_cons)],
+                          buffer, rc) )
             {
                 rc = -EFAULT;
                 break;



/*
 * Copyright (C) 2006 Hollis Blanchard <hollisb@us.ibm.com>, IBM
Corporation
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License as published by
 * the Free Software Foundation; either version 2 of the License, or
 * (at your option) any later version.
 * 
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 * 
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, write to the Free Software
 * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
 *
 */

#ifndef __PPC_UACCESS_H__
#define __PPC_UACCESS_H__

#include <xen/errno.h>
#include <asm/page.h>

/* since we run in real mode, we can safely access all addresses.
 * XXX well, except IO. should we check for that here? */
#define access_ok(addr,size) 1
#define array_access_ok(addr,count,size) 1

#define __copy_to_user copy_to_user
#define __copy_from_user copy_from_user
#define copy_to_user(to,from,len) xencomm_copy_to_user(to,from,len,0)
#define copy_from_user(to,from,len) xencomm_copy_from_user(to,from,len,0)

extern unsigned long xencomm_copy_to_user(void *to, const void *from,
        unsigned len, unsigned int skip); 
extern unsigned long xencomm_copy_from_user(void *to, const void *from,
        unsigned len, unsigned int skip); 

#define get_user_array(val,array,idx) __get_user_array(val,array,idx)
#define __get_user_array(val,array,idx) \
    xencomm_get_user_offset((val), (array), (idx) * sizeof(*(array)))

#define get_user_offset(val,ptr,offset) __get_user_offset(val,ptr,offset)
#define __get_user_offset(val,ptr,offset) \
    xencomm_get_user_offset((val), (ptr), (offset))

#define xencomm_get_user_offset(val,ptr,offset)                         \
({                                                                      \
    long __gu_err;                                                      \
    __typeof__(*(ptr)) __gu_tmp;                                        \
    __gu_err = xencomm_copy_from_user(&__gu_tmp, (ptr),                 \
            sizeof(__gu_tmp), offset);                                  \
    if (__gu_err)                                                       \
        __gu_err = -EFAULT;                                             \
    val = __gu_tmp;                                                     \
    __gu_err;                                                           \
})

#define put_user_array(val,array,idx) __put_user_array(val,array,idx)
#define __put_user_array(val,array,idx) \
    xencomm_put_user_offset((val), (array), (idx) * sizeof(*(array)))

#define put_user_offset(val,ptr,offset) __put_user_offset(val,ptr,offset)
#define __put_user_offset(val,ptr,offset) \
    xencomm_put_user_offset((val), (ptr), (offset))

#define xencomm_put_user_offset(val,ptr,offset)                         \
({                                                                      \
    long __pu_err;                                                      \
    __typeof__(*(ptr)) __pu_tmp = val;                                  \
    __pu_err = xencomm_copy_to_user((ptr), &__pu_tmp,                   \
            sizeof(__pu_tmp), offset);                                  \
    if (__pu_err)                                                       \
        __pu_err = -EFAULT;                                             \
    __pu_err;                                                           \
})

#endif /* __PPC_UACCESS_H__ */

-- 
Hollis Blanchard
IBM Linux Technology Center



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

On Mon, 2006-02-13 at 23:45 +0000, Keir Fraser wrote:
> 
> On 13 Feb 2006, at 22:56, Hollis Blanchard wrote:
> 
> > There are very few uses of get/put_user() in common code right now,
> and
> > those can be trivially converted. However, there are some in arch
> code
> > e.g. xen/arch/x86/domain.c) that cannot, and so the current put_user
> > interface would need to be preserved as some arch-specific macro. In
> > this case I''d call it "x86_put_user" to emphasize
that only x86 arch
> > code should be using it. Too many x86isms creep into places like
> > grant_table.c...
> 
> I expect copy_to/from_user would also need to change. There are places
> where an array of structs are passed to a hcall, a pointer into that 
> array is passed around in Xen, and individual array entries are copied
> in/out. 
I am very happy to report that, after converting the get/put_user calls
as described (and fixing my bugs), domain creation and domU booting is
now working with an unmodified libxc. I am still using the simple C
tools I hacked up because I need the simplicity/traceability.

I will be busy for a week or so, but I will try to come up with a patch
that converts x86 and ia64 and all common code to the proposed API:
	get_user_offset(val, base, offset)
	put_user_offset(val, base, offset)
	get_user_array(val, array, index)
	put_user_array(val, array, index)
	copy_to_user(to, from, len)
	copy_from_user(to, from, len)
... plus __ variants of all of these, with identical arguments
... plus arch-specific get/put_user for e.g. xen/arch/x86/domain.c,
since some of those usages don''t seem to fit above.

If you don''t like the API, please speak up now.

-- 
Hollis Blanchard
IBM Linux Technology Center


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

On 15 Feb 2006, at 10:48, Hollis Blanchard wrote:
> I will be busy for a week or so, but I will try to come up with a patch
> that converts x86 and ia64 and all common code to the proposed API:
> 	get_user_offset(val, base, offset)
> 	put_user_offset(val, base, offset)
> 	get_user_array(val, array, index)
> 	put_user_array(val, array, index)
> 	copy_to_user(to, from, len)
> 	copy_from_user(to, from, len)
> ... plus __ variants of all of these, with identical arguments
> ... plus arch-specific get/put_user for e.g. xen/arch/x86/domain.c,
> since some of those usages don''t seem to fit above.
>
> If you don''t like the API, please speak up now.
Should be called get_guest/put_guest/copy_from_guest/copy_to_guest. 
Better names, and the arch-specific old functions can still keep their 
old names.

What''s the difference between foo_offset() and foo_array()?

  -- Keir


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

On Feb 15, 2006, at 5:23 AM, Keir Fraser wrote:
>
> On 15 Feb 2006, at 10:48, Hollis Blanchard wrote:
>
>> I will be busy for a week or so, but I will try to come up with a 
>> patch
>> that converts x86 and ia64 and all common code to the proposed API:
>> 	get_user_offset(val, base, offset)
>> 	put_user_offset(val, base, offset)
>> 	get_user_array(val, array, index)
>> 	put_user_array(val, array, index)
>> 	copy_to_user(to, from, len)
>> 	copy_from_user(to, from, len)
>> ... plus __ variants of all of these, with identical arguments
>> ... plus arch-specific get/put_user for e.g. xen/arch/x86/domain.c,
>> since some of those usages don''t seem to fit above.
>>
>> If you don''t like the API, please speak up now.
>
> Should be called get_guest/put_guest/copy_from_guest/copy_to_guest. 
> Better names, and the arch-specific old functions can still keep their 
> old names.
"guest" not "domain"? Either way is fine with me. If we do
leave the
old names in, though, I''m sure people will slip them into common 
code...
> What''s the difference between foo_offset() and foo_array()?
Just convenience: many (most?) users of get/put_user() are to an 
address like "&array[index]". If we only had _offset(), those
callers
would look like:
	put_user(val, array, sizeof(*array)*index);
Actually that doesn''t look too bad. :) If you''re ok with that,
we can
stick with just the "offset" variant.

-- 
Hollis Blanchard
IBM Linux Technology Center


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

On Feb 15, 2006, at 5:23 AM, Keir Fraser wrote:
>
> Should be called get_guest/put_guest/copy_from_guest/copy_to_guest. 
> Better names, and the arch-specific old functions can still keep their 
> old names.
I don''t think I understood your full meaning before, but I''m
with you
now. get/put_user() will still exist on x86 and mean virtual addresses 
(kernel or user). The new get/put_guest_offset() implementations will 
be equivalent.

PowerPC will implement get/put_guest_offset(), but will not implement 
get/put_user() at all. If those latter calls do make their way into 
common code (after the PPC code has been merged), every PPC build in 
the regression test suite will fail, so the patch will be flagged as 
bad and not committed.

This sounds fine to me, other than that bit about confusing the word 
"user" to mean "virtual" (since it could of course be a
kernel address
as well).

-- 
Hollis Blanchard
IBM Linux Technology Center


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

On 17 Feb 2006, at 05:39, Hollis Blanchard wrote:
>> Should be called get_guest/put_guest/copy_from_guest/copy_to_guest. 
>> Better names, and the arch-specific old functions can still keep 
>> their old names.
>
> I don''t think I understood your full meaning before, but
I''m with you
> now. get/put_user() will still exist on x86 and mean virtual addresses 
> (kernel or user). The new get/put_guest_offset() implementations will 
> be equivalent.
>
> PowerPC will implement get/put_guest_offset(), but will not implement 
> get/put_user() at all. If those latter calls do make their way into 
> common code (after the PPC code has been merged), every PPC build in 
> the regression test suite will fail, so the patch will be flagged as 
> bad and not committed.
>
> This sounds fine to me, other than that bit about confusing the word 
> "user" to mean "virtual" (since it could of course be a
kernel address
> as well).
I actually managed to get rid of get/put_user from common code, so we 
only need replacements for copy_to/from_user.

I suggest e.g., copy_to_guest(handle, offset, xen_ptr, nr_items)

where handle is a ''guest pointer'' of type foo *, which we view
as
pointing at an array of foos. ''offset'' is the offset into the
foo array
to start copying to. xen_ptr must be of type foo *; it is the start 
address to copy from. nr_items is the number of foos to copy.

There are a few other thgings to consider too. We could have 
copy_array_to_guest with the above declaration, and have a simpler 
copy_to_guest with no ''nr_items'' parameter. We will need 
__copy_to_guest alternatives where the handle has been pre-validated.

How does that sound?

  -- Keir


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