[PATCH V1 0/7] Enable ACPI support for ARM KVM GIC

[PATCH V1 0/7] Enable ACPI support for ARM KVM GIC

[Wei Huang]

This patch set enables ACPI support for KVM GIC. Note that the patches
are in fact the V3 of previously submitted patches (search "Enable ACPI
support for KVM ARM"). But because Fu Wei includes the arch_timer part
in his series [1] and I have substantially re-written the GIC code in this
revision, the version number is reset to v1. 

By following Marc's prior comments, the main design idea is to let DT or
ACPI code to fill out the "struct vgic_params" which are extended to
include all GIC related info.

[1] https://lkml.org/lkml/2016/2/1/658

Thanks,
-Wei

Wei Huang (7):
  KVM: GIC: Move GIC DT probing code to GICv2 and GICv3 files
  KVM: GIC: Add extra fields to store GICH and GICV resource info
  KVM: GIC: Create a common probe function for GIC
  KVM: GICv2: Extract the common code from DT
  KVM: GICv2: Add ACPI probing function
  KVM: GICv3: Extract the common code from DT
  KVM: GICv3: Add ACPI probing function

 include/kvm/arm_vgic.h      |  14 ++--
 virt/kvm/arm/vgic-v2-emul.c |   4 +-
 virt/kvm/arm/vgic-v2.c      | 186 +++++++++++++++++++++++++++++++++-----------
 virt/kvm/arm/vgic-v3.c      | 159 ++++++++++++++++++++++++++++---------
 virt/kvm/arm/vgic.c         |  22 +-----
 5 files changed, 277 insertions(+), 108 deletions(-)

-- 
1.8.3.1

[PATCH V1 1/7] KVM: GIC: Move GIC DT probing code to GICv2 and GICv3 files

[Wei Huang]

This patch moves GIC DT probing code from vgic.c to GICv2 & GICv3
sub-files. The probing will start from GICv2. If the probing fails,
KVM will try to probe GICv3 then.

Signed-off-by: Wei Huang <wei@redhat.com>
---
 include/kvm/arm_vgic.h |  6 ++----
 virt/kvm/arm/vgic-v2.c | 17 ++++++++++++++---
 virt/kvm/arm/vgic-v3.c | 15 ++++++++++++---
 virt/kvm/arm/vgic.c    | 22 ++--------------------
 4 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 13a3d53..59428d4 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -357,12 +357,10 @@ bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
 #define vgic_initialized(k)	(!!((k)->arch.vgic.nr_cpus))
 #define vgic_ready(k)		((k)->arch.vgic.ready)
 
-int vgic_v2_probe(struct device_node *vgic_node,
-		  const struct vgic_ops **ops,
+int vgic_v2_probe(const struct vgic_ops **ops,
 		  const struct vgic_params **params);
 #ifdef CONFIG_KVM_ARM_VGIC_V3
-int vgic_v3_probe(struct device_node *vgic_node,
-		  const struct vgic_ops **ops,
+int vgic_v3_probe(const struct vgic_ops **ops,
 		  const struct vgic_params **params);
 #else
 static inline int vgic_v3_probe(struct device_node *vgic_node,
diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
index ff02f08..dc9ceab 100644
--- a/virt/kvm/arm/vgic-v2.c
+++ b/virt/kvm/arm/vgic-v2.c
@@ -175,10 +175,15 @@ static const struct vgic_ops vgic_v2_ops = {
 };
 
 static struct vgic_params vgic_v2_params;
+static const struct of_device_id vgic_v2_ids[] = {
+	{ .compatible = "arm,cortex-a15-gic" },
+	{ .compatible = "arm,cortex-a7-gic" },
+	{ .compatible = "arm,gic-400" },
+	{},
+};
 
 /**
  * vgic_v2_probe - probe for a GICv2 compatible interrupt controller in DT
- * @node:	pointer to the DT node
  * @ops: 	address of a pointer to the GICv2 operations
  * @params:	address of a pointer to HW-specific parameters
  *
@@ -186,15 +191,21 @@ static struct vgic_params vgic_v2_params;
  * in *ops and the HW parameters in *params. Returns an error code
  * otherwise.
  */
-int vgic_v2_probe(struct device_node *vgic_node,
-		  const struct vgic_ops **ops,
+int vgic_v2_probe(const struct vgic_ops **ops,
 		  const struct vgic_params **params)
 {
 	int ret;
 	struct resource vctrl_res;
 	struct resource vcpu_res;
+	struct device_node *vgic_node;
 	struct vgic_params *vgic = &vgic_v2_params;
 
+	vgic_node = of_find_matching_node(NULL, vgic_v2_ids);
+	if (!vgic_node) {
+		ret = -ENODEV;
+		goto out;
+	}
+
 	vgic->maint_irq = irq_of_parse_and_map(vgic_node, 0);
 	if (!vgic->maint_irq) {
 		kvm_err("error getting vgic maintenance irq from DT\n");
diff --git a/virt/kvm/arm/vgic-v3.c b/virt/kvm/arm/vgic-v3.c
index 453eafd..5fa5fa7 100644
--- a/virt/kvm/arm/vgic-v3.c
+++ b/virt/kvm/arm/vgic-v3.c
@@ -215,10 +215,13 @@ static const struct vgic_ops vgic_v3_ops = {
 };
 
 static struct vgic_params vgic_v3_params;
+static const struct of_device_id vgic_v3_ids[] = {
+	{ .compatible = "arm,gic-v3" },
+	{},
+};
 
 /**
  * vgic_v3_probe - probe for a GICv3 compatible interrupt controller in DT
- * @node:	pointer to the DT node
  * @ops: 	address of a pointer to the GICv3 operations
  * @params:	address of a pointer to HW-specific parameters
  *
@@ -226,15 +229,21 @@ static struct vgic_params vgic_v3_params;
  * in *ops and the HW parameters in *params. Returns an error code
  * otherwise.
  */
-int vgic_v3_probe(struct device_node *vgic_node,
-		  const struct vgic_ops **ops,
+int vgic_v3_probe(const struct vgic_ops **ops,
 		  const struct vgic_params **params)
 {
 	int ret = 0;
 	u32 gicv_idx;
 	struct resource vcpu_res;
+	struct device_node *vgic_node;
 	struct vgic_params *vgic = &vgic_v3_params;
 
+	vgic_node = of_find_matching_node(NULL, vgic_v3_ids);
+	if (!vgic_node) {
+		ret = -ENODEV;
+		goto out;
+	}
+
 	vgic->maint_irq = irq_of_parse_and_map(vgic_node, 0);
 	if (!vgic->maint_irq) {
 		kvm_err("error getting vgic maintenance irq from DT\n");
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 043032c..0d3d6b7 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -2389,34 +2389,16 @@ static struct notifier_block vgic_cpu_nb = {
 	.notifier_call = vgic_cpu_notify,
 };
 
-static const struct of_device_id vgic_ids[] = {
-	{ .compatible = "arm,cortex-a15-gic",	.data = vgic_v2_probe, },
-	{ .compatible = "arm,cortex-a7-gic",	.data = vgic_v2_probe, },
-	{ .compatible = "arm,gic-400",		.data = vgic_v2_probe, },
-	{ .compatible = "arm,gic-v3",		.data = vgic_v3_probe, },
-	{},
-};
-
 int kvm_vgic_hyp_init(void)
 {
-	const struct of_device_id *matched_id;
-	const int (*vgic_probe)(struct device_node *,const struct vgic_ops **,
-				const struct vgic_params **);
-	struct device_node *vgic_node;
 	int ret;
 
-	vgic_node = of_find_matching_node_and_match(NULL,
-						    vgic_ids, &matched_id);
-	if (!vgic_node) {
+	/* try GICv2 probing first, then GICv3 probing */
+	if (vgic_v2_probe(&vgic_ops, &vgic) || vgic_v3_probe(&vgic_ops, &vgic)) {
 		kvm_err("error: no compatible GIC node found\n");
 		return -ENODEV;
 	}
 
-	vgic_probe = matched_id->data;
-	ret = vgic_probe(vgic_node, &vgic_ops, &vgic);
-	if (ret)
-		return ret;
-
 	ret = request_percpu_irq(vgic->maint_irq, vgic_maintenance_handler,
 				 "vgic", kvm_get_running_vcpus());
 	if (ret) {
-- 
1.8.3.1

[PATCH V1 2/7] KVM: GIC: Add extra fields to store GICH and GICV resource info

[Wei Huang]

This patch adds new fields in the struct vgic_params to store the
resource info (base and size) of GICH & GICV interfaces. These new
fields will be used by the DT and ACPI probing code later.

Signed-off-by: Wei Huang <wei@redhat.com>
---
 include/kvm/arm_vgic.h      |  8 +++++++-
 virt/kvm/arm/vgic-v2-emul.c |  4 ++--
 virt/kvm/arm/vgic-v2.c      | 19 ++++++++++---------
 virt/kvm/arm/vgic-v3.c      | 10 +++++-----
 4 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 59428d4..8003ca8 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -127,11 +127,17 @@ struct vgic_params {
 	/* vgic type */
 	enum vgic_type	type;
 	/* Physical address of vgic virtual cpu interface */
-	phys_addr_t	vcpu_base;
+	phys_addr_t	vcpu_phys_base;
+	/* Size of vgic virtual cpu interface */
+	phys_addr_t	vcpu_size;
 	/* Number of list registers */
 	u32		nr_lr;
 	/* Interrupt number */
 	unsigned int	maint_irq;
+	/* Virtual control interface physical address */
+	phys_addr_t	vctrl_phys_base;
+	/* Size of virtual control interface */
+	phys_addr_t	vctrl_size;
 	/* Virtual control interface base address */
 	void __iomem	*vctrl_base;
 	int		max_gic_vcpus;
diff --git a/virt/kvm/arm/vgic-v2-emul.c b/virt/kvm/arm/vgic-v2-emul.c
index 1390797..e244afe 100644
--- a/virt/kvm/arm/vgic-v2-emul.c
+++ b/virt/kvm/arm/vgic-v2-emul.c
@@ -521,8 +521,8 @@ static int vgic_v2_map_resources(struct kvm *kvm,
 	}
 
 	ret = kvm_phys_addr_ioremap(kvm, dist->vgic_cpu_base,
-				    params->vcpu_base, KVM_VGIC_V2_CPU_SIZE,
-				    true);
+				    params->vcpu_phys_base,
+				    KVM_VGIC_V2_CPU_SIZE, true);
 	if (ret) {
 		kvm_err("Unable to remap VGIC CPU to VCPU\n");
 		goto out_unregister;
diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
index dc9ceab..6540a6d 100644
--- a/virt/kvm/arm/vgic-v2.c
+++ b/virt/kvm/arm/vgic-v2.c
@@ -218,6 +218,8 @@ int vgic_v2_probe(const struct vgic_ops **ops,
 		kvm_err("Cannot obtain GICH resource\n");
 		goto out;
 	}
+	vgic->vctrl_phys_base = vctrl_res.start;
+	vgic->vctrl_size = resource_size(&vctrl_res);
 
 	vgic->vctrl_base = of_iomap(vgic_node, 2);
 	if (!vgic->vctrl_base) {
@@ -230,8 +232,8 @@ int vgic_v2_probe(const struct vgic_ops **ops,
 	vgic->nr_lr = (vgic->nr_lr & 0x3f) + 1;
 
 	ret = create_hyp_io_mappings(vgic->vctrl_base,
-				     vgic->vctrl_base + resource_size(&vctrl_res),
-				     vctrl_res.start);
+				     vgic->vctrl_base + vgic->vctrl_size,
+				     vgic->vctrl_phys_base);
 	if (ret) {
 		kvm_err("Cannot map VCTRL into hyp\n");
 		goto out_unmap;
@@ -242,18 +244,19 @@ int vgic_v2_probe(const struct vgic_ops **ops,
 		ret = -ENXIO;
 		goto out_unmap;
 	}
+	vgic->vcpu_phys_base = vcpu_res.start;
+	vgic->vcpu_size = resource_size(&vcpu_res);
 
-	if (!PAGE_ALIGNED(vcpu_res.start)) {
+	if (!PAGE_ALIGNED(vgic->vcpu_phys_base)) {
 		kvm_err("GICV physical address 0x%llx not page aligned\n",
 			(unsigned long long)vcpu_res.start);
 		ret = -ENXIO;
 		goto out_unmap;
 	}
 
-	if (!PAGE_ALIGNED(resource_size(&vcpu_res))) {
+	if (!PAGE_ALIGNED(vgic->vcpu_size)) {
 		kvm_err("GICV size 0x%llx not a multiple of page size 0x%lx\n",
-			(unsigned long long)resource_size(&vcpu_res),
-			PAGE_SIZE);
+			(unsigned long long)vgic->vcpu_size, PAGE_SIZE);
 		ret = -ENXIO;
 		goto out_unmap;
 	}
@@ -261,10 +264,8 @@ int vgic_v2_probe(const struct vgic_ops **ops,
 	vgic->can_emulate_gicv2 = true;
 	kvm_register_device_ops(&kvm_arm_vgic_v2_ops, KVM_DEV_TYPE_ARM_VGIC_V2);
 
-	vgic->vcpu_base = vcpu_res.start;
-
 	kvm_info("%s@%llx IRQ%d\n", vgic_node->name,
-		 vctrl_res.start, vgic->maint_irq);
+		 vgic->vctrl_phys_base, vgic->maint_irq);
 
 	vgic->type = VGIC_V2;
 	vgic->max_gic_vcpus = VGIC_V2_MAX_CPUS;
diff --git a/virt/kvm/arm/vgic-v3.c b/virt/kvm/arm/vgic-v3.c
index 5fa5fa7..33a5fdf 100644
--- a/virt/kvm/arm/vgic-v3.c
+++ b/virt/kvm/arm/vgic-v3.c
@@ -266,23 +266,23 @@ int vgic_v3_probe(const struct vgic_ops **ops,
 	gicv_idx += 3; /* Also skip GICD, GICC, GICH */
 	if (of_address_to_resource(vgic_node, gicv_idx, &vcpu_res)) {
 		kvm_info("GICv3: no GICV resource entry\n");
-		vgic->vcpu_base = 0;
+		vgic->vcpu_phys_base = 0;
 	} else if (!PAGE_ALIGNED(vcpu_res.start)) {
 		pr_warn("GICV physical address 0x%llx not page aligned\n",
 			(unsigned long long)vcpu_res.start);
-		vgic->vcpu_base = 0;
+		vgic->vcpu_phys_base = 0;
 	} else if (!PAGE_ALIGNED(resource_size(&vcpu_res))) {
 		pr_warn("GICV size 0x%llx not a multiple of page size 0x%lx\n",
 			(unsigned long long)resource_size(&vcpu_res),
 			PAGE_SIZE);
-		vgic->vcpu_base = 0;
+		vgic->vcpu_phys_base = 0;
 	} else {
-		vgic->vcpu_base = vcpu_res.start;
+		vgic->vcpu_phys_base = vcpu_res.start;
 		vgic->can_emulate_gicv2 = true;
 		kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
 					KVM_DEV_TYPE_ARM_VGIC_V2);
 	}
-	if (vgic->vcpu_base == 0)
+	if (vgic->vcpu_phys_base == 0)
 		kvm_info("disabling GICv2 emulation\n");
 	kvm_register_device_ops(&kvm_arm_vgic_v3_ops, KVM_DEV_TYPE_ARM_VGIC_V3);
 
-- 
1.8.3.1

[PATCH V1 3/7] KVM: GIC: Create a common probe function for GIC

[Wei Huang]

This patch creates common functions for both GICv2 and GICv3. The
existing functions are renamed to new names specifically for DT.

Signed-off-by: Wei Huang <wei@redhat.com>
---
 virt/kvm/arm/vgic-v2.c | 44 +++++++++++++++++++++++++++++++-------------
 virt/kvm/arm/vgic-v3.c | 37 ++++++++++++++++++++++++-------------
 2 files changed, 55 insertions(+), 26 deletions(-)

diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
index 6540a6d..7dd5fb3 100644
--- a/virt/kvm/arm/vgic-v2.c
+++ b/virt/kvm/arm/vgic-v2.c
@@ -182,23 +182,16 @@ static const struct of_device_id vgic_v2_ids[] = {
 	{},
 };
 
-/**
- * vgic_v2_probe - probe for a GICv2 compatible interrupt controller in DT
- * @ops: 	address of a pointer to the GICv2 operations
- * @params:	address of a pointer to HW-specific parameters
- *
- * Returns 0 if a GICv2 has been found, with the low level operations
- * in *ops and the HW parameters in *params. Returns an error code
- * otherwise.
+/*
+ * Returns 0 if a GICv2 can be found in DT and the details of GICv2
+ * configuration are stored in *vgic. Otherwise returns an error code.
  */
-int vgic_v2_probe(const struct vgic_ops **ops,
-		  const struct vgic_params **params)
+static int vgic_v2_dt_probe(struct vgic_params *vgic)
 {
 	int ret;
 	struct resource vctrl_res;
 	struct resource vcpu_res;
 	struct device_node *vgic_node;
-	struct vgic_params *vgic = &vgic_v2_params;
 
 	vgic_node = of_find_matching_node(NULL, vgic_v2_ids);
 	if (!vgic_node) {
@@ -269,8 +262,6 @@ int vgic_v2_probe(const struct vgic_ops **ops,
 
 	vgic->type = VGIC_V2;
 	vgic->max_gic_vcpus = VGIC_V2_MAX_CPUS;
-	*ops = &vgic_v2_ops;
-	*params = vgic;
 	goto out;
 
 out_unmap:
@@ -279,3 +270,30 @@ out:
 	of_node_put(vgic_node);
 	return ret;
 }
+
+/**
+ * vgic_v2_probe - probe for a GICv2 compatible interrupt controller
+ * @ops:	address of a pointer to the GICv2 operations
+ * @params:	address of a pointer to HW-specific parameters
+ *
+ * Returns 0 if a GICv2 has been found, with the low level operations
+ * in *ops and the HW parameters in *params. Returns an error code
+ * otherwise.
+ */
+int vgic_v2_probe(const struct vgic_ops **ops,
+		  const struct vgic_params **params)
+{
+	int ret;
+	struct vgic_params *vgic = &vgic_v2_params;
+
+	ret = vgic_v2_dt_probe(vgic);
+	if (ret)
+		goto err_out;
+
+	*ops = &vgic_v2_ops;
+	*params = vgic;
+
+	return 0;
+err_out:
+	return ret;
+}
diff --git a/virt/kvm/arm/vgic-v3.c b/virt/kvm/arm/vgic-v3.c
index 33a5fdf..b036134 100644
--- a/virt/kvm/arm/vgic-v3.c
+++ b/virt/kvm/arm/vgic-v3.c
@@ -220,23 +220,16 @@ static const struct of_device_id vgic_v3_ids[] = {
 	{},
 };
 
-/**
- * vgic_v3_probe - probe for a GICv3 compatible interrupt controller in DT
- * @ops: 	address of a pointer to the GICv3 operations
- * @params:	address of a pointer to HW-specific parameters
- *
- * Returns 0 if a GICv3 has been found, with the low level operations
- * in *ops and the HW parameters in *params. Returns an error code
- * otherwise.
+/*
+ * Returns 0 if a GICv3 can be found in DT and the details of GICv3
+ * configuration are stored in *vgic. Otherwise returns an error code.
  */
-int vgic_v3_probe(const struct vgic_ops **ops,
-		  const struct vgic_params **params)
+static int vgic_v3_dt_probe(struct vgic_params *vgic)
 {
 	int ret = 0;
 	u32 gicv_idx;
 	struct resource vcpu_res;
 	struct device_node *vgic_node;
-	struct vgic_params *vgic = &vgic_v3_params;
 
 	vgic_node = of_find_matching_node(NULL, vgic_v3_ids);
 	if (!vgic_node) {
@@ -292,11 +285,29 @@ int vgic_v3_probe(const struct vgic_ops **ops,
 
 	kvm_info("%s@%llx IRQ%d\n", vgic_node->name,
 		 vcpu_res.start, vgic->maint_irq);
+out:
+	of_node_put(vgic_node);
+	return ret;
+}
+/**
+ * vgic_v3_probe - probe for a GICv3 compatible interrupt controller
+ * @ops:	address of a pointer to the GICv3 operations
+ * @params:	address of a pointer to HW-specific parameters
+ *
+ * Returns 0 if a GICv3 has been found, with the low level operations
+ * in *ops and the HW parameters in *params. Returns an error code
+ * otherwise.
+ */
+int vgic_v3_probe(const struct vgic_ops **ops,
+		  const struct vgic_params **params)
+{
+	int ret = 0;
+	struct vgic_params *vgic = &vgic_v3_params;
+
+	ret = vgic_v3_dt_probe(vgic);
 
 	*ops = &vgic_v3_ops;
 	*params = vgic;
 
-out:
-	of_node_put(vgic_node);
 	return ret;
 }
-- 
1.8.3.1

[PATCH V1 4/7] KVM: GICv2: Extract the common code from DT

[Wei Huang]

This patch extracts the common code from the DT probe function. With
this patch the DT function only fills out the following info in *vgic.
  - maint_irq (mapped)
  - GICH resource
  - GICV resource
Note that vgic->vctrl_base io-remapping is now moved to vgic_v2_probe().

Signed-off-by: Wei Huang <wei@redhat.com>
---
 virt/kvm/arm/vgic-v2.c | 92 ++++++++++++++++++++++++--------------------------
 1 file changed, 44 insertions(+), 48 deletions(-)

diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
index 7dd5fb3..b60e73a 100644
--- a/virt/kvm/arm/vgic-v2.c
+++ b/virt/kvm/arm/vgic-v2.c
@@ -188,7 +188,7 @@ static const struct of_device_id vgic_v2_ids[] = {
  */
 static int vgic_v2_dt_probe(struct vgic_params *vgic)
 {
-	int ret;
+	int ret = 0;
 	struct resource vctrl_res;
 	struct resource vcpu_res;
 	struct device_node *vgic_node;
@@ -201,24 +201,55 @@ static int vgic_v2_dt_probe(struct vgic_params *vgic)
 
 	vgic->maint_irq = irq_of_parse_and_map(vgic_node, 0);
 	if (!vgic->maint_irq) {
-		kvm_err("error getting vgic maintenance irq from DT\n");
+		kvm_err("Cannot get vgic maintenance irq from DT\n");
 		ret = -ENXIO;
 		goto out;
 	}
 
 	ret = of_address_to_resource(vgic_node, 2, &vctrl_res);
 	if (ret) {
-		kvm_err("Cannot obtain GICH resource\n");
+		kvm_err("Cannot obtain GICH resource from DT\n");
 		goto out;
 	}
 	vgic->vctrl_phys_base = vctrl_res.start;
 	vgic->vctrl_size = resource_size(&vctrl_res);
 
-	vgic->vctrl_base = of_iomap(vgic_node, 2);
+	if (of_address_to_resource(vgic_node, 3, &vcpu_res)) {
+		kvm_err("Cannot obtain GICV resource\n");
+		ret = -ENXIO;
+		goto out;
+	}
+	vgic->vcpu_phys_base = vcpu_res.start;
+	vgic->vcpu_size = resource_size(&vcpu_res);
+out:
+	of_node_put(vgic_node);
+	return ret;
+}
+
+/**
+ * vgic_v2_probe - probe for a GICv2 compatible interrupt controller
+ * @ops:	address of a pointer to the GICv2 operations
+ * @params:	address of a pointer to HW-specific parameters
+ *
+ * Returns 0 if a GICv2 has been found, with the low level operations
+ * in *ops and the HW parameters in *params. Returns an error code
+ * otherwise.
+ */
+int vgic_v2_probe(const struct vgic_ops **ops,
+		  const struct vgic_params **params)
+{
+	int ret;
+	struct vgic_params *vgic = &vgic_v2_params;
+
+	ret = vgic_v2_dt_probe(vgic);
+	if (ret)
+		goto err_out;
+
+	vgic->vctrl_base = ioremap(vgic->vctrl_phys_base, vgic->vctrl_size);
 	if (!vgic->vctrl_base) {
 		kvm_err("Cannot ioremap GICH\n");
 		ret = -ENOMEM;
-		goto out;
+		goto err_out;
 	}
 
 	vgic->nr_lr = readl_relaxed(vgic->vctrl_base + GICH_VTR);
@@ -229,71 +260,36 @@ static int vgic_v2_dt_probe(struct vgic_params *vgic)
 				     vgic->vctrl_phys_base);
 	if (ret) {
 		kvm_err("Cannot map VCTRL into hyp\n");
-		goto out_unmap;
-	}
-
-	if (of_address_to_resource(vgic_node, 3, &vcpu_res)) {
-		kvm_err("Cannot obtain GICV resource\n");
-		ret = -ENXIO;
-		goto out_unmap;
+		goto err_out;
 	}
-	vgic->vcpu_phys_base = vcpu_res.start;
-	vgic->vcpu_size = resource_size(&vcpu_res);
 
 	if (!PAGE_ALIGNED(vgic->vcpu_phys_base)) {
 		kvm_err("GICV physical address 0x%llx not page aligned\n",
-			(unsigned long long)vcpu_res.start);
+			(unsigned long long)vgic->vcpu_phys_base);
 		ret = -ENXIO;
-		goto out_unmap;
+		goto err_out;
 	}
 
 	if (!PAGE_ALIGNED(vgic->vcpu_size)) {
 		kvm_err("GICV size 0x%llx not a multiple of page size 0x%lx\n",
 			(unsigned long long)vgic->vcpu_size, PAGE_SIZE);
 		ret = -ENXIO;
-		goto out_unmap;
+		goto err_out;
 	}
 
-	vgic->can_emulate_gicv2 = true;
 	kvm_register_device_ops(&kvm_arm_vgic_v2_ops, KVM_DEV_TYPE_ARM_VGIC_V2);
-
-	kvm_info("%s@%llx IRQ%d\n", vgic_node->name,
-		 vgic->vctrl_phys_base, vgic->maint_irq);
-
+	vgic->can_emulate_gicv2 = true;
 	vgic->type = VGIC_V2;
 	vgic->max_gic_vcpus = VGIC_V2_MAX_CPUS;
-	goto out;
 
-out_unmap:
-	iounmap(vgic->vctrl_base);
-out:
-	of_node_put(vgic_node);
-	return ret;
-}
-
-/**
- * vgic_v2_probe - probe for a GICv2 compatible interrupt controller
- * @ops:	address of a pointer to the GICv2 operations
- * @params:	address of a pointer to HW-specific parameters
- *
- * Returns 0 if a GICv2 has been found, with the low level operations
- * in *ops and the HW parameters in *params. Returns an error code
- * otherwise.
- */
-int vgic_v2_probe(const struct vgic_ops **ops,
-		  const struct vgic_params **params)
-{
-	int ret;
-	struct vgic_params *vgic = &vgic_v2_params;
-
-	ret = vgic_v2_dt_probe(vgic);
-	if (ret)
-		goto err_out;
+	kvm_info("GICv2@%llx IRQ%d\n", vgic->vctrl_phys_base, vgic->maint_irq);
 
 	*ops = &vgic_v2_ops;
 	*params = vgic;
 
 	return 0;
 err_out:
+	if (vgic->vctrl_base)
+		iounmap(vgic->vctrl_base);
 	return ret;
 }
-- 
1.8.3.1

[PATCH V1 5/7] KVM: GICv2: Add ACPI probing function

[Wei Huang]

This patch implements ACPI probing for GICv2.

Signed-off-by: Wei Huang <wei@redhat.com>
---
 virt/kvm/arm/vgic-v2.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
index b60e73a..7de2a1f 100644
--- a/virt/kvm/arm/vgic-v2.c
+++ b/virt/kvm/arm/vgic-v2.c
@@ -23,6 +23,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
+#include <linux/acpi.h>
 
 #include <linux/irqchip/arm-gic.h>
 
@@ -226,6 +227,71 @@ out:
 	return ret;
 }
 
+#ifdef CONFIG_ACPI
+static struct acpi_madt_generic_interrupt *vgic_acpi;
+static void gic_get_acpi_header(struct acpi_subtable_header *header)
+{
+	vgic_acpi = (struct acpi_madt_generic_interrupt *)header;
+}
+
+static struct acpi_madt_generic_distributor *dist_acpi;
+static void gic_get_dist_header(struct acpi_subtable_header *header)
+{
+	dist_acpi = (struct acpi_madt_generic_distributor *)header;
+}
+
+static int vgic_v2_acpi_probe(struct vgic_params *vgic)
+{
+	int irq_mode, ret = 0;
+
+	ret = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
+			(acpi_tbl_entry_handler)gic_get_acpi_header, 0);
+	if (!ret) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	ret = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
+			(acpi_tbl_entry_handler)gic_get_dist_header, 0);
+	if (!ret) {
+		kvm_err("Cannot get distributor entry from ACPI\n");
+		ret = -ENODEV;
+		goto out;
+	}
+
+	if (dist_acpi->version >= ACPI_MADT_GIC_VERSION_V3) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	irq_mode = (vgic_acpi->flags & ACPI_MADT_VGIC_IRQ_MODE) ?
+		ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
+	/* According to GIC-400 manual, PPIs are active-LOW, level-sensative.
+	 * We register IRQ as active-low.
+	 */
+	vgic->maint_irq = acpi_register_gsi(NULL, vgic_acpi->vgic_interrupt,
+					    irq_mode, ACPI_ACTIVE_LOW);
+	if (!vgic->maint_irq) {
+		kvm_err("Cannot register vgic maintenance irq from ACPI\n");
+		ret = -ENXIO;
+		goto out;
+	}
+
+	vgic->vctrl_phys_base = vgic_acpi->gich_base_address;
+	vgic->vctrl_size = SZ_8K;
+
+	vgic->vcpu_phys_base = vgic_acpi->gicv_base_address;
+	vgic->vcpu_size = 0; /* unavailable in ACPI, set to 0 */
+out:
+	return ret;
+}
+#else
+static inline int vgic_v2_acpi_probe(struct vgic_params *vgic)
+{
+	return -ENODEV;
+}
+#endif /* CONFIG_ACPI */
+
 /**
  * vgic_v2_probe - probe for a GICv2 compatible interrupt controller
  * @ops:	address of a pointer to the GICv2 operations
@@ -242,6 +308,8 @@ int vgic_v2_probe(const struct vgic_ops **ops,
 	struct vgic_params *vgic = &vgic_v2_params;
 
 	ret = vgic_v2_dt_probe(vgic);
+	if (ret && !acpi_disabled)
+		ret = vgic_v2_acpi_probe(vgic);
 	if (ret)
 		goto err_out;
 
-- 
1.8.3.1

[PATCH V1 6/7] KVM: GICv3: Extract the common code from DT

[Wei Huang]

In preparation for ACPI probing, this patch extracts the DT-neutral
code into vgic_v3_probe(). DT function nows fills out the following
info in *vgic:
  - maint_irq (mapped)
  - GICv resources

Signed-off-by: Wei Huang <wei@redhat.com>
---
 virt/kvm/arm/vgic-v3.c | 75 ++++++++++++++++++++++++++++----------------------
 1 file changed, 42 insertions(+), 33 deletions(-)

diff --git a/virt/kvm/arm/vgic-v3.c b/virt/kvm/arm/vgic-v3.c
index b036134..5eca58a 100644
--- a/virt/kvm/arm/vgic-v3.c
+++ b/virt/kvm/arm/vgic-v3.c
@@ -239,20 +239,11 @@ static int vgic_v3_dt_probe(struct vgic_params *vgic)
 
 	vgic->maint_irq = irq_of_parse_and_map(vgic_node, 0);
 	if (!vgic->maint_irq) {
-		kvm_err("error getting vgic maintenance irq from DT\n");
+		kvm_err("Cannot getting vgic maintenance irq from DT\n");
 		ret = -ENXIO;
 		goto out;
 	}
 
-	ich_vtr_el2 = kvm_call_hyp(__vgic_v3_get_ich_vtr_el2);
-
-	/*
-	 * The ListRegs field is 5 bits, but there is a architectural
-	 * maximum of 16 list registers. Just ignore bit 4...
-	 */
-	vgic->nr_lr = (ich_vtr_el2 & 0xf) + 1;
-	vgic->can_emulate_gicv2 = false;
-
 	if (of_property_read_u32(vgic_node, "#redistributor-regions", &gicv_idx))
 		gicv_idx = 1;
 
@@ -260,35 +251,15 @@ static int vgic_v3_dt_probe(struct vgic_params *vgic)
 	if (of_address_to_resource(vgic_node, gicv_idx, &vcpu_res)) {
 		kvm_info("GICv3: no GICV resource entry\n");
 		vgic->vcpu_phys_base = 0;
-	} else if (!PAGE_ALIGNED(vcpu_res.start)) {
-		pr_warn("GICV physical address 0x%llx not page aligned\n",
-			(unsigned long long)vcpu_res.start);
-		vgic->vcpu_phys_base = 0;
-	} else if (!PAGE_ALIGNED(resource_size(&vcpu_res))) {
-		pr_warn("GICV size 0x%llx not a multiple of page size 0x%lx\n",
-			(unsigned long long)resource_size(&vcpu_res),
-			PAGE_SIZE);
-		vgic->vcpu_phys_base = 0;
 	} else {
 		vgic->vcpu_phys_base = vcpu_res.start;
-		vgic->can_emulate_gicv2 = true;
-		kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
-					KVM_DEV_TYPE_ARM_VGIC_V2);
+		vgic->vcpu_size = resource_size(&vcpu_res);
 	}
-	if (vgic->vcpu_phys_base == 0)
-		kvm_info("disabling GICv2 emulation\n");
-	kvm_register_device_ops(&kvm_arm_vgic_v3_ops, KVM_DEV_TYPE_ARM_VGIC_V3);
-
-	vgic->vctrl_base = NULL;
-	vgic->type = VGIC_V3;
-	vgic->max_gic_vcpus = VGIC_V3_MAX_CPUS;
-
-	kvm_info("%s@%llx IRQ%d\n", vgic_node->name,
-		 vcpu_res.start, vgic->maint_irq);
 out:
 	of_node_put(vgic_node);
 	return ret;
 }
+
 /**
  * vgic_v3_probe - probe for a GICv3 compatible interrupt controller
  * @ops:	address of a pointer to the GICv3 operations
@@ -304,10 +275,48 @@ int vgic_v3_probe(const struct vgic_ops **ops,
 	int ret = 0;
 	struct vgic_params *vgic = &vgic_v3_params;
 
+	/* DT probing first, then try ACPI probing */
 	ret = vgic_v3_dt_probe(vgic);
+	if (ret)
+		goto out;
+
+	ich_vtr_el2 = kvm_call_hyp(__vgic_v3_get_ich_vtr_el2);
+
+	/*
+	 * The ListRegs field is 5 bits, but there is a architectural
+	 * maximum of 16 list registers. Just ignore bit 4...
+	 */
+	vgic->nr_lr = (ich_vtr_el2 & 0xf) + 1;
+
+	if (!PAGE_ALIGNED(vgic->vcpu_phys_base)) {
+		pr_warn("GICV physical address 0x%llx not page aligned\n",
+			(unsigned long long)vgic->vcpu_phys_base);
+		vgic->vcpu_phys_base = 0;
+	} else if (!PAGE_ALIGNED(vgic->vcpu_size)) {
+		pr_warn("GICV size 0x%llx not a multiple of page size 0x%lx\n",
+			(unsigned long long)vgic->vcpu_size, PAGE_SIZE);
+		vgic->vcpu_phys_base = 0;
+	};
+
+	if (vgic->vcpu_phys_base != 0) {
+		vgic->can_emulate_gicv2 = true;
+		kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
+					KVM_DEV_TYPE_ARM_VGIC_V2);
+	} else {
+		vgic->can_emulate_gicv2 = false;
+		kvm_info("disabling GICv2 emulation\n");
+	}
+
+	kvm_register_device_ops(&kvm_arm_vgic_v3_ops, KVM_DEV_TYPE_ARM_VGIC_V3);
+
+	vgic->vctrl_base = NULL;
+	vgic->type = VGIC_V3;
+	vgic->max_gic_vcpus = VGIC_V3_MAX_CPUS;
+
+	kvm_info("GICv3@%llx IRQ%d\n", vgic->vcpu_phys_base, vgic->maint_irq);
 
 	*ops = &vgic_v3_ops;
 	*params = vgic;
-
+out:
 	return ret;
 }
-- 
1.8.3.1

[PATCH V1 7/7] KVM: GICv3: Add ACPI probing function

[Wei Huang]

This patch implements ACPI probing for GICv3.

Signed-off-by: Wei Huang <wei@redhat.com>
---
 virt/kvm/arm/vgic-v3.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 62 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic-v3.c b/virt/kvm/arm/vgic-v3.c
index 5eca58a..5bfb9cb 100644
--- a/virt/kvm/arm/vgic-v3.c
+++ b/virt/kvm/arm/vgic-v3.c
@@ -23,6 +23,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
+#include <linux/acpi.h>
 
 #include <linux/irqchip/arm-gic-v3.h>
 
@@ -260,6 +261,62 @@ out:
 	return ret;
 }
 
+#ifdef CONFIG_ACPI
+static struct acpi_madt_generic_interrupt *vgic_acpi;
+static void gic_v3_get_acpi_header(struct acpi_subtable_header *header)
+{
+	vgic_acpi = (struct acpi_madt_generic_interrupt *)header;
+}
+
+static struct acpi_madt_generic_distributor *dist_acpi;
+static void gic_v3_get_dist_header(struct acpi_subtable_header *header)
+{
+	dist_acpi = (struct acpi_madt_generic_distributor *)header;
+}
+
+static int vgic_v3_acpi_probe(struct vgic_params *vgic)
+{
+	int irq_mode;
+	int count = 0;
+	int ret = 0;
+
+	count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
+			(acpi_tbl_entry_handler)gic_v3_get_acpi_header, 0);
+	if (!count) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
+			(acpi_tbl_entry_handler)gic_v3_get_dist_header, 0);
+	if (!count || (dist_acpi->version != ACPI_MADT_GIC_VERSION_V3)) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	/* IRQ trigger mode */
+	irq_mode = (vgic_acpi->flags & ACPI_MADT_VGIC_IRQ_MODE) ?
+		ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
+	vgic->maint_irq = acpi_register_gsi(NULL, vgic_acpi->vgic_interrupt,
+					    irq_mode, ACPI_ACTIVE_LOW);
+	if (!vgic->maint_irq) {
+		kvm_err("failed to get vgic maintenance irq from ACPI\n");
+		ret = -ENXIO;
+		goto out;
+	}
+
+	vgic->vcpu_phys_base = vgic_acpi->gicv_base_address;
+	vgic->vcpu_size = SZ_8K;
+out:
+	return ret;
+}
+#else
+static inline int vgic_v3_acpi_probe(struct vgic_params *vgic)
+{
+	return -ENODEV;
+}
+#endif /* CONFIG_ACPI */
+
 /**
  * vgic_v3_probe - probe for a GICv3 compatible interrupt controller
  * @ops:	address of a pointer to the GICv3 operations
@@ -277,8 +334,11 @@ int vgic_v3_probe(const struct vgic_ops **ops,
 
 	/* DT probing first, then try ACPI probing */
 	ret = vgic_v3_dt_probe(vgic);
-	if (ret)
-		goto out;
+	if (ret && !acpi_disabled) {
+		ret = vgic_v3_acpi_probe(vgic);
+		if (ret)
+			goto out;
+	}
 
 	ich_vtr_el2 = kvm_call_hyp(__vgic_v3_get_ich_vtr_el2);
 
-- 
1.8.3.1

Re: [PATCH V1 0/7] Enable ACPI support for ARM KVM GIC

[Marc Zyngier]

Wei,

On 05/02/16 17:07, Wei Huang wrote:
> This patch set enables ACPI support for KVM GIC. Note that the patches
> are in fact the V3 of previously submitted patches (search "Enable ACPI
> support for KVM ARM"). But because Fu Wei includes the arch_timer part
> in his series [1] and I have substantially re-written the GIC code in this
> revision, the version number is reset to v1. 
> 
> By following Marc's prior comments, the main design idea is to let DT or
> ACPI code to fill out the "struct vgic_params" which are extended to
> include all GIC related info.

I think you misread the comments I gave back in June (!), where I said:

<quote>
[...]

Simply making available a global structure containing the base addresses
and interrupt should be enough, and could be shared with both DT and
ACPI. You could start your series by letting both GIC drivers expose
that information obtained through DT, convert KVM to use this structure,
and later on let ACPI fill in this structure too.

> Anyway the difficulty is to find a common place to store and share info
> between other modules & KVM.

Indeed. As a rule of thumb, I want to minimize the amount of gratuitous
divergence between DT and ACPI. So the sooner we extract the required
information from whatever firmware we have, the better.
</quote>

> 
> [1] https://lkml.org/lkml/2016/2/1/658
> 
> Thanks,
> -Wei
> 
> Wei Huang (7):
>   KVM: GIC: Move GIC DT probing code to GICv2 and GICv3 files
>   KVM: GIC: Add extra fields to store GICH and GICV resource info
>   KVM: GIC: Create a common probe function for GIC
>   KVM: GICv2: Extract the common code from DT
>   KVM: GICv2: Add ACPI probing function
>   KVM: GICv3: Extract the common code from DT
>   KVM: GICv3: Add ACPI probing function
> 
>  include/kvm/arm_vgic.h      |  14 ++--
>  virt/kvm/arm/vgic-v2-emul.c |   4 +-
>  virt/kvm/arm/vgic-v2.c      | 186 +++++++++++++++++++++++++++++++++-----------
>  virt/kvm/arm/vgic-v3.c      | 159 ++++++++++++++++++++++++++++---------
>  virt/kvm/arm/vgic.c         |  22 +-----
>  5 files changed, 277 insertions(+), 108 deletions(-)
> 

So when I see this diffstat and the patches that follow, I cannot help
but think that we do have a disconnect here. You do add a bunch of ACPI
probing in KVM, to which I've already said no.

I want to see the probing code in the GIC drivers, exported through a
common structure that KVM can then use. That's it. Nothing else.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH V1 0/7] Enable ACPI support for ARM KVM GIC

[Wei Huang]

On 2/8/16 03:59, Marc Zyngier wrote:
> Wei,
> 
> On 05/02/16 17:07, Wei Huang wrote:
>> This patch set enables ACPI support for KVM GIC. Note that the patches
>> are in fact the V3 of previously submitted patches (search "Enable ACPI
>> support for KVM ARM"). But because Fu Wei includes the arch_timer part
>> in his series [1] and I have substantially re-written the GIC code in this
>> revision, the version number is reset to v1. 
>>
>> By following Marc's prior comments, the main design idea is to let DT or
>> ACPI code to fill out the "struct vgic_params" which are extended to
>> include all GIC related info.
> 
> I think you misread the comments I gave back in June (!), where I said:
> 
> <quote>
> [...]
> 
> Simply making available a global structure containing the base addresses
> and interrupt should be enough, and could be shared with both DT and
> ACPI. You could start your series by letting both GIC drivers expose
> that information obtained through DT, convert KVM to use this structure,
> and later on let ACPI fill in this structure too.
> 
>> Anyway the difficulty is to find a common place to store and share info
>> between other modules & KVM.
> 
> Indeed. As a rule of thumb, I want to minimize the amount of gratuitous
> divergence between DT and ACPI. So the sooner we extract the required
> information from whatever firmware we have, the better.
> </quote>
> 
>>
>> [1] https://lkml.org/lkml/2016/2/1/658
>>
>> Thanks,
>> -Wei
>>
>> Wei Huang (7):
>>   KVM: GIC: Move GIC DT probing code to GICv2 and GICv3 files
>>   KVM: GIC: Add extra fields to store GICH and GICV resource info
>>   KVM: GIC: Create a common probe function for GIC
>>   KVM: GICv2: Extract the common code from DT
>>   KVM: GICv2: Add ACPI probing function
>>   KVM: GICv3: Extract the common code from DT
>>   KVM: GICv3: Add ACPI probing function
>>
>>  include/kvm/arm_vgic.h      |  14 ++--
>>  virt/kvm/arm/vgic-v2-emul.c |   4 +-
>>  virt/kvm/arm/vgic-v2.c      | 186 +++++++++++++++++++++++++++++++++-----------
>>  virt/kvm/arm/vgic-v3.c      | 159 ++++++++++++++++++++++++++++---------
>>  virt/kvm/arm/vgic.c         |  22 +-----
>>  5 files changed, 277 insertions(+), 108 deletions(-)
>>
> 
> So when I see this diffstat and the patches that follow, I cannot help
> but think that we do have a disconnect here. You do add a bunch of ACPI
> probing in KVM, to which I've already said no.
> 
> I want to see the probing code in the GIC drivers, exported through a
> common structure that KVM can then use. That's it. Nothing else.

OK. I will create a V2 based on that idea.

-Wei

> 
> Thanks,
> 
> 	M.
> 

Re: [PATCH V1 0/7] Enable ACPI support for ARM KVM GIC

[Julien Grall]

Hi,

On 08/02/16 09:59, Marc Zyngier wrote:
> On 05/02/16 17:07, Wei Huang wrote:
>> Wei Huang (7):
>>    KVM: GIC: Move GIC DT probing code to GICv2 and GICv3 files
>>    KVM: GIC: Add extra fields to store GICH and GICV resource info
>>    KVM: GIC: Create a common probe function for GIC
>>    KVM: GICv2: Extract the common code from DT
>>    KVM: GICv2: Add ACPI probing function
>>    KVM: GICv3: Extract the common code from DT
>>    KVM: GICv3: Add ACPI probing function
>>
>>   include/kvm/arm_vgic.h      |  14 ++--
>>   virt/kvm/arm/vgic-v2-emul.c |   4 +-
>>   virt/kvm/arm/vgic-v2.c      | 186 +++++++++++++++++++++++++++++++++-----------
>>   virt/kvm/arm/vgic-v3.c      | 159 ++++++++++++++++++++++++++++---------
>>   virt/kvm/arm/vgic.c         |  22 +-----
>>   5 files changed, 277 insertions(+), 108 deletions(-)
>>
>
> So when I see this diffstat and the patches that follow, I cannot help
> but think that we do have a disconnect here. You do add a bunch of ACPI
> probing in KVM, to which I've already said no.
>
> I want to see the probing code in the GIC drivers, exported through a
> common structure that KVM can then use. That's it. Nothing else.

I've been working on a patch series which rely on the GIC and arch timer 
drivers to get the necessary information to initialize the different KVM 
components (vGIC + vtimer).

I think this achieves the goal you have in mind. I will post the series 
in a few minutes.

Regards,

-- 
Julien Grall

Re: [PATCH V1 0/7] Enable ACPI support for ARM KVM GIC

[Marc Zyngier]

On 08/02/16 16:35, Wei Huang wrote:
> 
> 
> On 2/8/16 03:59, Marc Zyngier wrote:
>> Wei,
>>
>> On 05/02/16 17:07, Wei Huang wrote:
>>> This patch set enables ACPI support for KVM GIC. Note that the patches
>>> are in fact the V3 of previously submitted patches (search "Enable ACPI
>>> support for KVM ARM"). But because Fu Wei includes the arch_timer part
>>> in his series [1] and I have substantially re-written the GIC code in this
>>> revision, the version number is reset to v1. 
>>>
>>> By following Marc's prior comments, the main design idea is to let DT or
>>> ACPI code to fill out the "struct vgic_params" which are extended to
>>> include all GIC related info.
>>
>> I think you misread the comments I gave back in June (!), where I said:
>>
>> <quote>
>> [...]
>>
>> Simply making available a global structure containing the base addresses
>> and interrupt should be enough, and could be shared with both DT and
>> ACPI. You could start your series by letting both GIC drivers expose
>> that information obtained through DT, convert KVM to use this structure,
>> and later on let ACPI fill in this structure too.
>>
>>> Anyway the difficulty is to find a common place to store and share info
>>> between other modules & KVM.
>>
>> Indeed. As a rule of thumb, I want to minimize the amount of gratuitous
>> divergence between DT and ACPI. So the sooner we extract the required
>> information from whatever firmware we have, the better.
>> </quote>
>>
>>>
>>> [1] https://lkml.org/lkml/2016/2/1/658
>>>
>>> Thanks,
>>> -Wei
>>>
>>> Wei Huang (7):
>>>   KVM: GIC: Move GIC DT probing code to GICv2 and GICv3 files
>>>   KVM: GIC: Add extra fields to store GICH and GICV resource info
>>>   KVM: GIC: Create a common probe function for GIC
>>>   KVM: GICv2: Extract the common code from DT
>>>   KVM: GICv2: Add ACPI probing function
>>>   KVM: GICv3: Extract the common code from DT
>>>   KVM: GICv3: Add ACPI probing function
>>>
>>>  include/kvm/arm_vgic.h      |  14 ++--
>>>  virt/kvm/arm/vgic-v2-emul.c |   4 +-
>>>  virt/kvm/arm/vgic-v2.c      | 186 +++++++++++++++++++++++++++++++++-----------
>>>  virt/kvm/arm/vgic-v3.c      | 159 ++++++++++++++++++++++++++++---------
>>>  virt/kvm/arm/vgic.c         |  22 +-----
>>>  5 files changed, 277 insertions(+), 108 deletions(-)
>>>
>>
>> So when I see this diffstat and the patches that follow, I cannot help
>> but think that we do have a disconnect here. You do add a bunch of ACPI
>> probing in KVM, to which I've already said no.
>>
>> I want to see the probing code in the GIC drivers, exported through a
>> common structure that KVM can then use. That's it. Nothing else.
> 
> OK. I will create a V2 based on that idea.

You may want to synchronize with Julien (on CC), which has been working
on such a thing for a while.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH V1 0/7] Enable ACPI support for ARM KVM GIC

[Wei Huang]

On 2/8/16 10:39, Julien Grall wrote:
> Hi,
> 
> On 08/02/16 09:59, Marc Zyngier wrote:
>> On 05/02/16 17:07, Wei Huang wrote:
>>> Wei Huang (7):
>>>    KVM: GIC: Move GIC DT probing code to GICv2 and GICv3 files
>>>    KVM: GIC: Add extra fields to store GICH and GICV resource info
>>>    KVM: GIC: Create a common probe function for GIC
>>>    KVM: GICv2: Extract the common code from DT
>>>    KVM: GICv2: Add ACPI probing function
>>>    KVM: GICv3: Extract the common code from DT
>>>    KVM: GICv3: Add ACPI probing function
>>>
>>>   include/kvm/arm_vgic.h      |  14 ++--
>>>   virt/kvm/arm/vgic-v2-emul.c |   4 +-
>>>   virt/kvm/arm/vgic-v2.c      | 186
>>> +++++++++++++++++++++++++++++++++-----------
>>>   virt/kvm/arm/vgic-v3.c      | 159
>>> ++++++++++++++++++++++++++++---------
>>>   virt/kvm/arm/vgic.c         |  22 +-----
>>>   5 files changed, 277 insertions(+), 108 deletions(-)
>>>
>>
>> So when I see this diffstat and the patches that follow, I cannot help
>> but think that we do have a disconnect here. You do add a bunch of ACPI
>> probing in KVM, to which I've already said no.
>>
>> I want to see the probing code in the GIC drivers, exported through a
>> common structure that KVM can then use. That's it. Nothing else.
> 
> I've been working on a patch series which rely on the GIC and arch timer
> drivers to get the necessary information to initialize the different KVM
> components (vGIC + vtimer).
> 
> I think this achieves the goal you have in mind. I will post the series
> in a few minutes.
> 

Julien,

Regarding arch_timer, there is an effort going on by Linaro. It has
impact on KVM arch_timer init. See https://lkml.org/lkml/2016/2/1/658.

-Wei

> Regards,
> 

Re: [PATCH V1 0/7] Enable ACPI support for ARM KVM GIC

[Marc Zyngier]

On 08/02/16 16:47, Wei Huang wrote:
> 
> 
> On 2/8/16 10:39, Julien Grall wrote:
>> Hi,
>>
>> On 08/02/16 09:59, Marc Zyngier wrote:
>>> On 05/02/16 17:07, Wei Huang wrote:
>>>> Wei Huang (7):
>>>>    KVM: GIC: Move GIC DT probing code to GICv2 and GICv3 files
>>>>    KVM: GIC: Add extra fields to store GICH and GICV resource info
>>>>    KVM: GIC: Create a common probe function for GIC
>>>>    KVM: GICv2: Extract the common code from DT
>>>>    KVM: GICv2: Add ACPI probing function
>>>>    KVM: GICv3: Extract the common code from DT
>>>>    KVM: GICv3: Add ACPI probing function
>>>>
>>>>   include/kvm/arm_vgic.h      |  14 ++--
>>>>   virt/kvm/arm/vgic-v2-emul.c |   4 +-
>>>>   virt/kvm/arm/vgic-v2.c      | 186
>>>> +++++++++++++++++++++++++++++++++-----------
>>>>   virt/kvm/arm/vgic-v3.c      | 159
>>>> ++++++++++++++++++++++++++++---------
>>>>   virt/kvm/arm/vgic.c         |  22 +-----
>>>>   5 files changed, 277 insertions(+), 108 deletions(-)
>>>>
>>>
>>> So when I see this diffstat and the patches that follow, I cannot help
>>> but think that we do have a disconnect here. You do add a bunch of ACPI
>>> probing in KVM, to which I've already said no.
>>>
>>> I want to see the probing code in the GIC drivers, exported through a
>>> common structure that KVM can then use. That's it. Nothing else.
>>
>> I've been working on a patch series which rely on the GIC and arch timer
>> drivers to get the necessary information to initialize the different KVM
>> components (vGIC + vtimer).
>>
>> I think this achieves the goal you have in mind. I will post the series
>> in a few minutes.
>>
> 
> Julien,
> 
> Regarding arch_timer, there is an effort going on by Linaro. It has
> impact on KVM arch_timer init. See https://lkml.org/lkml/2016/2/1/658.

The KVM changes in this series are slightly pointless, and should be
addressed the same way the GIC should be addressed:

- Collect the information in the low-level code
- Expose it through a common infrastructure.

If you have different code paths for ACPI and DT outside of the core
probing code, then you are doing it wrong. ACPI is not "different".

	M.
-- 
Jazz is not dead. It just smells funny...