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

[Shrikanth Hegde]

Hi.

On 11/1/26 1:58 PM, Christian Zigotzky wrote:
> 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.

Thanks Christian.

 
> - Christian

Hi Ritesh, Narayana,

I am afraid I won't be able to work on this at-least for couple of weeks from now.

If v2 can be worked upon by our kvm team meanwhile, it would be helpful. It is always
good to fix long standing bugs sooner.

Where applicable, feel free to carry the tags.