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

[Jimi Xenidis <jimix@xxxxxxxxxxxxxx>]

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