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
|
Home