diff mbox

[Xen-devel,v2,4/8] xen: arm: only put_page for p2m operations which require it.

Message ID 1402504804-29173-4-git-send-email-ian.campbell@citrix.com
State Superseded
Headers show

Commit Message

Ian Campbell June 11, 2014, 4:40 p.m. UTC 
In particular do not do it for CACHEFLUSH.

INSERT, RELINQUISH and REMOVE should all put the page (if the current pte is
valid). ALLOCATE doesn't need to since it asserts the current PTE must be
invalid.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: actual move the put page instead of adding a second one.
---
 xen/arch/arm/p2m.c |   35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

Comments

Julien Grall June 11, 2014, 9:31 p.m. UTC | #1 
Hi Ian,

On 11/06/14 17:40, Ian Campbell wrote:
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 9960e17..ea05b6d 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -299,6 +299,23 @@ enum p2m_operation {
>       CACHEFLUSH,
>   };
>
> +static void p2m_put_page(const lpae_t pte)

NIT: I'm wondering we static inline would be better here.

Acked-by: Julien Grall <julien.grall@linaro.org>

Regards,
Ian Campbell June 12, 2014, 7:27 a.m. UTC | #2 
On Wed, 2014-06-11 at 22:31 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 11/06/14 17:40, Ian Campbell wrote:
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index 9960e17..ea05b6d 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -299,6 +299,23 @@ enum p2m_operation {
> >       CACHEFLUSH,
> >   };
> >
> > +static void p2m_put_page(const lpae_t pte)
> 
> NIT: I'm wondering we static inline would be better here.

I trust the compiler to do what it thinks is best here.

> 
> Acked-by: Julien Grall <julien.grall@linaro.org>
> 
> Regards,
>
diff mbox

Patch

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 9960e17..ea05b6d 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -299,6 +299,23 @@  enum p2m_operation {
     CACHEFLUSH,
 };
 
+static void p2m_put_page(const lpae_t pte)
+{
+    /* TODO: Handle other p2m types
+     *
+     * It's safe to do the put_page here because page_alloc will
+     * flush the TLBs if the page is reallocated before the end of
+     * this loop.
+     */
+    if ( p2m_is_foreign(pte.p2m.type) )
+    {
+        unsigned long mfn = pte.p2m.base;
+
+        ASSERT(mfn_valid(mfn));
+        put_page(mfn_to_page(mfn));
+    }
+}
+
 static int apply_p2m_changes(struct domain *d,
                      enum p2m_operation op,
                      paddr_t start_gpaddr,
@@ -400,20 +417,6 @@  static int apply_p2m_changes(struct domain *d,
 
         flush |= pte.p2m.valid;
 
-        /* TODO: Handle other p2m type
-         *
-         * It's safe to do the put_page here because page_alloc will
-         * flush the TLBs if the page is reallocated before the end of
-         * this loop.
-         */
-        if ( pte.p2m.valid && p2m_is_foreign(pte.p2m.type) )
-        {
-            unsigned long mfn = pte.p2m.base;
-
-            ASSERT(mfn_valid(mfn));
-            put_page(mfn_to_page(mfn));
-        }
-
         switch (op) {
             case ALLOCATE:
                 {
@@ -436,6 +439,8 @@  static int apply_p2m_changes(struct domain *d,
                 break;
             case INSERT:
                 {
+                    if ( pte.p2m.valid )
+                        p2m_put_page(pte);
                     pte = mfn_to_p2m_entry(maddr >> PAGE_SHIFT, mattr, t);
                     p2m_write_pte(&third[third_table_offset(addr)],
                                   pte, flush_pt);
@@ -451,6 +456,8 @@  static int apply_p2m_changes(struct domain *d,
                         break;
                     }
 
+                    p2m_put_page(pte);
+
                     count += 0x10;
 
                     memset(&pte, 0x00, sizeof(pte));