[PATCH 3/5] ARM: KVM: make sure maintainance operation complete before world switch

[Marc Zyngier]

On 20/06/13 18:14, Christoffer Dall wrote:
> On Thu, Jun 20, 2013 at 09:13:22AM +0100, Marc Zyngier wrote:
>> On 20/06/13 01:18, Christoffer Dall wrote:
>>> On Wed, Jun 19, 2013 at 02:20:04PM +0100, Marc Zyngier wrote:
>>>> We may have preempted the guest while it was performing a maintainance
>>>> operation (TLB invalidation, for example). Make sure it completes
>>>> before we do anything else by adding the necessary barriers.
>>>>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier at arm.com>
>>>> ---
>>>>  arch/arm/kvm/interrupts.S | 9 +++++++++
>>>>  1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
>>>> index afa6c04..3124e0f 100644
>>>> --- a/arch/arm/kvm/interrupts.S
>>>> +++ b/arch/arm/kvm/interrupts.S
>>>> @@ -149,6 +149,15 @@ __kvm_vcpu_return:
>>>>  	 * r0: vcpu pointer
>>>>  	 * r1: exception code
>>>>  	 */
>>>> +
>>>> +	/*
>>>> +	 * We may have preempted the guest while it was performing a
>>>> +	 * maintainance operation (TLB invalidation, for example). Make
>>>> +	 * sure it completes before we do anything else.
>>>> +	 */
>>>
>>> Can you explain what could go wrong here without these two instructions?
>>
>> There would be no guarantee that the TLB invalidation has effectively
>> completed, and is visible by other CPUs. Not sure that would be a
>> massive issue in any decent guest OS, but I thought it was worth plugging.
> 
> ok, I was trying to think about how it would break, and if a guest needs
> a TLB invalidation to be visisble by other CPUs it would have to have a
> dsb/isb itself after the operation, and that would eventually be
> executed once the VCPU was rescheduled, but potentially on another CPU,
> but then I wonder if the PCPU migration on the host wouldn't take care
> of it?
> 
> It sounds like you're not 100% sure it actually breaks something (or am
> I reading it wrong?), but if the performance impact is minor, why not be
> on the safe side I guess.

I think a well written guest wouldn't be affected.

>>
>> Another (more serious) thing I had doubts about was that we're about to
>> switch VMID to restore the host context. The ARM ARM doesn't clearly
>> specify the interaction between pending TLB maintainance and VMID
>> switch, and I'm worried that you could end up performing the TLB
>> maintainance on the *host* TLBs rather than on the guest's.
>>
>> Having this dsb/isb sequence before switching VMID gives us a strong
>> guarantee that such a mixup cannot occur.
>>
> This is really hurting my brain.
> 
> Again, it seems the argument is, why not, and maybe it's required.
> And indeed, if it gives us peace of mind, I'm ok with it.

I guess my problem here is that the spec isn't 100% clear about what
happens. Which means a compliant implementation could do things that
would go horribly wrong.

I'm fairly confident that Cortex-A15 doesn't require this. But other
implementations might, and that's what I'm trying to cover here.

> Sorry about this OCD.

No worries.

	M.
-- 
Jazz is not dead. It just smells funny...