Subject: Re: [PATCH v2 4/4] kvm/ppc: IRQ disabling cleanup

From: Scott Wood <[email-masked]>
Date: Fri, 10 May 2013 17:53:23 -0500

On 05/10/2013 12:01:38 AM, Bhushan Bharat-R65777 wrote:
>=20
>=20
> > -----Original Message-----
> > From: [email-masked] =20
> [mailto:[email-masked]] On
> > Behalf Of Scott Wood
> > Sent: Friday, May 10, 2013 8:40 AM
> > To: Alexander Graf; Benjamin Herrenschmidt
> > Cc: [email-masked]; [email-masked]; =20
> [email-masked];
> > Wood Scott-B07421
> > Subject: [PATCH v2 4/4] kvm/ppc: IRQ disabling cleanup
> >
> > -	WARN_ON_ONCE(!irqs_disabled());
> > +	WARN_ON(irqs_disabled());
> > +	hard_irq_disable();
>=20
> Here we hard disable in kvmppc_prepare_to_enter(), so my comment in =20
> other patch about interrupt loss is no more valid.
>=20
> So here
>   MSR.EE =3D 0
>   local_paca->soft_enabled =3D 0
>   local_paca->irq_happened |=3D PACA_IRQ_HARD_DIS;
>=20
> > +
> >  	while (true) {
> >  		if (need_resched()) {
> >  			local_irq_enable();
>=20
> This will make the state:
>   MSR.EE =3D 1
>   local_paca->soft_enabled =3D 1
>   local_paca->irq_happened =3D PACA_IRQ_HARD_DIS;  //same as before
>=20
> Is that a valid state where interrupts are fully enabled and =20
> irq_happend in not 0?

PACA_IRQ_HARD_DIS will have been cleared by local_irq_enable(), as =20
Tiejun pointed out.

> int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
> {
>         int r =3D 0;
>         WARN_ON_ONCE(!irqs_disabled());
>=20
>         kvmppc_core_check_exceptions(vcpu);
>=20
>         if (vcpu->requests) {
>                 /* Exception delivery raised request; start over */
>                 return 1;
>         }
>=20
>         if (vcpu->arch.shared->msr & MSR_WE) {
>                 local_irq_enable();
>                 kvm_vcpu_block(vcpu);
>                 clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
>                 local_irq_disable();
> ^^^
> We do not require hard_irq_disable() here?

Yes, that should be changed to hard_irq_disable(), and I'll add a =20
WARN_ON to double check that interrupts are hard-disabled (eventually =20
we'll probably want to make these critical-path assertions dependent on =20
a debug option...).  It doesn't really matter all that much, though, =20
since we don't have MSR_WE on any 64-bit booke chips. :-)

-Scott=