From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43434) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZXOW2-00059q-Gc for qemu-devel@nongnu.org; Thu, 03 Sep 2015 03:00:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZXOVz-0002J9-AU for qemu-devel@nongnu.org; Thu, 03 Sep 2015 03:00:58 -0400 References: <1441186198-991-1-git-send-email-aik@ozlabs.ru> <55E6FB18.7070101@redhat.com> From: Thomas Huth Message-ID: <55E7F020.8050308@redhat.com> Date: Thu, 3 Sep 2015 09:00:48 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu] spapr: Enable in-kernel H_SET_MODE handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: Alexey Kardashevskiy , "qemu-ppc@nongnu.org" , "qemu-devel@nongnu.org" , David Gibson On 02/09/15 19:34, Alexander Graf wrote: > > >> Am 02.09.2015 um 15:35 schrieb Thomas Huth : >> >>> 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 >>> Signed-off-by: Alexey Kardashevskiy >>> --- >>> hw/ppc/spapr.c | 1 + >>> target-ppc/kvm.c | 5 +++++ >>> target-ppc/kvm_ppc.h | 5 +++++ >>> 3 files changed, 11 insertions(+) ... >>> 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? > > You may want to conditionalize different in-kernel hcall enablement (machine version for example). You also may want to do the enable from a device rather than machine code, like woth the h_random hypercall maybe. > > So for now I think we're better off with individual calls. We could still handle such situations with additional wrappers. Simply call my suggested function kvmppc_enable_default_hcalls() instead, then it is clear that it only enables the h-calls that are always enabled by default. Thomas