Re: [Xen-devel] Re: [XenPPC] [PATCH 1/4] xencomm take 2: xen side varisous fixes and preparation for consolidation

[Isaku Yamahata <yamahata@xxxxxxxxxxxxx>]

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