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.
