On Wed, 2007-01-10 at 12:45 -0600, Hollis Blanchard wrote:
> 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.
Glad you pointed this out. Actually, in these cases I use 0 (why the
patch isn't perfect) to ensure that we are not returned a NULL pointer.
Since this is code that has just been added. Since the check is not
needed in theses cases, but perhaps it will always pass and this is not
going to be a worry.
>
> > 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.
Sounds good.
>
> > 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?
Yes this one has been tested. It boots. Not thoroughly as of yet. So I
just wanted comments of what I had for now.
Thanks for the comments :-)
>
> --
> Hollis Blanchard
> IBM Linux Technology Center
>
_______________________________________________
Xen-ppc-devel mailing list
Xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ppc-devel
|
Home