Re: [PATCH 4/6] target/ppc: optimize p9 exception handling routines

[BALATON Zoltan]

On Mon, 20 May 2024, BALATON Zoltan wrote:
> On Mon, 20 May 2024, Harsh Prateek Bora wrote:
> > Currently, p9 exception handling has multiple if-condition checks where
> > it does an indirect access to pending_interrupts via env. Cache the
> > value during entry and reuse later to avoid multiple indirect accesses.
> >
> > Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> > ---
> > target/ppc/excp_helper.c | 39 +++++++++++++++++++++------------------
> > 1 file changed, 21 insertions(+), 18 deletions(-)
> >
> > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > index 0712098cf7..4f158196bb 100644
> > --- a/target/ppc/excp_helper.c
> > +++ b/target/ppc/excp_helper.c
> > @@ -1844,8 +1844,10 @@ static int p8_next_unmasked_interrupt(CPUPPCState 
> > *env)
> >
> > static int p9_interrupt_powersave(CPUPPCState *env)
> > {
> > +    uint32_t pending_interrupts = env->pending_interrupts;
> > +
>
> LPCR also seems to be used a lot and other similar *_powersave functions only 
> use pending_interrupts and LPCR so maybe you could change these functions to 
> take the pending_interrupts and lpcr as parameters and pass them from the 
> caller that already have it so even this dereference could be saved?

(In that case this patch may need to be split in two as well.)

Regards,
BALATON Zoltan

> >     /* External Exception */
> > -    if ((env->pending_interrupts & PPC_INTERRUPT_EXT) &&
> > +    if ((pending_interrupts & PPC_INTERRUPT_EXT) &&
> >         (env->spr[SPR_LPCR] & LPCR_EEE)) {
> >         bool heic = !!(env->spr[SPR_LPCR] & LPCR_HEIC);
> >         if (!heic || !FIELD_EX64_HV(env->msr) ||
> > @@ -1854,35 +1856,35 @@ static int p9_interrupt_powersave(CPUPPCState *env)
> >         }
> >     }
> >     /* Decrementer Exception */
> > -    if ((env->pending_interrupts & PPC_INTERRUPT_DECR) &&
> > +    if ((pending_interrupts & PPC_INTERRUPT_DECR) &&
> >         (env->spr[SPR_LPCR] & LPCR_DEE)) {
> >         return PPC_INTERRUPT_DECR;
> >     }
> >     /* Machine Check or Hypervisor Maintenance Exception */
> >     if (env->spr[SPR_LPCR] & LPCR_OEE) {
> > -        if (env->pending_interrupts & PPC_INTERRUPT_MCK) {
> > +        if (pending_interrupts & PPC_INTERRUPT_MCK) {
> >             return PPC_INTERRUPT_MCK;
> >         }
> > -        if (env->pending_interrupts & PPC_INTERRUPT_HMI) {
> > +        if (pending_interrupts & PPC_INTERRUPT_HMI) {
> >             return PPC_INTERRUPT_HMI;
> >         }
> >     }
> >     /* Privileged Doorbell Exception */
> > -    if ((env->pending_interrupts & PPC_INTERRUPT_DOORBELL) &&
> > +    if ((pending_interrupts & PPC_INTERRUPT_DOORBELL) &&
> >         (env->spr[SPR_LPCR] & LPCR_PDEE)) {
> >         return PPC_INTERRUPT_DOORBELL;
> >     }
> >     /* Hypervisor Doorbell Exception */
> > -    if ((env->pending_interrupts & PPC_INTERRUPT_HDOORBELL) &&
> > +    if ((pending_interrupts & PPC_INTERRUPT_HDOORBELL) &&
> >         (env->spr[SPR_LPCR] & LPCR_HDEE)) {
> >         return PPC_INTERRUPT_HDOORBELL;
> >     }
> >     /* Hypervisor virtualization exception */
> > -    if ((env->pending_interrupts & PPC_INTERRUPT_HVIRT) &&
> > +    if ((pending_interrupts & PPC_INTERRUPT_HVIRT) &&
> >         (env->spr[SPR_LPCR] & LPCR_HVEE)) {
> >         return PPC_INTERRUPT_HVIRT;
> >     }
> > -    if (env->pending_interrupts & PPC_INTERRUPT_RESET) {
> > +    if (pending_interrupts & PPC_INTERRUPT_RESET) {
> >         return PPC_INTERRUPT_RESET;
> >     }
> >     return 0;
> > @@ -1891,11 +1893,12 @@ static int p9_interrupt_powersave(CPUPPCState *env)
> > static int p9_next_unmasked_interrupt(CPUPPCState *env)
> > {
> >     CPUState *cs = env_cpu(env);
> > +    uint32_t pending_interrupts = env->pending_interrupts;
> >
> >     /* Ignore MSR[EE] when coming out of some power management states */
> >     bool msr_ee = FIELD_EX64(env->msr, MSR, EE) || env->resume_as_sreset;
> >
> > -    assert((env->pending_interrupts & P9_UNUSED_INTERRUPTS) == 0);
> > +    assert((pending_interrupts & P9_UNUSED_INTERRUPTS) == 0);
> >
> >     if (cs->halted) {
> >         if (env->spr[SPR_PSSCR] & PSSCR_EC) {
> > @@ -1914,12 +1917,12 @@ static int p9_next_unmasked_interrupt(CPUPPCState 
> > *env)
> >     }
> >
> >     /* Machine check exception */
> > -    if (env->pending_interrupts & PPC_INTERRUPT_MCK) {
> > +    if (pending_interrupts & PPC_INTERRUPT_MCK) {
> >         return PPC_INTERRUPT_MCK;
> >     }
> >
> >     /* Hypervisor decrementer exception */
> > -    if (env->pending_interrupts & PPC_INTERRUPT_HDECR) {
> > +    if (pending_interrupts & PPC_INTERRUPT_HDECR) {
> >         /* LPCR will be clear when not supported so this will work */
> >         bool hdice = !!(env->spr[SPR_LPCR] & LPCR_HDICE);
> >         if ((msr_ee || !FIELD_EX64_HV(env->msr)) && hdice) {
> > @@ -1929,7 +1932,7 @@ static int p9_next_unmasked_interrupt(CPUPPCState 
> > *env)
> >     }
> >
> >     /* Hypervisor virtualization interrupt */
> > -    if (env->pending_interrupts & PPC_INTERRUPT_HVIRT) {
> > +    if (pending_interrupts & PPC_INTERRUPT_HVIRT) {
> >         /* LPCR will be clear when not supported so this will work */
> >         bool hvice = !!(env->spr[SPR_LPCR] & LPCR_HVICE);
> >         if ((msr_ee || !FIELD_EX64_HV(env->msr)) && hvice) {
> > @@ -1938,7 +1941,7 @@ static int p9_next_unmasked_interrupt(CPUPPCState 
> > *env)
> >     }
> >
> >     /* External interrupt can ignore MSR:EE under some circumstances */
> > -    if (env->pending_interrupts & PPC_INTERRUPT_EXT) {
> > +    if (pending_interrupts & PPC_INTERRUPT_EXT) {
> >         bool lpes0 = !!(env->spr[SPR_LPCR] & LPCR_LPES0);
> >         bool heic = !!(env->spr[SPR_LPCR] & LPCR_HEIC);
> >         /* HEIC blocks delivery to the hypervisor */
> > @@ -1950,20 +1953,20 @@ static int p9_next_unmasked_interrupt(CPUPPCState 
> > *env)
> >     }
> >     if (msr_ee != 0) {
> >         /* Decrementer exception */
> > -        if (env->pending_interrupts & PPC_INTERRUPT_DECR) {
> > +        if (pending_interrupts & PPC_INTERRUPT_DECR) {
> >             return PPC_INTERRUPT_DECR;
> >         }
> > -        if (env->pending_interrupts & PPC_INTERRUPT_DOORBELL) {
> > +        if (pending_interrupts & PPC_INTERRUPT_DOORBELL) {
> >             return PPC_INTERRUPT_DOORBELL;
> >         }
> > -        if (env->pending_interrupts & PPC_INTERRUPT_HDOORBELL) {
> > +        if (pending_interrupts & PPC_INTERRUPT_HDOORBELL) {
> >             return PPC_INTERRUPT_HDOORBELL;
> >         }
> > -        if (env->pending_interrupts & PPC_INTERRUPT_PERFM) {
> > +        if (pending_interrupts & PPC_INTERRUPT_PERFM) {
> >             return PPC_INTERRUPT_PERFM;
> >         }
> >         /* EBB exception */
> > -        if (env->pending_interrupts & PPC_INTERRUPT_EBB) {
> > +        if (pending_interrupts & PPC_INTERRUPT_EBB) {
> >             /*
> >              * EBB exception must be taken in problem state and
> >              * with BESCR_GE set.