Re: [PATCH v8 05/14] arch/arm: unmap partially-mapped I/O-memory regions

[Julien Grall <julien.grall@citrix.com>]

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