From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel De Graaf Subject: Re: [PATCH] libxc: fix xc_gntshr_munmap semantic Date: Tue, 30 Apr 2013 10:19:10 -0400 Message-ID: <517FD2DE.3050000@tycho.nsa.gov> References: <517A840F.7060105@invisiblethingslab.com> <20130426134535.5BA912AE@duch.mimuw.edu.pl> <1366987441.3142.128.camel@zakaz.uk.xensource.com> <517A9A06.8010004@tycho.nsa.gov> <1366990016.3142.145.camel@zakaz.uk.xensource.com> <517AA8AA.9060206@tycho.nsa.gov> <1367318396.3142.499.camel@zakaz.uk.xensource.com> <517FC561.1050309@tycho.nsa.gov> <1367330436.3142.531.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1367330436.3142.531.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: Marek Marczykowski , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 04/30/2013 10:00 AM, Ian Campbell wrote: > On Tue, 2013-04-30 at 14:21 +0100, Daniel De Graaf wrote: >> On 04/30/2013 06:39 AM, Ian Campbell wrote: >>> On Fri, 2013-04-26 at 17:17 +0100, Daniel De Graaf wrote: >>>> On 04/26/2013 11:26 AM, Ian Campbell wrote: >>>>> On Fri, 2013-04-26 at 16:15 +0100, Daniel De Graaf wrote: >>>>>> On 04/26/2013 10:44 AM, Ian Campbell wrote: >>>>>>> On Fri, 2013-04-26 at 13:40 +0100, Marek Marczykowski wrote: >>>>>>>> "count" parameter should be pages count (as stated in comment in >>>>>>>> xenctrl.h), not bytes count. >>>>>>>> This patch fixes also the only user of this function (in xen sources) - >>>>>>>> libvchan. >>>>>>> >>>>>>> Looks ok to me but Daniel De Graaf wrote all this stuff, Ccing him. >>>>>> >>>>>> This also looks good to me. >>>>> >>>>> May I take that as an Ack (or a Reviewed-by if you prefer)? >>>> >>>> Yes, either one is fine. >>>> >>>> Acked-by: Daniel De Graaf >>> >>> Is the change from munmap to xc_gntshr_munmap, which wasn't mentioned in >>> the changelog description (tut tut), correct? It seems like these >>> mappings can either be establish with xc_gntshr_share_pages or with "= >>> ((void*)ctrl->ring) + LARGE_RING_OFFSET", with the second one being the >>> case I'm concerned about... Should it not duplicate the switch used at >>> mapping time? >>> >>> Ian. >>> >> >> The unmap is only done if (ctrl->read.order >= PAGE_SHIFT), which is a >> functional duplicate of the switch statement. > > It's a pretty opaque transformation ;-) > > But for e.g. order 9, it isn't the same is it? The switch on alloc will > hit the default case while the free won't hit this if statement. Order 9 isn't valid: the only permitted values are 10, 11, and 12+, all of which are handled correctly here. >> However, this does bring >> up a different issue: the munmap() should be replaced with the correct >> one of xc_gntshr_munmap and xc_gnttab_munmap depending on ctrl->is_server, >> in the same way as ctrl->ring is unmapped. > > OK, I take it the Ack is rescinded for the time being? > The change is still a strict improvement over the old code, but since more changes are needed to complete the fix, it is rescinded for now. -- Daniel De Graaf National Security Agency