Re: [Qemu-devel] [RFC PATCH v4 4/5] target/ppc: Refactor kvm_handle_debug

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5395 bytes --]

On Thu, Feb 28, 2019 at 07:57:58PM -0300, Fabiano Rosas wrote:
> There are four scenarios being handled in this function:
> 
> - single stepping
> - hardware breakpoints
> - software breakpoints
> - fallback (no debug supported)
> 
> A future patch will add code to handle specific single step and
> software breakpoints cases so let's split each scenario into its own
> function now to avoid hurting readability.
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Again, a nice cleanup regardless of anything else.  Applied.

> ---
>  target/ppc/kvm.c | 86 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 50 insertions(+), 36 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 941c4e7523..9392fba192 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1620,52 +1620,66 @@ static int kvm_handle_hw_breakpoint(CPUState *cs,
>      return handle;
>  }
>  
> +static int kvm_handle_singlestep(void)
> +{
> +    return 1;
> +}
> +
> +static int kvm_handle_sw_breakpoint(void)
> +{
> +    return 1;
> +}
> +
>  static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run)
>  {
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
>      struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
> -    int handle = 0;
>  
>      if (cs->singlestep_enabled) {
> -        handle = 1;
> -    } else if (arch_info->status) {
> -        handle = kvm_handle_hw_breakpoint(cs, arch_info);
> -    } else if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
> -        handle = 1;
> -    } else {
> -        /* QEMU is not able to handle debug exception, so inject
> -         * program exception to guest;
> -         * Yes program exception NOT debug exception !!
> -         * When QEMU is using debug resources then debug exception must
> -         * be always set. To achieve this we set MSR_DE and also set
> -         * MSRP_DEP so guest cannot change MSR_DE.
> -         * When emulating debug resource for guest we want guest
> -         * to control MSR_DE (enable/disable debug interrupt on need).
> -         * Supporting both configurations are NOT possible.
> -         * So the result is that we cannot share debug resources
> -         * between QEMU and Guest on BOOKE architecture.
> -         * In the current design QEMU gets the priority over guest,
> -         * this means that if QEMU is using debug resources then guest
> -         * cannot use them;
> -         * For software breakpoint QEMU uses a privileged instruction;
> -         * So there cannot be any reason that we are here for guest
> -         * set debug exception, only possibility is guest executed a
> -         * privileged / illegal instruction and that's why we are
> -         * injecting a program interrupt.
> -         */
> +        return kvm_handle_singlestep();
> +    }
> +
> +    if (arch_info->status) {
> +        return kvm_handle_hw_breakpoint(cs, arch_info);
> +    }
>  
> -        cpu_synchronize_state(cs);
> -        /* env->nip is PC, so increment this by 4 to use
> -         * ppc_cpu_do_interrupt(), which set srr0 = env->nip - 4.
> -         */
> -        env->nip += 4;
> -        cs->exception_index = POWERPC_EXCP_PROGRAM;
> -        env->error_code = POWERPC_EXCP_INVAL;
> -        ppc_cpu_do_interrupt(cs);
> +    if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
> +        return kvm_handle_sw_breakpoint();
>      }
>  
> -    return handle;
> +    /*
> +     * QEMU is not able to handle debug exception, so inject
> +     * program exception to guest;
> +     * Yes program exception NOT debug exception !!
> +     * When QEMU is using debug resources then debug exception must
> +     * be always set. To achieve this we set MSR_DE and also set
> +     * MSRP_DEP so guest cannot change MSR_DE.
> +     * When emulating debug resource for guest we want guest
> +     * to control MSR_DE (enable/disable debug interrupt on need).
> +     * Supporting both configurations are NOT possible.
> +     * So the result is that we cannot share debug resources
> +     * between QEMU and Guest on BOOKE architecture.
> +     * In the current design QEMU gets the priority over guest,
> +     * this means that if QEMU is using debug resources then guest
> +     * cannot use them;
> +     * For software breakpoint QEMU uses a privileged instruction;
> +     * So there cannot be any reason that we are here for guest
> +     * set debug exception, only possibility is guest executed a
> +     * privileged / illegal instruction and that's why we are
> +     * injecting a program interrupt.
> +     */
> +    cpu_synchronize_state(cs);
> +    /*
> +     * env->nip is PC, so increment this by 4 to use
> +     * ppc_cpu_do_interrupt(), which set srr0 = env->nip - 4.
> +     */
> +    env->nip += 4;
> +    cs->exception_index = POWERPC_EXCP_PROGRAM;
> +    env->error_code = POWERPC_EXCP_INVAL;
> +    ppc_cpu_do_interrupt(cs);
> +
> +    return 0;
>  }
>  
>  int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]