[PATCH v2 5/7] virt: geniezone: Add irqchip support for virtual interrupt injection

Yi-De Wu posted 7 patches 2 years, 9 months ago
There is a newer version of this series
[PATCH v2 5/7] virt: geniezone: Add irqchip support for virtual interrupt injection
Posted by Yi-De Wu 2 years, 9 months ago
From: "Yingshiuan Pan" <yingshiuan.pan@mediatek.com>

Enable GenieZone to handle virtual interrupt injection request.

Signed-off-by: Yingshiuan Pan <yingshiuan.pan@mediatek.com>
Signed-off-by: Yi-De Wu <yi-de.wu@mediatek.com>
---
 arch/arm64/geniezone/Makefile       |  2 +-
 arch/arm64/geniezone/gzvm_arch.c    | 24 ++++++--
 arch/arm64/geniezone/gzvm_arch.h    | 11 ++++
 arch/arm64/geniezone/gzvm_irqchip.c | 88 +++++++++++++++++++++++++++++
 drivers/virt/geniezone/gzvm_vm.c    | 75 ++++++++++++++++++++++++
 include/linux/gzvm_drv.h            |  4 ++
 include/uapi/linux/gzvm.h           | 38 ++++++++++++-
 7 files changed, 235 insertions(+), 7 deletions(-)
 create mode 100644 arch/arm64/geniezone/gzvm_irqchip.c

diff --git a/arch/arm64/geniezone/Makefile b/arch/arm64/geniezone/Makefile
index 5720c076d73c..82af1ed870bc 100644
--- a/arch/arm64/geniezone/Makefile
+++ b/arch/arm64/geniezone/Makefile
@@ -4,6 +4,6 @@
 #
 include $(srctree)/drivers/virt/geniezone/Makefile
 
-gzvm-y += gzvm_arch.o
+gzvm-y += gzvm_arch.o gzvm_irqchip.o
 
 obj-$(CONFIG_MTK_GZVM) += gzvm.o
diff --git a/arch/arm64/geniezone/gzvm_arch.c b/arch/arm64/geniezone/gzvm_arch.c
index 3c91f3f1ae50..16ab5d4fd5b8 100644
--- a/arch/arm64/geniezone/gzvm_arch.c
+++ b/arch/arm64/geniezone/gzvm_arch.c
@@ -16,11 +16,10 @@
  *
  * 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)
+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);
@@ -259,3 +258,18 @@ int gzvm_arch_create_vcpu(gzvm_id_t vm_id, int vcpuid, void *run)
 
 	return ret;
 }
+
+int gzvm_arch_create_device(gzvm_id_t vm_id, struct gzvm_create_device *gzvm_dev)
+{
+	struct arm_smccc_res res;
+
+	return gzvm_hypcall_wrapper(MT_HVC_GZVM_CREATE_DEVICE, vm_id,
+				    virt_to_phys(gzvm_dev), 0, 0, 0, 0, 0, &res);
+}
+
+int gzvm_arch_inject_irq(struct gzvm *gzvm, unsigned int vcpu_idx, u32 irq_type,
+			 u32 irq, bool level)
+{
+	/* default use spi */
+	return gzvm_vgic_inject_spi(gzvm, vcpu_idx, irq, level);
+}
diff --git a/arch/arm64/geniezone/gzvm_arch.h b/arch/arm64/geniezone/gzvm_arch.h
index ecc24ff4e244..205bd0901333 100644
--- a/arch/arm64/geniezone/gzvm_arch.h
+++ b/arch/arm64/geniezone/gzvm_arch.h
@@ -71,4 +71,15 @@ disassemble_vm_vcpu_tuple(unsigned int tuple, gzvm_id_t *vmid,
 	*vcpuid = get_vcpuid_from_tuple(tuple);
 }
 
+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);
+
+void gzvm_sync_vgic_state(struct gzvm_vcpu *vcpu);
+int gzvm_vgic_inject_irq(struct gzvm *gzvm, unsigned int vcpu_idx, u32 irq_type,
+			 u32 irq, bool level);
+int gzvm_vgic_inject_spi(struct gzvm *gzvm, unsigned int vcpu_idx,
+			 u32 spi_irq, bool level);
+
 #endif /* __GZVM_ARCH_H__ */
diff --git a/arch/arm64/geniezone/gzvm_irqchip.c b/arch/arm64/geniezone/gzvm_irqchip.c
new file mode 100644
index 000000000000..c46bd34fee1b
--- /dev/null
+++ b/arch/arm64/geniezone/gzvm_irqchip.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2023 MediaTek Inc.
+ */
+
+#include <linux/irqchip/arm-gic-v3.h>
+#include <kvm/arm_vgic.h>
+
+#include <linux/gzvm.h>
+#include <linux/gzvm_drv.h>
+#include "gzvm_arch.h"
+
+/**
+ * gzvm_sync_vgic_state() - Check all LRs synced from gz hypervisor
+ *
+ * Traverse all LRs, see if any EOIed vint, notify_acked_irq if any.
+ * GZ does not fold/unfold everytime KVM_RUN, so we have to traverse all saved
+ * LRs. It will not takes much more time comparing to fold/unfold everytime
+ * GZVM_RUN, because there are only few LRs.
+ */
+void gzvm_sync_vgic_state(struct gzvm_vcpu *vcpu)
+{
+}
+
+/* is_irq_valid() - Check the irq number and irq_type are matched */
+static bool is_irq_valid(u32 irq, u32 irq_type)
+{
+	switch (irq_type) {
+	case GZVM_IRQ_TYPE_CPU:
+		/*  0 ~ 15: SGI */
+		if (likely(irq <= GZVM_IRQ_CPU_FIQ))
+			return true;
+		break;
+	case GZVM_IRQ_TYPE_PPI:
+		/* 16 ~ 31: PPI */
+		if (likely(irq >= VGIC_NR_SGIS && irq < VGIC_NR_PRIVATE_IRQS))
+			return true;
+		break;
+	case GZVM_IRQ_TYPE_SPI:
+		/* 32 ~ : SPT */
+		if (likely(irq >= VGIC_NR_PRIVATE_IRQS))
+			return true;
+		break;
+	default:
+		return false;
+	}
+	return false;
+}
+
+/**
+ * gzvm_vgic_inject_irq() - Inject virtual interrupt to a VM
+ * @vcpu_idx: vcpu index, only valid if PPI
+ * @irq: irq number
+ * @level: 1 if true else 0
+ */
+int gzvm_vgic_inject_irq(struct gzvm *gzvm, unsigned int vcpu_idx, u32 irq_type,
+			 u32 irq, bool level)
+{
+	unsigned long a1 = assemble_vm_vcpu_tuple(gzvm->vm_id, vcpu_idx);
+	struct arm_smccc_res res;
+
+	if (!unlikely(is_irq_valid(irq, irq_type)))
+		return -EINVAL;
+
+	gzvm_hypcall_wrapper(MT_HVC_GZVM_IRQ_LINE, a1, irq, level,
+			     0, 0, 0, 0, &res);
+	if (res.a0) {
+		pr_err("Failed to set IRQ level (%d) to irq#%u on vcpu %d with ret=%d\n",
+		       level, irq, vcpu_idx, (int)res.a0);
+		return -EFAULT;
+	}
+
+	return 0;
+}
+
+/**
+ * gzvm_vgic_inject_spi() - Inject virtual spi interrupt
+ *
+ * @spi_irq: This is spi interrupt number (starts from 0 instead of 32)
+ *
+ * Return 0 if succeed else other negative values indicating each errors
+ */
+int gzvm_vgic_inject_spi(struct gzvm *gzvm, unsigned int vcpu_idx,
+			 u32 spi_irq, bool level)
+{
+	return gzvm_vgic_inject_irq(gzvm, 0, GZVM_IRQ_TYPE_SPI,
+				    spi_irq + VGIC_NR_PRIVATE_IRQS, level);
+}
diff --git a/drivers/virt/geniezone/gzvm_vm.c b/drivers/virt/geniezone/gzvm_vm.c
index 5542065e82c6..a5444541b745 100644
--- a/drivers/virt/geniezone/gzvm_vm.c
+++ b/drivers/virt/geniezone/gzvm_vm.c
@@ -186,6 +186,67 @@ gzvm_vm_ioctl_set_memory_region(struct gzvm *gzvm,
 	return register_memslot_addr_range(gzvm, memslot);
 }
 
+static int gzvm_vm_ioctl_irq_line(struct gzvm *gzvm,
+				  struct gzvm_irq_level *irq_level)
+{
+	u32 irq = irq_level->irq;
+	unsigned int irq_type, vcpu_idx, irq_num;
+	bool level = irq_level->level;
+
+	irq_type = (irq >> GZVM_IRQ_TYPE_SHIFT) & GZVM_IRQ_TYPE_MASK;
+	vcpu_idx = (irq >> GZVM_IRQ_VCPU_SHIFT) & GZVM_IRQ_VCPU_MASK;
+	vcpu_idx += ((irq >> GZVM_IRQ_VCPU2_SHIFT) & GZVM_IRQ_VCPU2_MASK) *
+		(GZVM_IRQ_VCPU_MASK + 1);
+	irq_num = (irq >> GZVM_IRQ_NUM_SHIFT) & GZVM_IRQ_NUM_MASK;
+
+	return gzvm_arch_inject_irq(gzvm, vcpu_idx, irq_num, irq_type, level);
+}
+
+static int gzvm_vm_ioctl_create_device(struct gzvm *gzvm, void __user *argp)
+{
+	struct gzvm_create_device *gzvm_dev;
+	void *dev_data = NULL;
+	int ret;
+
+	gzvm_dev = (struct gzvm_create_device *)alloc_pages_exact(PAGE_SIZE,
+								  GFP_KERNEL);
+	if (!gzvm_dev)
+		return -ENOMEM;
+	if (copy_from_user(gzvm_dev, argp, sizeof(*gzvm_dev))) {
+		ret = -EFAULT;
+		goto err_free_dev;
+	}
+
+	if (gzvm_dev->attr_addr != 0 && gzvm_dev->attr_size != 0) {
+		size_t attr_size = gzvm_dev->attr_size;
+		void __user *attr_addr = (void __user *)gzvm_dev->attr_addr;
+
+		/* Size of device specific data should not be over a page. */
+		if (attr_size > PAGE_SIZE)
+			return -EINVAL;
+
+		dev_data = alloc_pages_exact(attr_size, GFP_KERNEL);
+		if (!dev_data) {
+			ret = -ENOMEM;
+			goto err_free_dev;
+		}
+
+		if (copy_from_user(dev_data, attr_addr, attr_size)) {
+			ret = -EFAULT;
+			goto err_free_dev_data;
+		}
+		gzvm_dev->attr_addr = virt_to_phys(dev_data);
+	}
+
+	ret = gzvm_arch_create_device(gzvm->vm_id, gzvm_dev);
+err_free_dev_data:
+	if (dev_data)
+		free_pages_exact(dev_data, 0);
+err_free_dev:
+	free_pages_exact(gzvm_dev, 0);
+	return ret;
+}
+
 static int gzvm_vm_ioctl_enable_cap(struct gzvm *gzvm,
 				    struct gzvm_enable_cap *cap,
 				    void __user *argp)
@@ -220,6 +281,20 @@ static long gzvm_vm_ioctl(struct file *filp, unsigned int ioctl,
 		ret = gzvm_vm_ioctl_set_memory_region(gzvm, &userspace_mem);
 		break;
 	}
+	case GZVM_IRQ_LINE: {
+		struct gzvm_irq_level irq_event;
+
+		ret = -EFAULT;
+		if (copy_from_user(&irq_event, argp, sizeof(irq_event)))
+			goto out;
+
+		ret = gzvm_vm_ioctl_irq_line(gzvm, &irq_event);
+		break;
+	}
+	case GZVM_CREATE_DEVICE: {
+		ret = gzvm_vm_ioctl_create_device(gzvm, argp);
+		break;
+	}
 	case GZVM_ENABLE_CAP: {
 		struct gzvm_enable_cap cap;
 
diff --git a/include/linux/gzvm_drv.h b/include/linux/gzvm_drv.h
index 5736ddf97741..1e7c81597e9a 100644
--- a/include/linux/gzvm_drv.h
+++ b/include/linux/gzvm_drv.h
@@ -107,6 +107,10 @@ int gzvm_arch_create_vcpu(gzvm_id_t vm_id, int vcpuid, void *run);
 int gzvm_arch_vcpu_run(struct gzvm_vcpu *vcpu, __u64 *exit_reason);
 int gzvm_arch_destroy_vcpu(gzvm_id_t vm_id, int vcpuid);
 
+int gzvm_arch_create_device(gzvm_id_t vm_id, struct gzvm_create_device *gzvm_dev);
+int gzvm_arch_inject_irq(struct gzvm *gzvm, unsigned int vcpu_idx, u32 irq_type,
+			 u32 irq, bool level);
+
 extern struct platform_device *gzvm_debug_dev;
 
 #endif /* __GZVM_DRV_H__ */
diff --git a/include/uapi/linux/gzvm.h b/include/uapi/linux/gzvm.h
index 6462961299eb..2f56a53efb27 100644
--- a/include/uapi/linux/gzvm.h
+++ b/include/uapi/linux/gzvm.h
@@ -87,7 +87,43 @@ struct gzvm_userspace_memory_region {
 #define GZVM_IRQ_CPU_IRQ		0
 #define GZVM_IRQ_CPU_FIQ		1
 
-/* ioctls for vcpu fds */
+struct gzvm_irq_level {
+	union {
+		__u32 irq;
+		__s32 status;
+	};
+	__u32 level;
+};
+
+#define GZVM_IRQ_LINE              _IOW(GZVM_IOC_MAGIC,  0x61, \
+					struct gzvm_irq_level)
+
+enum gzvm_device_type {
+	GZVM_DEV_TYPE_ARM_VGIC_V3_DIST,
+	GZVM_DEV_TYPE_ARM_VGIC_V3_REDIST,
+	GZVM_DEV_TYPE_MAX,
+};
+
+struct gzvm_create_device {
+	__u32 dev_type;			/* device type */
+	__u32 id;			/* out: device id */
+	__u64 flags;			/* device specific flags */
+	__u64 dev_addr;			/* device ipa address in VM's view */
+	__u64 dev_reg_size;		/* device register range size */
+	/*
+	 * If user -> kernel, this is user virtual address of device specific
+	 * attributes (if needed). If kernel->hypervisor, this is ipa.
+	 */
+	__u64 attr_addr;
+	__u64 attr_size;		/* size of device specific attributes */
+};
+
+#define GZVM_CREATE_DEVICE	   _IOWR(GZVM_IOC_MAGIC,  0xe0, \
+					struct gzvm_create_device)
+
+/*
+ * ioctls for vcpu fds
+ */
 #define GZVM_RUN                   _IO(GZVM_IOC_MAGIC,   0x80)
 
 /* VM exit reason */
-- 
2.18.0
Re: [PATCH v2 5/7] virt: geniezone: Add irqchip support for virtual interrupt injection
Posted by Marc Zyngier 2 years, 9 months ago
On 2023-04-28 11:36, Yi-De Wu wrote:
> From: "Yingshiuan Pan" <yingshiuan.pan@mediatek.com>
> 
> Enable GenieZone to handle virtual interrupt injection request.
> 
> Signed-off-by: Yingshiuan Pan <yingshiuan.pan@mediatek.com>
> Signed-off-by: Yi-De Wu <yi-de.wu@mediatek.com>
> ---
>  arch/arm64/geniezone/Makefile       |  2 +-
>  arch/arm64/geniezone/gzvm_arch.c    | 24 ++++++--
>  arch/arm64/geniezone/gzvm_arch.h    | 11 ++++
>  arch/arm64/geniezone/gzvm_irqchip.c | 88 +++++++++++++++++++++++++++++
>  drivers/virt/geniezone/gzvm_vm.c    | 75 ++++++++++++++++++++++++
>  include/linux/gzvm_drv.h            |  4 ++
>  include/uapi/linux/gzvm.h           | 38 ++++++++++++-
>  7 files changed, 235 insertions(+), 7 deletions(-)
>  create mode 100644 arch/arm64/geniezone/gzvm_irqchip.c

[...]

> +++ b/arch/arm64/geniezone/gzvm_irqchip.c
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2023 MediaTek Inc.
> + */
> +
> +#include <linux/irqchip/arm-gic-v3.h>
> +#include <kvm/arm_vgic.h>

NAK.

There is no way you can rely on anything from KVM in
your own hypervisor code.

> +
> +#include <linux/gzvm.h>
> +#include <linux/gzvm_drv.h>
> +#include "gzvm_arch.h"
> +
> +/**
> + * gzvm_sync_vgic_state() - Check all LRs synced from gz hypervisor
> + *
> + * Traverse all LRs, see if any EOIed vint, notify_acked_irq if any.
> + * GZ does not fold/unfold everytime KVM_RUN, so we have to traverse 
> all saved
> + * LRs. It will not takes much more time comparing to fold/unfold 
> everytime
> + * GZVM_RUN, because there are only few LRs.
> + */
> +void gzvm_sync_vgic_state(struct gzvm_vcpu *vcpu)
> +{
> +}
> +
> +/* is_irq_valid() - Check the irq number and irq_type are matched */
> +static bool is_irq_valid(u32 irq, u32 irq_type)
> +{
> +	switch (irq_type) {
> +	case GZVM_IRQ_TYPE_CPU:
> +		/*  0 ~ 15: SGI */
> +		if (likely(irq <= GZVM_IRQ_CPU_FIQ))
> +			return true;
> +		break;
> +	case GZVM_IRQ_TYPE_PPI:
> +		/* 16 ~ 31: PPI */
> +		if (likely(irq >= VGIC_NR_SGIS && irq < VGIC_NR_PRIVATE_IRQS))

What happens if I redefine all these macros tomorrow? None of
this code should rely on anything that is *INTERNAL* to KVM.

         M.
-- 
Jazz is not dead. It just smells funny...
Re: [PATCH v2 5/7] virt: geniezone: Add irqchip support for virtual interrupt injection
Posted by Yi-De Wu (吳一德) 2 years, 9 months ago
On Fri, 2023-04-28 at 19:59 +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@mediatek.com>
> > 
> > Enable GenieZone to handle virtual interrupt injection request.
> > 
> > Signed-off-by: Yingshiuan Pan <yingshiuan.pan@mediatek.com>
> > Signed-off-by: Yi-De Wu <yi-de.wu@mediatek.com>
> > ---
> >  arch/arm64/geniezone/Makefile       |  2 +-
> >  arch/arm64/geniezone/gzvm_arch.c    | 24 ++++++--
> >  arch/arm64/geniezone/gzvm_arch.h    | 11 ++++
> >  arch/arm64/geniezone/gzvm_irqchip.c | 88
> > +++++++++++++++++++++++++++++
> >  drivers/virt/geniezone/gzvm_vm.c    | 75 ++++++++++++++++++++++++
> >  include/linux/gzvm_drv.h            |  4 ++
> >  include/uapi/linux/gzvm.h           | 38 ++++++++++++-
> >  7 files changed, 235 insertions(+), 7 deletions(-)
> >  create mode 100644 arch/arm64/geniezone/gzvm_irqchip.c
> 
> [...]
> 
> > +++ b/arch/arm64/geniezone/gzvm_irqchip.c
> > @@ -0,0 +1,88 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2023 MediaTek Inc.
> > + */
> > +
> > +#include <linux/irqchip/arm-gic-v3.h>
> > +#include <kvm/arm_vgic.h>
> 
> NAK.
> 
> There is no way you can rely on anything from KVM in
> your own hypervisor code.
> 

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.

> > +
> > +#include <linux/gzvm.h>
> > +#include <linux/gzvm_drv.h>
> > +#include "gzvm_arch.h"
> > +
> > +/**
> > + * gzvm_sync_vgic_state() - Check all LRs synced from gz
> > hypervisor
> > + *
> > + * Traverse all LRs, see if any EOIed vint, notify_acked_irq if
> > any.
> > + * GZ does not fold/unfold everytime KVM_RUN, so we have to
> > traverse
> > all saved
> > + * LRs. It will not takes much more time comparing to fold/unfold
> > everytime
> > + * GZVM_RUN, because there are only few LRs.
> > + */
> > +void gzvm_sync_vgic_state(struct gzvm_vcpu *vcpu)
> > +{
> > +}
> > +
> > +/* is_irq_valid() - Check the irq number and irq_type are matched
> > */
> > +static bool is_irq_valid(u32 irq, u32 irq_type)
> > +{
> > +     switch (irq_type) {
> > +     case GZVM_IRQ_TYPE_CPU:
> > +             /*  0 ~ 15: SGI */
> > +             if (likely(irq <= GZVM_IRQ_CPU_FIQ))
> > +                     return true;
> > +             break;
> > +     case GZVM_IRQ_TYPE_PPI:
> > +             /* 16 ~ 31: PPI */
> > +             if (likely(irq >= VGIC_NR_SGIS && irq <
> > VGIC_NR_PRIVATE_IRQS))
> 
> What happens if I redefine all these macros tomorrow? None of
> this code should rely on anything that is *INTERNAL* to KVM.
> 
>          M.
> --
> Jazz is not dead. It just smells funny...
Re: [PATCH v2 5/7] virt: geniezone: Add irqchip support for virtual interrupt injection
Posted by Marc Zyngier 2 years, 9 months ago
On Fri, 12 May 2023 08:19:31 +0100,
"Yi-De Wu (吳一德)" <Yi-De.Wu@mediatek.com> wrote:
> 
> On Fri, 2023-04-28 at 19:59 +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@mediatek.com>
> > > 
> > > Enable GenieZone to handle virtual interrupt injection request.
> > > 
> > > Signed-off-by: Yingshiuan Pan <yingshiuan.pan@mediatek.com>
> > > Signed-off-by: Yi-De Wu <yi-de.wu@mediatek.com>
> > > ---
> > >  arch/arm64/geniezone/Makefile       |  2 +-
> > >  arch/arm64/geniezone/gzvm_arch.c    | 24 ++++++--
> > >  arch/arm64/geniezone/gzvm_arch.h    | 11 ++++
> > >  arch/arm64/geniezone/gzvm_irqchip.c | 88
> > > +++++++++++++++++++++++++++++
> > >  drivers/virt/geniezone/gzvm_vm.c    | 75 ++++++++++++++++++++++++
> > >  include/linux/gzvm_drv.h            |  4 ++
> > >  include/uapi/linux/gzvm.h           | 38 ++++++++++++-
> > >  7 files changed, 235 insertions(+), 7 deletions(-)
> > >  create mode 100644 arch/arm64/geniezone/gzvm_irqchip.c
> > 
> > [...]
> > 
> > > +++ b/arch/arm64/geniezone/gzvm_irqchip.c
> > > @@ -0,0 +1,88 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (c) 2023 MediaTek Inc.
> > > + */
> > > +
> > > +#include <linux/irqchip/arm-gic-v3.h>
> > > +#include <kvm/arm_vgic.h>
> > 
> > NAK.
> > 
> > There is no way you can rely on anything from KVM in
> > your own hypervisor code.
> > 
> 
> 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.

Why do you need *ANY* of the KVM stuff? Please fully enumerate these
dependencies and why you have them.

Directly using KVM stuff for something completely unrelated is not OK,
and will never be.

	M.

-- 
Without deviation from the norm, progress is not possible.
Re: [PATCH v2 5/7] virt: geniezone: Add irqchip support for virtual interrupt injection
Posted by Yi-De Wu (吳一德) 2 years, 9 months ago
On Fri, 2023-05-12 at 08:51 +0100, Marc Zyngier wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Fri, 12 May 2023 08:19:31 +0100,
> "Yi-De Wu (吳一德)" <Yi-De.Wu@mediatek.com> wrote:
> > 
> > On Fri, 2023-04-28 at 19:59 +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@mediatek.com>
> > > > 
> > > > Enable GenieZone to handle virtual interrupt injection request.
> > > > 
> > > > Signed-off-by: Yingshiuan Pan <yingshiuan.pan@mediatek.com>
> > > > Signed-off-by: Yi-De Wu <yi-de.wu@mediatek.com>
> > > > ---
> > > >  arch/arm64/geniezone/Makefile       |  2 +-
> > > >  arch/arm64/geniezone/gzvm_arch.c    | 24 ++++++--
> > > >  arch/arm64/geniezone/gzvm_arch.h    | 11 ++++
> > > >  arch/arm64/geniezone/gzvm_irqchip.c | 88
> > > > +++++++++++++++++++++++++++++
> > > >  drivers/virt/geniezone/gzvm_vm.c    | 75
> > > > ++++++++++++++++++++++++
> > > >  include/linux/gzvm_drv.h            |  4 ++
> > > >  include/uapi/linux/gzvm.h           | 38 ++++++++++++-
> > > >  7 files changed, 235 insertions(+), 7 deletions(-)
> > > >  create mode 100644 arch/arm64/geniezone/gzvm_irqchip.c
> > > 
> > > [...]
> > > 
> > > > +++ b/arch/arm64/geniezone/gzvm_irqchip.c
> > > > @@ -0,0 +1,88 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright (c) 2023 MediaTek Inc.
> > > > + */
> > > > +
> > > > +#include <linux/irqchip/arm-gic-v3.h>
> > > > +#include <kvm/arm_vgic.h>
> > > 
> > > NAK.
> > > 
> > > There is no way you can rely on anything from KVM in
> > > your own hypervisor code.
> > > 
> > 
> > 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.
> 
> Why do you need *ANY* of the KVM stuff? Please fully enumerate these
> dependencies and why you have them.
> 
> Directly using KVM stuff for something completely unrelated is not
> OK,
> and will never be.
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.

The particular part we'd like to leverage from KVM are as followed
1. `gfn_to_pfn_memslot` to convert guest physical address(or
intermediate physical address) to physical address
2. get ARM's number of interrupt of different types e.g. number of SGI,
number of PPI...etc

Re: [PATCH v2 5/7] virt: geniezone: Add irqchip support for virtual interrupt injection
Posted by Marc Zyngier 2 years, 9 months ago
On Fri, 12 May 2023 09:16:58 +0100,
"Yi-De Wu (吳一德)" <Yi-De.Wu@mediatek.com> wrote:
> 
> On Fri, 2023-05-12 at 08:51 +0100, Marc Zyngier wrote:
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > 
> > 
> > On Fri, 12 May 2023 08:19:31 +0100,
> > "Yi-De Wu (吳一德)" <Yi-De.Wu@mediatek.com> wrote:
> > > 
> > > On Fri, 2023-04-28 at 19:59 +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@mediatek.com>
> > > > > 
> > > > > Enable GenieZone to handle virtual interrupt injection request.
> > > > > 
> > > > > Signed-off-by: Yingshiuan Pan <yingshiuan.pan@mediatek.com>
> > > > > Signed-off-by: Yi-De Wu <yi-de.wu@mediatek.com>
> > > > > ---
> > > > >  arch/arm64/geniezone/Makefile       |  2 +-
> > > > >  arch/arm64/geniezone/gzvm_arch.c    | 24 ++++++--
> > > > >  arch/arm64/geniezone/gzvm_arch.h    | 11 ++++
> > > > >  arch/arm64/geniezone/gzvm_irqchip.c | 88
> > > > > +++++++++++++++++++++++++++++
> > > > >  drivers/virt/geniezone/gzvm_vm.c    | 75
> > > > > ++++++++++++++++++++++++
> > > > >  include/linux/gzvm_drv.h            |  4 ++
> > > > >  include/uapi/linux/gzvm.h           | 38 ++++++++++++-
> > > > >  7 files changed, 235 insertions(+), 7 deletions(-)
> > > > >  create mode 100644 arch/arm64/geniezone/gzvm_irqchip.c
> > > > 
> > > > [...]
> > > > 
> > > > > +++ b/arch/arm64/geniezone/gzvm_irqchip.c
> > > > > @@ -0,0 +1,88 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Copyright (c) 2023 MediaTek Inc.
> > > > > + */
> > > > > +
> > > > > +#include <linux/irqchip/arm-gic-v3.h>
> > > > > +#include <kvm/arm_vgic.h>
> > > > 
> > > > NAK.
> > > > 
> > > > There is no way you can rely on anything from KVM in
> > > > your own hypervisor code.
> > > > 
> > > 
> > > 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.
> > 
> > Why do you need *ANY* of the KVM stuff? Please fully enumerate these
> > dependencies and why you have them.
> > 
> > Directly using KVM stuff for something completely unrelated is not
> > OK,
> > and will never be.
> > 
> >         M.
> > 
> > --
> > Without deviation from the norm, progress is not possible.
> 
> The particular part we'd like to leverage from KVM are as followed
> 1. `gfn_to_pfn_memslot` to convert guest physical address(or
> intermediate physical address) to physical address

What is a memslot in your hypervisor? How does it relate to KVM's?
What about the use of struct kvm?

I'm sorry, but your use of *internal* structures and API would make it
impossible for us to make any further change without potentially
affecting your hypervisor. Which is closed source and untestable.

To sum it up, I'm strongly opposed to any use of these data
structures.  If you can spot some commonalities, expose them as a
0-cost abstraction. But don't use them as is your code.

> 2. get ARM's number of interrupt of different types e.g. number of SGI,
> number of PPI...etc

These are architectural constants, and you can define your own. That
will cost you nothing but a handful of #define, and keep the two
subsystem independent.

	M.

-- 
Without deviation from the norm, progress is not possible.
Re: [PATCH v2 5/7] virt: geniezone: Add irqchip support for virtual interrupt injection
Posted by Yi-De Wu (吳一德) 2 years, 8 months ago
On Fri, 2023-05-12 at 10:57 +0100, Marc Zyngier wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Fri, 12 May 2023 09:16:58 +0100,
> "Yi-De Wu (吳一德)" <Yi-De.Wu@mediatek.com> wrote:
> > 
> > On Fri, 2023-05-12 at 08:51 +0100, Marc Zyngier wrote:
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > > 
> > > 
> > > On Fri, 12 May 2023 08:19:31 +0100,
> > > "Yi-De Wu (吳一德)" <Yi-De.Wu@mediatek.com> wrote:
> > > > 
> > > > On Fri, 2023-04-28 at 19:59 +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@mediatek.com>
> > > > > > 
> > > > > > Enable GenieZone to handle virtual interrupt injection
> > > > > > request.
> > > > > > 
> > > > > > Signed-off-by: Yingshiuan Pan <yingshiuan.pan@mediatek.com>
> > > > > > Signed-off-by: Yi-De Wu <yi-de.wu@mediatek.com>
> > > > > > ---
> > > > > >  arch/arm64/geniezone/Makefile       |  2 +-
> > > > > >  arch/arm64/geniezone/gzvm_arch.c    | 24 ++++++--
> > > > > >  arch/arm64/geniezone/gzvm_arch.h    | 11 ++++
> > > > > >  arch/arm64/geniezone/gzvm_irqchip.c | 88
> > > > > > +++++++++++++++++++++++++++++
> > > > > >  drivers/virt/geniezone/gzvm_vm.c    | 75
> > > > > > ++++++++++++++++++++++++
> > > > > >  include/linux/gzvm_drv.h            |  4 ++
> > > > > >  include/uapi/linux/gzvm.h           | 38 ++++++++++++-
> > > > > >  7 files changed, 235 insertions(+), 7 deletions(-)
> > > > > >  create mode 100644 arch/arm64/geniezone/gzvm_irqchip.c
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > +++ b/arch/arm64/geniezone/gzvm_irqchip.c
> > > > > > @@ -0,0 +1,88 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > +/*
> > > > > > + * Copyright (c) 2023 MediaTek Inc.
> > > > > > + */
> > > > > > +
> > > > > > +#include <linux/irqchip/arm-gic-v3.h>
> > > > > > +#include <kvm/arm_vgic.h>
> > > > > 
> > > > > NAK.
> > > > > 
> > > > > There is no way you can rely on anything from KVM in
> > > > > your own hypervisor code.
> > > > > 
> > > > 
> > > > 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.
> > > 
> > > Why do you need *ANY* of the KVM stuff? Please fully enumerate
> > > these
> > > dependencies and why you have them.
> > > 
> > > Directly using KVM stuff for something completely unrelated is
> > > not
> > > OK,
> > > and will never be.
> > > 
> > >         M.
> > > 
> > > --
> > > Without deviation from the norm, progress is not possible.
> > 
> > The particular part we'd like to leverage from KVM are as followed
> > 1. `gfn_to_pfn_memslot` to convert guest physical address(or
> > intermediate physical address) to physical address
> 
> What is a memslot in your hypervisor? How does it relate to KVM's?
> What about the use of struct kvm?
> 
> I'm sorry, but your use of *internal* structures and API would make
> it
> impossible for us to make any further change without potentially
> affecting your hypervisor. Which is closed source and untestable.
> 
> To sum it up, I'm strongly opposed to any use of these data
> structures.  If you can spot some commonalities, expose them as a
> 0-cost abstraction. But don't use them as is your code.
> 
> > 2. get ARM's number of interrupt of different types e.g. number of
> > SGI,
> > number of PPI...etc
> 
> These are architectural constants, and you can define your own. That
> will cost you nothing but a handful of #define, and keep the two
> subsystem independent.
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.

As per discussion, we would define our own constants here and keep the
subsystem indepedent.