diff mbox

[v2,REPOST,01/12,x86|arm] : remove code duplication

Message ID 20170822145107.6877-2-paul.durrant@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paul Durrant Aug. 22, 2017, 2:50 p.m. UTC 
There is a substantial amount of code duplicated between the x86 and arm
implementations of mm.c:xenmem_add_to_physmap_one() for
XENMAPSPACE_grant_table. Also, the code in question looks like it really
should be in common/grant_table.c

This patch introduces a new function in common/grant_table.c to get the mfn
of a specified frame in the grant table of a specified guest, and calls to
that from the arch-specific code in mm.c.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/arm/mm.c             | 29 ++++-------------------------
 xen/arch/x86/mm.c             | 26 +++-----------------------
 xen/common/grant_table.c      | 33 +++++++++++++++++++++++++++++++++
 xen/include/xen/grant_table.h |  3 +++
 4 files changed, 43 insertions(+), 48 deletions(-)

Comments

Jan Beulich Aug. 24, 2017, 2:12 p.m. UTC | #1 
>>> On 22.08.17 at 16:50, <paul.durrant@citrix.com> wrote:
> There is a substantial amount of code duplicated between the x86 and arm
> implementations of mm.c:xenmem_add_to_physmap_one() for
> XENMAPSPACE_grant_table. Also, the code in question looks like it really
> should be in common/grant_table.c
> 
> This patch introduces a new function in common/grant_table.c to get the mfn
> of a specified frame in the grant table of a specified guest, and calls to
> that from the arch-specific code in mm.c.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

This would need re-basing over the XSA-235 fix, but I prefer
Jürgen's similar change, as it retains the present locking
properties better on ARM.

Jan
Paul Durrant Aug. 24, 2017, 2:16 p.m. UTC | #2 
> -----Original Message-----

> From: Jan Beulich [mailto:JBeulich@suse.com]

> Sent: 24 August 2017 15:13

> To: Paul Durrant <Paul.Durrant@citrix.com>

> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper

> <Andrew.Cooper3@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>;

> xen-devel@lists.xenproject.org

> Subject: Re: [PATCH v2 REPOST 01/12] [x86|arm]: remove code duplication

> 

> >>> On 22.08.17 at 16:50, <paul.durrant@citrix.com> wrote:

> > There is a substantial amount of code duplicated between the x86 and arm

> > implementations of mm.c:xenmem_add_to_physmap_one() for

> > XENMAPSPACE_grant_table. Also, the code in question looks like it really

> > should be in common/grant_table.c

> >

> > This patch introduces a new function in common/grant_table.c to get the

> mfn

> > of a specified frame in the grant table of a specified guest, and calls to

> > that from the arch-specific code in mm.c.

> >

> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

> 

> This would need re-basing over the XSA-235 fix, but I prefer

> Jürgen's similar change, as it retains the present locking

> properties better on ARM.


That's ok. I'm happy to rebase on that once it is committed.

  Paul

> 

> Jan
diff mbox

Patch

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index a810a056d7..5ae9607821 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1229,32 +1229,11 @@  int xenmem_add_to_physmap_one(
     switch ( space )
     {
     case XENMAPSPACE_grant_table:
-        grant_write_lock(d->grant_table);
-
-        if ( d->grant_table->gt_version == 0 )
-            d->grant_table->gt_version = 1;
-
-        if ( d->grant_table->gt_version == 2 &&
-                (idx & XENMAPIDX_grant_table_status) )
-        {
-            idx &= ~XENMAPIDX_grant_table_status;
-            if ( idx < nr_status_frames(d->grant_table) )
-                mfn = virt_to_mfn(d->grant_table->status[idx]);
-            else
-                return -EINVAL;
-        }
-        else
-        {
-            if ( (idx >= nr_grant_frames(d->grant_table)) &&
-                 (idx < max_grant_frames) )
-                gnttab_grow_table(d, idx + 1);
-
-            if ( idx < nr_grant_frames(d->grant_table) )
-                mfn = virt_to_mfn(d->grant_table->shared_raw[idx]);
-            else
-                return -EINVAL;
-        }
+        mfn = gnttab_get_frame(d, idx);
+        if ( mfn_eq(mfn, INVALID_MFN) )
+            return -EINVAL;
 
+        grant_write_lock(d->grant_table);
         d->arch.grant_table_gfn[idx] = gfn;
 
         t = p2m_ram_rw;
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 5b863c6fa6..0abb1e284f 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4639,29 +4639,9 @@  int xenmem_add_to_physmap_one(
                 mfn = virt_to_mfn(d->shared_info);
             break;
         case XENMAPSPACE_grant_table:
-            grant_write_lock(d->grant_table);
-
-            if ( d->grant_table->gt_version == 0 )
-                d->grant_table->gt_version = 1;
-
-            if ( d->grant_table->gt_version == 2 &&
-                 (idx & XENMAPIDX_grant_table_status) )
-            {
-                idx &= ~XENMAPIDX_grant_table_status;
-                if ( idx < nr_status_frames(d->grant_table) )
-                    mfn = virt_to_mfn(d->grant_table->status[idx]);
-            }
-            else
-            {
-                if ( (idx >= nr_grant_frames(d->grant_table)) &&
-                     (idx < max_grant_frames) )
-                    gnttab_grow_table(d, idx + 1);
-
-                if ( idx < nr_grant_frames(d->grant_table) )
-                    mfn = virt_to_mfn(d->grant_table->shared_raw[idx]);
-            }
-
-            grant_write_unlock(d->grant_table);
+            mfn = mfn_x(gnttab_get_frame(d, idx));
+            if ( mfn_eq(_mfn(mfn), INVALID_MFN) )
+                return -EINVAL;
             break;
         case XENMAPSPACE_gmfn_range:
         case XENMAPSPACE_gmfn:
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 0f9dd1e706..b327458301 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1621,6 +1621,39 @@  active_alloc_failed:
     return 0;
 }
 
+mfn_t
+gnttab_get_frame(struct domain *d, unsigned int idx)
+{
+    struct grant_table *gt = d->grant_table;
+    mfn_t mfn = INVALID_MFN;
+
+    grant_write_lock(gt);
+
+    if ( gt->gt_version == 0 )
+        gt->gt_version = 1;
+
+    if ( gt->gt_version == 2 &&
+         (idx & XENMAPIDX_grant_table_status) )
+    {
+        idx &= ~XENMAPIDX_grant_table_status;
+        if ( idx < nr_status_frames(gt) )
+            mfn = _mfn(virt_to_mfn(gt->status[idx]));
+    }
+    else
+    {
+        if ( (idx >= nr_grant_frames(gt)) &&
+             (idx < max_grant_frames) )
+            gnttab_grow_table(d, idx + 1);
+
+        if ( idx < nr_grant_frames(gt) )
+            mfn = _mfn(virt_to_mfn(gt->shared_raw[idx]));
+    }
+
+    grant_write_unlock(gt);
+
+    return mfn;
+}
+
 static long 
 gnttab_setup_table(
     XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count)
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index b5af21b53c..079cf82a1e 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -129,6 +129,9 @@  gnttab_release_mappings(
 int
 gnttab_grow_table(struct domain *d, unsigned int req_nr_frames);
 
+/* Get mfn of grant frame */
+mfn_t gnttab_get_frame(struct domain *d, unsigned int idx);
+
 /* Number of grant table frames. Caller must hold d's grant table lock. */
 static inline unsigned int nr_grant_frames(struct grant_table *gt)
 {