WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] Re: [XenPPC] [PATCH 1/4] xencomm take 2: xen side variso

To: Hollis Blanchard <hollisb@xxxxxxxxxx>
Subject: Re: [Xen-devel] Re: [XenPPC] [PATCH 1/4] xencomm take 2: xen side varisous fixes and preparation for consolidation
From: Isaku Yamahata <yamahata@xxxxxxxxxxxxx>
Date: Tue, 14 Aug 2007 10:39:50 +0900
Cc: xen-ia64-devel@xxxxxxxxxxxxxxxxxxx, xen-devel@xxxxxxxxxxxxxxxxxxx, xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Mon, 13 Aug 2007 18:40:23 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <1187035738.31317.21.camel@basalt>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <20070813035910.GA20100%yamahata@xxxxxxxxxxxxx> <1187035738.31317.21.camel@basalt>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.4.2.1i
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