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

Bhushan Bharat-R65777
2013-07-15T14:48:34Z

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

-Bharat

>
>
> Alex
>