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

Bhushan Bharat-R65777
2013-07-15T11:38:40Z

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

-Bharat

>
>
> Alex
>
> > break;
> > + }
> > #else
> > - case BOOKE_INTERRUPT_SYSCALL:
> > + case BOOKE_INTERRUPT_SYSCALL: {
> > + int i;
> > + r = RESUME_GUEST;
> > if (!(vcpu->arch.shared->msr & MSR_PR) &&
> > (((u32)kvmppc_get_gpr(vcpu, 0)) == KVM_SC_MAGIC_R0)) {
> > /* KVM PV hypercalls */
> > - kvmppc_set_gpr(vcpu, 3, kvmppc_kvm_pv(vcpu));
> > - r = RESUME_GUEST;
> > + 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;
> > + run->exit_reason = KVM_EXIT_EPAPR_HCALL;
> > + r = RESUME_HOST;
> > } else {
> > /* Guest syscalls */
> > kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_SYSCALL);
> > }
> > kvmppc_account_exit(vcpu, SYSCALL_EXITS);
> > - r = RESUME_GUEST;
> > break;
> > + }
> > #endif
> >
> > case BOOKE_INTERRUPT_DTLB_MISS: {
> > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> > index 4e05f8c..6c6199d 100644
> > --- a/arch/powerpc/kvm/powerpc.c
> > +++ b/arch/powerpc/kvm/powerpc.c
> > @@ -307,6 +307,7 @@ int kvm_dev_ioctl_check_extension(long ext)
> > case KVM_CAP_PPC_BOOKE_SREGS:
> > case KVM_CAP_PPC_BOOKE_WATCHDOG:
> > case KVM_CAP_PPC_EPR:
> > + case KVM_CAP_EXIT_EPAPR_HCALL:
> > #else
> > case KVM_CAP_PPC_SEGSTATE:
> > case KVM_CAP_PPC_HIOR:
> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index
> > 01ee50e..b5266e5 100644
> > --- a/include/uapi/linux/kvm.h
> > +++ b/include/uapi/linux/kvm.h
> > @@ -674,6 +674,7 @@ struct kvm_ppc_smmu_info { #define
> > KVM_CAP_PPC_RTAS 91 #define KVM_CAP_IRQ_XICS 92 #define
> > KVM_CAP_ARM_EL1_32BIT 93
> > +#define KVM_CAP_EXIT_EPAPR_HCALL 94
> >
> > #ifdef KVM_CAP_IRQ_ROUTING
> >
> > --
> > 1.7.0.4
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> > the body of a message to majordomo@vger.kernel.org More majordomo info
> > at http://vger.kernel.org/majordomo-info.html
>