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

[Thomas Huth]

On 02/09/15 19:34, Alexander Graf wrote:
> 
> 
>> Am 02.09.2015 um 15:35 schrieb Thomas Huth <address@hidden>:
>>
>>> 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 <address@hidden>
>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>> ---
>>> 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