Re: [Xen-devel] [PATCH] arm/monitor: flush the icache during SMC traps

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Thu, Jan 26, 2017 at 10:54 AM, Tamas K Lengyel
<tamas.lengyel@xxxxxxxxxxxx> wrote:
> On Thu, Jan 26, 2017 at 4:30 AM, Julien Grall <julien.grall@xxxxxxx> wrote:
>> (CC xen-devel, Ravzan, and Stefao)
>>
>> Hi Tamas,
>>
>> Not sure why you only CC me on the answer. I have CCed again xen-devel as I
>> don't see any sensible discussion in it.
>>
>> On 26/01/2017 00:11, Tamas K Lengyel wrote:
>>>
>>> On Wed, Jan 25, 2017 at 3:41 PM, Julien Grall <julien.grall@xxxxxxx>
>>> wrote:
>>>>
>>>> Hi Tamas,
>>>>
>>>> On 25/01/2017 20:02, Tamas K Lengyel wrote:
>>>>>
>>>>>
>>>>> During an SMC trap it is possible that the user may change the memory
>>>>
>>>>
>>>>
>>>> By user, do you mean the monitor application?
>>>
>>>
>>> Yes.
>>
>>
>> It would be worth clarifying in the commit message.
>>
>>
>>>
>>>>
>>>>> from where the SMC was fetched. However, without flushing the icache
>>>>> the SMC may still trigger if the pCPU was idle during the trap. Flush
>>>>> the icache to ensure consistency.
>>>>
>>>>
>>>>
>>>> invalidate_icache will invalidate the cache to PoU on all the pCPUs. But
>>>> here you only mention a problem on the current pCPU. So which behavior do
>>>> you want to achieve?
>>>
>>>
>>> It would be sufficient to flush the icache on the specific pCPU that
>>> trapped with the SMC. Didn't see anything defined in Xen to achieve
>>> that.
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
>>>>> ---
>>>>> Cc: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>>>>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>>>> Cc: Julien Grall <julien.grall@xxxxxxx>
>>>>> ---
>>>>>  xen/arch/arm/monitor.c | 3 +++
>>>>>  1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/xen/arch/arm/monitor.c b/xen/arch/arm/monitor.c
>>>>> index 59ce8f635f..ae1b97993f 100644
>>>>> --- a/xen/arch/arm/monitor.c
>>>>> +++ b/xen/arch/arm/monitor.c
>>>>> @@ -63,6 +63,9 @@ int monitor_smc(void)
>>>>>          .reason = VM_EVENT_REASON_PRIVILEGED_CALL
>>>>>      };
>>>>>
>>>>> +    /* Nuke the icache as the memory may get changed underneath us. */
>>>>> +    invalidate_icache();
>>>>> +
>>>>
>>>>
>>>>
>>>> Can you explain why you put this call before the monitor trap and not
>>>> after?
>>>> From my understanding the monitoring application may change the memory.
>>>> But
>>>> by the time you modify the instruction, the instruction cache may have
>>>> prefetched the previous instruction. So the problem is still there.
>>>
>>>
>>> Not sure how would that happen exactly? The vCPU gets paused until the
>>> user responds to the vm_event request, so where would it perform the
>>> prefetch again? Also, with this change I've verified that the repeated
>>> SMC issue no longer presents itself (it has been triggered quite
>>> regularly on the hikey before).
>>
>>
>> The ARM ARM provides a set of expected behavior and where to call the cache
>> maintenance instruction. If it is not done correctly, it may not affect your
>> platform but at some point in the future (or even already today) it will
>> break a platform. I wish you good luck to debug that when it happens. It is
>> a really nightmare :).
>>
>> So what I care the most is what is the correct behavior based on the ARM
>> ARM. A good overview can be found in a talk made by Mark Rutland [1].
>>
>> What I was concerned about is the instruction cache been shared between
>> multiple processors (for instance L2 cache or higher). A vCPU could also get
>> rescheduled to another processor afterwards. Leading to accessing an out of
>> date instruction cache.
>
>  I see, yea, in that case the instruction may still trigger regardless. Sigh.
>
>>
>>>
>>> Also, for multi-vCPU guest another vCPU fetching the SMC before the
>>> memory modification happen (ie. just after this flush) is not a
>>> problem and is expected behavior. Providing a non-racy setup for
>>> trapping on multi-vCPU guests requires other work (like altp2m).
>>
>>
>> I am not that concerned about the SMC itself, but the fact that you may
>> modify the guest memory whilst it is running. So another vCPU may end up to
>> execute a mix between new and old instructions depending of the state of its
>> instruction cache. That would result to a potential undefined behavior.
>>
>> Also, you want to make sure that if you write another SMC in memory, it is
>> effectively affecting all the vCPUs now and not a moment after.
>
> Yeap.
>
>>
>> So I still think the invalidate_icache should be done afterwards. IHMO
>> modifying guest instructions while other vCPU are running is very fragile as
>> other thread may execute the instructions your are running.
>
> I see your point, just got confused because the return from
> monitor_traps is not the return from the user. That just sends off the
> notification to the user. The actual return happens elsewhere once the
> user replies. That would be the point where the flush should happen.
>
>>
>>>
>>>>
>>>> Furthermore, the instruction cache may not snoop the data cache. So you
>>>> have
>>>> to ensure the data written reached the memory. Otherwise you may read the
>>>> previous instruction. Where is it done? If you expect the monitor app to
>>>> flush the data cache, then please comment it.
>>>
>>>
>>> AFAICT there is no public API for the user to call to request flushing
>>> the caches like that. Memory could get changed by the user any time
>>> under the VM, not just during event callbacks. So I'm not sure where
>>> that comment should be added.
>>
>>
>> If the monitor app can modify the guest memory at any time. Then you need to
>> provide an interface to clean the data cache + invalidate the instruction
>> cache. Otherwise the guest may execute stale instructions.
>>
>> An hypercall might not be necessary because I think (this need to be check)
>> that you could do execute both cache flush from the monitor app.
>
> True! I'll check what happens if I execute "ic ialluis" in the monitor
> app instead of Xen. If the monitor application can do its own cache
> maintenance than that would be the best option.
>

So according to the manual (C5.3.9-10) IALLU and IALLUIS are not
available from EL0. IVAU can be enabled to execute in EL0; however on
my 4.1 kernel it generates a permission fault. I'm also not sure if
IVAU would actually be a solution. So IMHO a hypercall would be a good
way to enable safe flushing of the caches while all vCPUs of the
target domain are paused. Doing so only in a vm_event reply may be
insufficient as the memory could be changed outside of vm_event
callbacks. Thoughts?

Tamas

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