From: marc.zyngier@arm•com (Marc Zyngier)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH 3/5] ARM: KVM: make sure maintainance operation complete before world switch
Date: Thu, 20 Jun 2013 18:29:49 +0100 [thread overview]
Message-ID: <51C33C0D.5020503@arm.com> (raw)
In-Reply-To: <20130620171409.GB4563@lvm>
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@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...
next prev parent reply other threads:[~2013-06-20 17:29 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-06-19 13:20 [PATCH 0/5] A handful of KVM/ARM fixes Marc Zyngier
2013-06-19 13:20 ` [PATCH 1/5] ARM: KVM: perform save/restore of PAR Marc Zyngier
2013-06-19 13:20 ` [PATCH 2/5] ARM: KVM: add missing dsb before invalidating Stage-2 TLBs Marc Zyngier
2013-06-20 0:05 ` Christoffer Dall
2013-06-20 0:08 ` Christoffer Dall
2013-06-20 10:47 ` Will Deacon
2013-06-19 13:20 ` [PATCH 3/5] ARM: KVM: make sure maintainance operation complete before world switch Marc Zyngier
2013-06-20 0:18 ` Christoffer Dall
2013-06-20 8:13 ` Marc Zyngier
2013-06-20 17:14 ` Christoffer Dall
2013-06-20 17:29 ` Marc Zyngier [this message]
2013-06-20 18:15 ` Will Deacon
2013-06-20 18:28 ` Christoffer Dall
2013-06-20 18:38 ` Will Deacon
2013-06-20 18:50 ` Christoffer Dall
2013-06-20 10:48 ` Will Deacon
2013-06-19 13:20 ` [PATCH 4/5] ARM: KVM: clear exclusive monitor on all exception returns Marc Zyngier
2013-06-20 0:27 ` Christoffer Dall
2013-06-20 8:29 ` Marc Zyngier
2013-06-19 13:20 ` [PATCH 5/5] ARM: KVM: issue a DSB after cache maintainance operations Marc Zyngier
2013-06-20 10:46 ` Will Deacon
2013-06-20 18:33 ` [PATCH 0/5] A handful of KVM/ARM fixes Christoffer Dall
2013-06-20 18:41 ` Marc Zyngier
2013-06-20 18:48 ` Christoffer Dall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=51C33C0D.5020503@arm.com \
--to=marc.zyngier@arm$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox