* Re: [PATCH] PPC: Fix race in mtmsr paravirt implementation
2011-10-13 5:52 [PATCH] PPC: Fix race in mtmsr paravirt implementation Bharat Bhushan
@ 2011-10-13 9:06 ` Alexander Graf
2011-10-13 9:22 ` Bhushan Bharat-R65777
2011-10-13 9:45 ` Alexander Graf
2 siblings, 0 replies; 4+ messages in thread
From: Alexander Graf @ 2011-10-13 9:06 UTC (permalink / raw)
To: kvm-ppc
Am 13.10.2011 um 07:40 schrieb Bharat Bhushan <r65777@freescale.com>:
> 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@freescale.com>
> ---
> 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?
> SCRATCH_RESTORE
>
> /* Nag hypervisor */
> @@ -167,22 +167,20 @@ maybe_stay_in_guest:
> kvm_emulate_mtmsr_reg2:
> ori r30, r0, 0
>
> - /* Check if we have to fetch an interrupt */
> - lwz r31, (KVM_MAGIC_PAGE + KVM_MAGIC_INT)(0)
> - cmpwi r31, 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
> beq no_mtmsr
>
> - b do_mtmsr
> + /* Check if we have to fetch an interrupt */
> + lwz r31, (KVM_MAGIC_PAGE + KVM_MAGIC_INT)(0)
> + cmpwi r31, 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
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* RE: [PATCH] PPC: Fix race in mtmsr paravirt implementation
2011-10-13 5:52 [PATCH] PPC: Fix race in mtmsr paravirt implementation Bharat Bhushan
2011-10-13 9:06 ` Alexander Graf
@ 2011-10-13 9:22 ` Bhushan Bharat-R65777
2011-10-13 9:45 ` Alexander Graf
2 siblings, 0 replies; 4+ messages in thread
From: Bhushan Bharat-R65777 @ 2011-10-13 9:22 UTC (permalink / raw)
To: kvm-ppc
> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, October 13, 2011 2:36 PM
> To: Bhushan Bharat-R65777
> Cc: <kvm-ppc@vger.kernel.org>; <bharatb.yadav@gmail.com>; Bhushan Bharat-
> R65777
> Subject: Re: [PATCH] PPC: Fix race in mtmsr paravirt implementation
>
>
> Am 13.10.2011 um 07:40 schrieb Bharat Bhushan <r65777@freescale.com>:
>
> > 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@freescale.com>
> > ---
> > 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.
Thanks
-Bharat
>
> > SCRATCH_RESTORE
> >
> > /* Nag hypervisor */
> > @@ -167,22 +167,20 @@ maybe_stay_in_guest:
> > kvm_emulate_mtmsr_reg2:
> > ori r30, r0, 0
> >
> > - /* Check if we have to fetch an interrupt */
> > - lwz r31, (KVM_MAGIC_PAGE + KVM_MAGIC_INT)(0)
> > - cmpwi r31, 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
> > beq no_mtmsr
> >
> > - b do_mtmsr
> > + /* Check if we have to fetch an interrupt */
> > + lwz r31, (KVM_MAGIC_PAGE + KVM_MAGIC_INT)(0)
> > + cmpwi r31, 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
> >
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PPC: Fix race in mtmsr paravirt implementation
2011-10-13 5:52 [PATCH] PPC: Fix race in mtmsr paravirt implementation Bharat Bhushan
2011-10-13 9:06 ` Alexander Graf
2011-10-13 9:22 ` Bhushan Bharat-R65777
@ 2011-10-13 9:45 ` Alexander Graf
2 siblings, 0 replies; 4+ messages in thread
From: Alexander Graf @ 2011-10-13 9:45 UTC (permalink / raw)
To: kvm-ppc
On 10/13/2011 11:22 AM, Bhushan Bharat-R65777 wrote:
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Thursday, October 13, 2011 2:36 PM
>> To: Bhushan Bharat-R65777
>> Cc:<kvm-ppc@vger.kernel.org>;<bharatb.yadav@gmail.com>; Bhushan Bharat-
>> R65777
>> Subject: Re: [PATCH] PPC: Fix race in mtmsr paravirt implementation
>>
>>
>> Am 13.10.2011 um 07:40 schrieb Bharat Bhushan<r65777@freescale.com>:
>>
>>> 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@freescale.com>
>>> ---
>>> 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
^ permalink raw reply [flat|nested] 4+ messages in thread