Re: [PATCH] PPC: Fix race in mtmsr paravirt implementation

[Alexander Graf <agraf@suse.de>]

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