Re: [Qemu-devel] [PATCH qemu] spapr: Enable in-kernel H_SET_MODE handling

[Thomas Huth <thuth@redhat.com>]

On 02/09/15 11:29, Alexey Kardashevskiy wrote:
> For setting debug watchpoints, sPAPR guests use H_SET_MODE hypercall.
> The existing QEMU H_SET_MODE handler does not support this but
> the KVM handler in HV KVM does. However it is not enabled.
> 
> This enables the in-kernel H_SET_MODE handler which handles:
> - Completed Instruction Address Breakpoint Register
> - Watch point 0 registers.
> 
> The rest is still handled in QEMU.
> 
> Reported-by: Anton Blanchard <anton@samba.org>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  hw/ppc/spapr.c       | 1 +
>  target-ppc/kvm.c     | 5 +++++
>  target-ppc/kvm_ppc.h | 5 +++++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ae82565..7df0e15 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1728,6 +1728,7 @@ static void ppc_spapr_init(MachineState *machine)
>      if (kvm_enabled()) {
>          /* Enable H_LOGICAL_CI_* so SLOF can talk to in-kernel devices */
>          kvmppc_enable_logical_ci_hcalls();
> +        kvmppc_enable_set_mode_hcall();
>      }
>  
>      /* allocate RAM */
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 110436d..9cf5308 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1953,6 +1953,11 @@ void kvmppc_enable_logical_ci_hcalls(void)
>      kvmppc_enable_hcall(kvm_state, H_LOGICAL_CI_STORE);
>  }
>  
> +void kvmppc_enable_set_mode_hcall(void)
> +{
> +    kvmppc_enable_hcall(kvm_state, H_SET_MODE);
> +}
> +
>  void kvmppc_set_papr(PowerPCCPU *cpu)
>  {
>      CPUState *cs = CPU(cpu);
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index 4d30e27..0714ba0 100644
> --- a/target-ppc/kvm_ppc.h
> +++ b/target-ppc/kvm_ppc.h
> @@ -25,6 +25,7 @@ int kvmppc_get_hasidle(CPUPPCState *env);
>  int kvmppc_get_hypercall(CPUPPCState *env, uint8_t *buf, int buf_len);
>  int kvmppc_set_interrupt(PowerPCCPU *cpu, int irq, int level);
>  void kvmppc_enable_logical_ci_hcalls(void);
> +void kvmppc_enable_set_mode_hcall(void);
>  void kvmppc_set_papr(PowerPCCPU *cpu);
>  int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version);
>  void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy);
> @@ -112,6 +113,10 @@ static inline void kvmppc_enable_logical_ci_hcalls(void)
>  {
>  }
>  
> +static inline void kvmppc_enable_set_mode_hcall(void)
> +{
> +}

This patch looks basically fine for me ... but I just started wondering
whether we want to add such kvmppc_enable_* wrappers for all h-calls
that we're going to enable in the future?

IMHO it would be more elegant to add a function called
kvmppc_enable_supported_hcalls() to target-ppc/kvm.c, which then turns
on all wanted h-calls. hw/ppc/spapr.c would then be agnostic of the
h-calls which are enabled by that function. We then also only need one
wrapper in kvm_ppc.h - the one for kvmppc_enable_supported_hcalls().
What do you think?

 Thomas