On 10/13/2011 11:22 AM, Bhushan Bharat-R65777 wrote:
-----Original Message-----
From: Alexander Graf [mailto:agraf@xxxxxxx]
Sent: Thursday, October 13, 2011 2:36 PM
To: Bhushan Bharat-R65777
Cc:<kvm-ppc@xxxxxxxxxxxxxxx>;<bharatb.yadav@xxxxxxxxx>; Bhushan Bharat-
R65777
Subject: Re: [PATCH] PPC: Fix race in mtmsr paravirt implementation
Am 13.10.2011 um 07:40 schrieb Bharat Bhushan<r65777@xxxxxxxxxxxxx>:
The current implementation of mtmsr and mtmsrd are racy in that it
does:
* check (int_pending == 0)
---> host sets int_pending = 1<---
* write shared page
* done
while instead we should check for int_pending after the shared page is
written.
Signed-off-by: Bharat Bhushan<bharat.bhushan@xxxxxxxxxxxxx>
---
arch/powerpc/kernel/kvm_emul.S | 22 ++++++++++------------
1 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/kernel/kvm_emul.S
b/arch/powerpc/kernel/kvm_emul.S index f2b1b25..65f853b 100644
--- a/arch/powerpc/kernel/kvm_emul.S
+++ b/arch/powerpc/kernel/kvm_emul.S
@@ -85,15 +85,15 @@ kvm_emulate_mtmsrd_reg:
/* Put MSR back into magic page */
STL64(r31, KVM_MAGIC_PAGE + KVM_MAGIC_MSR, 0)
+ /* Check if we may trigger an interrupt */
+ andi. r30, r30, MSR_EE
+ beq no_check
+
/* Check if we have to fetch an interrupt */
lwz r31, (KVM_MAGIC_PAGE + KVM_MAGIC_INT)(0)
cmpwi r31, 0
beq+ no_check
- /* Check if we may trigger an interrupt */
- andi. r30, r30, MSR_EE
- beq no_check
-
This chunk should actually be ok already. We're basically doing:
if(likely(!int_pending)&& !(new_msr& MSR_EE))
goto no_check;
Since we wrote shared.msr before the check, we're good, no?
Actually I borrowed this from wrtee implementation and keep consistant across mtmsr/ mtmsrd and wrtee/i.
I thought of it like: If we are qualified to take interrupt (EE set) then only check for pending interrupt rather than check for pending interrupt and then check whether we are qualified to take it or not.
If you think earlier is better way of understanding then I will change the patch.
The question is which one is the more likely case. I would assume it's
more unlikely for an interrupt to be active than for EE to be on. But
the real point is that it shouldn't matter, so there's no need to change
the code :).
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
