Re: [XenPPC] [PATCH] [UPDATE] Xencomm optimzation to work for modules

[Jerone Young <jyoung5@xxxxxxxxxx>]

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