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

Alexander Graf
2013-07-15T15:38:48Z

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

>
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Monday, July 15, 2013 8:59 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 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 ;).
>
> On PR, not HV :-)

Book3S HV doesn't support non-PAPR ;).

Alex