Re: [PATCH v5 15/17] xen/arm: p2m: Introduce helpers to insert and remove mapping

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

Hi Stefano,

On 06/07/16 11:59, Stefano Stabellini wrote:
> On Tue, 28 Jun 2016, Julien Grall wrote:
>> More the half of the arguments of INSERT and REMOVE are the same for
>> each callers. Simplify the callers of apply_p2m_changes by adding new
>> helpers which will fill common arguments with default values.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> I don't see much value in this patch. It looks good because it is before
> "Rework the interface of apply_p2m_changes and use typesafe" in this
> series (therefore eliminates a bunch of temporary casts), but otherwise
> I don't think it would be much of an improvement.

A lot of the parameters of apply_p2m_changes may not apply to a specific 
set of operations. Every time we have to add a new parameters for a 
specific operations, we have to modify all the callers.

Also, with this patch, it is easier to understand what does every 
function without caring of dummy parameters.

For instance if we take the example of INSERT:

return apply_p2m_changes(d, INSERT,
			 pfn_to_paddr(gfn_x(start_gfn)),
			 pfn_to_paddr(gfn_x(start_gfn) + nr),
                          pfn_to_paddr(mfn_x(mfn)),
	                 MATTR_DEV, 0, p2m_mmio_direct,
	                 d->arch.p2m.default_access);

Aside the pfn_to_paddr(*) which will be clean-up in a follow-up patch, 2 
of thoses parameters (2, 7 and 9) are common to all the callers. It is 
hard to understand what means '0' without looking carefully at 
apply_p2m_changes.

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel