Message-ID: <457219eb8c83b35fc8705d80abbf75fc2fd8741c.camel@mediatek.com>
Date:   Fri, 12 May 2023 07:17:58 +0000
From:   Yi-De Wu (吳一德) <Yi-De.Wu@...iatek.com>
To:     "maz@...nel.org" <maz@...nel.org>
CC:     "corbet@....net" <corbet@....net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "angelogioacchino.delregno@...labora.com" 
        <angelogioacchino.delregno@...labora.com>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
        MY Chuang (莊明躍) <MY.Chuang@...iatek.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "quic_tsoni@...cinc.com" <quic_tsoni@...cinc.com>,
        Shawn Hsiao (蕭志祥) 
        <shawn.hsiao@...iatek.com>,
        Miles Chen (陳民樺) 
        <Miles.Chen@...iatek.com>,
        PeiLun Suei (隋培倫) 
        <PeiLun.Suei@...iatek.com>,
        Liju-clr Chen (陳麗如) 
        <Liju-clr.Chen@...iatek.com>,
        Jades Shih (施向玨) 
        <jades.shih@...iatek.com>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        "dbrazdil@...gle.com" <dbrazdil@...gle.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Yingshiuan Pan (潘穎軒) 
        <Yingshiuan.Pan@...iatek.com>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
        "arnd@...db.de" <arnd@...db.de>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        Ze-yu Wang (王澤宇) 
        <Ze-yu.Wang@...iatek.com>, "will@...nel.org" <will@...nel.org>,
        Ivan Tseng (曾志軒) 
        <ivan.tseng@...iatek.com>
Subject: Re: [PATCH v2 3/7] virt: geniezone: Introduce GenieZone hypervisor
 support

On Fri, 2023-04-28 at 23:12 +0100, Marc Zyngier wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 2023-04-28 11:36, Yi-De Wu wrote:
> > From: "Yingshiuan Pan" <yingshiuan.pan@...iatek.com>
> > 
> > GenieZone is MediaTek hypervisor solution, and it is running in EL2
> > stand alone as a type-I hypervisor. This patch exports a set of
> > ioctl
> > interfaces for userspace VMM (e.g., crosvm) to operate guest VMs
> > lifecycle (creation and destroy) on GenieZone.
> > 
> > Signed-off-by: Yingshiuan Pan <yingshiuan.pan@...iatek.com>
> > Signed-off-by: Yi-De Wu <yi-de.wu@...iatek.com>
> > ---
> >  MAINTAINERS                             |   6 +
> >  arch/arm64/Kbuild                       |   1 +
> >  arch/arm64/geniezone/Makefile           |   9 +
> >  arch/arm64/geniezone/gzvm_arch.c        | 189 +++++++++++++
> >  arch/arm64/geniezone/gzvm_arch.h        |  50 ++++
> >  arch/arm64/include/uapi/asm/gzvm_arch.h |  18 ++
> >  drivers/virt/Kconfig                    |   2 +
> >  drivers/virt/geniezone/Kconfig          |  17 ++
> >  drivers/virt/geniezone/Makefile         |  10 +
> >  drivers/virt/geniezone/gzvm_main.c      | 146 ++++++++++
> >  drivers/virt/geniezone/gzvm_vm.c        | 336
> > ++++++++++++++++++++++++
> >  include/linux/gzvm_drv.h                |  98 +++++++
> >  include/uapi/asm-generic/gzvm_arch.h    |  10 +
> >  include/uapi/linux/gzvm.h               |  99 +++++++
> >  14 files changed, 991 insertions(+)
> >  create mode 100644 arch/arm64/geniezone/Makefile
> >  create mode 100644 arch/arm64/geniezone/gzvm_arch.c
> >  create mode 100644 arch/arm64/geniezone/gzvm_arch.h
> >  create mode 100644 arch/arm64/include/uapi/asm/gzvm_arch.h
> >  create mode 100644 drivers/virt/geniezone/Kconfig
> >  create mode 100644 drivers/virt/geniezone/Makefile
> >  create mode 100644 drivers/virt/geniezone/gzvm_main.c
> >  create mode 100644 drivers/virt/geniezone/gzvm_vm.c
> >  create mode 100644 include/linux/gzvm_drv.h
> >  create mode 100644 include/uapi/asm-generic/gzvm_arch.h
> >  create mode 100644 include/uapi/linux/gzvm.h
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 1e911d1d9741..09a8ccf77b01 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -8700,6 +8700,12 @@ M:     Ze-Yu Wang <ze-yu.wang@...iatek.com>
> >  M:   Yi-De Wu <yi-de.wu@...iatek.com>
> > 
> > F:    Documentation/devicetree/bindings/hypervisor/mediatek,geniezo
> > ne-hyp.yaml
> >  F:   Documentation/virt/geniezone/
> > +F:   arch/arm64/geniezone/
> > +F:   arch/arm64/include/uapi/asm/gzvm_arch.h
> > +F:   drivers/virt/geniezone/
> > +F:   include/linux/gzvm_drv.h
> > +F    include/uapi/asm-generic/gzvm_arch.h
> > +F:   include/uapi/linux/gzvm.h
> > 
> >  GENWQE (IBM Generic Workqueue Card)
> >  M:   Frank Haverkamp <haver@...ux.ibm.com>
> > diff --git a/arch/arm64/Kbuild b/arch/arm64/Kbuild
> > index 5bfbf7d79c99..0c3cca572919 100644
> > --- a/arch/arm64/Kbuild
> > +++ b/arch/arm64/Kbuild
> > @@ -4,6 +4,7 @@ obj-$(CONFIG_KVM)     += kvm/
> >  obj-$(CONFIG_XEN)    += xen/
> >  obj-$(subst m,y,$(CONFIG_HYPERV))    += hyperv/
> >  obj-$(CONFIG_CRYPTO) += crypto/
> > +obj-$(CONFIG_MTK_GZVM)       += geniezone/
> > 
> >  # for cleaning
> >  subdir- += boot
> > diff --git a/arch/arm64/geniezone/Makefile
> > b/arch/arm64/geniezone/Makefile
> > new file mode 100644
> > index 000000000000..5720c076d73c
> > --- /dev/null
> > +++ b/arch/arm64/geniezone/Makefile
> > @@ -0,0 +1,9 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +#
> > +# Main Makefile for gzvm, this one includes
> > drivers/virt/geniezone/Makefile
> > +#
> > +include $(srctree)/drivers/virt/geniezone/Makefile
> > +
> > +gzvm-y += gzvm_arch.o
> > +
> > +obj-$(CONFIG_MTK_GZVM) += gzvm.o
> > diff --git a/arch/arm64/geniezone/gzvm_arch.c
> > b/arch/arm64/geniezone/gzvm_arch.c
> > new file mode 100644
> > index 000000000000..2fc76f7d440f
> > --- /dev/null
> > +++ b/arch/arm64/geniezone/gzvm_arch.c
> > @@ -0,0 +1,189 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2023 MediaTek Inc.
> > + */
> > +
> > +#include <linux/arm-smccc.h>
> > +#include <linux/err.h>
> > +#include <linux/uaccess.h>
> > +
> > +#include <linux/gzvm.h>
> > +#include <linux/gzvm_drv.h>
> > +#include "gzvm_arch.h"
> > +
> > +/**
> > + * geniezone_hypercall_wrapper()
> > + *
> > + * Return: The wrapper helps caller to convert geniezone errno to
> > Linux errno.
> > + */
> > +static int gzvm_hypcall_wrapper(unsigned long a0, unsigned long
> > a1,
> > +                             unsigned long a2, unsigned long a3,
> > +                             unsigned long a4, unsigned long a5,
> > +                             unsigned long a6, unsigned long a7,
> > +                             struct arm_smccc_res *res)
> > +{
> > +     arm_smccc_hvc(a0, a1, a2, a3, a4, a5, a6, a7, res);
> > +     return gz_err_to_errno(res->a0);
> > +}
> > +
> > +int gzvm_arch_probe(void)
> > +{
> > +     struct arm_smccc_res res;
> > +
> > +     arm_smccc_hvc(MT_HVC_GZVM_PROBE, 0, 0, 0, 0, 0, 0, 0, &res);
> > +     if (res.a0 == 0)
> > +             return 0;
> > +
> > +     return -ENXIO;
> > +}
> > +
> > +int gzvm_arch_set_memregion(gzvm_id_t vm_id, size_t buf_size,
> > +                         phys_addr_t region)
> > +{
> > +     struct arm_smccc_res res;
> > +
> > +     return gzvm_hypcall_wrapper(MT_HVC_GZVM_SET_MEMREGION, vm_id,
> > +                                 buf_size, region, 0, 0, 0, 0,
> > &res);
> > +}
> > +
> > +static int gzvm_cap_arm_vm_ipa_size(void __user *argp)
> > +{
> > +     __u64 value = CONFIG_ARM64_PA_BITS;
> > +
> > +     if (copy_to_user(argp, &value, sizeof(__u64)))
> > +             return -EFAULT;
> > +
> > +     return 0;
> > +}
> > +
> > +int gzvm_arch_check_extension(struct gzvm *gzvm, __u64 cap, void
> > __user *argp)
> > +{
> > +     int ret = -EOPNOTSUPP;
> > +
> > +     switch (cap) {
> > +     case GZVM_CAP_ARM_PROTECTED_VM: {
> > +             __u64 success = 1;
> > +
> > +             if (copy_to_user(argp, &success, sizeof(__u64)))
> > +                     return -EFAULT;
> > +             ret = 0;
> > +             break;
> > +     }
> > +     case GZVM_CAP_ARM_VM_IPA_SIZE: {
> > +             ret = gzvm_cap_arm_vm_ipa_size(argp);
> > +             break;
> > +     }
> > +     default:
> > +             ret = -EOPNOTSUPP;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +/**
> > + * gzvm_arch_create_vm()
> > + *
> > + * Return:
> > + * * positive value  - VM ID
> > + * * -ENOMEM         - Memory not enough for storing VM data
> > + */
> > +int gzvm_arch_create_vm(void)
> > +{
> > +     struct arm_smccc_res res;
> > +     int ret;
> > +
> > +     ret = gzvm_hypcall_wrapper(MT_HVC_GZVM_CREATE_VM, 0, 0, 0, 0,
> > 0, 0,
> > 0,
> > +                                &res);
> > +
> > +     if (ret == 0)
> > +             return res.a1;
> > +     else
> > +             return ret;
> > +}
> > +
> > +int gzvm_arch_destroy_vm(gzvm_id_t vm_id)
> > +{
> > +     struct arm_smccc_res res;
> > +
> > +     return gzvm_hypcall_wrapper(MT_HVC_GZVM_DESTROY_VM, vm_id, 0,
> > 0, 0,
> > 0,
> > +                                 0, 0, &res);
> > +}
> > +
> > +int gzvm_vm_arch_enable_cap(struct gzvm *gzvm, struct
> > gzvm_enable_cap
> > *cap,
> > +                         struct arm_smccc_res *res)
> > +{
> > +     return gzvm_hypcall_wrapper(MT_HVC_GZVM_ENABLE_CAP, gzvm-
> > >vm_id,
> > +                                cap->cap, cap->args[0], cap-
> > >args[1],
> > +                                cap->args[2], cap->args[3], cap-
> > >args[4],
> > +                                res);
> > +}
> > +
> > +/**
> > + * gzvm_vm_ioctl_get_pvmfw_size() - Get pvmfw size from
> > hypervisor,
> > return
> > + *                               in x1, and return to userspace in
> > args.
> > + *
> > + * Return:
> > + * * 0                       - Succeed
> > + * * -EINVAL         - Hypervisor return invalid results
> > + * * -EFAULT         - Fail to copy back to userspace buffer
> > + */
> > +static int gzvm_vm_ioctl_get_pvmfw_size(struct gzvm *gzvm,
> > +                                     struct gzvm_enable_cap *cap,
> > +                                     void __user *argp)
> > +{
> > +     struct arm_smccc_res res = {0};
> > +
> > +     if (gzvm_vm_arch_enable_cap(gzvm, cap, &res) != 0)
> > +             return -EINVAL;
> > +
> > +     cap->args[1] = res.a1;
> > +     if (copy_to_user(argp, cap, sizeof(*cap)))
> > +             return -EFAULT;
> > +
> > +     return 0;
> > +}
> > +
> > +/**
> > + * gzvm_vm_ioctl_cap_pvm() - Proceed GZVM_CAP_ARM_PROTECTED_VM's
> > subcommands
> > + *
> > + * Return:
> > + * * 0                       - Succeed
> > + * * -EINVAL         - Invalid subcommand or arguments
> > + */
> > +static int gzvm_vm_ioctl_cap_pvm(struct gzvm *gzvm, struct
> > gzvm_enable_cap *cap,
> > +                              void __user *argp)
> > +{
> > +     int ret = -EINVAL;
> > +     struct arm_smccc_res res = {0};
> > +
> > +     switch (cap->args[0]) {
> > +     case GZVM_CAP_ARM_PVM_SET_PVMFW_IPA:
> > +             ret = gzvm_vm_arch_enable_cap(gzvm, cap, &res);
> > +             break;
> > +     case GZVM_CAP_ARM_PVM_GET_PVMFW_SIZE:
> > +             ret = gzvm_vm_ioctl_get_pvmfw_size(gzvm, cap, argp);
> > +             break;
> > +     default:
> > +             ret = -EINVAL;
> > +             break;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +int gzvm_vm_ioctl_arch_enable_cap(struct gzvm *gzvm, struct
> > gzvm_enable_cap *cap,
> > +                               void __user *argp)
> > +{
> > +     int ret = -EINVAL;
> > +
> > +     switch (cap->cap) {
> > +     case GZVM_CAP_ARM_PROTECTED_VM:
> > +             ret = gzvm_vm_ioctl_cap_pvm(gzvm, cap, argp);
> > +             break;
> > +     default:
> > +             ret = -EINVAL;
> > +             break;
> > +     }
> > +
> > +     return ret;
> > +}
> > diff --git a/arch/arm64/geniezone/gzvm_arch.h
> > b/arch/arm64/geniezone/gzvm_arch.h
> > new file mode 100644
> > index 000000000000..dd0b7b5f7c65
> > --- /dev/null
> > +++ b/arch/arm64/geniezone/gzvm_arch.h
> > @@ -0,0 +1,50 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2023 MediaTek Inc.
> > + */
> > +
> > +#ifndef __GZ_ARCH_H__
> > +#define __GZ_ARCH_H__
> > +
> > +#include <linux/arm-smccc.h>
> > +
> > +enum {
> > +     GZVM_FUNC_CREATE_VM = 0,
> > +     GZVM_FUNC_DESTROY_VM,
> > +     GZVM_FUNC_CREATE_VCPU,
> > +     GZVM_FUNC_DESTROY_VCPU,
> > +     GZVM_FUNC_SET_MEMREGION,
> > +     GZVM_FUNC_RUN,
> > +     GZVM_FUNC_GET_REGS,
> > +     GZVM_FUNC_SET_REGS,
> > +     GZVM_FUNC_GET_ONE_REG,
> > +     GZVM_FUNC_SET_ONE_REG,
> > +     GZVM_FUNC_IRQ_LINE,
> > +     GZVM_FUNC_CREATE_DEVICE,
> > +     GZVM_FUNC_PROBE,
> > +     GZVM_FUNC_ENABLE_CAP,
> > +     NR_GZVM_FUNC
> > +};
> > +
> > +#define SMC_ENTITY_MTK                       59
> > +#define GZVM_FUNCID_START            (0x1000)
> > +#define
> > GZVM_HCALL_ID(func)                                          \
> > +     ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,
> > ARM_SMCCC_SMC_32,       \
> > +                        SMC_ENTITY_MTK, (GZVM_FUNCID_START +
> > (func)))
> > +
> > +#define
> > MT_HVC_GZVM_CREATE_VM                GZVM_HCALL_ID(GZVM_FUNC_CREATE
> > _VM)
> > +#define
> > MT_HVC_GZVM_DESTROY_VM               GZVM_HCALL_ID(GZVM_FUNC_DESTRO
> > Y_VM)
> > +#define
> > MT_HVC_GZVM_CREATE_VCPU              GZVM_HCALL_ID(GZVM_FUNC_CREATE
> > _VCPU)
> > +#define
> > MT_HVC_GZVM_DESTROY_VCPU     GZVM_HCALL_ID(GZVM_FUNC_DESTROY_VCPU)
> > +#define
> > MT_HVC_GZVM_SET_MEMREGION     GZVM_HCALL_ID(GZVM_FUNC_SET_MEMREGION
> > )
> > +#define
> > MT_HVC_GZVM_RUN                      GZVM_HCALL_ID(GZVM_FUNC_RUN)
> > +#define
> > MT_HVC_GZVM_GET_REGS         GZVM_HCALL_ID(GZVM_FUNC_GET_REGS)
> > +#define
> > MT_HVC_GZVM_SET_REGS         GZVM_HCALL_ID(GZVM_FUNC_SET_REGS)
> > +#define
> > MT_HVC_GZVM_GET_ONE_REG              GZVM_HCALL_ID(GZVM_FUNC_GET_ON
> > E_REG)
> > +#define
> > MT_HVC_GZVM_SET_ONE_REG              GZVM_HCALL_ID(GZVM_FUNC_SET_ON
> > E_REG)
> > +#define
> > MT_HVC_GZVM_IRQ_LINE         GZVM_HCALL_ID(GZVM_FUNC_IRQ_LINE)
> > +#define
> > MT_HVC_GZVM_CREATE_DEVICE     GZVM_HCALL_ID(GZVM_FUNC_CREATE_DEVICE
> > )
> > +#define
> > MT_HVC_GZVM_PROBE            GZVM_HCALL_ID(GZVM_FUNC_PROBE)
> > +#define
> > MT_HVC_GZVM_ENABLE_CAP               GZVM_HCALL_ID(GZVM_FUNC_ENABLE
> > _CAP)
> > +
> > +#endif /* __GZVM_ARCH_H__ */
> > diff --git a/arch/arm64/include/uapi/asm/gzvm_arch.h
> > b/arch/arm64/include/uapi/asm/gzvm_arch.h
> > new file mode 100644
> > index 000000000000..e7927f3dcb11
> > --- /dev/null
> > +++ b/arch/arm64/include/uapi/asm/gzvm_arch.h
> > @@ -0,0 +1,18 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/*
> > + * Copyright (c) 2023 MediaTek Inc.
> > + */
> > +
> > +#ifndef __GZVM_ARCH_H__
> > +#define __GZVM_ARCH_H__
> > +
> > +#include <linux/types.h>
> > +
> > +#define GZVM_CAP_ARM_VM_IPA_SIZE     165
> > +#define GZVM_CAP_ARM_PROTECTED_VM    0xffbadab1
> > +
> > +/* sub-commands put in args[0] for GZVM_CAP_ARM_PROTECTED_VM */
> > +#define GZVM_CAP_ARM_PVM_SET_PVMFW_IPA               0
> > +#define GZVM_CAP_ARM_PVM_GET_PVMFW_SIZE              1
> > +
> > +#endif /* __GZVM_ARCH_H__ */
> > diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
> > index f79ab13a5c28..9bbf0bdf672c 100644
> > --- a/drivers/virt/Kconfig
> > +++ b/drivers/virt/Kconfig
> > @@ -54,4 +54,6 @@ source "drivers/virt/coco/sev-guest/Kconfig"
> > 
> >  source "drivers/virt/coco/tdx-guest/Kconfig"
> > 
> > +source "drivers/virt/geniezone/Kconfig"
> > +
> >  endif
> > diff --git a/drivers/virt/geniezone/Kconfig
> > b/drivers/virt/geniezone/Kconfig
> > new file mode 100644
> > index 000000000000..6fad3c30f8d9
> > --- /dev/null
> > +++ b/drivers/virt/geniezone/Kconfig
> > @@ -0,0 +1,17 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +
> > +config MTK_GZVM
> > +     tristate "GenieZone Hypervisor driver for guest VM operation"
> > +     depends on ARM64
> > +     depends on KVM
> 
> NAK.
> 
> Either this is KVM, and this code serves no purpose, or it is a
> standalone
> hypervisor, and it *cannot* have a dependency on KVM.
> 
> [...]
> 

In order to be self-contained and avoid dependency like with KVM, may
we leverage KVM's symbol, macro e.g. VGIC_NR_SGIS,
VGIC_NR_PRIVATE_IRQS...etc, and copy or rename the related part to
*/geniezone/?

> > +/**
> > + * gzvm_gfn_to_pfn_memslot() - Translate gfn (guest ipa) to pfn
> > (host
> > pa),
> > + *                          result is in @pfn
> > + *
> > + * Leverage KVM's gfn_to_pfn_memslot(). Because
> > gfn_to_pfn_memslot()
> > needs
> > + * kvm_memory_slot as parameter, this function populates necessary
> > fileds
> > + * for calling gfn_to_pfn_memslot().
> > + *
> > + * Return:
> > + * * 0                       - Succeed
> > + * * -EFAULT         - Failed to convert
> > + */
> > +static int gzvm_gfn_to_pfn_memslot(struct gzvm_memslot *memslot,
> > u64
> > gfn, u64 *pfn)
> > +{
> > +     hfn_t __pfn;
> > +     struct kvm_memory_slot kvm_slot = {0};
> > +
> > +     kvm_slot.base_gfn = memslot->base_gfn;
> > +     kvm_slot.npages = memslot->npages;
> > +     kvm_slot.dirty_bitmap = NULL;
> > +     kvm_slot.userspace_addr = memslot->userspace_addr;
> > +     kvm_slot.flags = memslot->flags;
> > +     kvm_slot.id = memslot->slot_id;
> > +     kvm_slot.as_id = 0;
> > +
> > +     __pfn = gfn_to_pfn_memslot(&kvm_slot, gfn);
> 
> Again, I absolutely oppose this horror. This is internal to KVM,
> and we want to be able to change this without having to mess
> with your own code that we cannot test anyway.
> 
> What if we start using the extra fields that you don't populate
> as they mean nothing to you? Or add a backpointer to the kvm
> structure to do fancy accounting?
> 
> You have your own hypervisor, that's well and good. Since your
> main argument is that it is supposed to be standalone, make it
> *really* standalone and don't use KVM as a prop.
> 
> Thanks,
> 
>          M.
> --
> Jazz is not dead. It just smells funny...

Same with previous discussion, we'd like to copy or rename the related
part from KVM and keep the maintainance at our own if it's ok.

Powered by blists - more mailing lists