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

[Punit Agrawal <punit.agrawal@xxxxxxx>]

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