[For Xen-4.10 PATCH v2 0/3] Reduce unnecessary icache maintenance operations

[For Xen-4.10 PATCH v2 0/3] Reduce unnecessary icache maintenance operations

[Punit Agrawal]

Hi,

This series improves the number of icache flushes performed when
allocating memory to a domain. This series was previously posted at
[0] and [1].

Changes
v1 -> v2:
* Updated tags
* Added a comment for icache_invalidate() definition for x86 to
  explain why it is empty.

RFC -> v1:

* Fixed coding style issue in Patch 1
* Added reviewed-by tags
* Re-worked Patch 3 to defer icache optimisation only during domain creation

Patch 1 adds a parameter to flush_page_to_ram() to prevent performing
icache maintenance per page. Current calls to flush_page_to_ram() loop
over pages and performing a full icache flush for each page is
excessive.

Patch 2 hoists icache maintenance from flush_page_to_ram() to
p2m_cache_flush().

Patch 3 introduces a new MEMF_ flag to indicate to alloc_heap_pages()
that icache maintenance will be performed by the caller. The icache
maintenance operations are performed in populate_physmap() during
domain creation. As I couldn't find icache maintenance operations for
x86, an empty helper is introduced.

If there are no further comments, please consider for inclusion.

Thanks,
Punit

[0] https://www.mail-archive.com/xen-devel@lists.xen.org/msg108002.html
[1] https://www.mail-archive.com/xen-devel@lists.xen.org/msg102934.html

Punit Agrawal (3):
  Allow control of icache invalidations when calling flush_page_to_ram()
  arm: p2m: Prevent redundant icache flushes
  Avoid excess icache flushes in populate_physmap() before domain has
    been created

 xen/arch/arm/mm.c              |  5 +++--
 xen/arch/arm/p2m.c             |  4 +++-
 xen/common/memory.c            | 31 ++++++++++++++++++++++---------
 xen/common/page_alloc.c        |  2 +-
 xen/include/asm-arm/page.h     |  2 +-
 xen/include/asm-x86/flushtlb.h |  2 +-
 xen/include/asm-x86/page.h     |  8 ++++++++
 xen/include/xen/mm.h           |  2 ++
 8 files changed, 41 insertions(+), 15 deletions(-)

-- 
2.11.0


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

[For Xen-4.10 PATCH v2 1/3] Allow control of icache invalidations when calling flush_page_to_ram()

[Punit Agrawal]

flush_page_to_ram() unconditionally drops the icache. In certain
situations this leads to execessive icache flushes when
flush_page_to_ram() ends up being repeatedly called in a loop.

Introduce a parameter to allow callers of flush_page_to_ram() to take
responsibility of synchronising the icache. This is in preparations for
adding logic to make the callers perform the necessary icache
maintenance operations.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/mm.c              | 5 +++--
 xen/arch/arm/p2m.c             | 2 +-
 xen/common/page_alloc.c        | 2 +-
 xen/include/asm-arm/page.h     | 2 +-
 xen/include/asm-x86/flushtlb.h | 2 +-
 5 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 48f74f6e65..082c872c72 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -420,7 +420,7 @@ unsigned long domain_page_map_to_mfn(const void *ptr)
 }
 #endif
 
-void flush_page_to_ram(unsigned long mfn)
+void flush_page_to_ram(unsigned long mfn, bool sync_icache)
 {
     void *v = map_domain_page(_mfn(mfn));
 
@@ -435,7 +435,8 @@ void flush_page_to_ram(unsigned long mfn)
      * I-Cache (See D4.9.2 in ARM DDI 0487A.k_iss10775). Instead of using flush
      * by VA on select platforms, we just flush the entire cache here.
      */
-    invalidate_icache();
+    if ( sync_icache )
+        invalidate_icache();
 }
 
 void __init arch_init_memory(void)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 752e948070..291998938e 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1398,7 +1398,7 @@ int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr)
         /* XXX: Implement preemption */
         while ( gfn_x(start) < gfn_x(next_gfn) )
         {
-            flush_page_to_ram(mfn_x(mfn));
+            flush_page_to_ram(mfn_x(mfn), true);
 
             start = gfn_add(start, 1);
             mfn = mfn_add(mfn, 1);
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 9e41fb4cd3..eba78f1a3d 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -833,7 +833,7 @@ static struct page_info *alloc_heap_pages(
         /* Ensure cache and RAM are consistent for platforms where the
          * guest can control its own visibility of/through the cache.
          */
-        flush_page_to_ram(page_to_mfn(&pg[i]));
+        flush_page_to_ram(page_to_mfn(&pg[i]), true);
     }
 
     spin_unlock(&heap_lock);
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 4b46e8831c..497b4c86ad 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -407,7 +407,7 @@ static inline void flush_xen_data_tlb_range_va(unsigned long va,
 }
 
 /* Flush the dcache for an entire page. */
-void flush_page_to_ram(unsigned long mfn);
+void flush_page_to_ram(unsigned long mfn, bool sync_icache);
 
 /*
  * Print a walk of a page table or p2m
diff --git a/xen/include/asm-x86/flushtlb.h b/xen/include/asm-x86/flushtlb.h
index 8b7adef7c5..bd2be7e482 100644
--- a/xen/include/asm-x86/flushtlb.h
+++ b/xen/include/asm-x86/flushtlb.h
@@ -118,7 +118,7 @@ void flush_area_mask(const cpumask_t *, const void *va, unsigned int flags);
 #define flush_tlb_one_all(v)                    \
     flush_tlb_one_mask(&cpu_online_map, v)
 
-static inline void flush_page_to_ram(unsigned long mfn) {}
+static inline void flush_page_to_ram(unsigned long mfn, bool sync_icache) {}
 static inline int invalidate_dcache_va_range(const void *p,
                                              unsigned long size)
 { return -EOPNOTSUPP; }
-- 
2.11.0


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

[For Xen-4.10 PATCH v2 2/3] arm: p2m: Prevent redundant icache flushes

[Punit Agrawal]

When toolstack requests flushing the caches, flush_page_to_ram() is
called for each page of the requested domain. This needs to unnecessary
icache invalidation operations.

Let's take the responsibility of performing icache operations and use
the recently introduced flag to prevent redundant icache operations by
flush_page_to_ram().

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/p2m.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 291998938e..b7bbea1d81 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1398,13 +1398,15 @@ int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr)
         /* XXX: Implement preemption */
         while ( gfn_x(start) < gfn_x(next_gfn) )
         {
-            flush_page_to_ram(mfn_x(mfn), true);
+            flush_page_to_ram(mfn_x(mfn), false);
 
             start = gfn_add(start, 1);
             mfn = mfn_add(mfn, 1);
         }
     }
 
+    invalidate_icache();
+
     p2m_read_unlock(p2m);
 
     return 0;
-- 
2.11.0


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

[For Xen-4.10 PATCH v2 3/3] Avoid excess icache flushes in populate_physmap() before domain has been created

[Punit Agrawal]

populate_physmap() calls alloc_heap_pages() per requested
extent. alloc_heap_pages() invalidates the entire icache per
extent. During domain creation, the icache invalidations can be deffered
until all the extents have been allocated as there is no risk of
executing stale instructions from the icache.

Introduce a new flag "MEMF_no_icache_flush" to be used to prevent
alloc_heap_pages() from performing icache maintenance operations. Use
the flag in populate_physmap() before the domain has been unpaused and
perform required icache maintenance function at the end of the
allocation.

One concern is the lack of synchronisation around testing for
"creation_finished". But it seems, in practice the window where it is
out of sync should be small enough to not matter.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/common/memory.c        | 31 ++++++++++++++++++++++---------
 xen/common/page_alloc.c    |  2 +-
 xen/include/asm-x86/page.h |  8 ++++++++
 xen/include/xen/mm.h       |  2 ++
 4 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 52879e7438..34d2dda8b4 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -152,16 +152,26 @@ static void populate_physmap(struct memop_args *a)
                             max_order(curr_d)) )
         return;
 
-    /*
-     * With MEMF_no_tlbflush set, alloc_heap_pages() will ignore
-     * TLB-flushes. After VM creation, this is a security issue (it can
-     * make pages accessible to guest B, when guest A may still have a
-     * cached mapping to them). So we do this only during domain creation,
-     * when the domain itself has not yet been unpaused for the first
-     * time.
-     */
     if ( unlikely(!d->creation_finished) )
+    {
+        /*
+         * With MEMF_no_tlbflush set, alloc_heap_pages() will ignore
+         * TLB-flushes. After VM creation, this is a security issue (it can
+         * make pages accessible to guest B, when guest A may still have a
+         * cached mapping to them). So we do this only during domain creation,
+         * when the domain itself has not yet been unpaused for the first
+         * time.
+         */
         a->memflags |= MEMF_no_tlbflush;
+        /*
+         * With MEMF_no_icache_flush, alloc_heap_pages() will skip
+         * performing icache flushes. We do it only before domain
+         * creation as once the domain is running there is a danger of
+         * executing instructions from stale caches if icache flush is
+         * delayed.
+         */
+        a->memflags |= MEMF_no_icache_flush;
+    }
 
     for ( i = a->nr_done; i < a->nr_extents; i++ )
     {
@@ -211,7 +221,6 @@ static void populate_physmap(struct memop_args *a)
                 }
 
                 mfn = gpfn;
-                page = mfn_to_page(mfn);
             }
             else
             {
@@ -255,6 +264,10 @@ static void populate_physmap(struct memop_args *a)
 out:
     if ( need_tlbflush )
         filtered_flush_tlb_mask(tlbflush_timestamp);
+
+    if ( a->memflags & MEMF_no_icache_flush )
+        invalidate_icache();
+
     a->nr_done = i;
 }
 
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index eba78f1a3d..8bcef6a547 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -833,7 +833,7 @@ static struct page_info *alloc_heap_pages(
         /* Ensure cache and RAM are consistent for platforms where the
          * guest can control its own visibility of/through the cache.
          */
-        flush_page_to_ram(page_to_mfn(&pg[i]), true);
+        flush_page_to_ram(page_to_mfn(&pg[i]), !(memflags & MEMF_no_icache_flush));
     }
 
     spin_unlock(&heap_lock);
diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
index 4cadb12646..9b86d5183a 100644
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -375,6 +375,14 @@ perms_strictly_increased(uint32_t old_flags, uint32_t new_flags)
 
 #define PAGE_ALIGN(x) (((x) + PAGE_SIZE - 1) & PAGE_MASK)
 
+static inline void invalidate_icache(void)
+{
+/*
+ * There is nothing to be done here as icaches are sufficiently
+ * coherent on x86.
+ */
+}
+
 #endif /* __X86_PAGE_H__ */
 
 /*
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 88de3c1fa6..ee50d4cd7b 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -224,6 +224,8 @@ struct npfec {
 #define  MEMF_no_owner    (1U<<_MEMF_no_owner)
 #define _MEMF_no_tlbflush 6
 #define  MEMF_no_tlbflush (1U<<_MEMF_no_tlbflush)
+#define _MEMF_no_icache_flush 7
+#define  MEMF_no_icache_flush (1U<<_MEMF_no_icache_flush)
 #define _MEMF_node        8
 #define  MEMF_node_mask   ((1U << (8 * sizeof(nodeid_t))) - 1)
 #define  MEMF_node(n)     ((((n) + 1) & MEMF_node_mask) << _MEMF_node)
-- 
2.11.0


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

Re: [For Xen-4.10 PATCH v2 3/3] Avoid excess icache flushes in populate_physmap() before domain has been created

[Jan Beulich]

>>> On 26.05.17 at 13:14, <punit.agrawal@arm.com> wrote:
> populate_physmap() calls alloc_heap_pages() per requested
> extent. alloc_heap_pages() invalidates the entire icache per
> extent. During domain creation, the icache invalidations can be deffered
> until all the extents have been allocated as there is no risk of
> executing stale instructions from the icache.
> 
> Introduce a new flag "MEMF_no_icache_flush" to be used to prevent
> alloc_heap_pages() from performing icache maintenance operations. Use
> the flag in populate_physmap() before the domain has been unpaused and
> perform required icache maintenance function at the end of the
> allocation.
> 
> One concern is the lack of synchronisation around testing for
> "creation_finished". But it seems, in practice the window where it is
> out of sync should be small enough to not matter.
> 
> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Acked-by: Jan Beulich <jbeulich@suse.com>



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

Re: [For Xen-4.10 PATCH v2 0/3] Reduce unnecessary icache maintenance operations

[Julien Grall]

On 26/05/17 12:14, Punit Agrawal wrote:
> Hi,

Hi,

It looks like this patch series has been fully acked. Can someone apply it?

Cheers,

-- 
Julien Grall

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

Re: [For Xen-4.10 PATCH v2 0/3] Reduce unnecessary icache maintenance operations

[Jan Beulich]

>>> On 06.06.17 at 18:32, <julien.grall@arm.com> wrote:
> On 26/05/17 12:14, Punit Agrawal wrote:
> It looks like this patch series has been fully acked. Can someone apply it?

Well, for a series mostly affecting ARM I was expecting Stefano to
do so.

Jan


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

Re: [For Xen-4.10 PATCH v2 0/3] Reduce unnecessary icache maintenance operations

[Stefano Stabellini]

On Tue, 6 Jun 2017, Julien Grall wrote:
> On 26/05/17 12:14, Punit Agrawal wrote:
> > Hi,
> 
> Hi,
> 
> It looks like this patch series has been fully acked. Can someone apply it?

Done. It was appropriately marked in my inbox, but I was somehow
reluctant to commit things until the release is done. I guess I don't
need to be. I committed the series now.

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

Re: [For Xen-4.10 PATCH v2 3/3] Avoid excess icache flushes in populate_physmap() before domain has been created

[Jan Beulich]

>>> On 26.05.17 at 13:14, <punit.agrawal@arm.com> wrote:
> --- a/xen/include/asm-x86/page.h
> +++ b/xen/include/asm-x86/page.h
> @@ -375,6 +375,14 @@ perms_strictly_increased(uint32_t old_flags, uint32_t new_flags)
>  
>  #define PAGE_ALIGN(x) (((x) + PAGE_SIZE - 1) & PAGE_MASK)
>  
> +static inline void invalidate_icache(void)
> +{
> +/*
> + * There is nothing to be done here as icaches are sufficiently
> + * coherent on x86.
> + */
> +}

According to osstest this change hasn't been build tested on x86:

/home/osstest/build.110052.build-amd64/xen/xen/include/asm/page.h: Assembler messages:
/home/osstest/build.110052.build-amd64/xen/xen/include/asm/page.h:378: Error: no such instruction: `static inline void invalidate_icache(void)'
/home/osstest/build.110052.build-amd64/xen/xen/include/asm/page.h:379: Error: junk at end of line, first unrecognized character is `{'
/home/osstest/build.110052.build-amd64/xen/xen/include/asm/page.h:384: Error: junk at end of line, first unrecognized character is `}'
/home/osstest/build.110052.build-amd64/xen/xen/Rules.mk:177: recipe for target 'head.o' failed
make[4]: Leaving directory '/home/osstest/build.110052.build-amd64/xen/xen/arch/x86/boot'
make[4]: *** [head.o] Error 1

Jan


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

Re: [For Xen-4.10 PATCH v2 3/3] Avoid excess icache flushes in populate_physmap() before domain has been created

[Punit Agrawal]

"Jan Beulich" <JBeulich@suse.com> writes:

>>>> On 26.05.17 at 13:14, <punit.agrawal@arm.com> wrote:
>> --- a/xen/include/asm-x86/page.h
>> +++ b/xen/include/asm-x86/page.h
>> @@ -375,6 +375,14 @@ perms_strictly_increased(uint32_t old_flags, uint32_t new_flags)
>>  
>>  #define PAGE_ALIGN(x) (((x) + PAGE_SIZE - 1) & PAGE_MASK)
>>  
>> +static inline void invalidate_icache(void)
>> +{
>> +/*
>> + * There is nothing to be done here as icaches are sufficiently
>> + * coherent on x86.
>> + */
>> +}
>
> According to osstest this change hasn't been build tested on x86:
>
> /home/osstest/build.110052.build-amd64/xen/xen/include/asm/page.h: Assembler messages:
> /home/osstest/build.110052.build-amd64/xen/xen/include/asm/page.h:378: Error: no such instruction: `static inline void invalidate_icache(void)'
> /home/osstest/build.110052.build-amd64/xen/xen/include/asm/page.h:379: Error: junk at end of line, first unrecognized character is `{'
> /home/osstest/build.110052.build-amd64/xen/xen/include/asm/page.h:384: Error: junk at end of line, first unrecognized character is `}'
> /home/osstest/build.110052.build-amd64/xen/xen/Rules.mk:177: recipe for target 'head.o' failed
> make[4]: Leaving directory '/home/osstest/build.110052.build-amd64/xen/xen/arch/x86/boot'
> make[4]: *** [head.o] Error 1

Apologies for the breakage. The above hunk needs to move into the
!__ASSEMBLY__ block.

I'll send a fix shortly.


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

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

[For Xen-4.10 PATCH] Ensure invalidate_icache() definition is visible only when !__ASSEMBLY__

[Punit Agrawal]

Commit edff605421 introduces an empty invalidate_icache() function in
page.h for x86 but mistakenly places it outside the !__ASSEMBLY__
block. This causes build failure on x86.

Address this by moving the function definition to within the existing
!__ASSEMBLY__ block.

Fixes: edff605421 ("Avoid excess icache flushes in populate_physmap() before domain has been created")
Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
---
Hi Stefano, Jan,

This commit fixes the build breakage in staging for me. I'm not sure
whether staging gets rebased so sending this as a separate commit.

Please feel free to fold it in if that makes sense.

Once again apologies for the breakage.

Punit

xen/include/asm-x86/page.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
index 9b86d5183a..474b9bde78 100644
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -371,10 +371,6 @@ perms_strictly_increased(uint32_t old_flags, uint32_t new_flags)
     return ((of | (of ^ nf)) == nf);
 }
 
-#endif /* !__ASSEMBLY__ */
-
-#define PAGE_ALIGN(x) (((x) + PAGE_SIZE - 1) & PAGE_MASK)
-
 static inline void invalidate_icache(void)
 {
 /*
@@ -383,6 +379,10 @@ static inline void invalidate_icache(void)
  */
 }
 
+#endif /* !__ASSEMBLY__ */
+
+#define PAGE_ALIGN(x) (((x) + PAGE_SIZE - 1) & PAGE_MASK)
+
 #endif /* __X86_PAGE_H__ */
 
 /*
-- 
2.11.0


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

Re: [For Xen-4.10 PATCH v2 0/3] Reduce unnecessary icache maintenance operations

[Punit Agrawal]

Stefano Stabellini <sstabellini@kernel.org> writes:

> On Tue, 6 Jun 2017, Julien Grall wrote:
>> On 26/05/17 12:14, Punit Agrawal wrote:
>> > Hi,
>> 
>> Hi,
>> 
>> It looks like this patch series has been fully acked. Can someone apply it?
>
> Done. It was appropriately marked in my inbox, but I was somehow
> reluctant to commit things until the release is done. I guess I don't
> need to be. I committed the series now.

Thanks Stefano for merging the series. As osstest discovered, there is a
build failure on x86 due to the last patch. I've send a fix[0] for it.

If the fix looks good, could you merge it as well.

[0] <20170607093449.25607-1-punit.agrawal@arm.com>

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

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

Re: [For Xen-4.10 PATCH] Ensure invalidate_icache() definition is visible only when !__ASSEMBLY__

[Jan Beulich]

>>> On 07.06.17 at 11:34, <punit.agrawal@arm.com> wrote:
> This commit fixes the build breakage in staging for me. I'm not sure
> whether staging gets rebased so sending this as a separate commit.

No, we don't re-base. If anything (e.g. when a fix doesn't
arrive pretty quickly or needs more thought) we revert.

Jan


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

Re: [For Xen-4.10 PATCH v2 3/3] Avoid excess icache flushes in populate_physmap() before domain has been created

[Andrew Cooper]

On 26/05/17 12:14, Punit Agrawal wrote:
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 52879e7438..34d2dda8b4 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -152,16 +152,26 @@ static void populate_physmap(struct memop_args *a)
>                              max_order(curr_d)) )
>          return;
>  
> -    /*
> -     * With MEMF_no_tlbflush set, alloc_heap_pages() will ignore
> -     * TLB-flushes. After VM creation, this is a security issue (it can
> -     * make pages accessible to guest B, when guest A may still have a
> -     * cached mapping to them). So we do this only during domain creation,
> -     * when the domain itself has not yet been unpaused for the first
> -     * time.
> -     */
>      if ( unlikely(!d->creation_finished) )
> +    {
> +        /*
> +         * With MEMF_no_tlbflush set, alloc_heap_pages() will ignore
> +         * TLB-flushes. After VM creation, this is a security issue (it can
> +         * make pages accessible to guest B, when guest A may still have a
> +         * cached mapping to them). So we do this only during domain creation,
> +         * when the domain itself has not yet been unpaused for the first
> +         * time.
> +         */
>          a->memflags |= MEMF_no_tlbflush;
> +        /*
> +         * With MEMF_no_icache_flush, alloc_heap_pages() will skip
> +         * performing icache flushes. We do it only before domain
> +         * creation as once the domain is running there is a danger of
> +         * executing instructions from stale caches if icache flush is
> +         * delayed.
> +         */
> +        a->memflags |= MEMF_no_icache_flush;
> +    }
>  
>      for ( i = a->nr_done; i < a->nr_extents; i++ )
>      {
> @@ -211,7 +221,6 @@ static void populate_physmap(struct memop_args *a)
>                  }
>  
>                  mfn = gpfn;
> -                page = mfn_to_page(mfn);

What is the purpose of this hunk?

It is not mentioned in the commit message at all, and looks unsafe to me.

~Andrew

>              }
>              else
>              {
>


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

Re: [For Xen-4.10 PATCH v2 3/3] Avoid excess icache flushes in populate_physmap() before domain has been created

[Julien Grall]

On 07/06/17 12:19, Andrew Cooper wrote:
> On 26/05/17 12:14, Punit Agrawal wrote:
>> @@ -211,7 +221,6 @@ static void populate_physmap(struct memop_args *a)
>>                  }
>>
>>                  mfn = gpfn;
>> -                page = mfn_to_page(mfn);
>
> What is the purpose of this hunk?
>
> It is not mentioned in the commit message at all, and looks unsafe to me.

Not answering why it has been dropped (leave Punit to answer this), I 
introduced this page = mfn_to_page(...) to match the else part of ( 
is_domain_direct_mapped). In the else, 'page' will always point to first 
base page.

However, today, nobody is using 'page' after. Unless there is a reason 
to drop it, I would prefer to keep it so the if and else part match.

Cheers,

-- 
Julien Grall

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

Re: [For Xen-4.10 PATCH v2 3/3] Avoid excess icache flushes in populate_physmap() before domain has been created

[Punit Agrawal]

Julien Grall <julien.grall@arm.com> writes:

> On 07/06/17 12:19, Andrew Cooper wrote:
>> On 26/05/17 12:14, Punit Agrawal wrote:
>>> @@ -211,7 +221,6 @@ static void populate_physmap(struct memop_args *a)
>>>                  }
>>>
>>>                  mfn = gpfn;
>>> -                page = mfn_to_page(mfn);
>>
>> What is the purpose of this hunk?
>>
>> It is not mentioned in the commit message at all, and looks unsafe to me.
>
> Not answering why it has been dropped (leave Punit to answer this), I
> introduced this page = mfn_to_page(...) to match the else part of (
> is_domain_direct_mapped). In the else, 'page' will always point to
> first base page.
>
> However, today, nobody is using 'page' after. Unless there is a reason
> to drop it, I would prefer to keep it so the if and else part match.

It's an unintended change that's crept into this patch. There's no
reason to drop it here.

I'll send a fix adding it back.

>
> Cheers,

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

[For Xen-4.10 PATCH] memory: Re-introduce an erroneously dropped line

[Punit Agrawal]

Commit 726b737574 makes an unrelated change deleting a line setting the
page from mfn. Although the page variable is not used, it is an
unrelated change. The setting of the page variable was introduced to
match the else part of is_domain_direct_mapped() in populate_physmap().

Re-introduce the missing hunk.

Fixes: 726b737574 ("Avoid excess icache flushes in populate_physmap() before domain has been created")
Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
---
 xen/common/memory.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 34d2dda8b4..a3cb572530 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -221,6 +221,7 @@ static void populate_physmap(struct memop_args *a)
                 }
 
                 mfn = gpfn;
+                page = mfn_to_page(mfn);
             }
             else
             {
-- 
2.11.0


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

Re: [For Xen-4.10 PATCH] memory: Re-introduce an erroneously dropped line

[Jan Beulich]

>>> On 07.06.17 at 14:04, <punit.agrawal@arm.com> wrote:
> Commit 726b737574 makes an unrelated change deleting a line setting the
> page from mfn. Although the page variable is not used, it is an
> unrelated change. The setting of the page variable was introduced to
> match the else part of is_domain_direct_mapped() in populate_physmap().
> 
> Re-introduce the missing hunk.
> 
> Fixes: 726b737574 ("Avoid excess icache flushes in populate_physmap() before 
> domain has been created")
> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> ---
>  xen/common/memory.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 34d2dda8b4..a3cb572530 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -221,6 +221,7 @@ static void populate_physmap(struct memop_args *a)
>                  }
>  
>                  mfn = gpfn;
> +                page = mfn_to_page(mfn);
>              }
>              else
>              {

While I certainly don't mind this being re-added, I'm also not sure
it's worthwhile now that the line is gone, and it's not needed for
anything. I'll let other REST maintainers give their opinions ...

Jan


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

Re: [For Xen-4.10 PATCH] memory: Re-introduce an erroneously dropped line

[Julien Grall]

On 07/06/17 13:13, Jan Beulich wrote:
>>>> On 07.06.17 at 14:04, <punit.agrawal@arm.com> wrote:
>> Commit 726b737574 makes an unrelated change deleting a line setting the
>> page from mfn. Although the page variable is not used, it is an
>> unrelated change. The setting of the page variable was introduced to
>> match the else part of is_domain_direct_mapped() in populate_physmap().
>>
>> Re-introduce the missing hunk.
>>
>> Fixes: 726b737574 ("Avoid excess icache flushes in populate_physmap() before
>> domain has been created")
>> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
>> ---
>>  xen/common/memory.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>> index 34d2dda8b4..a3cb572530 100644
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -221,6 +221,7 @@ static void populate_physmap(struct memop_args *a)
>>                  }
>>
>>                  mfn = gpfn;
>> +                page = mfn_to_page(mfn);
>>              }
>>              else
>>              {
>
> While I certainly don't mind this being re-added, I'm also not sure
> it's worthwhile now that the line is gone, and it's not needed for
> anything. I'll let other REST maintainers give their opinions ...

I am not a REST maintainers but I think it would be better to keep the 
page = mfn_to_page(mfn). This is matching the behavior of the else part 
where page will always point to first base page.

Indeed we don't use it today, but nothing prevent a future patch to do 
it and it would be difficult to spot the mismatch...

Cheers,

-- 
Julien Grall

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

Re: [For Xen-4.10 PATCH v2 0/3] Reduce unnecessary icache maintenance operations

[Stefano Stabellini]

On Wed, 7 Jun 2017, Punit Agrawal wrote:
> Stefano Stabellini <sstabellini@kernel.org> writes:
> 
> > On Tue, 6 Jun 2017, Julien Grall wrote:
> >> On 26/05/17 12:14, Punit Agrawal wrote:
> >> > Hi,
> >> 
> >> Hi,
> >> 
> >> It looks like this patch series has been fully acked. Can someone apply it?
> >
> > Done. It was appropriately marked in my inbox, but I was somehow
> > reluctant to commit things until the release is done. I guess I don't
> > need to be. I committed the series now.
> 
> Thanks Stefano for merging the series. As osstest discovered, there is a
> build failure on x86 due to the last patch. I've send a fix[0] for it.
> 
> If the fix looks good, could you merge it as well.
> 
> [0] <20170607093449.25607-1-punit.agrawal@arm.com>

I noticed, but that patch affects x86, so it needs an ack from one of
the x86 maintainers.

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

Re: [For Xen-4.10 PATCH v2 0/3] Reduce unnecessary icache maintenance operations

[Julien Grall]

On 07/06/17 18:46, Stefano Stabellini wrote:
> On Wed, 7 Jun 2017, Punit Agrawal wrote:
>> Stefano Stabellini <sstabellini@kernel.org> writes:
>>
>>> On Tue, 6 Jun 2017, Julien Grall wrote:
>>>> On 26/05/17 12:14, Punit Agrawal wrote:
>>>>> Hi,
>>>>
>>>> Hi,
>>>>
>>>> It looks like this patch series has been fully acked. Can someone apply it?
>>>
>>> Done. It was appropriately marked in my inbox, but I was somehow
>>> reluctant to commit things until the release is done. I guess I don't
>>> need to be. I committed the series now.
>>
>> Thanks Stefano for merging the series. As osstest discovered, there is a
>> build failure on x86 due to the last patch. I've send a fix[0] for it.
>>
>> If the fix looks good, could you merge it as well.
>>
>> [0] <20170607093449.25607-1-punit.agrawal@arm.com>
>
> I noticed, but that patch affects x86, so it needs an ack from one of
> the x86 maintainers.

Jan already merged it.

Cheers,

-- 
Julien Grall

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