I'll need to rework the patch. I see there are some inadvertant bugs ..
but somehow it did pass testing. I'll fix these up and redo the patch.
Thanks for the feedback.
On Fri, 2007-01-26 at 14:05 -0500, Jimi Xenidis wrote:
> On Jan 25, 2007, at 8:06 PM, Jerone Young wrote:
>
> > This patch should address all the issues Jimi has pointed out.
> yes it does... thank you.
> but still has a few issues..
>
> Throughout the patch:
> 1. xencomm_create_inline() could never fail, xencomm_map() can so you
> need to check ALL of them.
> 1. _inline() never allocates _map() can so you always have to call
> xencomm_free()
> 1. Please return a -ERRNO and not -1 all the time.
> 1. you made the descriptor void * but not everywhere.
>
> other specific issues:
>
> > @@ -246,9 +244,9 @@ static int xenppc_privcmd_domctl(privcmd
> > return -EACCES;
> > }
> > - ret = xencomm_create(&kern_op, sizeof(xen_domctl_t), &op_desc,
> > GFP_KERNEL);
> > - if (ret)
> > - return ret;
> > + op_desc = xencomm_map(&kern_op, sizeof(xen_domctl_t));
> > + if (op_desc)
> > + return -1;
>
> if (op_desc) cannot be right.
> Can't see how domain creation could have possibly worked, did you
> test that?
>
>
> > diff -r bbf2db4ddf54 drivers/xen/core/xencomm.c
> > --- a/drivers/xen/core/xencomm.c Tue Dec 19 09:22:37 2006 -0500
> > +++ b/drivers/xen/core/xencomm.c Wed Jan 26 02:59:31 2028 -0600
> > @@ -82,11 +82,11 @@ static struct xencomm_desc *xencomm_allo
> > void xencomm_free(struct xencomm_desc *desc)
>
> _map() returns a "void *" so _free() should take one.
>
> > {
> > - if (desc)
> > + if (desc && !((ulong)desc & XENCOMM_INLINE_FLAG))
> > free_page((unsigned long)desc);
> > }
> > -int xencomm_create(void *buffer, unsigned long bytes, struct
> > xencomm_desc **ret, gfp_t gfp_mask)
> > +static int xencomm_create(void *buffer, unsigned long bytes,
> > struct xencomm_desc **ret, gfp_t gfp_mask)
> > {
> > struct xencomm_desc *desc;
> > int rc;
> > @@ -119,13 +119,68 @@ int xencomm_create(void *buffer, unsigne
> > return 0;
> > }
> > -void *xencomm_create_inline(void *ptr)
> > +static void *xencomm_create_inline(void *ptr)
> > {
> > unsigned long paddr;
> > - BUG_ON(!is_kernel_addr((unsigned long)ptr));
> > + BUG_ON(!is_phys_contiguous((unsigned long)ptr));
> > paddr = (unsigned long)xencomm_pa(ptr);
> > BUG_ON(paddr & XENCOMM_INLINE_FLAG);
> > return (void *)(paddr | XENCOMM_INLINE_FLAG);
> > }
> > +
> > +/* "mini" routine, for stack-based communications: */
> > +static int xencomm_create_mini(int arealen, void *buffer,
> > + unsigned long bytes, struct xencomm_desc **ret)
> > +{
> > + struct xencomm_desc desc;
>
> You are returning a stack/auto variable here, thats a No No!
>
> > + int rc = 0;
> > +
> > + desc.nr_addrs = XENCOMM_MINI_ADDRS;
> > +
> > + if (! (rc = xencomm_init(&desc, buffer, bytes)));
> > + *ret = &desc;
> > +
> > + return rc;
> > +}
> > +
> > +void *xencomm_map(void *ptr, unsigned long bytes)
> > +{
> > + int rc;
> > + struct xencomm_desc *desc;
> > +
> > + if (is_phys_contiguous((unsigned long)ptr))
> > + return xencomm_create_inline(ptr);
> > +
> > + rc = xencomm_create(ptr, bytes, &desc, GFP_KERNEL);
> > +
> > + if (rc)
> > + return NULL;
> > +
> > + return xencomm_pa(desc);
> > +}
> > +
> > +void *__xencomm_map_early(void *ptr, unsigned long bytes,
> > + struct xencomm_mini *xc_area)
> > +{
> > + int rc;
> > + struct xencomm_desc *desc = NULL;
> > +
> > + if (is_phys_contiguous((unsigned long)ptr))
> > + return xencomm_create_inline(ptr);
> > +
> > + rc = xencomm_create_mini(XENCOMM_MINI_AREA,ptr, bytes,
>
> whitespace
>
> > + &desc);
> > +
> > + if (rc)
> > + return NULL;
> > +
> > + return (void*)__pa(desc);
> > +}
> > +
> > +/* check if is physically contiguous memory */
> > +int is_phys_contiguous(unsigned long addr)
>
> Can we please make this static now!
>
> > +{
> > + return (addr < VMALLOC_START) || (addr >= VMALLOC_END);
> > +}
> > diff -r bbf2db4ddf54 include/xen/xencomm.h
> > --- a/include/xen/xencomm.h Tue Dec 19 09:22:37 2006 -0500
> > +++ b/include/xen/xencomm.h Wed Jan 26 02:17:31 2028 -0600
> > @@ -16,6 +16,7 @@
> > * Copyright (C) IBM Corp. 2006
> > *
> > * Authors: Hollis Blanchard <hollisb@xxxxxxxxxx>
> > + * Jerone Young <jyoung5@xxxxxxxxxx>
> > */
> > #ifndef _LINUX_XENCOMM_H_
> > @@ -23,10 +24,23 @@
> > #include <xen/interface/xencomm.h>
> > -extern int xencomm_create(void *buffer, unsigned long bytes,
> > - struct xencomm_desc **desc, gfp_t type);
> > +#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)
>
> This is no longer used, right?
>
> > +
> > extern void xencomm_free(struct xencomm_desc *desc);
> > -extern void *xencomm_create_inline(void *ptr);
> > +extern void *xencomm_map(void *ptr, unsigned long bytes);
> > +extern void *__xencomm_map_early(void *ptr, unsigned long bytes,
> > + struct xencomm_mini *xc_area);
> > +extern int is_phys_contiguous(unsigned long addr);
> begone
>
> > +
> > +#define xencomm_map_early(ptr, bytes) \
> > + ({struct xencomm_mini xc_area\
> > + __attribute__((__aligned__(sizeof(struct xencomm_mini))));\
> > + __xencomm_map_early(ptr, bytes, &xc_area);})
> > /* provided by architecture code: */
> > extern unsigned long xencomm_vtop(unsigned long vaddr);
>
_______________________________________________
Xen-ppc-devel mailing list
Xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ppc-devel
|
Home