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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 26 Jan 2017, Tamas K Lengyel wrote:
> 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.

I think there is a Linux syscall available for this kind of things,
called cacheflush. See arch/arm/kernel/traps.c:arm_syscall.

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