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: Hollis Blanchard <hollisb@xxxxxxxxxx>
Subject: [Xen-devel] Re: [XenPPC] RFC: xencomm - linux side
From: Tristan Gingold <Tristan.Gingold@xxxxxxxx>
Date: Wed, 23 Aug 2006 09:59:55 +0200
Cc: xen-ia64-devel <xen-ia64-devel@xxxxxxxxxxxxxxxxxxx>, xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Wed, 23 Aug 2006 00:55:49 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <1156273410.8683.52.camel@xxxxxxxxxxxxxxxxxxxxx>
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>
References: <200608211718.50751.Tristan.Gingold@xxxxxxxx> <1156273410.8683.52.camel@xxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: KMail/1.5
Le Mardi 22 Août 2006 21:03, Hollis Blanchard a écrit :
> 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?
The arch Kconfig overrides this parameter.

> > 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.
Ok, I will send a separat patch.

[...]
> 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().
I simply prefer not to touch other people code, as I can't try xen/x86.

[...]
> > +/* 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.
I will try to improve this.

[...]
> > +/* 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.
Ok.

[...]
>
> *_mini are unused and should be removed entirely.
Ok.

> > +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);
> +}
It is defined in arch-xxx.h file.

> > +#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.
Ok.

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

[...]
> > +       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.
I hope this could be fixed once xencomm is used by ia64.

[...]
> > +       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.
The code branches to out iff xencomm allocation failed.  It is safe to call 
xencomm_free but useless.

> 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. ;)
Yes.

[...]
> > +#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.
Ok.

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

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

[...]
> > +/* 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"
Oops.

> 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.
See explaination above, basically xencomm_arch_* are the raw hypercalls.

Tristan.

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