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

[Christian Zigotzky <chzigotzky@xenosoft.de>]

On 30/10/26 17:21, Ritesh Harjani (IBM) wrote:
> Shrikanth Hegde <sshegde@linux.ibm.com> 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
>

Yes, I can help with testing.

- Christian

-- 
Sent with BrassMonkey 35.0.2 (https://github.com/chzigotzky/Web-Browsers-and-Suites-for-Linux-PPC/releases/tag/BrassMonkey_35.0.2)