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

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

[Punit Agrawal]

Hi,

This series was previously posted as an RFC[0]. An issue was discovered
in the RFC related to delaying icache invalidations when the domain is
active. Accordingly, Patch 3 has been modified to avoid per-page
icache invalidations only during domain creation.

Changes from RFC:

* 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.

Thanks,
Punit

[0] 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     |  4 ++++
 xen/include/xen/mm.h           |  2 ++
 8 files changed, 37 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 Resend 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 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 34d57760d7..29f2e2fad3 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1392,7 +1392,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 Resend 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>
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 29f2e2fad3..07357bce7d 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1392,13 +1392,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 Resend PATCH 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>
---
 xen/common/memory.c        | 31 ++++++++++++++++++++++---------
 xen/common/page_alloc.c    |  2 +-
 xen/include/asm-x86/page.h |  4 ++++
 xen/include/xen/mm.h       |  2 ++
 4 files changed, 29 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..3a375282f6 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 Resend PATCH 1/3] Allow control of icache invalidations when calling flush_page_to_ram()

[Jan Beulich]

>>> On 15.05.17 at 16:10, <punit.agrawal@arm.com> 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>

Non-ARM bits
Acked-by: Jan Beulich <jbeulich@suse.com>
provided the ARM maintainers agree with the ARM side (including
to use of this in subsequent patches).

Jan


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

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

[Jan Beulich]

>>> On 15.05.17 at 16:10, <punit.agrawal@arm.com> wrote:
> --- 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)
> +{
> +}

This function clearly does not what its name says, so there should
be a brief comment saying why.

Everything else looks reasonable.

Jan


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

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

[Punit Agrawal]

Jan Beulich <JBeulich@suse.com> writes:

>>>> On 15.05.17 at 16:10, <punit.agrawal@arm.com> wrote:
>> --- 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)
>> +{
>> +}
>
> This function clearly does not what its name says, so there should
> be a brief comment saying why.

Ack. I've added the following comment block above the function
definition.

/*
 * While allocating memory for a domain, invalidate_icache() is called
 * to ensure that guest vcpu does not execute any stale instructions
 * from the recently allocated memory. There is nothing to be done
 * here as icaches are coherent on x86.
 */

My x86 foo is weak and I'd appreciate if somebody familiar with x86
could give this a once over.

>
> Everything else looks reasonable.

Thanks, Jan. I'll post a new version once the ARM maintainers have had a
chance to comment.

>
> Jan

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

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

[Jan Beulich]

>>> On 17.05.17 at 18:15, <punit.agrawal@arm.com> wrote:
> Jan Beulich <JBeulich@suse.com> writes:
> 
>>>>> On 15.05.17 at 16:10, <punit.agrawal@arm.com> wrote:
>>> --- 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)
>>> +{
>>> +}
>>
>> This function clearly does not what its name says, so there should
>> be a brief comment saying why.
> 
> Ack. I've added the following comment block above the function
> definition.
> 
> /*
>  * While allocating memory for a domain, invalidate_icache() is called
>  * to ensure that guest vcpu does not execute any stale instructions
>  * from the recently allocated memory. There is nothing to be done
>  * here as icaches are coherent on x86.
>  */
> 
> My x86 foo is weak and I'd appreciate if somebody familiar with x86
> could give this a once over.

The text looks okay, but it ties the function to its _current_ single
user. I'd like to ask you to rather use just the last sentence, and it's
questionable (a matter of taste mostly) whether such a comment
wouldn't better be placed inside the function. And perhaps it would
be a good idea to insert "sufficiently", as they're not fully coherent
(see the self modifying code constraints on MP systems).

Jan


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

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

[Punit Agrawal]

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

>>>> On 17.05.17 at 18:15, <punit.agrawal@arm.com> wrote:
>> Jan Beulich <JBeulich@suse.com> writes:
>> 
>>>>>> On 15.05.17 at 16:10, <punit.agrawal@arm.com> wrote:
>>>> --- 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)
>>>> +{
>>>> +}
>>>
>>> This function clearly does not what its name says, so there should
>>> be a brief comment saying why.
>> 
>> Ack. I've added the following comment block above the function
>> definition.
>> 
>> /*
>>  * While allocating memory for a domain, invalidate_icache() is called
>>  * to ensure that guest vcpu does not execute any stale instructions
>>  * from the recently allocated memory. There is nothing to be done
>>  * here as icaches are coherent on x86.
>>  */
>> 
>> My x86 foo is weak and I'd appreciate if somebody familiar with x86
>> could give this a once over.
>
> The text looks okay, but it ties the function to its _current_ single
> user. I'd like to ask you to rather use just the last sentence, and it's
> questionable (a matter of taste mostly) whether such a comment
> wouldn't better be placed inside the function. And perhaps it would
> be a good idea to insert "sufficiently", as they're not fully coherent
> (see the self modifying code constraints on MP systems).

I've updated the text and moved it inside the function.

Thanks for the feedback!

>
> 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

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

[Stefano Stabellini]

On Mon, 15 May 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>

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 34d57760d7..29f2e2fad3 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1392,7 +1392,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

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

[Stefano Stabellini]

On Mon, 15 May 2017, Punit Agrawal 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>


> ---
>  xen/common/memory.c        | 31 ++++++++++++++++++++++---------
>  xen/common/page_alloc.c    |  2 +-
>  xen/include/asm-x86/page.h |  4 ++++
>  xen/include/xen/mm.h       |  2 ++
>  4 files changed, 29 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..3a375282f6 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 Resend PATCH 3/3] Avoid excess icache flushes in populate_physmap() before domain has been created

[Punit Agrawal]

Stefano Stabellini <sstabellini@kernel.org> writes:

> On Mon, 15 May 2017, Punit Agrawal 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>

Thanks, Stefano!

I'll add the tags and post a new version with the changes suggested by
Jan.

>
>
>> ---
>>  xen/common/memory.c        | 31 ++++++++++++++++++++++---------
>>  xen/common/page_alloc.c    |  2 +-
>>  xen/include/asm-x86/page.h |  4 ++++
>>  xen/include/xen/mm.h       |  2 ++
>>  4 files changed, 29 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..3a375282f6 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

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