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

[Scott Wood <scottwood@freescale.com>]

On 05/10/2013 12:01:38 AM, Bhushan Bharat-R65777 wrote:
> 
> 
> > -----Original Message-----
> > From: kvm-ppc-owner@vger.kernel.org  
> [mailto:kvm-ppc-owner@vger.kernel.org] On
> > Behalf Of Scott Wood
> > Sent: Friday, May 10, 2013 8:40 AM
> > To: Alexander Graf; Benjamin Herrenschmidt
> > Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org;  
> linuxppc-dev@lists.ozlabs.org;
> > 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();
> 
> Here we hard disable in kvmppc_prepare_to_enter(), so my comment in  
> other patch about interrupt loss is no more valid.
> 
> So here
>   MSR.EE = 0
>   local_paca->soft_enabled = 0
>   local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
> 
> > +
> >  	while (true) {
> >  		if (need_resched()) {
> >  			local_irq_enable();
> 
> This will make the state:
>   MSR.EE = 1
>   local_paca->soft_enabled = 1
>   local_paca->irq_happened = PACA_IRQ_HARD_DIS;  //same as before
> 
> Is that a valid state where interrupts are fully enabled and  
> irq_happend in not 0?

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

> int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
> {
>         int r = 0;
>         WARN_ON_ONCE(!irqs_disabled());
> 
>         kvmppc_core_check_exceptions(vcpu);
> 
>         if (vcpu->requests) {
>                 /* Exception delivery raised request; start over */
>                 return 1;
>         }
> 
>         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  
WARN_ON to double check that interrupts are hard-disabled (eventually  
we'll probably want to make these critical-path assertions dependent on  
a debug option...).  It doesn't really matter all that much, though,  
since we don't have MSR_WE on any 64-bit booke chips. :-)

-Scott