Thank you for review. I will try to simplfy/clean up it.
Probably I will split the patch into the consolidation part (maddr and vaddr),
bug fix part (page boundary check and get_page()/put_page()),
and new feature part(multipage support).
BTW although I know you need to test it before ack,
how do you like other patches (2/4 and 3/4)?
I'd like to finish linux side xencomm consolidation at first so that
I can focus on xen side xencomm.
On Mon, Aug 13, 2007 at 03:08:58PM -0500, Hollis Blanchard wrote:
> > +static int
> > +xencomm_paddr_to_vaddr(unsigned long paddr, unsigned long *vaddr,
> > + struct page_info **page)
>
> Since we can use page_to_vaddr(), I don't think you need to pass 'vaddr'
> here. That should simplify the code a little bit.
I see.
> By the way, this looks bogus:
> >
> > +static int
> > +xencomm_get_address(const void *handle, struct xencomm_desc *desc,
> > int i,
> > + unsigned long **address, struct page_info **page)
> > +{
> > + if (i == 0)
> > + *address = &desc->address[0];
> > + else
> > + (*address)++;
>
> Shouldn't that be *address = &desc->address[i] ?
This is very confusing point. The array is paddr contiguous, but not maddr
contiguous. So we can't calculate it by simple offsetting.
> I definitely agree that some of these fixes are needed (especially
> checking for the descriptor page overlap, and using get/put_page()).
> However, this code is difficult to follow already, and these patches are
> confusing *me* (and I wrote it! :) so I'm very nervous about increasing
> the complexity.
>
> Since the only issue you've identified is populate_physmap, and that has
> an easy workaround, I would prefer the easier solution.
Understood. I have to admit that the patch is complex.
I hope splitting the patch will help.
--
yamahata
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home