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

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

[Punit Agrawal]

Hi,

Although icache maintenance operations are required when changing page
table mappings, in certain situations they are being called far more
frequently than necessary.

As icache maintenance has a performance penalty, one that increases
with the number of cores in the system, this RFC attempts to improve
the situation by hoisting knowledge of required icache maintenance
operations to higher layers.

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(). As I
couldn't icache maintenance operations for x86, an empty helper is
introduced.

Is it reasonable to move cache maintenance operation to higher layers?
The alternative would be perform cache maintenance operations on
ranges larger than page size but the benefit (reduced icache
operations) might not be equivalent.

This is my first contribution to Xen, so please go gentle if I've
missed something obvious.

Thanks,
Punit

Punit Agrawal (3):
  Allow control of icache invalidations when calling flush_page_to_ram()
  arm: p2m: Prevent redundant icache flushes
  Prevent redundant icache flushes in populate_physmap()

 xen/arch/arm/mm.c              | 5 +++--
 xen/arch/arm/p2m.c             | 4 +++-
 xen/common/memory.c            | 6 ++++++
 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     | 4 ++++
 xen/include/xen/mm.h           | 2 ++
 8 files changed, 21 insertions(+), 6 deletions(-)

-- 
2.11.0


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

[For Xen-4.10 RFC PATCH 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>
---
 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 f0a2eddaaf..e1ab5f8694 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -383,7 +383,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));
 
@@ -398,7 +398,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 626376090d..76cd1c34f3 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1400,7 +1400,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 42c20cb201..15450a3b6d 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -838,7 +838,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

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

[Stefano Stabellini]

On Fri, 31 Mar 2017, Punit Agrawal wrote:
> 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>
> ---
>  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 f0a2eddaaf..e1ab5f8694 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -383,7 +383,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));
>  
> @@ -398,7 +398,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)

coding style is

    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 626376090d..76cd1c34f3 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1400,7 +1400,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 42c20cb201..15450a3b6d 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -838,7 +838,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 RFC PATCH 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>
---
 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 76cd1c34f3..8136522ed8 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1400,13 +1400,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

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

[Stefano Stabellini]

On Fri, 31 Mar 2017, Punit Agrawal wrote:
> 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 76cd1c34f3..8136522ed8 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1400,13 +1400,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

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

[Punit Agrawal]

Hi Stefano,

Stefano Stabellini <sstabellini@kernel.org> writes:

> On Fri, 31 Mar 2017, Punit Agrawal wrote:
>> 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>

Thanks for the review and comments on the other patches. I've now
addressed them locally.

I'll wait to see if there are anymore comments before posting an update.

Regards,
Punit

>
>
>> ---
>>  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 76cd1c34f3..8136522ed8 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1400,13 +1400,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 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap()

[Punit Agrawal]

populate_physmap() calls alloc_heap_pages() per requested extent. As
alloc_heap_pages() performs icache maintenance operations affecting the
entire instruction cache, this leads to redundant cache flushes when
allocating multiple extents in populate_physmap().

To alleviate this problem, introduce a new flag "MEMF_no_icache_flush"
which can be used prevent alloc_heap_pages() to perform unnecessary
icache maintenance operations. Use the flag in populate_physmap() and
perform the required icache maintenance function at the end of the
operation.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
---
 xen/common/memory.c        | 6 ++++++
 xen/common/page_alloc.c    | 2 +-
 xen/include/asm-x86/page.h | 4 ++++
 xen/include/xen/mm.h       | 2 ++
 4 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index ad0b33ceb6..507f363924 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -162,6 +162,8 @@ static void populate_physmap(struct memop_args *a)
     if ( unlikely(!d->creation_finished) )
         a->memflags |= MEMF_no_tlbflush;
 
+    a->memflags |= MEMF_no_icache_flush;
+
     for ( i = a->nr_done; i < a->nr_extents; i++ )
     {
         if ( i != a->nr_done && hypercall_preempt_check() )
@@ -253,6 +255,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 15450a3b6d..1a51bc6b15 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -838,7 +838,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 bc5946b9d2..13dc9e2299 100644
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -375,6 +375,10 @@ 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)
+{
+}
+
 #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 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap()

[Wei Liu]

On Fri, Mar 31, 2017 at 11:24:24AM +0100, Punit Agrawal wrote:
> populate_physmap() calls alloc_heap_pages() per requested extent. As
> alloc_heap_pages() performs icache maintenance operations affecting the
> entire instruction cache, this leads to redundant cache flushes when
> allocating multiple extents in populate_physmap().
> 
> To alleviate this problem, introduce a new flag "MEMF_no_icache_flush"
> which can be used prevent alloc_heap_pages() to perform unnecessary
> icache maintenance operations. Use the flag in populate_physmap() and
> perform the required icache maintenance function at the end of the
> operation.
> 
> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> ---
>  xen/common/memory.c        | 6 ++++++
>  xen/common/page_alloc.c    | 2 +-
>  xen/include/asm-x86/page.h | 4 ++++
>  xen/include/xen/mm.h       | 2 ++
>  4 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index ad0b33ceb6..507f363924 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -162,6 +162,8 @@ static void populate_physmap(struct memop_args *a)
>      if ( unlikely(!d->creation_finished) )
>          a->memflags |= MEMF_no_tlbflush;
>  
> +    a->memflags |= MEMF_no_icache_flush;
> +

Not a good idea.

I think what you need is probably to group this under
!d->creation_finished.

>      for ( i = a->nr_done; i < a->nr_extents; i++ )
>      {
>          if ( i != a->nr_done && hypercall_preempt_check() )
> @@ -253,6 +255,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 )

This should be inverted, right?

The modification to this function is definitely wrong as-is. You end up
always setting the no flush flag and later always flushing the cache.


Wei.

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

Re: [For Xen-4.10 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap()

[Punit Agrawal]

Hi Wei,

Thanks for taking a look at this RFC. Responses/questions below...

Wei Liu <wei.liu2@citrix.com> writes:

> On Fri, Mar 31, 2017 at 11:24:24AM +0100, Punit Agrawal wrote:
>> populate_physmap() calls alloc_heap_pages() per requested extent. As
>> alloc_heap_pages() performs icache maintenance operations affecting the
>> entire instruction cache, this leads to redundant cache flushes when
>> allocating multiple extents in populate_physmap().
>> 
>> To alleviate this problem, introduce a new flag "MEMF_no_icache_flush"
>> which can be used prevent alloc_heap_pages() to perform unnecessary
>> icache maintenance operations. Use the flag in populate_physmap() and
>> perform the required icache maintenance function at the end of the
>> operation.
>> 
>> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
>> ---
>>  xen/common/memory.c        | 6 ++++++
>>  xen/common/page_alloc.c    | 2 +-
>>  xen/include/asm-x86/page.h | 4 ++++
>>  xen/include/xen/mm.h       | 2 ++
>>  4 files changed, 13 insertions(+), 1 deletion(-)
>> 
>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>> index ad0b33ceb6..507f363924 100644
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -162,6 +162,8 @@ static void populate_physmap(struct memop_args *a)
>>      if ( unlikely(!d->creation_finished) )
>>          a->memflags |= MEMF_no_tlbflush;
>>  
>> +    a->memflags |= MEMF_no_icache_flush;
>> +
>
> Not a good idea.
>
> I think what you need is probably to group this under
> !d->creation_finished.

Can you explain why you think the above is not a good idea?

Without additional synchronisation there is a race between
d->creation_finished being set and used (Consider the case where
populate_physmap() being called on a different cpu to where
d->creation_finished is set to true).

>
>>      for ( i = a->nr_done; i < a->nr_extents; i++ )
>>      {
>>          if ( i != a->nr_done && hypercall_preempt_check() )
>> @@ -253,6 +255,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 )
>
> This should be inverted, right?

The MEMF_no_icache_flush flag is used to indicate to alloc_heap_pages()
(called via alloc_domheap_pages() here) that the caller is going to take
care of synchronising the icache with the dcache. As such, when the flag
is set, we want to perform icache maintenance operations here.

>
> The modification to this function is definitely wrong as-is. You end up
> always setting the no flush flag and later always flushing the cache.

Correct!

invalidate_icache() flushes the entire instruction cache which ends up
being called each time flush_page_to_ram() is invoked from
alloc_heap_pages(). The patch prevents repeated calls to
invalidate_icache() and moves the responsibility to populate_physmap()
which was the intention.

It would help if you can explain why you think the changes are
wrong.

Thanks,
Punit

>
>
> Wei.

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

Re: [For Xen-4.10 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap()

[Wei Liu]

On Fri, Mar 31, 2017 at 02:53:55PM +0100, Punit Agrawal wrote:
[...]
> 
> Correct!
> 
> invalidate_icache() flushes the entire instruction cache which ends up
> being called each time flush_page_to_ram() is invoked from
> alloc_heap_pages(). The patch prevents repeated calls to
> invalidate_icache() and moves the responsibility to populate_physmap()
> which was the intention.
> 
> It would help if you can explain why you think the changes are
> wrong.
> 

OK. So it turns out I misunderstood what you wanted to do here.

You can do this in a less confusing way. You don't need to fiddle with
a->memflags. Just add '| MEMF_no_icache_flush' to alloc_domheap_page.
And then you always call invalidate_icache at the end.

This would avoid people asking question whether the check should be
inverted...

Wei.

> Thanks,
> Punit
> 
> >
> >
> > Wei.

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

Re: [For Xen-4.10 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap()

[Punit Agrawal]

Wei Liu <wei.liu2@citrix.com> writes:

> On Fri, Mar 31, 2017 at 02:53:55PM +0100, Punit Agrawal wrote:
> [...]
>> 
>> Correct!
>> 
>> invalidate_icache() flushes the entire instruction cache which ends up
>> being called each time flush_page_to_ram() is invoked from
>> alloc_heap_pages(). The patch prevents repeated calls to
>> invalidate_icache() and moves the responsibility to populate_physmap()
>> which was the intention.
>> 
>> It would help if you can explain why you think the changes are
>> wrong.
>> 
>
> OK. So it turns out I misunderstood what you wanted to do here.
>
> You can do this in a less confusing way. You don't need to fiddle with
> a->memflags. Just add '| MEMF_no_icache_flush' to alloc_domheap_page.
> And then you always call invalidate_icache at the end.

Sure, I'll update it in the next version.

Thanks for your comments.

Punit

>
> This would avoid people asking question whether the check should be
> inverted...
>
> Wei.
>
>> Thanks,
>> Punit
>> 
>> >
>> >
>> > Wei.

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

Re: [For Xen-4.10 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap()

[Stefano Stabellini]

On Fri, 31 Mar 2017, Wei Liu wrote:
> On Fri, Mar 31, 2017 at 02:53:55PM +0100, Punit Agrawal wrote:
> [...]
> > 
> > Correct!
> > 
> > invalidate_icache() flushes the entire instruction cache which ends up
> > being called each time flush_page_to_ram() is invoked from
> > alloc_heap_pages(). The patch prevents repeated calls to
> > invalidate_icache() and moves the responsibility to populate_physmap()
> > which was the intention.
> > 
> > It would help if you can explain why you think the changes are
> > wrong.
> > 
> 
> OK. So it turns out I misunderstood what you wanted to do here.
> 
> You can do this in a less confusing way. You don't need to fiddle with
> a->memflags. Just add '| MEMF_no_icache_flush' to alloc_domheap_page.
> And then you always call invalidate_icache at the end.
> 
> This would avoid people asking question whether the check should be
> inverted...

Yes, good suggestion.

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

Re: [For Xen-4.10 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap()

[Julien Grall]

Hi Punit,

Sorry for the late answer.

On 31/03/17 11:24, Punit Agrawal wrote:
> populate_physmap() calls alloc_heap_pages() per requested extent. As
> alloc_heap_pages() performs icache maintenance operations affecting the
> entire instruction cache, this leads to redundant cache flushes when
> allocating multiple extents in populate_physmap().
>
> To alleviate this problem, introduce a new flag "MEMF_no_icache_flush"
> which can be used prevent alloc_heap_pages() to perform unnecessary
> icache maintenance operations. Use the flag in populate_physmap() and
> perform the required icache maintenance function at the end of the
> operation.
>
> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> ---
>  xen/common/memory.c        | 6 ++++++
>  xen/common/page_alloc.c    | 2 +-
>  xen/include/asm-x86/page.h | 4 ++++
>  xen/include/xen/mm.h       | 2 ++
>  4 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index ad0b33ceb6..507f363924 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -162,6 +162,8 @@ static void populate_physmap(struct memop_args *a)
>      if ( unlikely(!d->creation_finished) )
>          a->memflags |= MEMF_no_tlbflush;
>
> +    a->memflags |= MEMF_no_icache_flush;
> +
>      for ( i = a->nr_done; i < a->nr_extents; i++ )
>      {
>          if ( i != a->nr_done && hypercall_preempt_check() )
> @@ -253,6 +255,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();

I think it would be good to explain why it is always fine to defer the 
cache invalidation from a security point of view for future reference. 
This would help us in the future to know why we made this choice.

Cheers,

-- 
Julien Grall

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

Re: [For Xen-4.10 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap()

[Punit Agrawal]

Hi Julien,

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

> Hi Punit,
>
> Sorry for the late answer.
>
> On 31/03/17 11:24, Punit Agrawal wrote:
>> populate_physmap() calls alloc_heap_pages() per requested extent. As
>> alloc_heap_pages() performs icache maintenance operations affecting the
>> entire instruction cache, this leads to redundant cache flushes when
>> allocating multiple extents in populate_physmap().
>>
>> To alleviate this problem, introduce a new flag "MEMF_no_icache_flush"
>> which can be used prevent alloc_heap_pages() to perform unnecessary
>> icache maintenance operations. Use the flag in populate_physmap() and
>> perform the required icache maintenance function at the end of the
>> operation.
>>
>> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
>> ---
>>  xen/common/memory.c        | 6 ++++++
>>  xen/common/page_alloc.c    | 2 +-
>>  xen/include/asm-x86/page.h | 4 ++++
>>  xen/include/xen/mm.h       | 2 ++
>>  4 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>> index ad0b33ceb6..507f363924 100644
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -162,6 +162,8 @@ static void populate_physmap(struct memop_args *a)
>>      if ( unlikely(!d->creation_finished) )
>>          a->memflags |= MEMF_no_tlbflush;
>>
>> +    a->memflags |= MEMF_no_icache_flush;
>> +
>>      for ( i = a->nr_done; i < a->nr_extents; i++ )
>>      {
>>          if ( i != a->nr_done && hypercall_preempt_check() )
>> @@ -253,6 +255,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();
>
> I think it would be good to explain why it is always fine to defer the
> cache invalidation from a security point of view for future
> reference. This would help us in the future to know why we made this
> choice.

Thanks for raising this.

Discussing this with folks familiar with ARM, it turns out that
deferring icache invalidations risks other VCPUs to execute from stale
caches. We don't really want to debug issues from this. :)

Based on your suggestion (offlist), I'm going to try and restrict
delaying icache invalidations only during domain creation.

I'll work on getting a new version out in the next day or so.

>
> Cheers,

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