Re: [PATCH 6/6] target/ppc: Implement HEIR SPR

[Nicholas Piggin]

On Tue May 9, 2023 at 7:51 PM AEST, Harsh Prateek Bora wrote:
> On Thu, Mar 23, 2023 at 12:22:37PM +1000, Nicholas Piggin wrote:
> > The hypervisor emulation assistance interrupt modifies HEIR to
> > contain the value of the instruction which caused the exception.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  target/ppc/cpu.h         |  1 +
> >  target/ppc/cpu_init.c    | 23 +++++++++++++++++++++++
> >  target/ppc/excp_helper.c | 12 +++++++++++-
> >  3 files changed, 35 insertions(+), 1 deletion(-)
> > 
>
> <snip>
>
> > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > index 2e0321ab69..d206903562 100644
> > --- a/target/ppc/excp_helper.c
> > +++ b/target/ppc/excp_helper.c
> > @@ -1614,13 +1614,23 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int 
> > excp)
> >      case POWERPC_EXCP_HDECR:     /* Hypervisor decrementer exception       
> >   */
> >      case POWERPC_EXCP_HDSI:      /* Hypervisor data storage exception      
> >   */
> >      case POWERPC_EXCP_SDOOR_HV:  /* Hypervisor Doorbell interrupt          
> >   */
> > -    case POWERPC_EXCP_HV_EMU:
> >      case POWERPC_EXCP_HVIRT:     /* Hypervisor virtualization              
> >   */
> >          srr0 = SPR_HSRR0;
> >          srr1 = SPR_HSRR1;
> >          new_msr |= (target_ulong)MSR_HVB;
> >          new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
> >          break;
> > +    case POWERPC_EXCP_HV_EMU:
> > +        env->spr[SPR_HEIR] = insn;
> > +        if (is_prefix_excp(env, insn)) {
> > +            uint32_t insn2 = ppc_ldl_code(env, env->nip + 4);
> > +            env->spr[SPR_HEIR] |= (uint64_t)insn2 << 32;
> > +        }
> > +        srr0 = SPR_HSRR0;
> > +        srr1 = SPR_HSRR1;
> > +        new_msr |= (target_ulong)MSR_HVB;
> > +        new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
> > +        break;
>
> Since there is a common code, this could be better written like:
>     case POWERPC_EXCP_HV_EMU:
>         env->spr[SPR_HEIR] = insn;
>         if (is_prefix_excp(env, insn)) {
>             uint32_t insn2 = ppc_ldl_code(env, env->nip + 4);
>             env->spr[SPR_HEIR] |= (uint64_t)insn2 << 32;
>         }
>       /* fall through below common code for EXCP_HVIRT */
>     case POWERPC_EXCP_HVIRT:     /* Hypervisor virtualization                
> */
>         srr0 = SPR_HSRR0;
>         srr1 = SPR_HSRR1;
>         new_msr |= (target_ulong)MSR_HVB;
>         new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
>         break;

That would be wrong for the other HSRR fallthroughs above it.

Thanks,
Nick