Re: [PATCH 2/5] booke: exit to guest userspace for unimplemented hcalls in kvm

Alexander Graf
2013-07-15T15:29:00Z

On 15.07.2013, at 17:13, Bhushan Bharat-R65777 wrote:

>
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Monday, July 15, 2013 8:27 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm@vger.kernel.org; kvm-ppc@vger.kernel.org; Wood Scott-B07421; Yoder
>> Stuart-B08248
>> Subject: Re: [PATCH 2/5] booke: exit to guest userspace for unimplemented hcalls
>> in kvm
>>
>>
>> On 15.07.2013, at 16:50, Bhushan Bharat-R65777 wrote:
>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>> Sent: Monday, July 15, 2013 5:16 PM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: kvm@vger.kernel.org; kvm-ppc@vger.kernel.org; Wood Scott-B07421;
>>>> Yoder
>>>> Stuart-B08248
>>>> Subject: Re: [PATCH 2/5] booke: exit to guest userspace for
>>>> unimplemented hcalls in kvm
>>>>
>>>>
>>>> On 15.07.2013, at 13:38, Bhushan Bharat-R65777 wrote:
>>>>
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>>> Sent: Monday, July 15, 2013 5:02 PM
>>>>>> To: Bhushan Bharat-R65777
>>>>>> Cc: kvm@vger.kernel.org; kvm-ppc@vger.kernel.org; Wood
>>>>>> Scott-B07421; Yoder Stuart-B08248; Bhushan Bharat-R65777
>>>>>> Subject: Re: [PATCH 2/5] booke: exit to guest userspace for
>>>>>> unimplemented hcalls in kvm
>>>>>>
>>>>>>
>>>>>> On 15.07.2013, at 13:11, Bharat Bhushan wrote:
>>>>>>
>>>>>>> Exit to guest user space if kvm does not implement the hcall.
>>>>>>>
>>>>>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
>>>>>>> ---
>>>>>>> arch/powerpc/kvm/booke.c | 47 +++++++++++++++++++++++++++++++++++++---
>> --
>>>> -
>>>>>>> arch/powerpc/kvm/powerpc.c | 1 +
>>>>>>> include/uapi/linux/kvm.h | 1 +
>>>>>>> 3 files changed, 42 insertions(+), 7 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>>>>>>> index
>>>>>>> 17722d8..c8b41b4 100644
>>>>>>> --- a/arch/powerpc/kvm/booke.c
>>>>>>> +++ b/arch/powerpc/kvm/booke.c
>>>>>>> @@ -1005,9 +1005,25 @@ int kvmppc_handle_exit(struct kvm_run *run,
>>>>>>> struct
>>>>>> kvm_vcpu *vcpu,
>>>>>>> break;
>>>>>>>
>>>>>>> #ifdef CONFIG_KVM_BOOKE_HV
>>>>>>> - case BOOKE_INTERRUPT_HV_SYSCALL:
>>>>>>> + case BOOKE_INTERRUPT_HV_SYSCALL: {
>>>>>>
>>>>>> This is getting large. Please extract hcall handling into its own function.
>>>>>> Maybe you can merge the HV and non-HV case then too.
>>>>>>
>>>>>>> + int i;
>>>>>>> if (!(vcpu->arch.shared->msr & MSR_PR)) {
>>>>>>> - kvmppc_set_gpr(vcpu, 3, kvmppc_kvm_pv(vcpu));
>>>>>>> + r = kvmppc_kvm_pv(vcpu);
>>>>>>> + if (r != EV_UNIMPLEMENTED) {
>>>>>>> + /* except unimplemented return to guest */
>>>>>>> + kvmppc_set_gpr(vcpu, 3, r);
>>>>>>> + kvmppc_account_exit(vcpu, SYSCALL_EXITS);
>>>>>>> + r = RESUME_GUEST;
>>>>>>> + break;
>>>>>>> + }
>>>>>>> + /* Exit to userspace for unimplemented hcalls in kvm
>> */
>>>>>>> + run->epapr_hcall.nr = kvmppc_get_gpr(vcpu, 11);
>>>>>>> + run->epapr_hcall.ret = 0;
>>>>>>> + for (i = 0; i < 8; i++)
>>>>>>> + run->epapr_hcall.args[i] = kvmppc_get_gpr(vcpu,
>> 3 +
>>>>>> i);
>>>>>>> + vcpu->arch.hcall_needed = 1;
>>>>>>> + kvmppc_account_exit(vcpu, SYSCALL_EXITS);
>>>>>>> + r = RESUME_HOST;
>>>>>>> } else {
>>>>>>> /*
>>>>>>> * hcall from guest userspace -- send privileged @@ -1016,22
>>>>>>> +1032,39 @@ int kvmppc_handle_exit(struct kvm_run *run, struct
>>>>>>> +kvm_vcpu *vcpu,
>>>>>>> kvmppc_core_queue_program(vcpu, ESR_PPR);
>>>>>>> }
>>>>>>>
>>>>>>> - r = RESUME_GUEST;
>>>>>>> + run->exit_reason = KVM_EXIT_EPAPR_HCALL;
>>>>>
>>>>>
>>>>> Oops, what I have done, I wanted this to be
>>>>> kvmppc_account_exit(vcpu, SYSCALL_EXITS);
>>>>>
>>>>> s/ run->exit_reason = KVM_EXIT_EPAPR_HCALL;/
>>>>> kvmppc_account_exit(vcpu, SYSCALL_EXITS);
>>>>>
>>>>> -Bharat
>>>>>
>>>>>>
>>>>>> This looks odd. Your exit reason only changes when you do the hcall
>>>>>> exiting, right?
>>>>>>
>>>>>> You also need to guard user space hcall exits with an ENABLE_CAP.
>>>>>> Otherwise older user space will break, as it doesn't know about the
>>>>>> exit type
>>>> yet.
>>>>>
>>>>> So the user space so make enable_cap also?
>>>>
>>>> User space needs to call enable_cap on this cap, yes. Otherwise a
>>>> guest can confuse user space with an hcall exit it can't handle.
>>>
>>> We do not have enable_cap for book3s, any specific reason why ?
>>
>> We do. If you enable PAPR, you get PAPR hcalls. If you enable OSI, you get OSI
>> hcalls.
>
> Oh, We check this on book3s_PR and book3s_HV.
>
>> KVM hcalls on book3s don't return to user space.
>
> It exits, is not it? "arch/powerpc/kvm/book3s_pr.c" exits with KVM_EXIT_PAPR_HCALL. And same in book3s_pv.

It doesn't even start handling the hcall if papr_enabled isn't set ;).

Alex