* [For Xen-4.10 RFC PATCH 1/3] Allow control of icache invalidations when calling flush_page_to_ram()
2017-03-31 10:24 [For Xen-4.10 RFC PATCH 0/3] Reduce unnecessary icache maintenance operations Punit Agrawal
@ 2017-03-31 10:24 ` Punit Agrawal
2017-03-31 23:53 ` Stefano Stabellini
2017-03-31 10:24 ` [For Xen-4.10 RFC PATCH 2/3] arm: p2m: Prevent redundant icache flushes Punit Agrawal
2017-03-31 10:24 ` [For Xen-4.10 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap() Punit Agrawal
2 siblings, 1 reply; 14+ messages in thread
From: Punit Agrawal @ 2017-03-31 10:24 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
Punit Agrawal, tim, julien.grall, jbeulich, ian.jackson
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [For Xen-4.10 RFC PATCH 1/3] Allow control of icache invalidations when calling flush_page_to_ram()
2017-03-31 10:24 ` [For Xen-4.10 RFC PATCH 1/3] Allow control of icache invalidations when calling flush_page_to_ram() Punit Agrawal
@ 2017-03-31 23:53 ` Stefano Stabellini
0 siblings, 0 replies; 14+ messages in thread
From: Stefano Stabellini @ 2017-03-31 23:53 UTC (permalink / raw)
To: Punit Agrawal
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3, tim,
xen-devel, julien.grall, jbeulich, ian.jackson
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [For Xen-4.10 RFC PATCH 2/3] arm: p2m: Prevent redundant icache flushes
2017-03-31 10:24 [For Xen-4.10 RFC PATCH 0/3] Reduce unnecessary icache maintenance operations Punit Agrawal
2017-03-31 10:24 ` [For Xen-4.10 RFC PATCH 1/3] Allow control of icache invalidations when calling flush_page_to_ram() Punit Agrawal
@ 2017-03-31 10:24 ` Punit Agrawal
2017-03-31 23:54 ` Stefano Stabellini
2017-03-31 10:24 ` [For Xen-4.10 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap() Punit Agrawal
2 siblings, 1 reply; 14+ messages in thread
From: Punit Agrawal @ 2017-03-31 10:24 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
Punit Agrawal, tim, julien.grall, jbeulich, ian.jackson
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [For Xen-4.10 RFC PATCH 2/3] arm: p2m: Prevent redundant icache flushes
2017-03-31 10:24 ` [For Xen-4.10 RFC PATCH 2/3] arm: p2m: Prevent redundant icache flushes Punit Agrawal
@ 2017-03-31 23:54 ` Stefano Stabellini
2017-04-03 10:45 ` Punit Agrawal
0 siblings, 1 reply; 14+ messages in thread
From: Stefano Stabellini @ 2017-03-31 23:54 UTC (permalink / raw)
To: Punit Agrawal
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3, tim,
xen-devel, julien.grall, jbeulich, ian.jackson
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [For Xen-4.10 RFC PATCH 2/3] arm: p2m: Prevent redundant icache flushes
2017-03-31 23:54 ` Stefano Stabellini
@ 2017-04-03 10:45 ` Punit Agrawal
0 siblings, 0 replies; 14+ messages in thread
From: Punit Agrawal @ 2017-04-03 10:45 UTC (permalink / raw)
To: Stefano Stabellini
Cc: wei.liu2, George.Dunlap, andrew.cooper3, tim, xen-devel,
julien.grall, jbeulich, ian.jackson
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [For Xen-4.10 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap()
2017-03-31 10:24 [For Xen-4.10 RFC PATCH 0/3] Reduce unnecessary icache maintenance operations Punit Agrawal
2017-03-31 10:24 ` [For Xen-4.10 RFC PATCH 1/3] Allow control of icache invalidations when calling flush_page_to_ram() Punit Agrawal
2017-03-31 10:24 ` [For Xen-4.10 RFC PATCH 2/3] arm: p2m: Prevent redundant icache flushes Punit Agrawal
@ 2017-03-31 10:24 ` Punit Agrawal
2017-03-31 11:34 ` Wei Liu
2017-05-09 15:16 ` Julien Grall
2 siblings, 2 replies; 14+ messages in thread
From: Punit Agrawal @ 2017-03-31 10:24 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
Punit Agrawal, tim, julien.grall, jbeulich, ian.jackson
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [For Xen-4.10 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap()
2017-03-31 10:24 ` [For Xen-4.10 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap() Punit Agrawal
@ 2017-03-31 11:34 ` Wei Liu
2017-03-31 13:53 ` Punit Agrawal
2017-05-09 15:16 ` Julien Grall
1 sibling, 1 reply; 14+ messages in thread
From: Wei Liu @ 2017-03-31 11:34 UTC (permalink / raw)
To: Punit Agrawal
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3, tim,
xen-devel, julien.grall, jbeulich, ian.jackson
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [For Xen-4.10 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap()
2017-03-31 11:34 ` Wei Liu
@ 2017-03-31 13:53 ` Punit Agrawal
2017-03-31 14:10 ` Wei Liu
0 siblings, 1 reply; 14+ messages in thread
From: Punit Agrawal @ 2017-03-31 13:53 UTC (permalink / raw)
To: Wei Liu
Cc: sstabellini, George.Dunlap, andrew.cooper3, tim, xen-devel,
julien.grall, jbeulich, ian.jackson
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [For Xen-4.10 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap()
2017-03-31 13:53 ` Punit Agrawal
@ 2017-03-31 14:10 ` Wei Liu
2017-03-31 14:27 ` Punit Agrawal
2017-03-31 23:58 ` Stefano Stabellini
0 siblings, 2 replies; 14+ messages in thread
From: Wei Liu @ 2017-03-31 14:10 UTC (permalink / raw)
To: Punit Agrawal
Cc: sstabellini, Wei Liu, George.Dunlap, andrew.cooper3, tim,
xen-devel, julien.grall, jbeulich, ian.jackson
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [For Xen-4.10 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap()
2017-03-31 14:10 ` Wei Liu
@ 2017-03-31 14:27 ` Punit Agrawal
2017-03-31 23:58 ` Stefano Stabellini
1 sibling, 0 replies; 14+ messages in thread
From: Punit Agrawal @ 2017-03-31 14:27 UTC (permalink / raw)
To: Wei Liu
Cc: sstabellini, George.Dunlap, andrew.cooper3, tim, xen-devel,
julien.grall, jbeulich, ian.jackson
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [For Xen-4.10 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap()
2017-03-31 14:10 ` Wei Liu
2017-03-31 14:27 ` Punit Agrawal
@ 2017-03-31 23:58 ` Stefano Stabellini
1 sibling, 0 replies; 14+ messages in thread
From: Stefano Stabellini @ 2017-03-31 23:58 UTC (permalink / raw)
To: Wei Liu
Cc: sstabellini, George.Dunlap, andrew.cooper3, Punit Agrawal, tim,
xen-devel, julien.grall, jbeulich, ian.jackson
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [For Xen-4.10 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap()
2017-03-31 10:24 ` [For Xen-4.10 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap() Punit Agrawal
2017-03-31 11:34 ` Wei Liu
@ 2017-05-09 15:16 ` Julien Grall
2017-05-12 12:35 ` Punit Agrawal
1 sibling, 1 reply; 14+ messages in thread
From: Julien Grall @ 2017-05-09 15:16 UTC (permalink / raw)
To: Punit Agrawal, xen-devel
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3, tim,
jbeulich, ian.jackson
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [For Xen-4.10 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap()
2017-05-09 15:16 ` Julien Grall
@ 2017-05-12 12:35 ` Punit Agrawal
0 siblings, 0 replies; 14+ messages in thread
From: Punit Agrawal @ 2017-05-12 12:35 UTC (permalink / raw)
To: Julien Grall
Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3, tim,
xen-devel, jbeulich, ian.jackson
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
^ permalink raw reply [flat|nested] 14+ messages in thread