From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v8 05/14] arch/arm: unmap partially-mapped I/O-memory regions Date: Thu, 5 Jun 2014 15:09:28 +0100 Message-ID: <53907A18.8060608@citrix.com> References: <1401015115-7610-1-git-send-email-avanzini.arianna@gmail.com> <1401015115-7610-6-git-send-email-avanzini.arianna@gmail.com> <5382147F.3010408@linaro.org> <1401976980.15729.99.camel@hastur.hellion.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1401976980.15729.99.camel@hastur.hellion.org.uk> 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 , Julien Grall Cc: paolo.valente@unimore.it, keir@xen.org, stefano.stabellini@eu.citrix.com, tim@xen.org, dario.faggioli@citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, Ian.Campbell@eu.citrix.com, etrudeau@broadcom.com, andrew.cooper3@citrix.com, JBeulich@suse.com, Arianna Avanzini , viktor.kleinik@globallogic.com List-Id: xen-devel@lists.xenproject.org On 06/05/2014 03:03 PM, Ian Campbell wrote: > On Sun, 2014-05-25 at 17:04 +0100, Julien Grall wrote: >> Hi Arianna, >> >> On 25/05/14 11:51, Arianna Avanzini wrote: >>> This commit changes the interface of apply_p2m_changes() to accept >>> optionally the pointer to a counter of successfully performed >>> mappings; such a counter is used only in case of INSERT operation. >>> If an error is encountered during the operation, and therefore the >>> mapping is only partially performed, such a counter is useful to >>> let the caller be able to undo what has just been done. >> >> I don't think you need to introduce another argument to >> apply_p2m_changes. You can directly call apply_p2m_changes with the >> whole range. > > Due to the sanity checks which Arianna is adding to the unmap/REMOVE > case I think it would no longer be valid to call unmap on the entire > range unless the entire range was successfully mapped. > > I was nearly going to suggest that apply_p2m_changes could return the > number of successful mappings so that the caller could check actual == > expected, but that removes the possibility to provide a specific error > code as well, so I think that is not acceptable. > > I can't remember: did we consider and discard the possibility that > apply_p2m_changes should unwind its own progress on failure to INSERT > (perhaps by recursively calling itself with REMOVE)? That would seem to > be what most callers of INSERT would expect I think (despite many of > them passing NULL for this new argument here) We didn't consider this solution. I think unwind the mapping in this function would be better. Most the function which pass NULL handle only one mapping at the time (except guest_physmap_add_entry but there is only one caller with an order > 0 which is located in arch/arm/domain_build.c). In anycase, I think it's safer to unwind the whole range if we fail to add on mapping. Regards, -- Julien Grall