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

[Christian Zigotzky]

On 30/10/26 17:21, Ritesh Harjani (IBM) wrote:
> 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

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)