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

[Jerone Young <jyoung5@xxxxxxxxxx>]

On Thu, 2007-01-11 at 13:06 -0600, Hollis Blanchard wrote:
> 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.

So the issue here is what else to use to use the userspace function. It
looked liked like this was actually a correct way of going about it in
this case. I'll figure something better.

> 
> > -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.

Will drop it.

> 
> 
> 
> 
> 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)?

Got the basics. I'll get out another patch and that will show if I got
what you are saying.

> 
> --
> Hollis Blanchard
> IBM Linux Technology Center
> 


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