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.

Xen-ppc-devel mailing list

Reply via email to