Re: [RFC 21/55] KVM: arm64: Forward HVC instruction to the guest hypervisor

[Jintack Lim]

On Mon, Jul 3, 2017 at 5:08 AM, Christoffer Dall <cdall@xxxxxxxxxx> wrote:
> On Mon, Jun 26, 2017 at 11:21:25AM -0400, Jintack Lim wrote:
>> On Wed, Feb 22, 2017 at 6:47 AM, Christoffer Dall <cdall@xxxxxxxxxx> wrote:
>> > On Mon, Jan 09, 2017 at 01:24:17AM -0500, Jintack Lim wrote:
>> >> Forward exceptions due to hvc instruction to the guest hypervisor.
>> >>
>> >> Signed-off-by: Jintack Lim <jintack@xxxxxxxxxxxxxxx>
>> >> ---
>> >>  arch/arm64/include/asm/kvm_nested.h |  5 +++++
>> >>  arch/arm64/kvm/Makefile             |  1 +
>> >>  arch/arm64/kvm/handle_exit.c        | 11 +++++++++++
>> >>  arch/arm64/kvm/handle_exit_nested.c | 27 +++++++++++++++++++++++++++
>> >>  4 files changed, 44 insertions(+)
>> >>  create mode 100644 arch/arm64/include/asm/kvm_nested.h
>> >>  create mode 100644 arch/arm64/kvm/handle_exit_nested.c
>> >>
>> >> diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h
>> >> new file mode 100644
>> >> index 0000000..620b4d3
>> >> --- /dev/null
>> >> +++ b/arch/arm64/include/asm/kvm_nested.h
>> >> @@ -0,0 +1,5 @@
>> >> +#ifndef __ARM64_KVM_NESTED_H__
>> >> +#define __ARM64_KVM_NESTED_H__
>> >> +
>> >> +int handle_hvc_nested(struct kvm_vcpu *vcpu);
>> >> +#endif
>> >> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>> >> index b342bdd..9c35e9a 100644
>> >> --- a/arch/arm64/kvm/Makefile
>> >> +++ b/arch/arm64/kvm/Makefile
>> >> @@ -35,4 +35,5 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/irqchip.o
>> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
>> >>  kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
>> >>
>> >> +kvm-$(CONFIG_KVM_ARM_NESTED_HYP) += handle_exit_nested.o
>> >>  kvm-$(CONFIG_KVM_ARM_NESTED_HYP) += emulate-nested.o
>> >> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>> >> index a891684..208be16 100644
>> >> --- a/arch/arm64/kvm/handle_exit.c
>> >> +++ b/arch/arm64/kvm/handle_exit.c
>> >> @@ -29,6 +29,10 @@
>> >>  #include <asm/kvm_mmu.h>
>> >>  #include <asm/kvm_psci.h>
>> >>
>> >> +#ifdef CONFIG_KVM_ARM_NESTED_HYP
>> >> +#include <asm/kvm_nested.h>
>> >> +#endif
>> >> +
>> >>  #define CREATE_TRACE_POINTS
>> >>  #include "trace.h"
>> >>
>> >> @@ -42,6 +46,13 @@ static int handle_hvc(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> >>                           kvm_vcpu_hvc_get_imm(vcpu));
>> >>       vcpu->stat.hvc_exit_stat++;
>> >>
>> >> +#ifdef CONFIG_KVM_ARM_NESTED_HYP
>> >> +     ret = handle_hvc_nested(vcpu);
>> >> +     if (ret < 0 && ret != -EINVAL)
>> >> +             return ret;
>> >> +     else if (ret >= 0)
>> >> +             return ret;
>> >> +#endif
>> >>       ret = kvm_psci_call(vcpu);
>> >>       if (ret < 0) {
>> >>               kvm_inject_undefined(vcpu);
>> >> diff --git a/arch/arm64/kvm/handle_exit_nested.c b/arch/arm64/kvm/handle_exit_nested.c
>> >> new file mode 100644
>> >> index 0000000..a6ce23b
>> >> --- /dev/null
>> >> +++ b/arch/arm64/kvm/handle_exit_nested.c
>> >> @@ -0,0 +1,27 @@
>> >> +/*
>> >> + * Copyright (C) 2016 - Columbia University
>> >> + * Author: Jintack Lim <jintack@xxxxxxxxxxxxxxx>
>> >> + *
>> >> + * This program is free software; you can redistribute it and/or modify
>> >> + * it under the terms of the GNU General Public License version 2 as
>> >> + * published by the Free Software Foundation.
>> >> + *
>> >> + * This program is distributed in the hope that it will be useful,
>> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> >> + * GNU General Public License for more details.
>> >> + *
>> >> + * You should have received a copy of the GNU General Public License
>> >> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> >> + */
>> >> +
>> >> +#include <linux/kvm.h>
>> >> +#include <linux/kvm_host.h>
>> >> +
>> >> +#include <asm/kvm_emulate.h>
>> >> +
>> >> +/* We forward all hvc instruction to the guest hypervisor. */
>> >> +int handle_hvc_nested(struct kvm_vcpu *vcpu)
>> >> +{
>> >> +     return kvm_inject_nested_sync(vcpu, kvm_vcpu_get_hsr(vcpu));
>> >> +}
>> >
>> > I don't understand the logic here or in the caller above.  Do we really
>> > forward *all" hvc calls to the guest hypervisor now, so that we no
>> > longer support any hypercalls from the VM?  That seems a little rough
>> > and probably requires some more discussions.
>>
>> So I think if we run a VM with the EL2 support, then all hvc calls
>> from the VM should be forwarded to the virtual EL2.
>
> But do we actually check if the guest has EL2 here?  It seems you cann
> handle_hvc_nested unconditionally when you have
> OCNFIG_KVM_ARM_NESTED_HYP.  I think that's what threw me off when first
> reading your patch.

You're right. We should check it first.

>
>>
>> I may miss something obvious, so can you (or anyone) come up with some
>> cases that the host hypervisor needs to directly handle hvc from the
>> VM with the EL2 support?
>>
>
> So I'm a little unsure what to say here.  On one hand you are absolutely
> correct, that architecturally if we emulated virtual EL2, then all
> hypercalls are handled by the virtual EL2 (even hypercalls from virtual
> EL2 which should become self-hypercalls).
>
> On the other hand, an enlightened guest may want to use hypercalls to
> the hypervisor for some reason, but that would require some numbering
> scheme to separate the two concepts.
>
> Do we currently have support for the guest to use SMC calls for PSCI
> when it has virtual EL2?

Yes, we do in "[RFC,22/55] KVM: arm64: Handle PSCI call from the
guest" as you figured out.

>
> Thanks,
> -Christoffer