Re: [XenPPC] [PATCH] Fix xenminicom optimazation to work for modules

[Hollis Blanchard <hollisb@xxxxxxxxxx>]

On Thu, 2007-01-11 at 12:12 -0600, Jerone Young wrote:
> 
> @@ -121,7 +156,7 @@ int HYPERVISOR_xen_version(int cmd, void
>  int HYPERVISOR_xen_version(int cmd, void *arg)
>  {
>         if (is_kernel_addr((unsigned long)arg)) {
> -               void *desc = xencomm_create_inline(arg);
> +               void *desc = xencomm_create_inline(arg, sizeof(__u64));
>                 return plpar_hcall_norets(XEN_MARK(__HYPERVISOR_xen_version), 
> cmd, desc);
>         }
>         return HYPERVISOR_xen_version_userspace(cmd, arg);

Any use of is_kernel_addr() is a red flag.

> -void *xencomm_create_inline(void *ptr)
> +void *xencomm_create_inline(void *ptr, unsigned long bytes)
>  {
>         unsigned long paddr;
> +       unsigned long first_page;
> +       unsigned long last_page;
> 
> -       BUG_ON(!is_kernel_addr((unsigned long)ptr));
> +       first_page = xencomm_vtop(ptr) & PAGE_MASK;
> +       last_page = xencomm_vtop(ptr + bytes) & PAGE_MASK;
> +
> +       if (first_page != last_page)
> +               return NULL;

I still think you should drop xencomm_vtop(). If ptr and ptr+bytes are
on different virtual pages, they will be on different physical pages
too, so we don't need to do the more expensive virtual-to-physical
translation you're doing here.




But anyways, let's think about abstracting this a little bit.
Pseudo-code below.

First, the test we really want is "is this area of memory physically
contiguous?" If so, we can do inline.

void *xencomm_map(void *ptr, ulong bytes)
{
        if (is_phys_contiguous(ptr))
                return xencomm_create_inline(ptr);
        return xencomm_create(ptr, bytes);
}

In particular we know that vmalloc space, from which modules are
allocated, is not physically contiguous. Other code makes reference to
VMALLOC_START/END, so we can too:

int is_physically_contiguous(ulong addr)
{
        return (ptr < VMALLOC_START) || (ptr >= VMALLOC_END);
}

We can have a separate "early" function:

#define xencomm_map_early(ptr, bytes) \
        char xc_area[bytes*2]; \
        __xencomm_map_early(ptr, bytes, xc_area)

void *__xencomm_map_early(void *ptr, ulong bytes, char *xc_area)
{
        if (is_phys_contiguous(ptr))
                return xencomm_create_inline(ptr);
        return xencomm_create_mini(ptr, bytes, xc_area);
}

(We need that macro because the *caller* needs to allocate stack space.)

With these interfaces, all a caller needs to do is use xencomm_map() or
xencomm_map_early(), and all the details of inline or mini or regular
are hidden.

Does this make sense (to anybody)?

-- 
Hollis Blanchard
IBM Linux Technology Center


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