Re: [PATCH v2 1/3] KVM: PPC: Book3S HV: Fix race in re-enabling XIVE escalation interrupts

[Paul Mackerras <paulus@ozlabs.org>]

On Wed, Aug 14, 2019 at 02:46:38PM +1000, Jordan Niethe wrote:
> On Tue, 2019-08-13 at 20:03 +1000, Paul Mackerras wrote:

[snip]
> > diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > index 337e644..2e7e788 100644
> > --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > @@ -2831,29 +2831,39 @@ kvm_cede_prodded:
> >  kvm_cede_exit:
> >  	ld	r9, HSTATE_KVM_VCPU(r13)
> >  #ifdef CONFIG_KVM_XICS
> > -	/* Abort if we still have a pending escalation */
> > +	/* are we using XIVE with single escalation? */
> > +	ld	r10, VCPU_XIVE_ESC_VADDR(r9)
> > +	cmpdi	r10, 0
> > +	beq	3f
> > +	li	r6, XIVE_ESB_SET_PQ_00
> Would it make sense to put the above instruction down into the 4: label
> instead? If we do not branch to 4, r6 is overwriten anyway. 

Right.

> I think that would save a load when we do not branch to 4. Also it

Well, li is a load immediate rather than a load ("load" would normally
imply a load from memory).  Load-immediate instructions are
essentially free since they can easily be executed in parallel with
other instructions and execute in a single cycle.

> would mean that you could use r5 everywhere instead of changing it to
> r6? 

Yes.  If I have to respin the patch for other reasons then I will
rearrange things as you suggest.  I don't think it's worth respinning
just for this change -- it won't reduce the total number of
instructions, and I strongly doubt there would be any measurable
performance difference.

> > +	/*
> > +	 * If we still have a pending escalation, abort the cede,
> > +	 * and we must set PQ to 10 rather than 00 so that we don't
> > +	 * potentially end up with two entries for the escalation
> > +	 * interrupt in the XIVE interrupt queue.  In that case
> > +	 * we also don't want to set xive_esc_on to 1 here in
> > +	 * case we race with xive_esc_irq().
> > +	 */
> >  	lbz	r5, VCPU_XIVE_ESC_ON(r9)
> >  	cmpwi	r5, 0
> > -	beq	1f
> > +	beq	4f
> >  	li	r0, 0
> >  	stb	r0, VCPU_CEDED(r9)
> > -1:	/* Enable XIVE escalation */
> > -	li	r5, XIVE_ESB_SET_PQ_00
> > +	li	r6, XIVE_ESB_SET_PQ_10
> > +	b	5f
> > +4:	li	r0, 1
> > +	stb	r0, VCPU_XIVE_ESC_ON(r9)
> > +	/* make sure store to xive_esc_on is seen before xive_esc_irq
> > runs */
> > +	sync
> > +5:	/* Enable XIVE escalation */
> >  	mfmsr	r0
> >  	andi.	r0, r0, MSR_DR		/* in real mode? */
> >  	beq	1f
> > -	ld	r10, VCPU_XIVE_ESC_VADDR(r9)
> > -	cmpdi	r10, 0
> > -	beq	3f
> > -	ldx	r0, r10, r5
> > +	ldx	r0, r10, r6
> >  	b	2f
> >  1:	ld	r10, VCPU_XIVE_ESC_RADDR(r9)
> > -	cmpdi	r10, 0
> > -	beq	3f
> > -	ldcix	r0, r10, r5
> > +	ldcix	r0, r10, r6
> >  2:	sync
> > -	li	r0, 1
> > -	stb	r0, VCPU_XIVE_ESC_ON(r9)
> >  #endif /* CONFIG_KVM_XICS */
> >  3:	b	guest_exit_cont
> >  

Paul.