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

[Alexander Graf]

> 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/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?

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.


Alex