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-ppc-devel mailing list
Xen-ppc-devel@lists.xensource.com
http://lists.xensource.com/xen-ppc-devel

Reply via email to