Re: [XenPPC] [PATCH] [RFC] Fix xenminicom optimizations to work for module

[Hollis Blanchard <hollisb@xxxxxxxxxx>]

On Wed, 2007-01-10 at 11:51 -0600, Jerone Young wrote:
> 
> diff -r bbf2db4ddf54 arch/powerpc/platforms/xen/gnttab.c
> --- a/arch/powerpc/platforms/xen/gnttab.c       Tue Dec 19 09:22:37 2006 -0500
> +++ b/arch/powerpc/platforms/xen/gnttab.c       Wed Jan 10 00:50:24 2007 -0600
> @@ -264,7 +264,7 @@ int HYPERVISOR_grant_table_op(unsigned i
>                 argsize = sizeof(setup);
> 
>                 frame_list = xencomm_create_inline(
> -                       xen_guest_handle(setup.frame_list));
> +                       xen_guest_handle(setup.frame_list), 0);
> 
>                 set_xen_guest_handle(setup.frame_list, frame_list);
>                 memcpy(op, &setup, sizeof(setup));
> @@ -286,7 +286,7 @@ int HYPERVISOR_grant_table_op(unsigned i
>                 return -ENOSYS;
>         }
> 
> -       desc = xencomm_create_inline(op);
> +       desc = xencomm_create_inline(op, 0);
> 
>         ret = plpar_hcall_norets(XEN_MARK(__HYPERVISOR_grant_table_op), cmd,
>                                  desc, count);

Throughout your entire patch you're using a size of 0. That can't be
right.

> diff -r bbf2db4ddf54 arch/powerpc/platforms/xen/hcall.c
> --- a/arch/powerpc/platforms/xen/hcall.c        Tue Dec 19 09:22:37 2006 -0500
> +++ b/arch/powerpc/platforms/xen/hcall.c        Wed Jan 10 10:30:08 2007 -0600
> @@ -63,7 +83,22 @@ EXPORT_SYMBOL(HYPERVISOR_console_io);
> 
>  int HYPERVISOR_event_channel_op(int cmd, void *op)
>  {
> -       void *desc = xencomm_create_inline(op);
> +       char xc_area[XENCOMM_MINI_AREA];
> +       struct xencomm_desc *x_desc;
> +       int rc;
> +
> +       void *desc = xencomm_create_inline(op, sizeof(evtchn_op_t));
> +
> +       if (desc == NULL) {
> +               rc = xencomm_create_mini(xc_area, XENCOMM_MINI_AREA,
> +                                       op, sizeof(evtchn_op_t), &x_desc);
> +               if (rc)
> +                       return rc;
> +
> +               rc = 
> plpar_hcall_norets(XEN_MARK(__HYPERVISOR_event_channel_op),
> +                                       cmd, __pa(x_desc));
> +               return rc;
> +       }
> 
>         return plpar_hcall_norets(XEN_MARK(__HYPERVISOR_event_channel_op),
>                                 cmd, desc);

You don't need both desc and x_desc. Just remove x_desc.

> 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 10 01:15:50 2007 -0600
> @@ -119,13 +119,59 @@ int xencomm_create(void *buffer, unsigne
>         return 0;
>  }
> 
> -void *xencomm_create_inline(void *ptr)
> +void *xencomm_create_inline(void *ptr, unsigned long bytes)
>  {
>         unsigned long paddr;
> -
> -       BUG_ON(!is_kernel_addr((unsigned long)ptr));
> -
> +       unsigned long first;
> +       unsigned long last;
> +       
> +       first = xencomm_vtop(ptr) & PAGE_MASK;
> +       last = xencomm_vtop(ptr+bytes) & PAGE_MASK;

Rename "first" and "last" to something like "first_phys_page" and
"last_phys_page".

Does this code actually work? It seemed like the problem with your other
patch was that xencomm_vtop() doesn't work early. A simpler but
overly-broad test could work here:
        first_page = ptr & PAGE_MASK;
        last_page = (ptr + bytes) & PAGE_MASK;

> +       if (first != last)
> +               return NULL;
>         paddr = (unsigned long)xencomm_pa(ptr);
>         BUG_ON(paddr & XENCOMM_INLINE_FLAG);
>         return (void *)(paddr | XENCOMM_INLINE_FLAG);
>  }

How has this patch been tested?

-- 
Hollis Blanchard
IBM Linux Technology Center


_______________________________________________
Xen-ppc-devel mailing list
Xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ppc-devel