[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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@xxxxxxx>

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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.