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

Alexander Graf
2013-07-15T11:46:02Z

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.

Alex