Re: [PATCH 1/2] KVM: PPC: BookE: Disable preemption before loading guest FP and Altivec

[IBM]

Shrikanth Hegde <sshegde@xxxxxxxxxxxxx> writes:

> Hi Narayana.
>
> On 10/1/26 4:56 PM, Narayana Murty N wrote:
>> Hi Shrikanth,
>> Thanks for the fixes. I had one question on patch 1.
>
> See response below.
>
>> 
>>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>>> index 13ad4cf5fa71..5b9118eefe1d 100644
>>> --- a/arch/powerpc/kvm/booke.c
>>> +++ b/arch/powerpc/kvm/booke.c
>>> @@ -1404,10 +1404,17 @@ int kvmppc_handle_exit(struct kvm_vcpu *vcpu, unsigned int exit_nr)
>>>           if (s <= 0)
>>>               r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV);
>>>           else {
>>> -            /* interrupts now hard-disabled */
>>> +            /*
>>> +             * kvmppc_fix_ee_before_entry() marks the software
>>> +             * IRQ state enabled while interrupts are still
>>> +             * hard-disabled. So disable preemption while loading
>>> +             * guest FP and Altivec.
>>> +             */
>>>               kvmppc_fix_ee_before_entry();
>>> +            preempt_disable();
>>>               kvmppc_load_guest_fp(vcpu);
>>>               kvmppc_load_guest_altivec(vcpu);
>>> +            preempt_enable_no_resched();
>> Would it be simpler to move kvmppc_fix_ee_before_entry() after the
>> FP/Altivec loads instead?
>> 
>> The normal kvmppc_vcpu_run() entry path already loads the guest
>> FP/Altivec state while interrupts are still disabled and calls
>> kvmppc_fix_ee_before_entry() immediately before entering the guest.
>> 
>> So could this path follow the same ordering:
>> 
>> kvmppc_load_guest_fp(vcpu);
>> kvmppc_load_guest_altivec(vcpu);
>> kvmppc_fix_ee_before_entry();
>> 

Thanks Narayana, that actually make more sense to me.


>> That would avoid making the software IRQ state enabled before loading
>> the guest FP/Altivec state, and also avoid the additional
>> preempt_disable()/preempt_enable_no_resched() pair.
>> 
>> Thanks,
>> Narayana Murty.
>>>           }
>>>       }
>> 
>
> I thought I had put that for discussion after ---, but looks like I forgot.
>
> I don't mind the above too.  but I didn't have a way to test it.

Actually I agree with Narayan's reasoning here. 

Sorry for being so late to the game. As you know we have been occupied
with other things internally.

> So kept it as is based on what Christian said works for him.
> If you have a way to test the patches, please let me know.
> We can try that too.

Let's ask Christian directly :)
@Christian - Once we have v2, would you kindly help us test that
please?

We think the other approach which Narayana suggested might be a better
fix for this and it can avoid preempt_enable/disable() dance. However,
looks like we may not have the board where we can reproduce this.


-ritesh