Re: [PATCH] PPC: Fix race in mtmsr paravirt implementation
On 10/13/2011 11:22 AM, Bhushan Bharat-R65777 wrote: -Original Message- From: Alexander Graf [mailto:ag...@suse.de] Sent: Thursday, October 13, 2011 2:36 PM To: Bhushan Bharat-R65777 Cc:;; Bhushan Bharat- R65777 Subject: Re: [PATCH] PPC: Fix race in mtmsr paravirt implementation Am 13.10.2011 um 07:40 schrieb Bharat Bhushan: 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 --- 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 +beqno_check + /* Check if we have to fetch an interrupt */ lwzr31, (KVM_MAGIC_PAGE + KVM_MAGIC_INT)(0) cmpwir31, 0 beq+no_check -/* Check if we may trigger an interrupt */ -andi.r30, r30, MSR_EE -beqno_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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] PPC: Fix race in mtmsr paravirt implementation
> -Original Message- > From: Alexander Graf [mailto:ag...@suse.de] > Sent: Thursday, October 13, 2011 2:36 PM > To: Bhushan Bharat-R65777 > Cc: ; ; Bhushan Bharat- > R65777 > Subject: Re: [PATCH] PPC: Fix race in mtmsr paravirt implementation > > > Am 13.10.2011 um 07:40 schrieb Bharat Bhushan : > > > 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 > > --- > > 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 > > +beqno_check > > + > >/* Check if we have to fetch an interrupt */ > >lwzr31, (KVM_MAGIC_PAGE + KVM_MAGIC_INT)(0) > >cmpwir31, 0 > >beq+no_check > > > > -/* Check if we may trigger an interrupt */ > > -andi.r30, r30, MSR_EE > > -beqno_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. Thanks -Bharat > > >SCRATCH_RESTORE > > > >/* Nag hypervisor */ > > @@ -167,22 +167,20 @@ maybe_stay_in_guest: > > kvm_emulate_mtmsr_reg2: > >orir30, r0, 0 > > > > -/* Check if we have to fetch an interrupt */ > > -lwzr31, (KVM_MAGIC_PAGE + KVM_MAGIC_INT)(0) > > -cmpwir31, 0 > > -beq+no_mtmsr > > +/* Put MSR into magic page because we don't call mtmsr */ > > +STL64(r30, KVM_MAGIC_PAGE + KVM_MAGIC_MSR, 0) > > > >/* Check if we may trigger an interrupt */ > >andi.r31, r30, MSR_EE > >beqno_mtmsr > > > > -bdo_mtmsr > > +/* Check if we have to fetch an interrupt */ > > +lwzr31, (KVM_MAGIC_PAGE + KVM_MAGIC_INT)(0) > > +cmpwir31, 0 > > +bne-do_mtmsr > > > > no_mtmsr: > > > > -/* Put MSR into magic page because we don't call mtmsr */ > > -STL64(r30, KVM_MAGIC_PAGE + KVM_MAGIC_MSR, 0) > > - > > This one looks good. > > Alex > > >SCRATCH_RESTORE > > > >/* Go back to caller */ > > -- > > 1.7.0.4 > > > > -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] PPC: Fix race in mtmsr paravirt implementation
Am 13.10.2011 um 07:40 schrieb Bharat Bhushan : > 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 > --- > 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 > +beqno_check > + >/* Check if we have to fetch an interrupt */ >lwzr31, (KVM_MAGIC_PAGE + KVM_MAGIC_INT)(0) >cmpwir31, 0 >beq+no_check > > -/* Check if we may trigger an interrupt */ > -andi.r30, r30, MSR_EE > -beqno_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? >SCRATCH_RESTORE > >/* Nag hypervisor */ > @@ -167,22 +167,20 @@ maybe_stay_in_guest: > kvm_emulate_mtmsr_reg2: >orir30, r0, 0 > > -/* Check if we have to fetch an interrupt */ > -lwzr31, (KVM_MAGIC_PAGE + KVM_MAGIC_INT)(0) > -cmpwir31, 0 > -beq+no_mtmsr > +/* Put MSR into magic page because we don't call mtmsr */ > +STL64(r30, KVM_MAGIC_PAGE + KVM_MAGIC_MSR, 0) > >/* Check if we may trigger an interrupt */ >andi.r31, r30, MSR_EE >beqno_mtmsr > > -bdo_mtmsr > +/* Check if we have to fetch an interrupt */ > +lwzr31, (KVM_MAGIC_PAGE + KVM_MAGIC_INT)(0) > +cmpwir31, 0 > +bne-do_mtmsr > > no_mtmsr: > > -/* Put MSR into magic page because we don't call mtmsr */ > -STL64(r30, KVM_MAGIC_PAGE + KVM_MAGIC_MSR, 0) > - This one looks good. Alex >SCRATCH_RESTORE > >/* Go back to caller */ > -- > 1.7.0.4 > > -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
