diff mbox

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

Message ID 1467130643-23868-16-git-send-email-julien.grall@arm.com
State Superseded
Headers show

Commit Message

Julien Grall June 28, 2016, 4:17 p.m. UTC 
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>

---
    Changes in v5:
        - Add missing Signed-off-by

    Changes in v4:
        - Patch added
---
 xen/arch/arm/p2m.c | 70 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 36 insertions(+), 34 deletions(-)

Comments

Julien Grall July 6, 2016, 11:20 a.m. UTC | #1 
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,
diff mbox

Patch

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 0fdd11f..a5b584b 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1158,17 +1158,40 @@  out:
     return rc;
 }
 
+static inline int p2m_insert_mapping(struct domain *d,
+                                     gfn_t start_gfn,
+                                     unsigned long nr,
+                                     mfn_t mfn,
+                                     int mattr, p2m_type_t t)
+{
+    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, 0, t, d->arch.p2m.default_access);
+}
+
+static inline int p2m_remove_mapping(struct domain *d,
+                                     gfn_t start_gfn,
+                                     unsigned long nr,
+                                     mfn_t mfn)
+{
+    return apply_p2m_changes(d, REMOVE,
+                             pfn_to_paddr(gfn_x(start_gfn)),
+                             pfn_to_paddr(gfn_x(start_gfn) + nr),
+                             pfn_to_paddr(mfn_x(mfn)),
+                             /* arguments below not used when removing mapping */
+                             MATTR_MEM, 0, p2m_invalid,
+                             d->arch.p2m.default_access);
+}
+
 int map_regions_rw_cache(struct domain *d,
                          gfn_t gfn,
                          unsigned long nr,
                          mfn_t mfn)
 {
-    return apply_p2m_changes(d, INSERT,
-                             pfn_to_paddr(gfn_x(gfn)),
-                             pfn_to_paddr(gfn_x(gfn) + nr),
-                             pfn_to_paddr(mfn_x(mfn)),
-                             MATTR_MEM, 0, p2m_mmio_direct,
-                             d->arch.p2m.default_access);
+    return p2m_insert_mapping(d, gfn, nr, mfn,
+                              MATTR_MEM, p2m_mmio_direct);
 }
 
 int unmap_regions_rw_cache(struct domain *d,
@@ -1176,12 +1199,7 @@  int unmap_regions_rw_cache(struct domain *d,
                            unsigned long nr,
                            mfn_t mfn)
 {
-    return apply_p2m_changes(d, REMOVE,
-                             pfn_to_paddr(gfn_x(gfn)),
-                             pfn_to_paddr(gfn_x(gfn) + nr),
-                             pfn_to_paddr(mfn_x(mfn)),
-                             MATTR_MEM, 0, p2m_invalid,
-                             d->arch.p2m.default_access);
+    return p2m_remove_mapping(d, gfn, nr, mfn);
 }
 
 int map_mmio_regions(struct domain *d,
@@ -1189,12 +1207,8 @@  int map_mmio_regions(struct domain *d,
                      unsigned long nr,
                      mfn_t mfn)
 {
-    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);
+    return p2m_insert_mapping(d, start_gfn, nr, mfn,
+                              MATTR_MEM, p2m_mmio_direct);
 }
 
 int unmap_mmio_regions(struct domain *d,
@@ -1202,12 +1216,7 @@  int unmap_mmio_regions(struct domain *d,
                        unsigned long nr,
                        mfn_t mfn)
 {
-    return apply_p2m_changes(d, REMOVE,
-                             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_invalid,
-                             d->arch.p2m.default_access);
+    return p2m_remove_mapping(d, start_gfn, nr, mfn);
 }
 
 int map_dev_mmio_region(struct domain *d,
@@ -1237,22 +1246,15 @@  int guest_physmap_add_entry(struct domain *d,
                             unsigned long page_order,
                             p2m_type_t t)
 {
-    return apply_p2m_changes(d, INSERT,
-                             pfn_to_paddr(gfn_x(gfn)),
-                             pfn_to_paddr(gfn_x(gfn) + (1 << page_order)),
-                             pfn_to_paddr(mfn_x(mfn)), MATTR_MEM, 0, t,
-                             d->arch.p2m.default_access);
+    return p2m_insert_mapping(d, gfn, (1 << page_order), mfn,
+                              MATTR_MEM, t);
 }
 
 void guest_physmap_remove_page(struct domain *d,
                                gfn_t gfn,
                                mfn_t mfn, unsigned int page_order)
 {
-    apply_p2m_changes(d, REMOVE,
-                      pfn_to_paddr(gfn_x(gfn)),
-                      pfn_to_paddr(gfn_x(gfn) + (1<<page_order)),
-                      pfn_to_paddr(mfn_x(mfn)), MATTR_MEM, 0, p2m_invalid,
-                      d->arch.p2m.default_access);
+    p2m_remove_mapping(d, gfn, (1 << page_order), mfn);
 }
 
 int p2m_alloc_table(struct domain *d)