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