[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] Re: [XenPPC] RFC: xencomm - linux side
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |