[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi Julien,

Julien Grall <julien.grall@xxxxxxx> 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@xxxxxxx>
>> ---
>>  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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.