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-devel] Re: [XenPPC] RFC: xencomm - linux side

To: Tristan Gingold <Tristan.Gingold@xxxxxxxx>
Subject: [Xen-devel] Re: [XenPPC] RFC: xencomm - linux side
From: Hollis Blanchard <hollisb@xxxxxxxxxx>
Date: Tue, 22 Aug 2006 14:03:29 -0500
Cc: xen-ia64-devel <xen-ia64-devel@xxxxxxxxxxxxxxxxxxx>, xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Tue, 22 Aug 2006 12:03:24 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <200608211718.50751.Tristan.Gingold@xxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: IBM Linux Technology Center
References: <200608211718.50751.Tristan.Gingold@xxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
I apologize for my mailer line-wrapping the patch as I quote it below.

On Mon, 2006-08-21 at 17:18 +0200, Tristan Gingold wrote:
> diff -r b7db009d622c linux-2.6-xen-sparse/drivers/xen/Kconfig
> --- a/linux-2.6-xen-sparse/drivers/xen/Kconfig  Mon Aug 21 09:41:24
> 2006 +0200
> +++ b/linux-2.6-xen-sparse/drivers/xen/Kconfig  Mon Aug 21 15:04:32
> 2006 +0200
> @@ -257,4 +257,7 @@ config XEN_SMPBOOT
>         default y
>         depends on SMP
>  
> +config XEN_XENCOMM
> +       bool
> +       default n
>  endif

Shouldn't IA64 "select XEN_XENCOMM"? Or is your kernel in a separate
tree?

> diff -r b7db009d622c linux-2.6-xen-sparse/drivers/xen/Makefile
> --- a/linux-2.6-xen-sparse/drivers/xen/Makefile Mon Aug 21 09:41:24
> 2006 +0200
> +++ b/linux-2.6-xen-sparse/drivers/xen/Makefile Mon Aug 21 15:04:32
> 2006 +0200
> @@ -1,10 +1,10 @@ obj-y += core/
>  obj-y  += core/
>  obj-y  += console/
>  obj-y  += evtchn/
> -obj-y  += privcmd/
>  obj-y  += xenbus/
>  
>  obj-$(CONFIG_XEN_UTIL)                 += util.o
> +obj-$(CONFIG_XEN_PRIVCMD)              += privcmd/
>  obj-$(CONFIG_XEN_BALLOON)              += balloon/
>  obj-$(CONFIG_XEN_DEVMEM)               += char/
>  obj-$(CONFIG_XEN_BLKDEV_BACKEND)       += blkback/

Not really part of this patch.

> diff -r b7db009d622c linux-2.6-xen-sparse/drivers/xen/core/Makefile
> --- a/linux-2.6-xen-sparse/drivers/xen/core/Makefile    Mon Aug 21
> 09:41:24 2006 +0200
> +++ b/linux-2.6-xen-sparse/drivers/xen/core/Makefile    Mon Aug 21
> 15:04:32 2006 +0200
> @@ -11,3 +11,4 @@ obj-$(CONFIG_XEN_SKBUFF)      += skbuff.o
>  obj-$(CONFIG_XEN_SKBUFF)       += skbuff.o
>  obj-$(CONFIG_XEN_REBOOT)       += reboot.o
>  obj-$(CONFIG_XEN_SMPBOOT)      += smpboot.o
> +obj-$(CONFIG_XEN_XENCOMM)      += xencomm.o xencomm_hcall.o
> diff -r b7db009d622c linux-2.6-xen-sparse/drivers/xen/privcmd/Makefile
> --- a/linux-2.6-xen-sparse/drivers/xen/privcmd/Makefile Mon Aug 21
> 09:41:24 2006 +0200
> +++ b/linux-2.6-xen-sparse/drivers/xen/privcmd/Makefile Mon Aug 21
> 15:04:32 2006 +0200
> @@ -1,2 +1,3 @@
> +obj-y                          := privcmd.o
>  
> -obj-$(CONFIG_XEN_PRIVCMD)      := privcmd.o
> +obj-$(CONFIG_XEN_XENCOMM)      += xencomm.o

I agree with the CONFIG_XEN_PRIVCMD stuff, but I think that should be a
separate patch.

> diff -r b7db009d622c
> linux-2.6-xen-sparse/drivers/xen/privcmd/privcmd.c
> --- a/linux-2.6-xen-sparse/drivers/xen/privcmd/privcmd.c        Mon
> Aug 21 09:41:24 2006 +0200
> +++ b/linux-2.6-xen-sparse/drivers/xen/privcmd/privcmd.c        Mon
> Aug 21 15:04:32 2006 +0200
> @@ -34,6 +34,10 @@
>  
>  static struct proc_dir_entry *privcmd_intf;
>  static struct proc_dir_entry *capabilities_intf;
> +
> +#ifdef CONFIG_XEN_XENCOMM
> +extern int xencomm_privcmd_hypercall(privcmd_hypercall_t *hypercall);
> +#endif
>  
>  #define NR_HYPERCALLS 64
>  static DECLARE_BITMAP(hypercall_permission_map, NR_HYPERCALLS);
> @@ -91,19 +95,8 @@ static int privcmd_ioctl(struct inode *i
>                                 "g" ((unsigned long)hypercall.arg[4])
>                                 : "r8", "r10", "memory" );
>                 }
> -#elif defined (__ia64__)
> -               __asm__ __volatile__ (
> -                       ";; mov r14=%2; mov r15=%3; "
> -                       "mov r16=%4; mov r17=%5; mov r18=%6;"
> -                       "mov r2=%1; break 0x1000;; mov %0=r8 ;;"
> -                       : "=r" (ret)
> -                       : "r" (hypercall.op),
> -                       "r" (hypercall.arg[0]),
> -                       "r" (hypercall.arg[1]),
> -                       "r" (hypercall.arg[2]),
> -                       "r" (hypercall.arg[3]),
> -                       "r" (hypercall.arg[4])
> -                       :
> "r14","r15","r16","r17","r18","r2","r8","memory");
> +#elif defined (CONFIG_XEN_XENCOMM)
> +               ret = xencomm_privcmd_hypercall (&hypercall);
>  #endif
>         }
>         break;

Move all the #ifdef stuff into appropriate header files, then have every
arch unconditionally call arch_privcmd_hypercall().

> diff -r b7db009d622c linux-2.6-xen-sparse/drivers/xen/core/xencomm.c
> --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> +++ b/linux-2.6-xen-sparse/drivers/xen/core/xencomm.c   Mon Aug 21
> 15:04:32 2006 +0200
> @@ -0,0 +1,213 @@
> +/*
> + * Copyright (C) 2006 Hollis Blanchard <hollisb@xxxxxxxxxx>, 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
> + */
> +
> +#include <linux/gfp.h>
> +#include <linux/mm.h>
> +#include <asm/page.h>
> +#include <xen/xencomm.h>
> +#include <xen/interface/xen.h>
> +
> +int xencomm_debug;
> +
> +/* translate virtual address to physical address */
> +static unsigned long xen_vaddr_to_paddr(unsigned long vaddr)
> +{
> +       struct page *page;
> +       struct vm_area_struct *vma;
> +
> +#ifdef __ia64__
> +       /* On ia64, TASK_SIZE refers to current.  It is not
> initialized
> +          during boot.
> +          Furthermore the kernel is relocatable and __pa() doesn't
> work on
> +          kernel addresses.  */
> +       if (vaddr >= KERNEL_START
> +           && vaddr < (KERNEL_START + KERNEL_TR_PAGE_SIZE)) {
> +               extern unsigned long kernel_start_pa;
> +               return vaddr - kernel_start_pa;
> +       }
> +#endif
> +       if (vaddr > TASK_SIZE) {
> +               /* kernel address */
> +               return __pa(vaddr);
> +       }
> +
> +       /* XXX double-check (lack of) locking */
> +       vma = find_extend_vma(current->mm, vaddr);
> +       if (!vma)
> +               return ~0UL;
> +
> +       page = follow_page(vma, vaddr, 0);
> +       if (!page)
> +               return ~0UL;
> +
> +       return (page_to_pfn(page) << PAGE_SHIFT) | (vaddr &
> ~PAGE_MASK);
> +}

If there really is no way to implement xen_vaddr_to_paddr() in an
arch-neutral way (and I'm willing to believe that's true), just make the
whole function arch-specific. It wouldn't be too much duplicated code.

> +static int xencomm_init(struct xencomm_desc *desc,
> +                       void *buffer, unsigned long bytes)
> +{
> +       unsigned long recorded = 0;
> +       int i = 0;
> +
> +       BUG_ON((buffer == NULL) && (bytes > 0));
> +
> +       /* record the physical pages used */
> +       if (buffer == NULL)
> +               desc->nr_addrs = 0;
> +
> +       while ((recorded < bytes) && (i < desc->nr_addrs)) {
> +               unsigned long vaddr = (unsigned long)buffer +
> recorded;
> +               unsigned long paddr;
> +               int offset;
> +               int chunksz;
> +
> +               offset = vaddr % PAGE_SIZE; /* handle partial pages */
> +               chunksz = min(PAGE_SIZE - offset, bytes - recorded);
> +
> +               paddr = xen_vaddr_to_paddr(vaddr);
> +               if (paddr == ~0UL) {
> +                       printk("%s: couldn't translate vaddr %lx\n",
> +                              __func__, vaddr);
> +                       return -EINVAL;
> +               }
> +
> +               desc->address[i++] = paddr;
> +               recorded += chunksz;
> +       }
> +
> +       if (recorded < bytes) {
> +               printk("%s: could only translate %ld of %ld bytes\n",
> +                      __func__, recorded, bytes);
> +               return -ENOSPC;
> +       }
> +
> +       /* mark remaining addresses invalid (just for safety) */
> +       while (i < desc->nr_addrs)
> +               desc->address[i++] = XENCOMM_INVALID;
> +
> +       desc->magic = XENCOMM_MAGIC;
> +
> +       return 0;
> +}
> +
> +/* XXX use slab allocator */
> +static struct xencomm_desc *xencomm_alloc(gfp_t gfp_mask)
> +{
> +       struct xencomm_desc *desc;
> +
> +       /* XXX could we call this from irq context? */

You can remove this comment. It's historical, and we're passing in
gfp_mask now.

> +       desc = (struct xencomm_desc *)__get_free_page(gfp_mask);
> +       if (desc == NULL) {
> +               panic("%s: page allocation failed\n", __func__);
> +       }
> +       desc->nr_addrs = (PAGE_SIZE - sizeof(struct xencomm_desc)) /
> +                       sizeof(*desc->address);
> +
> +       return desc;
> +}
> +
> +void xencomm_free(struct xencomm_handle *desc)
> +{
> +       if (desc)
> +               free_page((unsigned long)__va(desc));
> +}
> +
> +int xencomm_create(void *buffer, unsigned long bytes,
> +                  struct xencomm_handle **ret, gfp_t gfp_mask)
> +{
> +       struct xencomm_desc *desc;
> +       struct xencomm_handle *handle;
> +       int rc;
> +
> +       if (xencomm_debug) {
> +               printk("%s: %p[%ld]\n", __func__, buffer, bytes);
> +       }
> +
> +       if (buffer == NULL || bytes == 0) {
> +               *ret = (struct xencomm_handle *)NULL;
> +               return 0;
> +       }
> +
> +       desc = xencomm_alloc(gfp_mask);
> +       if (!desc) {
> +               printk("%s failure\n", "xencomm_alloc");
> +               return -ENOMEM;
> +       }
> +       handle = (struct xencomm_handle *)__pa(desc);
> +
> +       rc = xencomm_init(desc, buffer, bytes);
> +       if (rc) {
> +               printk("%s failure: %d\n", "xencomm_init", rc);
> +               xencomm_free(handle);
> +               return rc;
> +       }
> +
> +       *ret = handle;
> +       return 0;
> +}
> +
> +/* "mini" routines, for stack-based communications: */
> +
> +static void *xencomm_alloc_mini(void *area, int arealen)
> +{
> +       unsigned long base = (unsigned long)area;
> +       unsigned int pageoffset;
> +
> +       pageoffset = base % PAGE_SIZE;
> +
> +       /* we probably fit right at the front of area */
> +       if ((PAGE_SIZE - pageoffset) >= sizeof(struct xencomm_mini)) {
> +               return area;
> +       }
> +
> +       /* if not, see if area is big enough to advance to the next
> page */
> +       if ((arealen - pageoffset) >= sizeof(struct xencomm_mini))
> +               return (void *)(base + pageoffset);
> +
> +       /* area was too small */
> +       return NULL;
> +}
> +
> +int xencomm_create_mini(void *area, int arealen, void *buffer,
> +                       unsigned long bytes, struct xencomm_handle
> **ret)
> +{
> +       struct xencomm_desc *desc;
> +       int rc;
> +
> +       desc = xencomm_alloc_mini(area, arealen);
> +       if (!desc)
> +               return -ENOMEM;
> +       desc->nr_addrs = XENCOMM_MINI_ADDRS;
> +
> +       rc = xencomm_init(desc, buffer, bytes);
> +       if (rc)
> +               return rc;
> +
> +       *ret = (struct xencomm_handle *)__pa(desc);
> +       return 0;
> +}

*_mini are unused and should be removed entirely.

> +struct xencomm_handle *xencomm_create_inline (void *buffer,
> +                                             unsigned long bytes)
> +{
> +       unsigned long paddr;
> +
> +       paddr = xen_vaddr_to_paddr((unsigned long)buffer);
> +       return (struct xencomm_handle *)XENCOMM_INLINE_CREATE(paddr);
> +}

XENCOMM_INLINE_CREATE in undefined in this patch. I liked your old patch
just fine:
+struct xencomm_desc *xencomm_create_inline (void *buffer, unsigned long
bytes)
+{
+       return (struct xencomm_desc *)
+               (__kern_paddr((unsigned long)buffer) | XENCOMM_INLINE);
+}

> diff -r b7db009d622c
> linux-2.6-xen-sparse/drivers/xen/core/xencomm_hcall.c
> --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> +++ b/linux-2.6-xen-sparse/drivers/xen/core/xencomm_hcall.c     Mon
> Aug 21 15:04:32 2006 +0200
> @@ -0,0 +1,311 @@
> +#include <linux/types.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/gfp.h>
> +#include <linux/module.h>
> +#include <xen/interface/xen.h>
> +#include <xen/interface/dom0_ops.h>
> +#include <xen/interface/memory.h>
> +#include <xen/interface/xencomm.h>
> +#include <xen/interface/version.h>
> +#include <xen/interface/sched.h>
> +#include <xen/interface/event_channel.h>
> +#include <xen/interface/physdev.h>
> +#include <xen/interface/grant_table.h>
> +#include <xen/interface/callback.h>
> +#include <xen/interface/acm_ops.h>
> +#include <xen/public/privcmd.h>
> +#include <asm/hypercall.h>
> +#include <asm/page.h>
> +#include <asm/uaccess.h>
> +#include <xen/xencomm.h>
> +
> +/* Xencomm notes:
> + *
> + * Some hypercalls are made before the memory subsystem is up, so
> instead of
> + * calling xencomm_create(), we allocate XENCOMM_MINI_AREA bytes from
> the stack
> + * to hold the xencomm descriptor.

Remove above comment.

> + * In general, we need a xencomm descriptor to cover the top-level
> data
> + * structure (e.g. the dom0 op), plus another for every embedded
> pointer to
> + * another data structure (i.e. for every GUEST_HANDLE).
> + */
> +
> +int xencomm_hypercall_console_io(int cmd, int count, char *str)
> +{
> +       struct xencomm_handle *desc;
> +       int rc;
> +
> +       desc = xencomm_create_inline (str, count);
> +
> +       rc = xencomm_arch_hypercall_console_io (cmd, count, desc);
> +
> +       return rc;
> +}

I don't understand the point of all these routines if they just call
arch_foo anyways.


> diff -r b7db009d622c
> linux-2.6-xen-sparse/drivers/xen/privcmd/xencomm.c
> --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> +++ b/linux-2.6-xen-sparse/drivers/xen/privcmd/xencomm.c        Mon
> Aug 21 15:04:32 2006 +0200
> @@ -0,0 +1,358 @@
> +#include <linux/types.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/gfp.h>
> +#include <linux/module.h>
> +#include <xen/interface/xen.h>
> +#include <xen/interface/dom0_ops.h>
> +#include <xen/interface/memory.h>
> +#include <xen/interface/version.h>
> +#include <xen/interface/event_channel.h>
> +#include <xen/interface/acm_ops.h>
> +#include <xen/public/privcmd.h>
> +#include <asm/hypercall.h>
> +#include <asm/page.h>
> +#include <asm/uaccess.h>
> +#include <xen/xencomm.h>
> +
> +static int xencomm_privcmd_dom0_op(privcmd_hypercall_t *hypercall)
> +{
> +       dom0_op_t kern_op;
> +       dom0_op_t __user *user_op = (dom0_op_t __user
> *)hypercall->arg[0];
> +       struct xencomm_handle *op_desc;
> +       struct xencomm_handle *desc = NULL;
> +       int ret = 0;
> +
> +       if (copy_from_user(&kern_op, user_op, sizeof(dom0_op_t)))
> +               return -EFAULT;
> +
> +       if (kern_op.interface_version != DOM0_INTERFACE_VERSION)
> +               return -EACCES;
> +
> +       op_desc = xencomm_create_inline (&kern_op, sizeof(dom0_op_t));
> +
> +       switch (kern_op.cmd) {
> +       case DOM0_GETMEMLIST:
> +       {
> +               unsigned long nr_pages =
> kern_op.u.getmemlist.max_pfns;
> +#ifdef __ia64__
> +               /* Xen/ia64 pass first_page and nr_pages in max_pfns!
> */
> +               nr_pages &= 0xffffffff;
> +#endif

I'm willing to put up with this only if you guys promise to fix this
silly API incompatibility, at which point it will be removed.

> +               ret = xencomm_create(
> +                       xen_guest_handle(kern_op.u.getmemlist.buffer),
> +                       nr_pages * sizeof(unsigned long),
> +                       &desc, GFP_KERNEL);
> +               set_xen_guest_handle(kern_op.u.getmemlist.buffer,
> +                                    (void *)desc);
> +               break;
> +       }
> +       case DOM0_SETVCPUCONTEXT:
> +               ret = xencomm_create(
> +                       xen_guest_handle(kern_op.u.setvcpucontext.ctxt),
> +                       sizeof(vcpu_guest_context_t),
> +                       &desc, GFP_KERNEL);
> +               set_xen_guest_handle(kern_op.u.setvcpucontext.ctxt,
> +                                    (void *)desc);
> +               break;
> +       case DOM0_READCONSOLE:
> +               ret = xencomm_create(
> +                       xen_guest_handle(kern_op.u.readconsole.buffer),
> +                       kern_op.u.readconsole.count,
> +                       &desc, GFP_KERNEL);
> +               set_xen_guest_handle(kern_op.u.readconsole.buffer,
> +                                    (void *)desc);
> +               break;
> +       case DOM0_GETPAGEFRAMEINFO2:
> +               ret = xencomm_create(
> +                       xen_guest_handle(kern_op.u.getpageframeinfo2.array),
> +                       kern_op.u.getpageframeinfo2.num,
> +                       &desc, GFP_KERNEL);
> +               set_xen_guest_handle(kern_op.u.getpageframeinfo2.array,
> +                                    (void *)desc);
> +               break;
> +       case DOM0_PERFCCONTROL:
> +               ret = xencomm_create(
> +                       xen_guest_handle(kern_op.u.perfccontrol.desc),
> +                       kern_op.u.perfccontrol.nr_counters *
> +                       sizeof(dom0_perfc_desc_t),
> +                       &desc, GFP_KERNEL);
> +               set_xen_guest_handle(kern_op.u.perfccontrol.desc,
> +                                    (void *)desc);
> +               break;
> +       case DOM0_GETVCPUCONTEXT:
> +               ret = xencomm_create(
> +                       xen_guest_handle(kern_op.u.getvcpucontext.ctxt),
> +                       sizeof(vcpu_guest_context_t),
> +                       &desc, GFP_KERNEL);
> +               set_xen_guest_handle(kern_op.u.getvcpucontext.ctxt,
> +                                    (void *)desc);
> +               break;
> +       case DOM0_GETDOMAININFOLIST:
> +               ret = xencomm_create(
> +                       xen_guest_handle(kern_op.u.getdomaininfolist.buffer),
> +                       kern_op.u.getdomaininfolist.num_domains *
> +                       sizeof(dom0_getdomaininfo_t),
> +                       &desc, GFP_KERNEL);
> +               set_xen_guest_handle(kern_op.u.getdomaininfolist.buffer,
> +                                    (void *)desc);
> +               break;
> +       case DOM0_PHYSICAL_MEMORY_MAP:
> +               ret = xencomm_create(
> +                       
> xen_guest_handle(kern_op.u.physical_memory_map.memory_map),
> +                       kern_op.u.physical_memory_map.nr_map_entries *
> +                       sizeof(struct dom0_memory_map_entry),
> +                       &desc, GFP_KERNEL);
> +               set_xen_guest_handle(kern_op.u.physical_memory_map.memory_map,
> +                                    (void *)desc);
> +               break;
> +
> +       case DOM0_SCHEDCTL:
> +       case DOM0_ADJUSTDOM:
> +       case DOM0_CREATEDOMAIN:
> +       case DOM0_DESTROYDOMAIN:
> +       case DOM0_PAUSEDOMAIN:
> +       case DOM0_UNPAUSEDOMAIN:
> +       case DOM0_GETDOMAININFO:
> +       case DOM0_MSR:
> +       case DOM0_SETTIME:
> +       case DOM0_GETPAGEFRAMEINFO:
> +       case DOM0_SETVCPUAFFINITY:
> +       case DOM0_TBUFCONTROL:
> +       case DOM0_PHYSINFO:
> +       case DOM0_SCHED_ID:
> +       case DOM0_SETDOMAINMAXMEM:
> +       case DOM0_ADD_MEMTYPE:
> +       case DOM0_DEL_MEMTYPE:
> +       case DOM0_READ_MEMTYPE:
> +       case DOM0_IOPORT_PERMISSION:
> +       case DOM0_GETVCPUINFO:
> +       case DOM0_PLATFORM_QUIRK:
> +       case DOM0_MAX_VCPUS:
> +       case DOM0_SETDOMAINHANDLE:
> +       case DOM0_SETDEBUGGING:
> +       case DOM0_DOMAIN_SETUP:
> +               /* no munging needed */
> +               break;
> +
> +       default:
> +               printk("%s: unknown dom0 cmd %d\n", __func__,
> kern_op.cmd);
> +               return -ENOSYS;
> +       }
> +
> +       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);

That's a good question about the copy_to_user(). I thought we never
exposed the modified handles back to the user, but I guess I was wrong.

Also please check whitespace throughout. In particular you seem to be
doing this:
        function (args);
and not even Keir's shall-we-say-unique style does that. ;)

> +static int xencomm_privcmd_acm_op(privcmd_hypercall_t *hypercall)
> +{
> +       int cmd = hypercall->arg[0];
> +       void __user *arg = (void __user *)hypercall->arg[1];
> +       struct xencomm_handle *op_desc;
> +       struct xencomm_handle *desc = NULL;
> +       int ret;
> +
> +       switch (cmd) {
> +       case ACMOP_getssid:
> +       {
> +               struct acm_getssid kern_arg;
> +
> +               if (copy_from_user (&kern_arg, arg, sizeof
> (kern_arg)))
> +                       return -EFAULT;
> +
> +               op_desc = xencomm_create_inline (&kern_arg,
> sizeof(kern_arg));
> +
> +               ret =
> xencomm_create(xen_guest_handle(kern_arg.ssidbuf),
> +                                    kern_arg.ssidbuf_size,
> +                                    &desc, GFP_KERNEL);
> +               if (ret)
> +                       return ret;
> +
> +               set_xen_guest_handle(kern_arg.ssidbuf, (void *)desc);
> +
> +               ret = xencomm_arch_hypercall_acm_op (cmd, op_desc);
> +
> +               xencomm_free (desc);
> +
> +               if (copy_to_user (arg, &kern_arg, sizeof (kern_arg)))
> +                       return -EFAULT;
> +
> +               return ret;
> +       }
> +       default:
> +               printk("%s: unknown acm_op cmd %d\n", __func__, cmd);
> +               return -ENOSYS;
> +       }
> +
> +       return ret;
> +}
> +
> +static int xencomm_privcmd_memory_op(privcmd_hypercall_t *hypercall)
> +{
> +       const unsigned long cmd = hypercall->arg[0];
> +       int ret = 0;
> +
> +       switch (cmd) {
> +       case XENMEM_increase_reservation:
> +       case XENMEM_decrease_reservation:
> +       {
> +               xen_memory_reservation_t kern_op;
> +               xen_memory_reservation_t __user *user_op;
> +               struct xencomm_handle *desc = NULL;
> +               struct xencomm_handle *desc_op;
> +
> +               user_op = (xen_memory_reservation_t __user
> *)hypercall->arg[1];
> +               if (copy_from_user(&kern_op, user_op,
> +                                  sizeof(xen_memory_reservation_t)))
> +                       return -EFAULT;
> +               desc_op = xencomm_create_inline (&kern_op, sizeof
> (kern_op));
> +
> +               if (xen_guest_handle(kern_op.extent_start)) {
> +                       void * addr;
> +
> +                       addr = xen_guest_handle(kern_op.extent_start);
> +                       ret = xencomm_create
> +                               (addr,
> +                                kern_op.nr_extents *
> +                                sizeof(*xen_guest_handle
> +                                       (kern_op.extent_start)),
> +                                &desc, GFP_KERNEL);
> +                       if (ret)
> +                               return ret;
> +                       set_xen_guest_handle(kern_op.extent_start,
> +                                            (void *)desc);
> +               }
> +
> +               ret = xencomm_arch_hypercall_memory_op (cmd, desc_op);
> +
> +               if (desc)
> +                       xencomm_free (desc);
> +
> +               if (ret != 0)
> +                       return ret;
> +
> +               if (copy_to_user(user_op, &kern_op,
> +                                sizeof(xen_memory_reservation_t)))
> +                       return -EFAULT;
> +
> +               return ret;
> +       }
> +       default:
> +               printk("%s: unknown memory op %lu\n", __func__, cmd);
> +               ret = -ENOSYS;
> +       }
> +       return ret;
> +}
> +
> +static int xencomm_privcmd_xen_version(privcmd_hypercall_t
> *hypercall)
> +{
> +       int cmd = hypercall->arg[0];
> +       void __user *arg = (void __user *)hypercall->arg[1];
> +       struct xencomm_handle *desc;
> +       size_t argsize;
> +       int rc;
> +
> +       switch (cmd) {
> +       case XENVER_version:
> +               /* do not actually pass an argument */
> +               return xencomm_arch_hypercall_xen_version (cmd, 0);
> +       case XENVER_extraversion:
> +               argsize = sizeof(xen_extraversion_t);
> +               break;
> +       case XENVER_compile_info:
> +               argsize = sizeof(xen_compile_info_t);
> +               break;
> +       case XENVER_capabilities:
> +               argsize = sizeof(xen_capabilities_info_t);
> +               break;
> +       case XENVER_changeset:
> +               argsize = sizeof(xen_changeset_info_t);
> +               break;
> +       case XENVER_platform_parameters:
> +               argsize = sizeof(xen_platform_parameters_t);
> +               break;
> +       case XENVER_pagesize:
> +               argsize = (arg == NULL) ? 0 : sizeof(void *);
> +               break;
> +       case XENVER_get_features:
> +               argsize = (arg == NULL) ? 0 :
> sizeof(xen_feature_info_t);
> +               break;
> +
> +       default:
> +               printk("%s: unknown version op %d\n", __func__, cmd);
> +               return -ENOSYS;
> +       }
> +
> +       rc = xencomm_create(arg, argsize, &desc, GFP_KERNEL);
> +       if (rc)
> +               return rc;
> +
> +       rc = xencomm_arch_hypercall_xen_version (cmd, desc);
> +
> +       xencomm_free(desc);
> +
> +       return rc;
> +}
> +
> +static int xencomm_privcmd_event_channel_op(privcmd_hypercall_t
> *hypercall)
> +{
> +       int cmd = hypercall->arg[0];
> +       struct xencomm_handle *desc;
> +       unsigned int argsize;
> +       int ret;
> +
> +       switch (cmd) {
> +       case EVTCHNOP_alloc_unbound:
> +               argsize = sizeof(evtchn_alloc_unbound_t);
> +               break;
> +
> +       case EVTCHNOP_status:
> +               argsize = sizeof(evtchn_status_t);
> +               break;
> +
> +       default:
> +               printk("%s: unknown EVTCHNOP %d\n", __func__, cmd);
> +               return -EINVAL;
> +       }
> +
> +       ret = xencomm_create((void *)hypercall->arg[1], argsize,
> +                            &desc, GFP_KERNEL);
> +       if (ret)
> +               return ret;
> +
> +       ret = xencomm_arch_hypercall_event_channel_op (cmd, desc);
> +
> +       xencomm_free(desc);
> +       return ret;
> +}
> +
> +int xencomm_privcmd_hypercall(privcmd_hypercall_t *hypercall)
> +{
> +       switch (hypercall->op) {
> +       case __HYPERVISOR_dom0_op:
> +               return xencomm_privcmd_dom0_op(hypercall);
> +        case __HYPERVISOR_acm_op:
> +               return xencomm_privcmd_acm_op(hypercall);
> +       case __HYPERVISOR_xen_version:
> +               return xencomm_privcmd_xen_version(hypercall);
> +       case __HYPERVISOR_memory_op:
> +               return xencomm_privcmd_memory_op(hypercall);
> +       case __HYPERVISOR_event_channel_op:
> +               return xencomm_privcmd_event_channel_op(hypercall);
> +       default:
> +               printk("%s: unknown hcall (%ld)\n", __func__,
> hypercall->op);
> +               return -ENOSYS;
> +       }
> +}
> +
> diff -r b7db009d622c linux-2.6-xen-sparse/include/xen/xencomm.h
> --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> +++ b/linux-2.6-xen-sparse/include/xen/xencomm.h        Mon Aug 21
> 15:04:32 2006 +0200
> @@ -0,0 +1,45 @@
> +/*
> + * Copyright (C) 2006 Hollis Blanchard <hollisb@xxxxxxxxxx>, 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 _LINUX_XENCOMM_H_
> +#define _LINUX_XENCOMM_H_
> +
> +#include <xen/interface/xencomm.h>
> +
> +#define XENCOMM_MINI_ADDRS 3
> +struct xencomm_mini {
> +    struct xencomm_desc _desc;
> +    uint64_t address[XENCOMM_MINI_ADDRS];
> +};
> +#define XENCOMM_MINI_AREA (sizeof(struct xencomm_mini) * 2)

Remove above.

> +/* To avoid additionnal virt to phys convertion, the user only sees
> handle
> +   which are opaque structures.  */
> +struct xencomm_handle;

Typos in the comment.

> +extern int xencomm_create(void *buffer, unsigned long bytes,
> +                         struct xencomm_handle **desc, gfp_t type);
> +extern void xencomm_free(struct xencomm_handle *desc);
> +extern int xencomm_create_mini(void *area, int arealen, void *buffer,
> +            unsigned long bytes, struct xencomm_handle **ret);

Remove above.

> +struct xencomm_handle *xencomm_create_inline (void *buffer,
> +                                             unsigned long bytes);
> +
> +#define xen_guest_handle(hnd)  ((hnd).p)
> +
> +#endif /* _LINUX_XENCOMM_H_ */
> diff -r b7db009d622c linux-2.6-xen-sparse/include/xen/xencomm_hcall.h
> --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> +++ b/linux-2.6-xen-sparse/include/xen/xencomm_hcall.h  Mon Aug 21
> 15:04:32 2006 +0200
> @@ -0,0 +1,45 @@
> +/*
> + * Copyright (C) 2006 Tristan Gingold <tristan.gingold@xxxxxxxx>,
> Bull SAS
> + *
> + * 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 _LINUX_XENCOMM_HCALL_H_
> +#define _LINUX_XENCOMM_HCALL_H_
> +
> +/* These function creates inline descriptor for the parameters and
> +   calls the correspondig xencomm_arch_hypercall_X.
> +   Architectures should defines HYPERVISOR_xxx as
> xencomm_hypercall_xxx unless
> +   they want to use their own wrapper.  */

"corresponding"

And I'm not clear on the reason for all the xencomm_arch_*, especially
because I haven't seen IA64's. If you're worried about the structure
size conversion I mentioned earlier, I think PowerPC will need to fix
that *before* the xencomm stuff is called anyways. So unless IA64 needs
something funny in xencomm_arch_*, they should all be removed.

-- 
Hollis Blanchard
IBM Linux Technology Center


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