Re: [PATCH v2 6/8] target/ppc: 74xx: System Call exception cleanup

[BALATON Zoltan]

On Thu, 27 Jan 2022, Fabiano Rosas wrote:
> Remove the BookE code and add a comment explaining why we need to keep
> hypercall support even though this CPU does not have a hypervisor
> mode.

Thanks, this should do it. Just some small comments below.

> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
> target/ppc/excp_helper.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index bb17b65dc0..396e25ed35 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -556,7 +556,7 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
>     CPUState *cs = CPU(cpu);
>     CPUPPCState *env = &cpu->env;
>     target_ulong msr, new_msr, vector;
> -    int srr0, srr1, lev = -1;
> +    int srr0, srr1;
>
>     if (excp <= POWERPC_EXCP_NONE || excp >= POWERPC_EXCP_NB) {
>         cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
> @@ -669,7 +669,8 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
>         }
>         break;
>     case POWERPC_EXCP_SYSCALL:   /* System call exception                    */
> -        lev = env->error_code;
> +    {
> +        int lev = env->error_code;
>
>         if ((lev == 1) && cpu->vhyp) {

In case you'll have another version, maybe drop the extra parenthesis 
around lev == here and below...

>             dump_hcall(env);
> @@ -683,17 +684,21 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
>          */
>         env->nip += 4;
>
> -        /* "PAPR mode" built-in hypercall emulation */
> +        /*
> +         * The Virtual Open Firmware (VOF) relies on the 'sc 1'
> +         * instruction to communicate with QEMU. The pegasos2 machine
> +         * uses VOF and the 74xx CPUs, so although the 74xx don't have
> +         * HV mode, we need to keep hypercall support.
> +         */
>         if ((lev == 1) && cpu->vhyp) {

...here. Or maybe even drop the lev local variable and just use 
env->error_code in the ifs directly? Only apprears twice now so could do 
without copying it to a local. But not a big deal so it's not worth a 
respin only for this.

Regards,
BALATON Zoltan

>             PPCVirtualHypervisorClass *vhc =
>                 PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
>             vhc->hypercall(cpu->vhyp, cpu);
>             return;
>         }
> -        if (lev == 1) {
> -            new_msr |= (target_ulong)MSR_HVB;
> -        }
> +
>         break;
> +    }
>     case POWERPC_EXCP_FPU:       /* Floating-point unavailable exception     */
>     case POWERPC_EXCP_DECR:      /* Decrementer exception                    */
>         break;