All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/8] kvmtool: arm64: GICv3 guest support
@ 2015-06-05  8:37 ` Andre Przywara
  0 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-05  8:37 UTC (permalink / raw)
  To: will.deacon, marc.zyngier, penberg; +Cc: linux-arm-kernel, kvmarm, kvm

Hi,

a rework of the GICv3 support series for kvmtool.
I addressed Will's comments on the broken fallback in VGIC creation,
also changed the command line parameter to --irqchip=[gicv2,gicv3].
The default is still GICv2 emulation for the sake of reproducibility,
not sure we want to have an automatic switch-over in case GICv2
emulation is not supported by the hardware.
This is also the base for ITS support, which I will send later as
a follow-up series.

Cheers,
Andre.
-----

Since Linux 3.19 the kernel can emulate a GICv3 for KVM guests.
This allows more than 8 VCPUs in a guest and enables in-kernel irqchip
for non-backwards-compatible GICv3 implementations.

This series updates kvmtool to support this feature.
The first half of the series is mostly from Marc and supports some
newer features of the virtual GIC which we later depend on. The second
part enables support for a guest GICv3 by adding a new command line
parameter (--irqchip=).

We now use the KVM_CREATE_DEVICE interface to create a virtual GIC
and only fall back to the now legacy KVM_CREATE_IRQCHIP call if the
former is not supported by the kernel.
Also we use two new features the KVM_CREATE_DEVICE interface
introduces:
* We now set the number of actually used interrupts to avoid
  allocating too many of them without ever using them.
* We tell the kernel explicitly that we are finished with the GIC
  initialisation. This is a requirement for future VGIC versions.

The final three patches introduce virtual GICv3 support, so on
supported hardware (and given kernel support) the user can ask KVM to
emulate a GICv3, lifting the 8 VCPU limit of KVM. This is done by
specifying "--irqchip=gicv3" on the command line.
As the kernel currently only supports this on ARM64, this parameter
is valid for the arm64 kvmtool build. But as the GIC is shared in
kvmtool, I had to add the macro definitions to not break the build
on ARM.

This series goes on top of the new official stand-alone repo hosted
on Will's kernel.org git [1].
Find a branch with those patches included at my repo [2].

[1] git://git.kernel.org/pub/scm/linux/kernel/git/will/kvmtool.git
[2] git://linux-arm.org/kvmtool.git (branch gicv3/v2)
    http://www.linux-arm.org/git?p=kvmtool.git;a=log;h=refs/heads/gicv3/v2

Andre Przywara (4):
  arm: finish VGIC initialisation explicitly
  arm: prepare for instantiating different IRQ chip devices
  arm: add support for supplying GICv3 redistributor addresses
  arm: use new irqchip parameter to create different vGIC types

Marc Zyngier (4):
  AArch64: Reserve two 64k pages for GIC CPU interface
  AArch{32,64}: use KVM_CREATE_DEVICE & co to instanciate the GIC
  irq: add irq__get_nr_allocated_lines
  AArch{32,64}: dynamically configure the number of GIC interrupts

 arm/aarch32/arm-cpu.c                    |   2 +-
 arm/aarch64/arm-cpu.c                    |   2 +-
 arm/aarch64/include/kvm/kvm-arch.h       |   2 +-
 arm/gic.c                                | 202 +++++++++++++++++++++++++++++--
 arm/include/arm-common/gic.h             |   6 +-
 arm/include/arm-common/kvm-arch.h        |  18 ++-
 arm/include/arm-common/kvm-config-arch.h |   9 +-
 arm/kvm-cpu.c                            |  10 +-
 arm/kvm.c                                |   8 +-
 include/kvm/irq.h                        |   1 +
 irq.c                                    |   5 +
 11 files changed, 240 insertions(+), 25 deletions(-)

-- 
2.3.5

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH v2 0/8] kvmtool: arm64: GICv3 guest support
@ 2015-06-05  8:37 ` Andre Przywara
  0 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-05  8:37 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

a rework of the GICv3 support series for kvmtool.
I addressed Will's comments on the broken fallback in VGIC creation,
also changed the command line parameter to --irqchip=[gicv2,gicv3].
The default is still GICv2 emulation for the sake of reproducibility,
not sure we want to have an automatic switch-over in case GICv2
emulation is not supported by the hardware.
This is also the base for ITS support, which I will send later as
a follow-up series.

Cheers,
Andre.
-----

Since Linux 3.19 the kernel can emulate a GICv3 for KVM guests.
This allows more than 8 VCPUs in a guest and enables in-kernel irqchip
for non-backwards-compatible GICv3 implementations.

This series updates kvmtool to support this feature.
The first half of the series is mostly from Marc and supports some
newer features of the virtual GIC which we later depend on. The second
part enables support for a guest GICv3 by adding a new command line
parameter (--irqchip=).

We now use the KVM_CREATE_DEVICE interface to create a virtual GIC
and only fall back to the now legacy KVM_CREATE_IRQCHIP call if the
former is not supported by the kernel.
Also we use two new features the KVM_CREATE_DEVICE interface
introduces:
* We now set the number of actually used interrupts to avoid
  allocating too many of them without ever using them.
* We tell the kernel explicitly that we are finished with the GIC
  initialisation. This is a requirement for future VGIC versions.

The final three patches introduce virtual GICv3 support, so on
supported hardware (and given kernel support) the user can ask KVM to
emulate a GICv3, lifting the 8 VCPU limit of KVM. This is done by
specifying "--irqchip=gicv3" on the command line.
As the kernel currently only supports this on ARM64, this parameter
is valid for the arm64 kvmtool build. But as the GIC is shared in
kvmtool, I had to add the macro definitions to not break the build
on ARM.

This series goes on top of the new official stand-alone repo hosted
on Will's kernel.org git [1].
Find a branch with those patches included at my repo [2].

[1] git://git.kernel.org/pub/scm/linux/kernel/git/will/kvmtool.git
[2] git://linux-arm.org/kvmtool.git (branch gicv3/v2)
    http://www.linux-arm.org/git?p=kvmtool.git;a=log;h=refs/heads/gicv3/v2

Andre Przywara (4):
  arm: finish VGIC initialisation explicitly
  arm: prepare for instantiating different IRQ chip devices
  arm: add support for supplying GICv3 redistributor addresses
  arm: use new irqchip parameter to create different vGIC types

Marc Zyngier (4):
  AArch64: Reserve two 64k pages for GIC CPU interface
  AArch{32,64}: use KVM_CREATE_DEVICE & co to instanciate the GIC
  irq: add irq__get_nr_allocated_lines
  AArch{32,64}: dynamically configure the number of GIC interrupts

 arm/aarch32/arm-cpu.c                    |   2 +-
 arm/aarch64/arm-cpu.c                    |   2 +-
 arm/aarch64/include/kvm/kvm-arch.h       |   2 +-
 arm/gic.c                                | 202 +++++++++++++++++++++++++++++--
 arm/include/arm-common/gic.h             |   6 +-
 arm/include/arm-common/kvm-arch.h        |  18 ++-
 arm/include/arm-common/kvm-config-arch.h |   9 +-
 arm/kvm-cpu.c                            |  10 +-
 arm/kvm.c                                |   8 +-
 include/kvm/irq.h                        |   1 +
 irq.c                                    |   5 +
 11 files changed, 240 insertions(+), 25 deletions(-)

-- 
2.3.5

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH v2 1/8] AArch64: Reserve two 64k pages for GIC CPU interface
  2015-06-05  8:37 ` Andre Przywara
@ 2015-06-05  8:37   ` Andre Przywara
  -1 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-05  8:37 UTC (permalink / raw)
  To: will.deacon, marc.zyngier, penberg; +Cc: kvm, kvmarm, linux-arm-kernel

From: Marc Zyngier <marc.zyngier@arm.com>

On AArch64 system with a GICv2, the GICC range can be aligned
to the last 4k block of a 64k page, ending up straddling two
64k pages. In order not to conflict with the distributor mapping,
allocate two 64k pages to the CPU interface.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/aarch64/include/kvm/kvm-arch.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arm/aarch64/include/kvm/kvm-arch.h b/arm/aarch64/include/kvm/kvm-arch.h
index 2f08a26..4925736 100644
--- a/arm/aarch64/include/kvm/kvm-arch.h
+++ b/arm/aarch64/include/kvm/kvm-arch.h
@@ -2,7 +2,7 @@
 #define KVM__KVM_ARCH_H
 
 #define ARM_GIC_DIST_SIZE	0x10000
-#define ARM_GIC_CPUI_SIZE	0x10000
+#define ARM_GIC_CPUI_SIZE	0x20000
 
 #define ARM_KERN_OFFSET(kvm)	((kvm)->cfg.arch.aarch32_guest	?	\
 				0x8000				:	\
-- 
2.3.5


^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 1/8] AArch64: Reserve two 64k pages for GIC CPU interface
@ 2015-06-05  8:37   ` Andre Przywara
  0 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-05  8:37 UTC (permalink / raw)
  To: linux-arm-kernel

From: Marc Zyngier <marc.zyngier@arm.com>

On AArch64 system with a GICv2, the GICC range can be aligned
to the last 4k block of a 64k page, ending up straddling two
64k pages. In order not to conflict with the distributor mapping,
allocate two 64k pages to the CPU interface.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/aarch64/include/kvm/kvm-arch.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arm/aarch64/include/kvm/kvm-arch.h b/arm/aarch64/include/kvm/kvm-arch.h
index 2f08a26..4925736 100644
--- a/arm/aarch64/include/kvm/kvm-arch.h
+++ b/arm/aarch64/include/kvm/kvm-arch.h
@@ -2,7 +2,7 @@
 #define KVM__KVM_ARCH_H
 
 #define ARM_GIC_DIST_SIZE	0x10000
-#define ARM_GIC_CPUI_SIZE	0x10000
+#define ARM_GIC_CPUI_SIZE	0x20000
 
 #define ARM_KERN_OFFSET(kvm)	((kvm)->cfg.arch.aarch32_guest	?	\
 				0x8000				:	\
-- 
2.3.5

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 2/8] AArch{32, 64}: use KVM_CREATE_DEVICE & co to instanciate the GIC
  2015-06-05  8:37 ` Andre Przywara
@ 2015-06-05  8:37   ` Andre Przywara
  -1 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-05  8:37 UTC (permalink / raw)
  To: will.deacon, marc.zyngier, penberg; +Cc: linux-arm-kernel, kvmarm, kvm

From: Marc Zyngier <marc.zyngier@arm.com>

As of 3.14, KVM/arm supports the creation/configuration of the GIC through
a more generic device API, which is now the preferred way to do so.

Plumb the new API in, and allow the old code to be used as a fallback.

[Andre: Rename some functions on the way to differentiate between
creation and initialisation more clearly.]

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c                    | 60 ++++++++++++++++++++++++++++++++++++++------
 arm/include/arm-common/gic.h |  2 +-
 arm/kvm.c                    |  6 ++---
 3 files changed, 57 insertions(+), 11 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 5d8cbe6..ce5f7fa 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -7,7 +7,41 @@
 #include <linux/byteorder.h>
 #include <linux/kvm.h>
 
-int gic__init_irqchip(struct kvm *kvm)
+static int gic_fd = -1;
+
+static int gic__create_device(struct kvm *kvm)
+{
+	int err;
+	u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
+	u64 dist_addr = ARM_GIC_DIST_BASE;
+	struct kvm_create_device gic_device = {
+		.type	= KVM_DEV_TYPE_ARM_VGIC_V2,
+	};
+	struct kvm_device_attr cpu_if_attr = {
+		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
+		.attr	= KVM_VGIC_V2_ADDR_TYPE_CPU,
+		.addr	= (u64)(unsigned long)&cpu_if_addr,
+	};
+	struct kvm_device_attr dist_attr = {
+		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
+		.attr	= KVM_VGIC_V2_ADDR_TYPE_DIST,
+		.addr	= (u64)(unsigned long)&dist_addr,
+	};
+
+	err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &gic_device);
+	if (err)
+		return err;
+
+	gic_fd = gic_device.fd;
+
+	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
+	if (err)
+		return err;
+
+	return ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
+}
+
+static int gic__create_irqchip(struct kvm *kvm)
 {
 	int err;
 	struct kvm_arm_device_addr gic_addr[] = {
@@ -23,12 +57,6 @@ int gic__init_irqchip(struct kvm *kvm)
 		}
 	};
 
-	if (kvm->nrcpus > GIC_MAX_CPUS) {
-		pr_warning("%d CPUS greater than maximum of %d -- truncating\n",
-				kvm->nrcpus, GIC_MAX_CPUS);
-		kvm->nrcpus = GIC_MAX_CPUS;
-	}
-
 	err = ioctl(kvm->vm_fd, KVM_CREATE_IRQCHIP);
 	if (err)
 		return err;
@@ -41,6 +69,24 @@ int gic__init_irqchip(struct kvm *kvm)
 	return err;
 }
 
+int gic__create(struct kvm *kvm)
+{
+	int err;
+
+	if (kvm->nrcpus > GIC_MAX_CPUS) {
+		pr_warning("%d CPUS greater than maximum of %d -- truncating\n",
+				kvm->nrcpus, GIC_MAX_CPUS);
+		kvm->nrcpus = GIC_MAX_CPUS;
+	}
+
+	/* Try the new way first, and fallback on legacy method otherwise */
+	err = gic__create_device(kvm);
+	if (err)
+		err = gic__create_irqchip(kvm);
+
+	return err;
+}
+
 void gic__generate_fdt_nodes(void *fdt, u32 phandle)
 {
 	u64 reg_prop[] = {
diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
index 5a36f2c..44859f7 100644
--- a/arm/include/arm-common/gic.h
+++ b/arm/include/arm-common/gic.h
@@ -24,7 +24,7 @@
 struct kvm;
 
 int gic__alloc_irqnum(void);
-int gic__init_irqchip(struct kvm *kvm);
+int gic__create(struct kvm *kvm);
 void gic__generate_fdt_nodes(void *fdt, u32 phandle);
 
 #endif /* ARM_COMMON__GIC_H */
diff --git a/arm/kvm.c b/arm/kvm.c
index 58ad9fa..bcd2533 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -81,7 +81,7 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
 	madvise(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size,
 		MADV_MERGEABLE | MADV_HUGEPAGE);
 
-	/* Initialise the virtual GIC. */
-	if (gic__init_irqchip(kvm))
-		die("Failed to initialise virtual GIC");
+	/* Create the virtual GIC. */
+	if (gic__create(kvm))
+		die("Failed to create virtual GIC");
 }
-- 
2.3.5

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 2/8] AArch{32, 64}: use KVM_CREATE_DEVICE & co to instanciate the GIC
@ 2015-06-05  8:37   ` Andre Przywara
  0 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-05  8:37 UTC (permalink / raw)
  To: linux-arm-kernel

From: Marc Zyngier <marc.zyngier@arm.com>

As of 3.14, KVM/arm supports the creation/configuration of the GIC through
a more generic device API, which is now the preferred way to do so.

Plumb the new API in, and allow the old code to be used as a fallback.

[Andre: Rename some functions on the way to differentiate between
creation and initialisation more clearly.]

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c                    | 60 ++++++++++++++++++++++++++++++++++++++------
 arm/include/arm-common/gic.h |  2 +-
 arm/kvm.c                    |  6 ++---
 3 files changed, 57 insertions(+), 11 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 5d8cbe6..ce5f7fa 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -7,7 +7,41 @@
 #include <linux/byteorder.h>
 #include <linux/kvm.h>
 
-int gic__init_irqchip(struct kvm *kvm)
+static int gic_fd = -1;
+
+static int gic__create_device(struct kvm *kvm)
+{
+	int err;
+	u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
+	u64 dist_addr = ARM_GIC_DIST_BASE;
+	struct kvm_create_device gic_device = {
+		.type	= KVM_DEV_TYPE_ARM_VGIC_V2,
+	};
+	struct kvm_device_attr cpu_if_attr = {
+		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
+		.attr	= KVM_VGIC_V2_ADDR_TYPE_CPU,
+		.addr	= (u64)(unsigned long)&cpu_if_addr,
+	};
+	struct kvm_device_attr dist_attr = {
+		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
+		.attr	= KVM_VGIC_V2_ADDR_TYPE_DIST,
+		.addr	= (u64)(unsigned long)&dist_addr,
+	};
+
+	err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &gic_device);
+	if (err)
+		return err;
+
+	gic_fd = gic_device.fd;
+
+	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
+	if (err)
+		return err;
+
+	return ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
+}
+
+static int gic__create_irqchip(struct kvm *kvm)
 {
 	int err;
 	struct kvm_arm_device_addr gic_addr[] = {
@@ -23,12 +57,6 @@ int gic__init_irqchip(struct kvm *kvm)
 		}
 	};
 
-	if (kvm->nrcpus > GIC_MAX_CPUS) {
-		pr_warning("%d CPUS greater than maximum of %d -- truncating\n",
-				kvm->nrcpus, GIC_MAX_CPUS);
-		kvm->nrcpus = GIC_MAX_CPUS;
-	}
-
 	err = ioctl(kvm->vm_fd, KVM_CREATE_IRQCHIP);
 	if (err)
 		return err;
@@ -41,6 +69,24 @@ int gic__init_irqchip(struct kvm *kvm)
 	return err;
 }
 
+int gic__create(struct kvm *kvm)
+{
+	int err;
+
+	if (kvm->nrcpus > GIC_MAX_CPUS) {
+		pr_warning("%d CPUS greater than maximum of %d -- truncating\n",
+				kvm->nrcpus, GIC_MAX_CPUS);
+		kvm->nrcpus = GIC_MAX_CPUS;
+	}
+
+	/* Try the new way first, and fallback on legacy method otherwise */
+	err = gic__create_device(kvm);
+	if (err)
+		err = gic__create_irqchip(kvm);
+
+	return err;
+}
+
 void gic__generate_fdt_nodes(void *fdt, u32 phandle)
 {
 	u64 reg_prop[] = {
diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
index 5a36f2c..44859f7 100644
--- a/arm/include/arm-common/gic.h
+++ b/arm/include/arm-common/gic.h
@@ -24,7 +24,7 @@
 struct kvm;
 
 int gic__alloc_irqnum(void);
-int gic__init_irqchip(struct kvm *kvm);
+int gic__create(struct kvm *kvm);
 void gic__generate_fdt_nodes(void *fdt, u32 phandle);
 
 #endif /* ARM_COMMON__GIC_H */
diff --git a/arm/kvm.c b/arm/kvm.c
index 58ad9fa..bcd2533 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -81,7 +81,7 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
 	madvise(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size,
 		MADV_MERGEABLE | MADV_HUGEPAGE);
 
-	/* Initialise the virtual GIC. */
-	if (gic__init_irqchip(kvm))
-		die("Failed to initialise virtual GIC");
+	/* Create the virtual GIC. */
+	if (gic__create(kvm))
+		die("Failed to create virtual GIC");
 }
-- 
2.3.5

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 3/8] irq: add irq__get_nr_allocated_lines
  2015-06-05  8:37 ` Andre Przywara
@ 2015-06-05  8:37   ` Andre Przywara
  -1 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-05  8:37 UTC (permalink / raw)
  To: will.deacon, marc.zyngier, penberg; +Cc: kvm, kvmarm, linux-arm-kernel

From: Marc Zyngier <marc.zyngier@arm.com>

The ARM GIC emulation needs to be told the number of interrupts
it has to support. As commit 1c262fa1dc7bc ("kvm tools: irq: make
irq__alloc_line generic") made the interrupt counter private,
add a new accessor returning the number of interrupt lines we've
allocated so far.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/kvm/irq.h | 1 +
 irq.c             | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/include/kvm/irq.h b/include/kvm/irq.h
index 4cec6f0..8a78e43 100644
--- a/include/kvm/irq.h
+++ b/include/kvm/irq.h
@@ -11,6 +11,7 @@
 struct kvm;
 
 int irq__alloc_line(void);
+int irq__get_nr_allocated_lines(void);
 
 int irq__init(struct kvm *kvm);
 int irq__exit(struct kvm *kvm);
diff --git a/irq.c b/irq.c
index 33ea8d2..71eaa05 100644
--- a/irq.c
+++ b/irq.c
@@ -7,3 +7,8 @@ int irq__alloc_line(void)
 {
 	return next_line++;
 }
+
+int irq__get_nr_allocated_lines(void)
+{
+	return next_line - KVM_IRQ_OFFSET;
+}
-- 
2.3.5


^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 3/8] irq: add irq__get_nr_allocated_lines
@ 2015-06-05  8:37   ` Andre Przywara
  0 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-05  8:37 UTC (permalink / raw)
  To: linux-arm-kernel

From: Marc Zyngier <marc.zyngier@arm.com>

The ARM GIC emulation needs to be told the number of interrupts
it has to support. As commit 1c262fa1dc7bc ("kvm tools: irq: make
irq__alloc_line generic") made the interrupt counter private,
add a new accessor returning the number of interrupt lines we've
allocated so far.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/kvm/irq.h | 1 +
 irq.c             | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/include/kvm/irq.h b/include/kvm/irq.h
index 4cec6f0..8a78e43 100644
--- a/include/kvm/irq.h
+++ b/include/kvm/irq.h
@@ -11,6 +11,7 @@
 struct kvm;
 
 int irq__alloc_line(void);
+int irq__get_nr_allocated_lines(void);
 
 int irq__init(struct kvm *kvm);
 int irq__exit(struct kvm *kvm);
diff --git a/irq.c b/irq.c
index 33ea8d2..71eaa05 100644
--- a/irq.c
+++ b/irq.c
@@ -7,3 +7,8 @@ int irq__alloc_line(void)
 {
 	return next_line++;
 }
+
+int irq__get_nr_allocated_lines(void)
+{
+	return next_line - KVM_IRQ_OFFSET;
+}
-- 
2.3.5

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 4/8] AArch{32,64}: dynamically configure the number of GIC interrupts
  2015-06-05  8:37 ` Andre Przywara
@ 2015-06-05  8:37   ` Andre Przywara
  -1 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-05  8:37 UTC (permalink / raw)
  To: will.deacon, marc.zyngier, penberg; +Cc: kvm, kvmarm, linux-arm-kernel

From: Marc Zyngier <marc.zyngier@arm.com>

In order to reduce the memory usage of large guests (as well
as improve performance), tell KVM about the number of interrupts
we require.

To avoid synchronization with the various device creation,
use a late_init callback to compute the GIC configuration.
[Andre: rename to gic__init_gic() to ease future expansion]

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arm/gic.c b/arm/gic.c
index ce5f7fa..6277af8 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -1,10 +1,12 @@
 #include "kvm/fdt.h"
+#include "kvm/irq.h"
 #include "kvm/kvm.h"
 #include "kvm/virtio.h"
 
 #include "arm-common/gic.h"
 
 #include <linux/byteorder.h>
+#include <linux/kernel.h>
 #include <linux/kvm.h>
 
 static int gic_fd = -1;
@@ -87,6 +89,29 @@ int gic__create(struct kvm *kvm)
 	return err;
 }
 
+static int gic__init_gic(struct kvm *kvm)
+{
+	int lines = irq__get_nr_allocated_lines();
+	u32 nr_irqs = ALIGN(lines, 32) + GIC_SPI_IRQ_BASE;
+	struct kvm_device_attr nr_irqs_attr = {
+		.group	= KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
+		.addr	= (u64)(unsigned long)&nr_irqs,
+	};
+
+	/*
+	 * If we didn't use the KVM_CREATE_DEVICE method, KVM will
+	 * give us some default number of interrupts.
+	 */
+	if (gic_fd < 0)
+		return 0;
+
+	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &nr_irqs_attr))
+		return ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &nr_irqs_attr);
+
+	return 0;
+}
+late_init(gic__init_gic)
+
 void gic__generate_fdt_nodes(void *fdt, u32 phandle)
 {
 	u64 reg_prop[] = {
-- 
2.3.5


^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 4/8] AArch{32, 64}: dynamically configure the number of GIC interrupts
@ 2015-06-05  8:37   ` Andre Przywara
  0 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-05  8:37 UTC (permalink / raw)
  To: linux-arm-kernel

From: Marc Zyngier <marc.zyngier@arm.com>

In order to reduce the memory usage of large guests (as well
as improve performance), tell KVM about the number of interrupts
we require.

To avoid synchronization with the various device creation,
use a late_init callback to compute the GIC configuration.
[Andre: rename to gic__init_gic() to ease future expansion]

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arm/gic.c b/arm/gic.c
index ce5f7fa..6277af8 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -1,10 +1,12 @@
 #include "kvm/fdt.h"
+#include "kvm/irq.h"
 #include "kvm/kvm.h"
 #include "kvm/virtio.h"
 
 #include "arm-common/gic.h"
 
 #include <linux/byteorder.h>
+#include <linux/kernel.h>
 #include <linux/kvm.h>
 
 static int gic_fd = -1;
@@ -87,6 +89,29 @@ int gic__create(struct kvm *kvm)
 	return err;
 }
 
+static int gic__init_gic(struct kvm *kvm)
+{
+	int lines = irq__get_nr_allocated_lines();
+	u32 nr_irqs = ALIGN(lines, 32) + GIC_SPI_IRQ_BASE;
+	struct kvm_device_attr nr_irqs_attr = {
+		.group	= KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
+		.addr	= (u64)(unsigned long)&nr_irqs,
+	};
+
+	/*
+	 * If we didn't use the KVM_CREATE_DEVICE method, KVM will
+	 * give us some default number of interrupts.
+	 */
+	if (gic_fd < 0)
+		return 0;
+
+	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &nr_irqs_attr))
+		return ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &nr_irqs_attr);
+
+	return 0;
+}
+late_init(gic__init_gic)
+
 void gic__generate_fdt_nodes(void *fdt, u32 phandle)
 {
 	u64 reg_prop[] = {
-- 
2.3.5

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 5/8] arm: finish VGIC initialisation explicitly
  2015-06-05  8:37 ` Andre Przywara
@ 2015-06-05  8:37   ` Andre Przywara
  -1 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-05  8:37 UTC (permalink / raw)
  To: will.deacon, marc.zyngier, penberg; +Cc: kvm, kvmarm, linux-arm-kernel

Since Linux 3.19-rc1 there is a new API to explicitly initialise
the in-kernel GIC emulation by a userland KVM device call.
Use that to tell the kernel we are finished with the GIC
initialisation, since the automatic GIC init will only be provided
as a legacy functionality in the future.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 6277af8..8d47562 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -89,24 +89,43 @@ int gic__create(struct kvm *kvm)
 	return err;
 }
 
+/*
+ * Sets the number of used interrupts and finalizes the GIC init explicitly.
+ */
 static int gic__init_gic(struct kvm *kvm)
 {
+	int ret;
+
 	int lines = irq__get_nr_allocated_lines();
 	u32 nr_irqs = ALIGN(lines, 32) + GIC_SPI_IRQ_BASE;
 	struct kvm_device_attr nr_irqs_attr = {
 		.group	= KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
 		.addr	= (u64)(unsigned long)&nr_irqs,
 	};
+	struct kvm_device_attr vgic_init_attr = {
+		.group	= KVM_DEV_ARM_VGIC_GRP_CTRL,
+		.attr	= KVM_DEV_ARM_VGIC_CTRL_INIT,
+	};
 
 	/*
 	 * If we didn't use the KVM_CREATE_DEVICE method, KVM will
-	 * give us some default number of interrupts.
+	 * give us some default number of interrupts. The GIC initialization
+	 * will be done automatically in this case.
 	 */
 	if (gic_fd < 0)
 		return 0;
 
-	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &nr_irqs_attr))
-		return ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &nr_irqs_attr);
+	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &nr_irqs_attr)) {
+		ret = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &nr_irqs_attr);
+		if (ret)
+			return ret;
+	}
+
+	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &vgic_init_attr)) {
+		ret = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &vgic_init_attr);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
-- 
2.3.5


^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 5/8] arm: finish VGIC initialisation explicitly
@ 2015-06-05  8:37   ` Andre Przywara
  0 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-05  8:37 UTC (permalink / raw)
  To: linux-arm-kernel

Since Linux 3.19-rc1 there is a new API to explicitly initialise
the in-kernel GIC emulation by a userland KVM device call.
Use that to tell the kernel we are finished with the GIC
initialisation, since the automatic GIC init will only be provided
as a legacy functionality in the future.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 6277af8..8d47562 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -89,24 +89,43 @@ int gic__create(struct kvm *kvm)
 	return err;
 }
 
+/*
+ * Sets the number of used interrupts and finalizes the GIC init explicitly.
+ */
 static int gic__init_gic(struct kvm *kvm)
 {
+	int ret;
+
 	int lines = irq__get_nr_allocated_lines();
 	u32 nr_irqs = ALIGN(lines, 32) + GIC_SPI_IRQ_BASE;
 	struct kvm_device_attr nr_irqs_attr = {
 		.group	= KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
 		.addr	= (u64)(unsigned long)&nr_irqs,
 	};
+	struct kvm_device_attr vgic_init_attr = {
+		.group	= KVM_DEV_ARM_VGIC_GRP_CTRL,
+		.attr	= KVM_DEV_ARM_VGIC_CTRL_INIT,
+	};
 
 	/*
 	 * If we didn't use the KVM_CREATE_DEVICE method, KVM will
-	 * give us some default number of interrupts.
+	 * give us some default number of interrupts. The GIC initialization
+	 * will be done automatically in this case.
 	 */
 	if (gic_fd < 0)
 		return 0;
 
-	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &nr_irqs_attr))
-		return ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &nr_irqs_attr);
+	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &nr_irqs_attr)) {
+		ret = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &nr_irqs_attr);
+		if (ret)
+			return ret;
+	}
+
+	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &vgic_init_attr)) {
+		ret = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &vgic_init_attr);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
-- 
2.3.5

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 6/8] arm: prepare for instantiating different IRQ chip devices
  2015-06-05  8:37 ` Andre Przywara
@ 2015-06-05  8:37   ` Andre Przywara
  -1 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-05  8:37 UTC (permalink / raw)
  To: will.deacon, marc.zyngier, penberg; +Cc: linux-arm-kernel, kvmarm, kvm

Extend the vGIC handling code to potentially deal with different IRQ
chip devices instead of hard-coding the GICv2 in.
We extend most vGIC functions to take a type parameter, but still put
GICv2 in at the top for the time being.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/aarch32/arm-cpu.c        |  2 +-
 arm/aarch64/arm-cpu.c        |  2 +-
 arm/gic.c                    | 66 ++++++++++++++++++++++++++++++++++----------
 arm/include/arm-common/gic.h |  6 ++--
 arm/kvm.c                    |  2 +-
 5 files changed, 58 insertions(+), 20 deletions(-)

diff --git a/arm/aarch32/arm-cpu.c b/arm/aarch32/arm-cpu.c
index 946e443..d8d6293 100644
--- a/arm/aarch32/arm-cpu.c
+++ b/arm/aarch32/arm-cpu.c
@@ -12,7 +12,7 @@ static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
 {
 	int timer_interrupts[4] = {13, 14, 11, 10};
 
-	gic__generate_fdt_nodes(fdt, gic_phandle);
+	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
 	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
 }
 
diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
index 8efe877..f702b9e 100644
--- a/arm/aarch64/arm-cpu.c
+++ b/arm/aarch64/arm-cpu.c
@@ -12,7 +12,7 @@
 static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
 {
 	int timer_interrupts[4] = {13, 14, 11, 10};
-	gic__generate_fdt_nodes(fdt, gic_phandle);
+	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
 	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
 }
 
diff --git a/arm/gic.c b/arm/gic.c
index 8d47562..0ce40e4 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -11,13 +11,13 @@
 
 static int gic_fd = -1;
 
-static int gic__create_device(struct kvm *kvm)
+static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 {
 	int err;
 	u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
 	u64 dist_addr = ARM_GIC_DIST_BASE;
 	struct kvm_create_device gic_device = {
-		.type	= KVM_DEV_TYPE_ARM_VGIC_V2,
+		.flags	= 0,
 	};
 	struct kvm_device_attr cpu_if_attr = {
 		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
@@ -26,21 +26,37 @@ static int gic__create_device(struct kvm *kvm)
 	};
 	struct kvm_device_attr dist_attr = {
 		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
-		.attr	= KVM_VGIC_V2_ADDR_TYPE_DIST,
 		.addr	= (u64)(unsigned long)&dist_addr,
 	};
 
+	switch (type) {
+	case IRQCHIP_GICV2:
+		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
+		break;
+	default:
+		return -ENODEV;
+	}
+
 	err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &gic_device);
 	if (err)
 		return err;
 
 	gic_fd = gic_device.fd;
 
-	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
+	switch (type) {
+	case IRQCHIP_GICV2:
+		dist_attr.attr = KVM_VGIC_V2_ADDR_TYPE_DIST;
+		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
+		break;
+	default:
+		return -ENODEV;
+	}
 	if (err)
 		return err;
 
-	return ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
+	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
+
+	return err;
 }
 
 static int gic__create_irqchip(struct kvm *kvm)
@@ -71,19 +87,28 @@ static int gic__create_irqchip(struct kvm *kvm)
 	return err;
 }
 
-int gic__create(struct kvm *kvm)
+int gic__create(struct kvm *kvm, enum irqchip_type type)
 {
+	int max_cpus;
 	int err;
 
-	if (kvm->nrcpus > GIC_MAX_CPUS) {
+	switch (type) {
+	case IRQCHIP_GICV2:
+		max_cpus = GIC_MAX_CPUS;
+		break;
+	default:
+		return -ENODEV;
+	}
+
+	if (kvm->nrcpus > max_cpus) {
 		pr_warning("%d CPUS greater than maximum of %d -- truncating\n",
-				kvm->nrcpus, GIC_MAX_CPUS);
-		kvm->nrcpus = GIC_MAX_CPUS;
+				kvm->nrcpus, max_cpus);
+		kvm->nrcpus = max_cpus;
 	}
 
 	/* Try the new way first, and fallback on legacy method otherwise */
-	err = gic__create_device(kvm);
-	if (err)
+	err = gic__create_device(kvm, type);
+	if (err && type == IRQCHIP_GICV2)
 		err = gic__create_irqchip(kvm);
 
 	return err;
@@ -131,15 +156,26 @@ static int gic__init_gic(struct kvm *kvm)
 }
 late_init(gic__init_gic)
 
-void gic__generate_fdt_nodes(void *fdt, u32 phandle)
+void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
 {
+	const char *compatible;
 	u64 reg_prop[] = {
-		cpu_to_fdt64(ARM_GIC_DIST_BASE), cpu_to_fdt64(ARM_GIC_DIST_SIZE),
-		cpu_to_fdt64(ARM_GIC_CPUI_BASE), cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
+		cpu_to_fdt64(ARM_GIC_DIST_BASE),
+		cpu_to_fdt64(ARM_GIC_DIST_SIZE),
+		cpu_to_fdt64(ARM_GIC_CPUI_BASE),
+		cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
 	};
 
+	switch (type) {
+	case IRQCHIP_GICV2:
+		compatible = "arm,cortex-a15-gic";
+		break;
+	default:
+		return;
+	}
+
 	_FDT(fdt_begin_node(fdt, "intc"));
-	_FDT(fdt_property_string(fdt, "compatible", "arm,cortex-a15-gic"));
+	_FDT(fdt_property_string(fdt, "compatible", compatible));
 	_FDT(fdt_property_cell(fdt, "#interrupt-cells", GIC_FDT_IRQ_NUM_CELLS));
 	_FDT(fdt_property(fdt, "interrupt-controller", NULL, 0));
 	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
index 44859f7..f5f6707 100644
--- a/arm/include/arm-common/gic.h
+++ b/arm/include/arm-common/gic.h
@@ -21,10 +21,12 @@
 #define GIC_MAX_CPUS			8
 #define GIC_MAX_IRQ			255
 
+enum irqchip_type {IRQCHIP_DEFAULT, IRQCHIP_GICV2};
+
 struct kvm;
 
 int gic__alloc_irqnum(void);
-int gic__create(struct kvm *kvm);
-void gic__generate_fdt_nodes(void *fdt, u32 phandle);
+int gic__create(struct kvm *kvm, enum irqchip_type type);
+void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type);
 
 #endif /* ARM_COMMON__GIC_H */
diff --git a/arm/kvm.c b/arm/kvm.c
index bcd2533..f9685c2 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -82,6 +82,6 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
 		MADV_MERGEABLE | MADV_HUGEPAGE);
 
 	/* Create the virtual GIC. */
-	if (gic__create(kvm))
+	if (gic__create(kvm, IRQCHIP_GICV2))
 		die("Failed to create virtual GIC");
 }
-- 
2.3.5

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 6/8] arm: prepare for instantiating different IRQ chip devices
@ 2015-06-05  8:37   ` Andre Przywara
  0 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-05  8:37 UTC (permalink / raw)
  To: linux-arm-kernel

Extend the vGIC handling code to potentially deal with different IRQ
chip devices instead of hard-coding the GICv2 in.
We extend most vGIC functions to take a type parameter, but still put
GICv2 in at the top for the time being.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/aarch32/arm-cpu.c        |  2 +-
 arm/aarch64/arm-cpu.c        |  2 +-
 arm/gic.c                    | 66 ++++++++++++++++++++++++++++++++++----------
 arm/include/arm-common/gic.h |  6 ++--
 arm/kvm.c                    |  2 +-
 5 files changed, 58 insertions(+), 20 deletions(-)

diff --git a/arm/aarch32/arm-cpu.c b/arm/aarch32/arm-cpu.c
index 946e443..d8d6293 100644
--- a/arm/aarch32/arm-cpu.c
+++ b/arm/aarch32/arm-cpu.c
@@ -12,7 +12,7 @@ static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
 {
 	int timer_interrupts[4] = {13, 14, 11, 10};
 
-	gic__generate_fdt_nodes(fdt, gic_phandle);
+	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
 	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
 }
 
diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
index 8efe877..f702b9e 100644
--- a/arm/aarch64/arm-cpu.c
+++ b/arm/aarch64/arm-cpu.c
@@ -12,7 +12,7 @@
 static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
 {
 	int timer_interrupts[4] = {13, 14, 11, 10};
-	gic__generate_fdt_nodes(fdt, gic_phandle);
+	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
 	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
 }
 
diff --git a/arm/gic.c b/arm/gic.c
index 8d47562..0ce40e4 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -11,13 +11,13 @@
 
 static int gic_fd = -1;
 
-static int gic__create_device(struct kvm *kvm)
+static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 {
 	int err;
 	u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
 	u64 dist_addr = ARM_GIC_DIST_BASE;
 	struct kvm_create_device gic_device = {
-		.type	= KVM_DEV_TYPE_ARM_VGIC_V2,
+		.flags	= 0,
 	};
 	struct kvm_device_attr cpu_if_attr = {
 		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
@@ -26,21 +26,37 @@ static int gic__create_device(struct kvm *kvm)
 	};
 	struct kvm_device_attr dist_attr = {
 		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
-		.attr	= KVM_VGIC_V2_ADDR_TYPE_DIST,
 		.addr	= (u64)(unsigned long)&dist_addr,
 	};
 
+	switch (type) {
+	case IRQCHIP_GICV2:
+		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
+		break;
+	default:
+		return -ENODEV;
+	}
+
 	err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &gic_device);
 	if (err)
 		return err;
 
 	gic_fd = gic_device.fd;
 
-	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
+	switch (type) {
+	case IRQCHIP_GICV2:
+		dist_attr.attr = KVM_VGIC_V2_ADDR_TYPE_DIST;
+		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
+		break;
+	default:
+		return -ENODEV;
+	}
 	if (err)
 		return err;
 
-	return ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
+	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
+
+	return err;
 }
 
 static int gic__create_irqchip(struct kvm *kvm)
@@ -71,19 +87,28 @@ static int gic__create_irqchip(struct kvm *kvm)
 	return err;
 }
 
-int gic__create(struct kvm *kvm)
+int gic__create(struct kvm *kvm, enum irqchip_type type)
 {
+	int max_cpus;
 	int err;
 
-	if (kvm->nrcpus > GIC_MAX_CPUS) {
+	switch (type) {
+	case IRQCHIP_GICV2:
+		max_cpus = GIC_MAX_CPUS;
+		break;
+	default:
+		return -ENODEV;
+	}
+
+	if (kvm->nrcpus > max_cpus) {
 		pr_warning("%d CPUS greater than maximum of %d -- truncating\n",
-				kvm->nrcpus, GIC_MAX_CPUS);
-		kvm->nrcpus = GIC_MAX_CPUS;
+				kvm->nrcpus, max_cpus);
+		kvm->nrcpus = max_cpus;
 	}
 
 	/* Try the new way first, and fallback on legacy method otherwise */
-	err = gic__create_device(kvm);
-	if (err)
+	err = gic__create_device(kvm, type);
+	if (err && type == IRQCHIP_GICV2)
 		err = gic__create_irqchip(kvm);
 
 	return err;
@@ -131,15 +156,26 @@ static int gic__init_gic(struct kvm *kvm)
 }
 late_init(gic__init_gic)
 
-void gic__generate_fdt_nodes(void *fdt, u32 phandle)
+void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
 {
+	const char *compatible;
 	u64 reg_prop[] = {
-		cpu_to_fdt64(ARM_GIC_DIST_BASE), cpu_to_fdt64(ARM_GIC_DIST_SIZE),
-		cpu_to_fdt64(ARM_GIC_CPUI_BASE), cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
+		cpu_to_fdt64(ARM_GIC_DIST_BASE),
+		cpu_to_fdt64(ARM_GIC_DIST_SIZE),
+		cpu_to_fdt64(ARM_GIC_CPUI_BASE),
+		cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
 	};
 
+	switch (type) {
+	case IRQCHIP_GICV2:
+		compatible = "arm,cortex-a15-gic";
+		break;
+	default:
+		return;
+	}
+
 	_FDT(fdt_begin_node(fdt, "intc"));
-	_FDT(fdt_property_string(fdt, "compatible", "arm,cortex-a15-gic"));
+	_FDT(fdt_property_string(fdt, "compatible", compatible));
 	_FDT(fdt_property_cell(fdt, "#interrupt-cells", GIC_FDT_IRQ_NUM_CELLS));
 	_FDT(fdt_property(fdt, "interrupt-controller", NULL, 0));
 	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
index 44859f7..f5f6707 100644
--- a/arm/include/arm-common/gic.h
+++ b/arm/include/arm-common/gic.h
@@ -21,10 +21,12 @@
 #define GIC_MAX_CPUS			8
 #define GIC_MAX_IRQ			255
 
+enum irqchip_type {IRQCHIP_DEFAULT, IRQCHIP_GICV2};
+
 struct kvm;
 
 int gic__alloc_irqnum(void);
-int gic__create(struct kvm *kvm);
-void gic__generate_fdt_nodes(void *fdt, u32 phandle);
+int gic__create(struct kvm *kvm, enum irqchip_type type);
+void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type);
 
 #endif /* ARM_COMMON__GIC_H */
diff --git a/arm/kvm.c b/arm/kvm.c
index bcd2533..f9685c2 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -82,6 +82,6 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
 		MADV_MERGEABLE | MADV_HUGEPAGE);
 
 	/* Create the virtual GIC. */
-	if (gic__create(kvm))
+	if (gic__create(kvm, IRQCHIP_GICV2))
 		die("Failed to create virtual GIC");
 }
-- 
2.3.5

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 7/8] arm: add support for supplying GICv3 redistributor addresses
  2015-06-05  8:37 ` Andre Przywara
@ 2015-06-05  8:37   ` Andre Przywara
  -1 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-05  8:37 UTC (permalink / raw)
  To: will.deacon, marc.zyngier, penberg; +Cc: kvm, kvmarm, linux-arm-kernel

The code currently is assuming fixed sized memory regions for the
distributor and CPU interface. GICv3 needs a dynamic allocation of
its redistributor region, since its size depends on the number of
vCPUs.
Also add the necessary code to create a GICv3 IRQ chip instance.
This contains some defines which are not (yet) in the (32 bit) header
files to allow compilation for ARM.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c                         | 37 +++++++++++++++++++++++++++++++++++--
 arm/include/arm-common/gic.h      |  2 +-
 arm/include/arm-common/kvm-arch.h | 18 ++++++++++++++----
 arm/kvm-cpu.c                     |  4 +++-
 4 files changed, 53 insertions(+), 8 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 0ce40e4..c50d662 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -9,13 +9,24 @@
 #include <linux/kernel.h>
 #include <linux/kvm.h>
 
+/* Those names are not defined for ARM (yet) */
+#ifndef KVM_VGIC_V3_ADDR_TYPE_DIST
+#define KVM_VGIC_V3_ADDR_TYPE_DIST 2
+#endif
+
+#ifndef KVM_VGIC_V3_ADDR_TYPE_REDIST
+#define KVM_VGIC_V3_ADDR_TYPE_REDIST 3
+#endif
+
 static int gic_fd = -1;
+static int nr_redists;
 
 static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 {
 	int err;
 	u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
 	u64 dist_addr = ARM_GIC_DIST_BASE;
+	u64 redist_addr = dist_addr - nr_redists * ARM_GIC_REDIST_SIZE;
 	struct kvm_create_device gic_device = {
 		.flags	= 0,
 	};
@@ -28,11 +39,19 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
 		.addr	= (u64)(unsigned long)&dist_addr,
 	};
+	struct kvm_device_attr redist_attr = {
+		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
+		.attr	= KVM_VGIC_V3_ADDR_TYPE_REDIST,
+		.addr	= (u64)(unsigned long)&redist_addr,
+	};
 
 	switch (type) {
 	case IRQCHIP_GICV2:
 		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
 		break;
+	case IRQCHIP_GICV3:
+		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V3;
+		break;
 	default:
 		return -ENODEV;
 	}
@@ -48,6 +67,10 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 		dist_attr.attr = KVM_VGIC_V2_ADDR_TYPE_DIST;
 		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
 		break;
+	case IRQCHIP_GICV3:
+		dist_attr.attr = KVM_VGIC_V3_ADDR_TYPE_DIST;
+		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &redist_attr);
+		break;
 	default:
 		return -ENODEV;
 	}
@@ -55,6 +78,8 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 		return err;
 
 	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
+	if (err)
+		return err;
 
 	return err;
 }
@@ -162,17 +187,25 @@ void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
 	u64 reg_prop[] = {
 		cpu_to_fdt64(ARM_GIC_DIST_BASE),
 		cpu_to_fdt64(ARM_GIC_DIST_SIZE),
-		cpu_to_fdt64(ARM_GIC_CPUI_BASE),
-		cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
+		0, 0,				/* to be filled */
 	};
 
 	switch (type) {
 	case IRQCHIP_GICV2:
 		compatible = "arm,cortex-a15-gic";
+		reg_prop[2] = ARM_GIC_CPUI_BASE;
+		reg_prop[3] = ARM_GIC_CPUI_SIZE;
+		break;
+	case IRQCHIP_GICV3:
+		compatible = "arm,gic-v3";
+		reg_prop[2] = ARM_GIC_DIST_BASE - nr_redists * ARM_GIC_REDIST_SIZE;
+		reg_prop[3] = ARM_GIC_REDIST_SIZE * nr_redists;
 		break;
 	default:
 		return;
 	}
+	reg_prop[2] = cpu_to_fdt64(reg_prop[2]);
+	reg_prop[3] = cpu_to_fdt64(reg_prop[3]);
 
 	_FDT(fdt_begin_node(fdt, "intc"));
 	_FDT(fdt_property_string(fdt, "compatible", compatible));
diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
index f5f6707..8d6ab01 100644
--- a/arm/include/arm-common/gic.h
+++ b/arm/include/arm-common/gic.h
@@ -21,7 +21,7 @@
 #define GIC_MAX_CPUS			8
 #define GIC_MAX_IRQ			255
 
-enum irqchip_type {IRQCHIP_DEFAULT, IRQCHIP_GICV2};
+enum irqchip_type {IRQCHIP_DEFAULT, IRQCHIP_GICV2, IRQCHIP_GICV3};
 
 struct kvm;
 
diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
index 082131d..be66a76 100644
--- a/arm/include/arm-common/kvm-arch.h
+++ b/arm/include/arm-common/kvm-arch.h
@@ -17,10 +17,8 @@
 
 #define ARM_GIC_DIST_BASE	(ARM_AXI_AREA - ARM_GIC_DIST_SIZE)
 #define ARM_GIC_CPUI_BASE	(ARM_GIC_DIST_BASE - ARM_GIC_CPUI_SIZE)
-#define ARM_GIC_SIZE		(ARM_GIC_DIST_SIZE + ARM_GIC_CPUI_SIZE)
 
 #define ARM_IOPORT_SIZE		(ARM_MMIO_AREA - ARM_IOPORT_AREA)
-#define ARM_VIRTIO_MMIO_SIZE	(ARM_AXI_AREA - (ARM_MMIO_AREA + ARM_GIC_SIZE))
 #define ARM_PCI_CFG_SIZE	(1ULL << 24)
 #define ARM_PCI_MMIO_SIZE	(ARM_MEMORY_AREA - \
 				(ARM_AXI_AREA + ARM_PCI_CFG_SIZE))
@@ -30,6 +28,13 @@
 #define KVM_PCI_MMIO_AREA	(KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE)
 #define KVM_VIRTIO_MMIO_AREA	ARM_MMIO_AREA
 
+/*
+ * On a GICv3 there must be one redistributor per vCPU.
+ * The value here is the size for one, we multiply this at runtime with
+ * the number of requested vCPUs to get the actual size.
+ */
+#define ARM_GIC_REDIST_SIZE	0x20000
+
 #define KVM_IRQ_OFFSET		GIC_SPI_IRQ_BASE
 
 #define KVM_VM_TYPE		0
@@ -45,9 +50,14 @@ static inline bool arm_addr_in_ioport_region(u64 phys_addr)
 	return phys_addr >= KVM_IOPORT_AREA && phys_addr < limit;
 }
 
-static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
+static inline bool arm_addr_in_virtio_mmio_region(int nr_redists, u64 phys_addr)
 {
-	u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
+	u64 limit = ARM_AXI_AREA - ARM_GIC_DIST_SIZE;
+
+	if (nr_redists)
+		limit -= ARM_GIC_REDIST_SIZE * nr_redists;
+	else
+		limit -= ARM_GIC_CPUI_SIZE;
 	return phys_addr >= KVM_VIRTIO_MMIO_AREA && phys_addr < limit;
 }
 
diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
index ab08815..a3344fa 100644
--- a/arm/kvm-cpu.c
+++ b/arm/kvm-cpu.c
@@ -142,7 +142,9 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
 bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
 			   u32 len, u8 is_write)
 {
-	if (arm_addr_in_virtio_mmio_region(phys_addr)) {
+	int nr_redists = 0;
+
+	if (arm_addr_in_virtio_mmio_region(nr_redists, phys_addr)) {
 		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
 	} else if (arm_addr_in_ioport_region(phys_addr)) {
 		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
-- 
2.3.5


^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 7/8] arm: add support for supplying GICv3 redistributor addresses
@ 2015-06-05  8:37   ` Andre Przywara
  0 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-05  8:37 UTC (permalink / raw)
  To: linux-arm-kernel

The code currently is assuming fixed sized memory regions for the
distributor and CPU interface. GICv3 needs a dynamic allocation of
its redistributor region, since its size depends on the number of
vCPUs.
Also add the necessary code to create a GICv3 IRQ chip instance.
This contains some defines which are not (yet) in the (32 bit) header
files to allow compilation for ARM.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c                         | 37 +++++++++++++++++++++++++++++++++++--
 arm/include/arm-common/gic.h      |  2 +-
 arm/include/arm-common/kvm-arch.h | 18 ++++++++++++++----
 arm/kvm-cpu.c                     |  4 +++-
 4 files changed, 53 insertions(+), 8 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 0ce40e4..c50d662 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -9,13 +9,24 @@
 #include <linux/kernel.h>
 #include <linux/kvm.h>
 
+/* Those names are not defined for ARM (yet) */
+#ifndef KVM_VGIC_V3_ADDR_TYPE_DIST
+#define KVM_VGIC_V3_ADDR_TYPE_DIST 2
+#endif
+
+#ifndef KVM_VGIC_V3_ADDR_TYPE_REDIST
+#define KVM_VGIC_V3_ADDR_TYPE_REDIST 3
+#endif
+
 static int gic_fd = -1;
+static int nr_redists;
 
 static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 {
 	int err;
 	u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
 	u64 dist_addr = ARM_GIC_DIST_BASE;
+	u64 redist_addr = dist_addr - nr_redists * ARM_GIC_REDIST_SIZE;
 	struct kvm_create_device gic_device = {
 		.flags	= 0,
 	};
@@ -28,11 +39,19 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
 		.addr	= (u64)(unsigned long)&dist_addr,
 	};
+	struct kvm_device_attr redist_attr = {
+		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
+		.attr	= KVM_VGIC_V3_ADDR_TYPE_REDIST,
+		.addr	= (u64)(unsigned long)&redist_addr,
+	};
 
 	switch (type) {
 	case IRQCHIP_GICV2:
 		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
 		break;
+	case IRQCHIP_GICV3:
+		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V3;
+		break;
 	default:
 		return -ENODEV;
 	}
@@ -48,6 +67,10 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 		dist_attr.attr = KVM_VGIC_V2_ADDR_TYPE_DIST;
 		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
 		break;
+	case IRQCHIP_GICV3:
+		dist_attr.attr = KVM_VGIC_V3_ADDR_TYPE_DIST;
+		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &redist_attr);
+		break;
 	default:
 		return -ENODEV;
 	}
@@ -55,6 +78,8 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 		return err;
 
 	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
+	if (err)
+		return err;
 
 	return err;
 }
@@ -162,17 +187,25 @@ void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
 	u64 reg_prop[] = {
 		cpu_to_fdt64(ARM_GIC_DIST_BASE),
 		cpu_to_fdt64(ARM_GIC_DIST_SIZE),
-		cpu_to_fdt64(ARM_GIC_CPUI_BASE),
-		cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
+		0, 0,				/* to be filled */
 	};
 
 	switch (type) {
 	case IRQCHIP_GICV2:
 		compatible = "arm,cortex-a15-gic";
+		reg_prop[2] = ARM_GIC_CPUI_BASE;
+		reg_prop[3] = ARM_GIC_CPUI_SIZE;
+		break;
+	case IRQCHIP_GICV3:
+		compatible = "arm,gic-v3";
+		reg_prop[2] = ARM_GIC_DIST_BASE - nr_redists * ARM_GIC_REDIST_SIZE;
+		reg_prop[3] = ARM_GIC_REDIST_SIZE * nr_redists;
 		break;
 	default:
 		return;
 	}
+	reg_prop[2] = cpu_to_fdt64(reg_prop[2]);
+	reg_prop[3] = cpu_to_fdt64(reg_prop[3]);
 
 	_FDT(fdt_begin_node(fdt, "intc"));
 	_FDT(fdt_property_string(fdt, "compatible", compatible));
diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
index f5f6707..8d6ab01 100644
--- a/arm/include/arm-common/gic.h
+++ b/arm/include/arm-common/gic.h
@@ -21,7 +21,7 @@
 #define GIC_MAX_CPUS			8
 #define GIC_MAX_IRQ			255
 
-enum irqchip_type {IRQCHIP_DEFAULT, IRQCHIP_GICV2};
+enum irqchip_type {IRQCHIP_DEFAULT, IRQCHIP_GICV2, IRQCHIP_GICV3};
 
 struct kvm;
 
diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
index 082131d..be66a76 100644
--- a/arm/include/arm-common/kvm-arch.h
+++ b/arm/include/arm-common/kvm-arch.h
@@ -17,10 +17,8 @@
 
 #define ARM_GIC_DIST_BASE	(ARM_AXI_AREA - ARM_GIC_DIST_SIZE)
 #define ARM_GIC_CPUI_BASE	(ARM_GIC_DIST_BASE - ARM_GIC_CPUI_SIZE)
-#define ARM_GIC_SIZE		(ARM_GIC_DIST_SIZE + ARM_GIC_CPUI_SIZE)
 
 #define ARM_IOPORT_SIZE		(ARM_MMIO_AREA - ARM_IOPORT_AREA)
-#define ARM_VIRTIO_MMIO_SIZE	(ARM_AXI_AREA - (ARM_MMIO_AREA + ARM_GIC_SIZE))
 #define ARM_PCI_CFG_SIZE	(1ULL << 24)
 #define ARM_PCI_MMIO_SIZE	(ARM_MEMORY_AREA - \
 				(ARM_AXI_AREA + ARM_PCI_CFG_SIZE))
@@ -30,6 +28,13 @@
 #define KVM_PCI_MMIO_AREA	(KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE)
 #define KVM_VIRTIO_MMIO_AREA	ARM_MMIO_AREA
 
+/*
+ * On a GICv3 there must be one redistributor per vCPU.
+ * The value here is the size for one, we multiply this@runtime with
+ * the number of requested vCPUs to get the actual size.
+ */
+#define ARM_GIC_REDIST_SIZE	0x20000
+
 #define KVM_IRQ_OFFSET		GIC_SPI_IRQ_BASE
 
 #define KVM_VM_TYPE		0
@@ -45,9 +50,14 @@ static inline bool arm_addr_in_ioport_region(u64 phys_addr)
 	return phys_addr >= KVM_IOPORT_AREA && phys_addr < limit;
 }
 
-static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
+static inline bool arm_addr_in_virtio_mmio_region(int nr_redists, u64 phys_addr)
 {
-	u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
+	u64 limit = ARM_AXI_AREA - ARM_GIC_DIST_SIZE;
+
+	if (nr_redists)
+		limit -= ARM_GIC_REDIST_SIZE * nr_redists;
+	else
+		limit -= ARM_GIC_CPUI_SIZE;
 	return phys_addr >= KVM_VIRTIO_MMIO_AREA && phys_addr < limit;
 }
 
diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
index ab08815..a3344fa 100644
--- a/arm/kvm-cpu.c
+++ b/arm/kvm-cpu.c
@@ -142,7 +142,9 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
 bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
 			   u32 len, u8 is_write)
 {
-	if (arm_addr_in_virtio_mmio_region(phys_addr)) {
+	int nr_redists = 0;
+
+	if (arm_addr_in_virtio_mmio_region(nr_redists, phys_addr)) {
 		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
 	} else if (arm_addr_in_ioport_region(phys_addr)) {
 		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
-- 
2.3.5

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 8/8] arm: use new irqchip parameter to create different vGIC types
  2015-06-05  8:37 ` Andre Przywara
@ 2015-06-05  8:37   ` Andre Przywara
  -1 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-05  8:37 UTC (permalink / raw)
  To: will.deacon, marc.zyngier, penberg; +Cc: kvm, kvmarm, linux-arm-kernel

Currently we unconditionally create a virtual GICv2 in the guest.
Add a --irqchip= parameter to let the user specify a different GIC
type for the guest.
For now we the only other supported type is GICv3.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/aarch64/arm-cpu.c                    |  2 +-
 arm/gic.c                                | 21 +++++++++++++++++++++
 arm/include/arm-common/kvm-config-arch.h |  9 ++++++++-
 arm/kvm-cpu.c                            |  6 ++++++
 arm/kvm.c                                |  4 +++-
 5 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
index f702b9e..3dc8ea3 100644
--- a/arm/aarch64/arm-cpu.c
+++ b/arm/aarch64/arm-cpu.c
@@ -12,7 +12,7 @@
 static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
 {
 	int timer_interrupts[4] = {13, 14, 11, 10};
-	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
+	gic__generate_fdt_nodes(fdt, gic_phandle, kvm->cfg.arch.irqchip);
 	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
 }
 
diff --git a/arm/gic.c b/arm/gic.c
index c50d662..ab0f594 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -21,6 +21,23 @@
 static int gic_fd = -1;
 static int nr_redists;
 
+int irqchip_parser(const struct option *opt, const char *arg, int unset)
+{
+	enum irqchip_type *type = opt->value;
+
+	*type = IRQCHIP_DEFAULT;
+	if (!strcmp(arg, "gicv2")) {
+		*type = IRQCHIP_GICV2;
+	} else if (!strcmp(arg, "gicv3")) {
+		*type = IRQCHIP_GICV3;
+	} else if (strcmp(arg, "default")) {
+		fprintf(stderr, "irqchip: unknown type \"%s\"\n", arg);
+		return -1;
+	}
+
+	return 0;
+}
+
 static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 {
 	int err;
@@ -121,6 +138,10 @@ int gic__create(struct kvm *kvm, enum irqchip_type type)
 	case IRQCHIP_GICV2:
 		max_cpus = GIC_MAX_CPUS;
 		break;
+	case IRQCHIP_GICV3:
+		nr_redists = kvm->cfg.nrcpus;
+		max_cpus = 255;
+		break;
 	default:
 		return -ENODEV;
 	}
diff --git a/arm/include/arm-common/kvm-config-arch.h b/arm/include/arm-common/kvm-config-arch.h
index a8ebd94..ae4e89b 100644
--- a/arm/include/arm-common/kvm-config-arch.h
+++ b/arm/include/arm-common/kvm-config-arch.h
@@ -8,8 +8,11 @@ struct kvm_config_arch {
 	unsigned int	force_cntfrq;
 	bool		virtio_trans_pci;
 	bool		aarch32_guest;
+	int		irqchip;
 };
 
+int irqchip_parser(const struct option *opt, const char *arg, int unset);
+
 #define OPT_ARCH_RUN(pfx, cfg)							\
 	pfx,									\
 	ARM_OPT_ARCH_RUN(cfg)							\
@@ -21,6 +24,10 @@ struct kvm_config_arch {
 		     "updated to program CNTFRQ correctly*"),			\
 	OPT_BOOLEAN('\0', "force-pci", &(cfg)->virtio_trans_pci,		\
 		    "Force virtio devices to use PCI as their default "		\
-		    "transport"),
+		    "transport"),						\
+        OPT_CALLBACK('\0', "irqchip", &(cfg)->irqchip,				\
+		     "[gicv2|gicv3]",					\
+		     "type of interrupt controller to emulate in the guest",	\
+		     irqchip_parser, NULL),
 
 #endif /* ARM_COMMON__KVM_CONFIG_ARCH_H */
diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
index a3344fa..aacc172 100644
--- a/arm/kvm-cpu.c
+++ b/arm/kvm-cpu.c
@@ -144,6 +144,12 @@ bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
 {
 	int nr_redists = 0;
 
+	switch (vcpu->kvm->cfg.arch.irqchip) {
+	case IRQCHIP_GICV3:
+		nr_redists = vcpu->kvm->nrcpus;
+		break;
+	}
+
 	if (arm_addr_in_virtio_mmio_region(nr_redists, phys_addr)) {
 		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
 	} else if (arm_addr_in_ioport_region(phys_addr)) {
diff --git a/arm/kvm.c b/arm/kvm.c
index f9685c2..2628d31 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -82,6 +82,8 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
 		MADV_MERGEABLE | MADV_HUGEPAGE);
 
 	/* Create the virtual GIC. */
-	if (gic__create(kvm, IRQCHIP_GICV2))
+	if (kvm->cfg.arch.irqchip == IRQCHIP_DEFAULT)
+		kvm->cfg.arch.irqchip = IRQCHIP_GICV2;
+	if (gic__create(kvm, kvm->cfg.arch.irqchip))
 		die("Failed to create virtual GIC");
 }
-- 
2.3.5


^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 8/8] arm: use new irqchip parameter to create different vGIC types
@ 2015-06-05  8:37   ` Andre Przywara
  0 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-05  8:37 UTC (permalink / raw)
  To: linux-arm-kernel

Currently we unconditionally create a virtual GICv2 in the guest.
Add a --irqchip= parameter to let the user specify a different GIC
type for the guest.
For now we the only other supported type is GICv3.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/aarch64/arm-cpu.c                    |  2 +-
 arm/gic.c                                | 21 +++++++++++++++++++++
 arm/include/arm-common/kvm-config-arch.h |  9 ++++++++-
 arm/kvm-cpu.c                            |  6 ++++++
 arm/kvm.c                                |  4 +++-
 5 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
index f702b9e..3dc8ea3 100644
--- a/arm/aarch64/arm-cpu.c
+++ b/arm/aarch64/arm-cpu.c
@@ -12,7 +12,7 @@
 static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
 {
 	int timer_interrupts[4] = {13, 14, 11, 10};
-	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
+	gic__generate_fdt_nodes(fdt, gic_phandle, kvm->cfg.arch.irqchip);
 	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
 }
 
diff --git a/arm/gic.c b/arm/gic.c
index c50d662..ab0f594 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -21,6 +21,23 @@
 static int gic_fd = -1;
 static int nr_redists;
 
+int irqchip_parser(const struct option *opt, const char *arg, int unset)
+{
+	enum irqchip_type *type = opt->value;
+
+	*type = IRQCHIP_DEFAULT;
+	if (!strcmp(arg, "gicv2")) {
+		*type = IRQCHIP_GICV2;
+	} else if (!strcmp(arg, "gicv3")) {
+		*type = IRQCHIP_GICV3;
+	} else if (strcmp(arg, "default")) {
+		fprintf(stderr, "irqchip: unknown type \"%s\"\n", arg);
+		return -1;
+	}
+
+	return 0;
+}
+
 static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 {
 	int err;
@@ -121,6 +138,10 @@ int gic__create(struct kvm *kvm, enum irqchip_type type)
 	case IRQCHIP_GICV2:
 		max_cpus = GIC_MAX_CPUS;
 		break;
+	case IRQCHIP_GICV3:
+		nr_redists = kvm->cfg.nrcpus;
+		max_cpus = 255;
+		break;
 	default:
 		return -ENODEV;
 	}
diff --git a/arm/include/arm-common/kvm-config-arch.h b/arm/include/arm-common/kvm-config-arch.h
index a8ebd94..ae4e89b 100644
--- a/arm/include/arm-common/kvm-config-arch.h
+++ b/arm/include/arm-common/kvm-config-arch.h
@@ -8,8 +8,11 @@ struct kvm_config_arch {
 	unsigned int	force_cntfrq;
 	bool		virtio_trans_pci;
 	bool		aarch32_guest;
+	int		irqchip;
 };
 
+int irqchip_parser(const struct option *opt, const char *arg, int unset);
+
 #define OPT_ARCH_RUN(pfx, cfg)							\
 	pfx,									\
 	ARM_OPT_ARCH_RUN(cfg)							\
@@ -21,6 +24,10 @@ struct kvm_config_arch {
 		     "updated to program CNTFRQ correctly*"),			\
 	OPT_BOOLEAN('\0', "force-pci", &(cfg)->virtio_trans_pci,		\
 		    "Force virtio devices to use PCI as their default "		\
-		    "transport"),
+		    "transport"),						\
+        OPT_CALLBACK('\0', "irqchip", &(cfg)->irqchip,				\
+		     "[gicv2|gicv3]",					\
+		     "type of interrupt controller to emulate in the guest",	\
+		     irqchip_parser, NULL),
 
 #endif /* ARM_COMMON__KVM_CONFIG_ARCH_H */
diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
index a3344fa..aacc172 100644
--- a/arm/kvm-cpu.c
+++ b/arm/kvm-cpu.c
@@ -144,6 +144,12 @@ bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
 {
 	int nr_redists = 0;
 
+	switch (vcpu->kvm->cfg.arch.irqchip) {
+	case IRQCHIP_GICV3:
+		nr_redists = vcpu->kvm->nrcpus;
+		break;
+	}
+
 	if (arm_addr_in_virtio_mmio_region(nr_redists, phys_addr)) {
 		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
 	} else if (arm_addr_in_ioport_region(phys_addr)) {
diff --git a/arm/kvm.c b/arm/kvm.c
index f9685c2..2628d31 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -82,6 +82,8 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
 		MADV_MERGEABLE | MADV_HUGEPAGE);
 
 	/* Create the virtual GIC. */
-	if (gic__create(kvm, IRQCHIP_GICV2))
+	if (kvm->cfg.arch.irqchip == IRQCHIP_DEFAULT)
+		kvm->cfg.arch.irqchip = IRQCHIP_GICV2;
+	if (gic__create(kvm, kvm->cfg.arch.irqchip))
 		die("Failed to create virtual GIC");
 }
-- 
2.3.5

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 6/8] arm: prepare for instantiating different IRQ chip devices
  2015-06-05  8:37   ` Andre Przywara
@ 2015-06-09 11:31     ` Andre Przywara
  -1 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-09 11:31 UTC (permalink / raw)
  To: will.deacon, marc.zyngier; +Cc: penberg, linux-arm-kernel, kvmarm, kvm

Hi, contrary to my boasting in the cover letter I managed to
accidentially drop the fix for the GIC device initialization error
handling Will requested from this series.
If we fail the GIC initialization sequence at some point, we should
make sure to not let the gic_fd initialized, so that subsequent
accesses to the GIC device fail appropriately.
So the fix below should be merged in, actually a part of it already
in patch 2/8.
If there are no further comments, I will re-spin this series with
those fixes contained in the proper patches later this week.

Cheers,
Andre.

---
diff --git a/arm/gic.c b/arm/gic.c
index ab0f594..5fb94c1 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -89,15 +89,21 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &redist_attr);
 		break;
 	default:
-		return -ENODEV;
+		err = -ENODEV;
+		break;
 	}
 	if (err)
-		return err;
+		goto out_err;
 
 	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
 	if (err)
-		return err;
+		goto out_err;
+
+	return err;
 
+out_err:
+	close(gic_fd);
+	gic_fd = -1;
 	return err;
 }
 
-- 
2.3.5


^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 6/8] arm: prepare for instantiating different IRQ chip devices
@ 2015-06-09 11:31     ` Andre Przywara
  0 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-09 11:31 UTC (permalink / raw)
  To: linux-arm-kernel

Hi, contrary to my boasting in the cover letter I managed to
accidentially drop the fix for the GIC device initialization error
handling Will requested from this series.
If we fail the GIC initialization sequence at some point, we should
make sure to not let the gic_fd initialized, so that subsequent
accesses to the GIC device fail appropriately.
So the fix below should be merged in, actually a part of it already
in patch 2/8.
If there are no further comments, I will re-spin this series with
those fixes contained in the proper patches later this week.

Cheers,
Andre.

---
diff --git a/arm/gic.c b/arm/gic.c
index ab0f594..5fb94c1 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -89,15 +89,21 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &redist_attr);
 		break;
 	default:
-		return -ENODEV;
+		err = -ENODEV;
+		break;
 	}
 	if (err)
-		return err;
+		goto out_err;
 
 	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
 	if (err)
-		return err;
+		goto out_err;
+
+	return err;
 
+out_err:
+	close(gic_fd);
+	gic_fd = -1;
 	return err;
 }
 
-- 
2.3.5

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 5/8] arm: finish VGIC initialisation explicitly
  2015-06-05  8:37   ` Andre Przywara
@ 2015-06-10 17:07     ` Marc Zyngier
  -1 siblings, 0 replies; 34+ messages in thread
From: Marc Zyngier @ 2015-06-10 17:07 UTC (permalink / raw)
  To: Andre Przywara, Will Deacon, penberg; +Cc: kvm, kvmarm, linux-arm-kernel

On 05/06/15 09:37, Andre Przywara wrote:
> Since Linux 3.19-rc1 there is a new API to explicitly initialise
> the in-kernel GIC emulation by a userland KVM device call.
> Use that to tell the kernel we are finished with the GIC
> initialisation, since the automatic GIC init will only be provided
> as a legacy functionality in the future.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

> ---
>  arm/gic.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index 6277af8..8d47562 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -89,24 +89,43 @@ int gic__create(struct kvm *kvm)
>  	return err;
>  }
>  
> +/*
> + * Sets the number of used interrupts and finalizes the GIC init explicitly.
> + */
>  static int gic__init_gic(struct kvm *kvm)
>  {
> +	int ret;
> +
>  	int lines = irq__get_nr_allocated_lines();
>  	u32 nr_irqs = ALIGN(lines, 32) + GIC_SPI_IRQ_BASE;
>  	struct kvm_device_attr nr_irqs_attr = {
>  		.group	= KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
>  		.addr	= (u64)(unsigned long)&nr_irqs,
>  	};
> +	struct kvm_device_attr vgic_init_attr = {
> +		.group	= KVM_DEV_ARM_VGIC_GRP_CTRL,
> +		.attr	= KVM_DEV_ARM_VGIC_CTRL_INIT,
> +	};
>  
>  	/*
>  	 * If we didn't use the KVM_CREATE_DEVICE method, KVM will
> -	 * give us some default number of interrupts.
> +	 * give us some default number of interrupts. The GIC initialization
> +	 * will be done automatically in this case.
>  	 */
>  	if (gic_fd < 0)
>  		return 0;
>  
> -	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &nr_irqs_attr))
> -		return ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &nr_irqs_attr);
> +	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &nr_irqs_attr)) {
> +		ret = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &nr_irqs_attr);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &vgic_init_attr)) {
> +		ret = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &vgic_init_attr);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	return 0;
>  }
> 


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

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH v2 5/8] arm: finish VGIC initialisation explicitly
@ 2015-06-10 17:07     ` Marc Zyngier
  0 siblings, 0 replies; 34+ messages in thread
From: Marc Zyngier @ 2015-06-10 17:07 UTC (permalink / raw)
  To: linux-arm-kernel

On 05/06/15 09:37, Andre Przywara wrote:
> Since Linux 3.19-rc1 there is a new API to explicitly initialise
> the in-kernel GIC emulation by a userland KVM device call.
> Use that to tell the kernel we are finished with the GIC
> initialisation, since the automatic GIC init will only be provided
> as a legacy functionality in the future.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

> ---
>  arm/gic.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index 6277af8..8d47562 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -89,24 +89,43 @@ int gic__create(struct kvm *kvm)
>  	return err;
>  }
>  
> +/*
> + * Sets the number of used interrupts and finalizes the GIC init explicitly.
> + */
>  static int gic__init_gic(struct kvm *kvm)
>  {
> +	int ret;
> +
>  	int lines = irq__get_nr_allocated_lines();
>  	u32 nr_irqs = ALIGN(lines, 32) + GIC_SPI_IRQ_BASE;
>  	struct kvm_device_attr nr_irqs_attr = {
>  		.group	= KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
>  		.addr	= (u64)(unsigned long)&nr_irqs,
>  	};
> +	struct kvm_device_attr vgic_init_attr = {
> +		.group	= KVM_DEV_ARM_VGIC_GRP_CTRL,
> +		.attr	= KVM_DEV_ARM_VGIC_CTRL_INIT,
> +	};
>  
>  	/*
>  	 * If we didn't use the KVM_CREATE_DEVICE method, KVM will
> -	 * give us some default number of interrupts.
> +	 * give us some default number of interrupts. The GIC initialization
> +	 * will be done automatically in this case.
>  	 */
>  	if (gic_fd < 0)
>  		return 0;
>  
> -	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &nr_irqs_attr))
> -		return ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &nr_irqs_attr);
> +	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &nr_irqs_attr)) {
> +		ret = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &nr_irqs_attr);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &vgic_init_attr)) {
> +		ret = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &vgic_init_attr);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	return 0;
>  }
> 


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

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 6/8] arm: prepare for instantiating different IRQ chip devices
  2015-06-05  8:37   ` Andre Przywara
@ 2015-06-10 17:21     ` Marc Zyngier
  -1 siblings, 0 replies; 34+ messages in thread
From: Marc Zyngier @ 2015-06-10 17:21 UTC (permalink / raw)
  To: Andre Przywara, Will Deacon, penberg; +Cc: kvm, kvmarm, linux-arm-kernel

On 05/06/15 09:37, Andre Przywara wrote:
> Extend the vGIC handling code to potentially deal with different IRQ
> chip devices instead of hard-coding the GICv2 in.
> We extend most vGIC functions to take a type parameter, but still put
> GICv2 in at the top for the time being.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/aarch32/arm-cpu.c        |  2 +-
>  arm/aarch64/arm-cpu.c        |  2 +-
>  arm/gic.c                    | 66 ++++++++++++++++++++++++++++++++++----------
>  arm/include/arm-common/gic.h |  6 ++--
>  arm/kvm.c                    |  2 +-
>  5 files changed, 58 insertions(+), 20 deletions(-)
> 
> diff --git a/arm/aarch32/arm-cpu.c b/arm/aarch32/arm-cpu.c
> index 946e443..d8d6293 100644
> --- a/arm/aarch32/arm-cpu.c
> +++ b/arm/aarch32/arm-cpu.c
> @@ -12,7 +12,7 @@ static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
>  {
>  	int timer_interrupts[4] = {13, 14, 11, 10};
>  
> -	gic__generate_fdt_nodes(fdt, gic_phandle);
> +	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
>  	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
>  }
>  
> diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
> index 8efe877..f702b9e 100644
> --- a/arm/aarch64/arm-cpu.c
> +++ b/arm/aarch64/arm-cpu.c
> @@ -12,7 +12,7 @@
>  static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
>  {
>  	int timer_interrupts[4] = {13, 14, 11, 10};
> -	gic__generate_fdt_nodes(fdt, gic_phandle);
> +	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
>  	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
>  }
>  
> diff --git a/arm/gic.c b/arm/gic.c
> index 8d47562..0ce40e4 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -11,13 +11,13 @@
>  
>  static int gic_fd = -1;
>  
> -static int gic__create_device(struct kvm *kvm)
> +static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  {
>  	int err;
>  	u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
>  	u64 dist_addr = ARM_GIC_DIST_BASE;
>  	struct kvm_create_device gic_device = {
> -		.type	= KVM_DEV_TYPE_ARM_VGIC_V2,
> +		.flags	= 0,
>  	};
>  	struct kvm_device_attr cpu_if_attr = {
>  		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
> @@ -26,21 +26,37 @@ static int gic__create_device(struct kvm *kvm)
>  	};
>  	struct kvm_device_attr dist_attr = {
>  		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
> -		.attr	= KVM_VGIC_V2_ADDR_TYPE_DIST,
>  		.addr	= (u64)(unsigned long)&dist_addr,
>  	};
>  
> +	switch (type) {
> +	case IRQCHIP_GICV2:
> +		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
> +		break;
> +	default:
> +		return -ENODEV;
> +	}
> +
>  	err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &gic_device);
>  	if (err)
>  		return err;
>  
>  	gic_fd = gic_device.fd;
>  
> -	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
> +	switch (type) {
> +	case IRQCHIP_GICV2:
> +		dist_attr.attr = KVM_VGIC_V2_ADDR_TYPE_DIST;

You could move the structure patching in the first switch statement.

> +		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
> +		break;
> +	default:
> +		return -ENODEV;

This default cannot be reached, as you've already caught the weird stuff
above.

> +	}
>  	if (err)
>  		return err;
>  
> -	return ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
> +	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
> +
> +	return err;
>  }
>  
>  static int gic__create_irqchip(struct kvm *kvm)
> @@ -71,19 +87,28 @@ static int gic__create_irqchip(struct kvm *kvm)
>  	return err;
>  }
>  
> -int gic__create(struct kvm *kvm)
> +int gic__create(struct kvm *kvm, enum irqchip_type type)
>  {
> +	int max_cpus;
>  	int err;
>  
> -	if (kvm->nrcpus > GIC_MAX_CPUS) {
> +	switch (type) {
> +	case IRQCHIP_GICV2:
> +		max_cpus = GIC_MAX_CPUS;
> +		break;
> +	default:
> +		return -ENODEV;
> +	}
> +
> +	if (kvm->nrcpus > max_cpus) {
>  		pr_warning("%d CPUS greater than maximum of %d -- truncating\n",
> -				kvm->nrcpus, GIC_MAX_CPUS);
> -		kvm->nrcpus = GIC_MAX_CPUS;
> +				kvm->nrcpus, max_cpus);
> +		kvm->nrcpus = max_cpus;
>  	}
>  
>  	/* Try the new way first, and fallback on legacy method otherwise */
> -	err = gic__create_device(kvm);
> -	if (err)
> +	err = gic__create_device(kvm, type);
> +	if (err && type == IRQCHIP_GICV2)
>  		err = gic__create_irqchip(kvm);
>  
>  	return err;
> @@ -131,15 +156,26 @@ static int gic__init_gic(struct kvm *kvm)
>  }
>  late_init(gic__init_gic)
>  
> -void gic__generate_fdt_nodes(void *fdt, u32 phandle)
> +void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
>  {
> +	const char *compatible;
>  	u64 reg_prop[] = {
> -		cpu_to_fdt64(ARM_GIC_DIST_BASE), cpu_to_fdt64(ARM_GIC_DIST_SIZE),
> -		cpu_to_fdt64(ARM_GIC_CPUI_BASE), cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
> +		cpu_to_fdt64(ARM_GIC_DIST_BASE),
> +		cpu_to_fdt64(ARM_GIC_DIST_SIZE),
> +		cpu_to_fdt64(ARM_GIC_CPUI_BASE),
> +		cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
>  	};

Any particular reason for this change? I found the original more readable...

>  
> +	switch (type) {
> +	case IRQCHIP_GICV2:
> +		compatible = "arm,cortex-a15-gic";
> +		break;
> +	default:
> +		return;
> +	}
> +
>  	_FDT(fdt_begin_node(fdt, "intc"));
> -	_FDT(fdt_property_string(fdt, "compatible", "arm,cortex-a15-gic"));
> +	_FDT(fdt_property_string(fdt, "compatible", compatible));
>  	_FDT(fdt_property_cell(fdt, "#interrupt-cells", GIC_FDT_IRQ_NUM_CELLS));
>  	_FDT(fdt_property(fdt, "interrupt-controller", NULL, 0));
>  	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
> diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
> index 44859f7..f5f6707 100644
> --- a/arm/include/arm-common/gic.h
> +++ b/arm/include/arm-common/gic.h
> @@ -21,10 +21,12 @@
>  #define GIC_MAX_CPUS			8
>  #define GIC_MAX_IRQ			255
>  
> +enum irqchip_type {IRQCHIP_DEFAULT, IRQCHIP_GICV2};
> +

Can you use the standard enum style:

enum blah {
	E1,
	E2,
};

>  struct kvm;
>  
>  int gic__alloc_irqnum(void);
> -int gic__create(struct kvm *kvm);
> -void gic__generate_fdt_nodes(void *fdt, u32 phandle);
> +int gic__create(struct kvm *kvm, enum irqchip_type type);
> +void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type);
>  
>  #endif /* ARM_COMMON__GIC_H */
> diff --git a/arm/kvm.c b/arm/kvm.c
> index bcd2533..f9685c2 100644
> --- a/arm/kvm.c
> +++ b/arm/kvm.c
> @@ -82,6 +82,6 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
>  		MADV_MERGEABLE | MADV_HUGEPAGE);
>  
>  	/* Create the virtual GIC. */
> -	if (gic__create(kvm))
> +	if (gic__create(kvm, IRQCHIP_GICV2))
>  		die("Failed to create virtual GIC");
>  }
> 

Thanks,

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

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH v2 6/8] arm: prepare for instantiating different IRQ chip devices
@ 2015-06-10 17:21     ` Marc Zyngier
  0 siblings, 0 replies; 34+ messages in thread
From: Marc Zyngier @ 2015-06-10 17:21 UTC (permalink / raw)
  To: linux-arm-kernel

On 05/06/15 09:37, Andre Przywara wrote:
> Extend the vGIC handling code to potentially deal with different IRQ
> chip devices instead of hard-coding the GICv2 in.
> We extend most vGIC functions to take a type parameter, but still put
> GICv2 in at the top for the time being.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/aarch32/arm-cpu.c        |  2 +-
>  arm/aarch64/arm-cpu.c        |  2 +-
>  arm/gic.c                    | 66 ++++++++++++++++++++++++++++++++++----------
>  arm/include/arm-common/gic.h |  6 ++--
>  arm/kvm.c                    |  2 +-
>  5 files changed, 58 insertions(+), 20 deletions(-)
> 
> diff --git a/arm/aarch32/arm-cpu.c b/arm/aarch32/arm-cpu.c
> index 946e443..d8d6293 100644
> --- a/arm/aarch32/arm-cpu.c
> +++ b/arm/aarch32/arm-cpu.c
> @@ -12,7 +12,7 @@ static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
>  {
>  	int timer_interrupts[4] = {13, 14, 11, 10};
>  
> -	gic__generate_fdt_nodes(fdt, gic_phandle);
> +	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
>  	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
>  }
>  
> diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
> index 8efe877..f702b9e 100644
> --- a/arm/aarch64/arm-cpu.c
> +++ b/arm/aarch64/arm-cpu.c
> @@ -12,7 +12,7 @@
>  static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
>  {
>  	int timer_interrupts[4] = {13, 14, 11, 10};
> -	gic__generate_fdt_nodes(fdt, gic_phandle);
> +	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
>  	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
>  }
>  
> diff --git a/arm/gic.c b/arm/gic.c
> index 8d47562..0ce40e4 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -11,13 +11,13 @@
>  
>  static int gic_fd = -1;
>  
> -static int gic__create_device(struct kvm *kvm)
> +static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  {
>  	int err;
>  	u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
>  	u64 dist_addr = ARM_GIC_DIST_BASE;
>  	struct kvm_create_device gic_device = {
> -		.type	= KVM_DEV_TYPE_ARM_VGIC_V2,
> +		.flags	= 0,
>  	};
>  	struct kvm_device_attr cpu_if_attr = {
>  		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
> @@ -26,21 +26,37 @@ static int gic__create_device(struct kvm *kvm)
>  	};
>  	struct kvm_device_attr dist_attr = {
>  		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
> -		.attr	= KVM_VGIC_V2_ADDR_TYPE_DIST,
>  		.addr	= (u64)(unsigned long)&dist_addr,
>  	};
>  
> +	switch (type) {
> +	case IRQCHIP_GICV2:
> +		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
> +		break;
> +	default:
> +		return -ENODEV;
> +	}
> +
>  	err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &gic_device);
>  	if (err)
>  		return err;
>  
>  	gic_fd = gic_device.fd;
>  
> -	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
> +	switch (type) {
> +	case IRQCHIP_GICV2:
> +		dist_attr.attr = KVM_VGIC_V2_ADDR_TYPE_DIST;

You could move the structure patching in the first switch statement.

> +		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
> +		break;
> +	default:
> +		return -ENODEV;

This default cannot be reached, as you've already caught the weird stuff
above.

> +	}
>  	if (err)
>  		return err;
>  
> -	return ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
> +	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
> +
> +	return err;
>  }
>  
>  static int gic__create_irqchip(struct kvm *kvm)
> @@ -71,19 +87,28 @@ static int gic__create_irqchip(struct kvm *kvm)
>  	return err;
>  }
>  
> -int gic__create(struct kvm *kvm)
> +int gic__create(struct kvm *kvm, enum irqchip_type type)
>  {
> +	int max_cpus;
>  	int err;
>  
> -	if (kvm->nrcpus > GIC_MAX_CPUS) {
> +	switch (type) {
> +	case IRQCHIP_GICV2:
> +		max_cpus = GIC_MAX_CPUS;
> +		break;
> +	default:
> +		return -ENODEV;
> +	}
> +
> +	if (kvm->nrcpus > max_cpus) {
>  		pr_warning("%d CPUS greater than maximum of %d -- truncating\n",
> -				kvm->nrcpus, GIC_MAX_CPUS);
> -		kvm->nrcpus = GIC_MAX_CPUS;
> +				kvm->nrcpus, max_cpus);
> +		kvm->nrcpus = max_cpus;
>  	}
>  
>  	/* Try the new way first, and fallback on legacy method otherwise */
> -	err = gic__create_device(kvm);
> -	if (err)
> +	err = gic__create_device(kvm, type);
> +	if (err && type == IRQCHIP_GICV2)
>  		err = gic__create_irqchip(kvm);
>  
>  	return err;
> @@ -131,15 +156,26 @@ static int gic__init_gic(struct kvm *kvm)
>  }
>  late_init(gic__init_gic)
>  
> -void gic__generate_fdt_nodes(void *fdt, u32 phandle)
> +void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
>  {
> +	const char *compatible;
>  	u64 reg_prop[] = {
> -		cpu_to_fdt64(ARM_GIC_DIST_BASE), cpu_to_fdt64(ARM_GIC_DIST_SIZE),
> -		cpu_to_fdt64(ARM_GIC_CPUI_BASE), cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
> +		cpu_to_fdt64(ARM_GIC_DIST_BASE),
> +		cpu_to_fdt64(ARM_GIC_DIST_SIZE),
> +		cpu_to_fdt64(ARM_GIC_CPUI_BASE),
> +		cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
>  	};

Any particular reason for this change? I found the original more readable...

>  
> +	switch (type) {
> +	case IRQCHIP_GICV2:
> +		compatible = "arm,cortex-a15-gic";
> +		break;
> +	default:
> +		return;
> +	}
> +
>  	_FDT(fdt_begin_node(fdt, "intc"));
> -	_FDT(fdt_property_string(fdt, "compatible", "arm,cortex-a15-gic"));
> +	_FDT(fdt_property_string(fdt, "compatible", compatible));
>  	_FDT(fdt_property_cell(fdt, "#interrupt-cells", GIC_FDT_IRQ_NUM_CELLS));
>  	_FDT(fdt_property(fdt, "interrupt-controller", NULL, 0));
>  	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
> diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
> index 44859f7..f5f6707 100644
> --- a/arm/include/arm-common/gic.h
> +++ b/arm/include/arm-common/gic.h
> @@ -21,10 +21,12 @@
>  #define GIC_MAX_CPUS			8
>  #define GIC_MAX_IRQ			255
>  
> +enum irqchip_type {IRQCHIP_DEFAULT, IRQCHIP_GICV2};
> +

Can you use the standard enum style:

enum blah {
	E1,
	E2,
};

>  struct kvm;
>  
>  int gic__alloc_irqnum(void);
> -int gic__create(struct kvm *kvm);
> -void gic__generate_fdt_nodes(void *fdt, u32 phandle);
> +int gic__create(struct kvm *kvm, enum irqchip_type type);
> +void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type);
>  
>  #endif /* ARM_COMMON__GIC_H */
> diff --git a/arm/kvm.c b/arm/kvm.c
> index bcd2533..f9685c2 100644
> --- a/arm/kvm.c
> +++ b/arm/kvm.c
> @@ -82,6 +82,6 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
>  		MADV_MERGEABLE | MADV_HUGEPAGE);
>  
>  	/* Create the virtual GIC. */
> -	if (gic__create(kvm))
> +	if (gic__create(kvm, IRQCHIP_GICV2))
>  		die("Failed to create virtual GIC");
>  }
> 

Thanks,

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

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 7/8] arm: add support for supplying GICv3 redistributor addresses
  2015-06-05  8:37   ` Andre Przywara
@ 2015-06-10 17:40     ` Marc Zyngier
  -1 siblings, 0 replies; 34+ messages in thread
From: Marc Zyngier @ 2015-06-10 17:40 UTC (permalink / raw)
  To: Andre Przywara, Will Deacon, penberg; +Cc: linux-arm-kernel, kvmarm, kvm

On 05/06/15 09:37, Andre Przywara wrote:
> The code currently is assuming fixed sized memory regions for the
> distributor and CPU interface. GICv3 needs a dynamic allocation of
> its redistributor region, since its size depends on the number of
> vCPUs.
> Also add the necessary code to create a GICv3 IRQ chip instance.
> This contains some defines which are not (yet) in the (32 bit) header
> files to allow compilation for ARM.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c                         | 37 +++++++++++++++++++++++++++++++++++--
>  arm/include/arm-common/gic.h      |  2 +-
>  arm/include/arm-common/kvm-arch.h | 18 ++++++++++++++----
>  arm/kvm-cpu.c                     |  4 +++-
>  4 files changed, 53 insertions(+), 8 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index 0ce40e4..c50d662 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -9,13 +9,24 @@
>  #include <linux/kernel.h>
>  #include <linux/kvm.h>
>  
> +/* Those names are not defined for ARM (yet) */
> +#ifndef KVM_VGIC_V3_ADDR_TYPE_DIST
> +#define KVM_VGIC_V3_ADDR_TYPE_DIST 2
> +#endif
> +
> +#ifndef KVM_VGIC_V3_ADDR_TYPE_REDIST
> +#define KVM_VGIC_V3_ADDR_TYPE_REDIST 3
> +#endif
> +
>  static int gic_fd = -1;
> +static int nr_redists;

Who sets this variable?
>  
>  static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  {
>  	int err;
>  	u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
>  	u64 dist_addr = ARM_GIC_DIST_BASE;
> +	u64 redist_addr = dist_addr - nr_redists * ARM_GIC_REDIST_SIZE;

You are doing a similar offsetting further down. Consider having a macro
that computes the redist base from the dist base.

>  	struct kvm_create_device gic_device = {
>  		.flags	= 0,
>  	};
> @@ -28,11 +39,19 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
>  		.addr	= (u64)(unsigned long)&dist_addr,
>  	};
> +	struct kvm_device_attr redist_attr = {
> +		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
> +		.attr	= KVM_VGIC_V3_ADDR_TYPE_REDIST,
> +		.addr	= (u64)(unsigned long)&redist_addr,
> +	};
>  
>  	switch (type) {
>  	case IRQCHIP_GICV2:
>  		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
>  		break;
> +	case IRQCHIP_GICV3:
> +		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V3;
> +		break;
>  	default:
>  		return -ENODEV;
>  	}
> @@ -48,6 +67,10 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  		dist_attr.attr = KVM_VGIC_V2_ADDR_TYPE_DIST;
>  		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
>  		break;
> +	case IRQCHIP_GICV3:
> +		dist_attr.attr = KVM_VGIC_V3_ADDR_TYPE_DIST;
> +		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &redist_attr);
> +		break;
>  	default:
>  		return -ENODEV;
>  	}
> @@ -55,6 +78,8 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  		return err;
>  
>  	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
> +	if (err)
> +		return err;

Looks like a fairly useless statement...

>  
>  	return err;
>  }
> @@ -162,17 +187,25 @@ void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
>  	u64 reg_prop[] = {
>  		cpu_to_fdt64(ARM_GIC_DIST_BASE),
>  		cpu_to_fdt64(ARM_GIC_DIST_SIZE),
> -		cpu_to_fdt64(ARM_GIC_CPUI_BASE),
> -		cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
> +		0, 0,				/* to be filled */
>  	};
>  
>  	switch (type) {
>  	case IRQCHIP_GICV2:
>  		compatible = "arm,cortex-a15-gic";
> +		reg_prop[2] = ARM_GIC_CPUI_BASE;
> +		reg_prop[3] = ARM_GIC_CPUI_SIZE;
> +		break;
> +	case IRQCHIP_GICV3:
> +		compatible = "arm,gic-v3";
> +		reg_prop[2] = ARM_GIC_DIST_BASE - nr_redists * ARM_GIC_REDIST_SIZE;
> +		reg_prop[3] = ARM_GIC_REDIST_SIZE * nr_redists;
>  		break;
>  	default:
>  		return;
>  	}
> +	reg_prop[2] = cpu_to_fdt64(reg_prop[2]);
> +	reg_prop[3] = cpu_to_fdt64(reg_prop[3]);

I'd find it more readable if you did the cpu_to_fdt64() as part of the
initial assignment.

>  
>  	_FDT(fdt_begin_node(fdt, "intc"));
>  	_FDT(fdt_property_string(fdt, "compatible", compatible));
> diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
> index f5f6707..8d6ab01 100644
> --- a/arm/include/arm-common/gic.h
> +++ b/arm/include/arm-common/gic.h
> @@ -21,7 +21,7 @@
>  #define GIC_MAX_CPUS			8
>  #define GIC_MAX_IRQ			255
>  
> -enum irqchip_type {IRQCHIP_DEFAULT, IRQCHIP_GICV2};
> +enum irqchip_type {IRQCHIP_DEFAULT, IRQCHIP_GICV2, IRQCHIP_GICV3};
>  
>  struct kvm;
>  
> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
> index 082131d..be66a76 100644
> --- a/arm/include/arm-common/kvm-arch.h
> +++ b/arm/include/arm-common/kvm-arch.h
> @@ -17,10 +17,8 @@
>  
>  #define ARM_GIC_DIST_BASE	(ARM_AXI_AREA - ARM_GIC_DIST_SIZE)
>  #define ARM_GIC_CPUI_BASE	(ARM_GIC_DIST_BASE - ARM_GIC_CPUI_SIZE)
> -#define ARM_GIC_SIZE		(ARM_GIC_DIST_SIZE + ARM_GIC_CPUI_SIZE)
>  
>  #define ARM_IOPORT_SIZE		(ARM_MMIO_AREA - ARM_IOPORT_AREA)
> -#define ARM_VIRTIO_MMIO_SIZE	(ARM_AXI_AREA - (ARM_MMIO_AREA + ARM_GIC_SIZE))
>  #define ARM_PCI_CFG_SIZE	(1ULL << 24)
>  #define ARM_PCI_MMIO_SIZE	(ARM_MEMORY_AREA - \
>  				(ARM_AXI_AREA + ARM_PCI_CFG_SIZE))
> @@ -30,6 +28,13 @@
>  #define KVM_PCI_MMIO_AREA	(KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE)
>  #define KVM_VIRTIO_MMIO_AREA	ARM_MMIO_AREA
>  
> +/*
> + * On a GICv3 there must be one redistributor per vCPU.
> + * The value here is the size for one, we multiply this at runtime with
> + * the number of requested vCPUs to get the actual size.
> + */
> +#define ARM_GIC_REDIST_SIZE	0x20000
> +
>  #define KVM_IRQ_OFFSET		GIC_SPI_IRQ_BASE
>  
>  #define KVM_VM_TYPE		0
> @@ -45,9 +50,14 @@ static inline bool arm_addr_in_ioport_region(u64 phys_addr)
>  	return phys_addr >= KVM_IOPORT_AREA && phys_addr < limit;
>  }
>  
> -static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
> +static inline bool arm_addr_in_virtio_mmio_region(int nr_redists, u64 phys_addr)
>  {
> -	u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
> +	u64 limit = ARM_AXI_AREA - ARM_GIC_DIST_SIZE;
> +
> +	if (nr_redists)
> +		limit -= ARM_GIC_REDIST_SIZE * nr_redists;
> +	else
> +		limit -= ARM_GIC_CPUI_SIZE;
>  	return phys_addr >= KVM_VIRTIO_MMIO_AREA && phys_addr < limit;
>  }
>  
> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
> index ab08815..a3344fa 100644
> --- a/arm/kvm-cpu.c
> +++ b/arm/kvm-cpu.c
> @@ -142,7 +142,9 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
>  bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
>  			   u32 len, u8 is_write)
>  {
> -	if (arm_addr_in_virtio_mmio_region(phys_addr)) {
> +	int nr_redists = 0;
> +
> +	if (arm_addr_in_virtio_mmio_region(nr_redists, phys_addr)) {
>  		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>  	} else if (arm_addr_in_ioport_region(phys_addr)) {
>  		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
> 

Ouch. This feels really ugly. Why don't you have the GIC code export a
structure that contains the boundaries of the GIC (irrespective of its
type), and use that to compute the limit? I don't think we want this
nr_redists to propagate beyond the GIC code at all.

Thanks,

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

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH v2 7/8] arm: add support for supplying GICv3 redistributor addresses
@ 2015-06-10 17:40     ` Marc Zyngier
  0 siblings, 0 replies; 34+ messages in thread
From: Marc Zyngier @ 2015-06-10 17:40 UTC (permalink / raw)
  To: linux-arm-kernel

On 05/06/15 09:37, Andre Przywara wrote:
> The code currently is assuming fixed sized memory regions for the
> distributor and CPU interface. GICv3 needs a dynamic allocation of
> its redistributor region, since its size depends on the number of
> vCPUs.
> Also add the necessary code to create a GICv3 IRQ chip instance.
> This contains some defines which are not (yet) in the (32 bit) header
> files to allow compilation for ARM.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c                         | 37 +++++++++++++++++++++++++++++++++++--
>  arm/include/arm-common/gic.h      |  2 +-
>  arm/include/arm-common/kvm-arch.h | 18 ++++++++++++++----
>  arm/kvm-cpu.c                     |  4 +++-
>  4 files changed, 53 insertions(+), 8 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index 0ce40e4..c50d662 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -9,13 +9,24 @@
>  #include <linux/kernel.h>
>  #include <linux/kvm.h>
>  
> +/* Those names are not defined for ARM (yet) */
> +#ifndef KVM_VGIC_V3_ADDR_TYPE_DIST
> +#define KVM_VGIC_V3_ADDR_TYPE_DIST 2
> +#endif
> +
> +#ifndef KVM_VGIC_V3_ADDR_TYPE_REDIST
> +#define KVM_VGIC_V3_ADDR_TYPE_REDIST 3
> +#endif
> +
>  static int gic_fd = -1;
> +static int nr_redists;

Who sets this variable?
>  
>  static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  {
>  	int err;
>  	u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
>  	u64 dist_addr = ARM_GIC_DIST_BASE;
> +	u64 redist_addr = dist_addr - nr_redists * ARM_GIC_REDIST_SIZE;

You are doing a similar offsetting further down. Consider having a macro
that computes the redist base from the dist base.

>  	struct kvm_create_device gic_device = {
>  		.flags	= 0,
>  	};
> @@ -28,11 +39,19 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
>  		.addr	= (u64)(unsigned long)&dist_addr,
>  	};
> +	struct kvm_device_attr redist_attr = {
> +		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
> +		.attr	= KVM_VGIC_V3_ADDR_TYPE_REDIST,
> +		.addr	= (u64)(unsigned long)&redist_addr,
> +	};
>  
>  	switch (type) {
>  	case IRQCHIP_GICV2:
>  		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
>  		break;
> +	case IRQCHIP_GICV3:
> +		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V3;
> +		break;
>  	default:
>  		return -ENODEV;
>  	}
> @@ -48,6 +67,10 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  		dist_attr.attr = KVM_VGIC_V2_ADDR_TYPE_DIST;
>  		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
>  		break;
> +	case IRQCHIP_GICV3:
> +		dist_attr.attr = KVM_VGIC_V3_ADDR_TYPE_DIST;
> +		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &redist_attr);
> +		break;
>  	default:
>  		return -ENODEV;
>  	}
> @@ -55,6 +78,8 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  		return err;
>  
>  	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
> +	if (err)
> +		return err;

Looks like a fairly useless statement...

>  
>  	return err;
>  }
> @@ -162,17 +187,25 @@ void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
>  	u64 reg_prop[] = {
>  		cpu_to_fdt64(ARM_GIC_DIST_BASE),
>  		cpu_to_fdt64(ARM_GIC_DIST_SIZE),
> -		cpu_to_fdt64(ARM_GIC_CPUI_BASE),
> -		cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
> +		0, 0,				/* to be filled */
>  	};
>  
>  	switch (type) {
>  	case IRQCHIP_GICV2:
>  		compatible = "arm,cortex-a15-gic";
> +		reg_prop[2] = ARM_GIC_CPUI_BASE;
> +		reg_prop[3] = ARM_GIC_CPUI_SIZE;
> +		break;
> +	case IRQCHIP_GICV3:
> +		compatible = "arm,gic-v3";
> +		reg_prop[2] = ARM_GIC_DIST_BASE - nr_redists * ARM_GIC_REDIST_SIZE;
> +		reg_prop[3] = ARM_GIC_REDIST_SIZE * nr_redists;
>  		break;
>  	default:
>  		return;
>  	}
> +	reg_prop[2] = cpu_to_fdt64(reg_prop[2]);
> +	reg_prop[3] = cpu_to_fdt64(reg_prop[3]);

I'd find it more readable if you did the cpu_to_fdt64() as part of the
initial assignment.

>  
>  	_FDT(fdt_begin_node(fdt, "intc"));
>  	_FDT(fdt_property_string(fdt, "compatible", compatible));
> diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
> index f5f6707..8d6ab01 100644
> --- a/arm/include/arm-common/gic.h
> +++ b/arm/include/arm-common/gic.h
> @@ -21,7 +21,7 @@
>  #define GIC_MAX_CPUS			8
>  #define GIC_MAX_IRQ			255
>  
> -enum irqchip_type {IRQCHIP_DEFAULT, IRQCHIP_GICV2};
> +enum irqchip_type {IRQCHIP_DEFAULT, IRQCHIP_GICV2, IRQCHIP_GICV3};
>  
>  struct kvm;
>  
> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
> index 082131d..be66a76 100644
> --- a/arm/include/arm-common/kvm-arch.h
> +++ b/arm/include/arm-common/kvm-arch.h
> @@ -17,10 +17,8 @@
>  
>  #define ARM_GIC_DIST_BASE	(ARM_AXI_AREA - ARM_GIC_DIST_SIZE)
>  #define ARM_GIC_CPUI_BASE	(ARM_GIC_DIST_BASE - ARM_GIC_CPUI_SIZE)
> -#define ARM_GIC_SIZE		(ARM_GIC_DIST_SIZE + ARM_GIC_CPUI_SIZE)
>  
>  #define ARM_IOPORT_SIZE		(ARM_MMIO_AREA - ARM_IOPORT_AREA)
> -#define ARM_VIRTIO_MMIO_SIZE	(ARM_AXI_AREA - (ARM_MMIO_AREA + ARM_GIC_SIZE))
>  #define ARM_PCI_CFG_SIZE	(1ULL << 24)
>  #define ARM_PCI_MMIO_SIZE	(ARM_MEMORY_AREA - \
>  				(ARM_AXI_AREA + ARM_PCI_CFG_SIZE))
> @@ -30,6 +28,13 @@
>  #define KVM_PCI_MMIO_AREA	(KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE)
>  #define KVM_VIRTIO_MMIO_AREA	ARM_MMIO_AREA
>  
> +/*
> + * On a GICv3 there must be one redistributor per vCPU.
> + * The value here is the size for one, we multiply this at runtime with
> + * the number of requested vCPUs to get the actual size.
> + */
> +#define ARM_GIC_REDIST_SIZE	0x20000
> +
>  #define KVM_IRQ_OFFSET		GIC_SPI_IRQ_BASE
>  
>  #define KVM_VM_TYPE		0
> @@ -45,9 +50,14 @@ static inline bool arm_addr_in_ioport_region(u64 phys_addr)
>  	return phys_addr >= KVM_IOPORT_AREA && phys_addr < limit;
>  }
>  
> -static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
> +static inline bool arm_addr_in_virtio_mmio_region(int nr_redists, u64 phys_addr)
>  {
> -	u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
> +	u64 limit = ARM_AXI_AREA - ARM_GIC_DIST_SIZE;
> +
> +	if (nr_redists)
> +		limit -= ARM_GIC_REDIST_SIZE * nr_redists;
> +	else
> +		limit -= ARM_GIC_CPUI_SIZE;
>  	return phys_addr >= KVM_VIRTIO_MMIO_AREA && phys_addr < limit;
>  }
>  
> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
> index ab08815..a3344fa 100644
> --- a/arm/kvm-cpu.c
> +++ b/arm/kvm-cpu.c
> @@ -142,7 +142,9 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
>  bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
>  			   u32 len, u8 is_write)
>  {
> -	if (arm_addr_in_virtio_mmio_region(phys_addr)) {
> +	int nr_redists = 0;
> +
> +	if (arm_addr_in_virtio_mmio_region(nr_redists, phys_addr)) {
>  		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>  	} else if (arm_addr_in_ioport_region(phys_addr)) {
>  		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
> 

Ouch. This feels really ugly. Why don't you have the GIC code export a
structure that contains the boundaries of the GIC (irrespective of its
type), and use that to compute the limit? I don't think we want this
nr_redists to propagate beyond the GIC code at all.

Thanks,

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

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 8/8] arm: use new irqchip parameter to create different vGIC types
  2015-06-05  8:37   ` Andre Przywara
@ 2015-06-10 17:48     ` Marc Zyngier
  -1 siblings, 0 replies; 34+ messages in thread
From: Marc Zyngier @ 2015-06-10 17:48 UTC (permalink / raw)
  To: Andre Przywara, Will Deacon, penberg; +Cc: linux-arm-kernel, kvmarm, kvm

On 05/06/15 09:37, Andre Przywara wrote:
> Currently we unconditionally create a virtual GICv2 in the guest.
> Add a --irqchip= parameter to let the user specify a different GIC
> type for the guest.
> For now we the only other supported type is GICv3.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/aarch64/arm-cpu.c                    |  2 +-
>  arm/gic.c                                | 21 +++++++++++++++++++++
>  arm/include/arm-common/kvm-config-arch.h |  9 ++++++++-
>  arm/kvm-cpu.c                            |  6 ++++++
>  arm/kvm.c                                |  4 +++-
>  5 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
> index f702b9e..3dc8ea3 100644
> --- a/arm/aarch64/arm-cpu.c
> +++ b/arm/aarch64/arm-cpu.c
> @@ -12,7 +12,7 @@
>  static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
>  {
>  	int timer_interrupts[4] = {13, 14, 11, 10};
> -	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
> +	gic__generate_fdt_nodes(fdt, gic_phandle, kvm->cfg.arch.irqchip);
>  	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
>  }
>  
> diff --git a/arm/gic.c b/arm/gic.c
> index c50d662..ab0f594 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -21,6 +21,23 @@
>  static int gic_fd = -1;
>  static int nr_redists;
>  
> +int irqchip_parser(const struct option *opt, const char *arg, int unset)
> +{
> +	enum irqchip_type *type = opt->value;
> +
> +	*type = IRQCHIP_DEFAULT;
> +	if (!strcmp(arg, "gicv2")) {
> +		*type = IRQCHIP_GICV2;
> +	} else if (!strcmp(arg, "gicv3")) {
> +		*type = IRQCHIP_GICV3;
> +	} else if (strcmp(arg, "default")) {
> +		fprintf(stderr, "irqchip: unknown type \"%s\"\n", arg);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>  static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  {
>  	int err;
> @@ -121,6 +138,10 @@ int gic__create(struct kvm *kvm, enum irqchip_type type)
>  	case IRQCHIP_GICV2:
>  		max_cpus = GIC_MAX_CPUS;
>  		break;
> +	case IRQCHIP_GICV3:
> +		nr_redists = kvm->cfg.nrcpus;
> +		max_cpus = 255;

Having a #define for this would be nice. But more importantly, the
actual limit is the one KVM imposes, and the code should probably
reflect this.

> +		break;
>  	default:
>  		return -ENODEV;
>  	}
> diff --git a/arm/include/arm-common/kvm-config-arch.h b/arm/include/arm-common/kvm-config-arch.h
> index a8ebd94..ae4e89b 100644
> --- a/arm/include/arm-common/kvm-config-arch.h
> +++ b/arm/include/arm-common/kvm-config-arch.h
> @@ -8,8 +8,11 @@ struct kvm_config_arch {
>  	unsigned int	force_cntfrq;
>  	bool		virtio_trans_pci;
>  	bool		aarch32_guest;
> +	int		irqchip;
>  };
>  
> +int irqchip_parser(const struct option *opt, const char *arg, int unset);
> +
>  #define OPT_ARCH_RUN(pfx, cfg)							\
>  	pfx,									\
>  	ARM_OPT_ARCH_RUN(cfg)							\
> @@ -21,6 +24,10 @@ struct kvm_config_arch {
>  		     "updated to program CNTFRQ correctly*"),			\
>  	OPT_BOOLEAN('\0', "force-pci", &(cfg)->virtio_trans_pci,		\
>  		    "Force virtio devices to use PCI as their default "		\
> -		    "transport"),
> +		    "transport"),						\
> +        OPT_CALLBACK('\0', "irqchip", &(cfg)->irqchip,				\
> +		     "[gicv2|gicv3]",					\
> +		     "type of interrupt controller to emulate in the guest",	\
> +		     irqchip_parser, NULL),
>  
>  #endif /* ARM_COMMON__KVM_CONFIG_ARCH_H */
> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
> index a3344fa..aacc172 100644
> --- a/arm/kvm-cpu.c
> +++ b/arm/kvm-cpu.c
> @@ -144,6 +144,12 @@ bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
>  {
>  	int nr_redists = 0;
>  
> +	switch (vcpu->kvm->cfg.arch.irqchip) {
> +	case IRQCHIP_GICV3:
> +		nr_redists = vcpu->kvm->nrcpus;
> +		break;
> +	}
> +
>  	if (arm_addr_in_virtio_mmio_region(nr_redists, phys_addr)) {
>  		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>  	} else if (arm_addr_in_ioport_region(phys_addr)) {
> diff --git a/arm/kvm.c b/arm/kvm.c
> index f9685c2..2628d31 100644
> --- a/arm/kvm.c
> +++ b/arm/kvm.c
> @@ -82,6 +82,8 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
>  		MADV_MERGEABLE | MADV_HUGEPAGE);
>  
>  	/* Create the virtual GIC. */
> -	if (gic__create(kvm, IRQCHIP_GICV2))
> +	if (kvm->cfg.arch.irqchip == IRQCHIP_DEFAULT)
> +		kvm->cfg.arch.irqchip = IRQCHIP_GICV2;
> +	if (gic__create(kvm, kvm->cfg.arch.irqchip))
>  		die("Failed to create virtual GIC");
>  }
> 

I think you really need to revisit this the way you propagate the GIC
stuff outside of the GIC code. IRQCHIP_DEFAULT should be IRQCHIP_GICV2,
there is no reason for it to be otherwise. Same goes for the nr_redists
thing. Keep it localized as much as possible, and only export synthetic
information to the rest of kvmtool.

Thanks,

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

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH v2 8/8] arm: use new irqchip parameter to create different vGIC types
@ 2015-06-10 17:48     ` Marc Zyngier
  0 siblings, 0 replies; 34+ messages in thread
From: Marc Zyngier @ 2015-06-10 17:48 UTC (permalink / raw)
  To: linux-arm-kernel

On 05/06/15 09:37, Andre Przywara wrote:
> Currently we unconditionally create a virtual GICv2 in the guest.
> Add a --irqchip= parameter to let the user specify a different GIC
> type for the guest.
> For now we the only other supported type is GICv3.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/aarch64/arm-cpu.c                    |  2 +-
>  arm/gic.c                                | 21 +++++++++++++++++++++
>  arm/include/arm-common/kvm-config-arch.h |  9 ++++++++-
>  arm/kvm-cpu.c                            |  6 ++++++
>  arm/kvm.c                                |  4 +++-
>  5 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
> index f702b9e..3dc8ea3 100644
> --- a/arm/aarch64/arm-cpu.c
> +++ b/arm/aarch64/arm-cpu.c
> @@ -12,7 +12,7 @@
>  static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
>  {
>  	int timer_interrupts[4] = {13, 14, 11, 10};
> -	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
> +	gic__generate_fdt_nodes(fdt, gic_phandle, kvm->cfg.arch.irqchip);
>  	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
>  }
>  
> diff --git a/arm/gic.c b/arm/gic.c
> index c50d662..ab0f594 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -21,6 +21,23 @@
>  static int gic_fd = -1;
>  static int nr_redists;
>  
> +int irqchip_parser(const struct option *opt, const char *arg, int unset)
> +{
> +	enum irqchip_type *type = opt->value;
> +
> +	*type = IRQCHIP_DEFAULT;
> +	if (!strcmp(arg, "gicv2")) {
> +		*type = IRQCHIP_GICV2;
> +	} else if (!strcmp(arg, "gicv3")) {
> +		*type = IRQCHIP_GICV3;
> +	} else if (strcmp(arg, "default")) {
> +		fprintf(stderr, "irqchip: unknown type \"%s\"\n", arg);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>  static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  {
>  	int err;
> @@ -121,6 +138,10 @@ int gic__create(struct kvm *kvm, enum irqchip_type type)
>  	case IRQCHIP_GICV2:
>  		max_cpus = GIC_MAX_CPUS;
>  		break;
> +	case IRQCHIP_GICV3:
> +		nr_redists = kvm->cfg.nrcpus;
> +		max_cpus = 255;

Having a #define for this would be nice. But more importantly, the
actual limit is the one KVM imposes, and the code should probably
reflect this.

> +		break;
>  	default:
>  		return -ENODEV;
>  	}
> diff --git a/arm/include/arm-common/kvm-config-arch.h b/arm/include/arm-common/kvm-config-arch.h
> index a8ebd94..ae4e89b 100644
> --- a/arm/include/arm-common/kvm-config-arch.h
> +++ b/arm/include/arm-common/kvm-config-arch.h
> @@ -8,8 +8,11 @@ struct kvm_config_arch {
>  	unsigned int	force_cntfrq;
>  	bool		virtio_trans_pci;
>  	bool		aarch32_guest;
> +	int		irqchip;
>  };
>  
> +int irqchip_parser(const struct option *opt, const char *arg, int unset);
> +
>  #define OPT_ARCH_RUN(pfx, cfg)							\
>  	pfx,									\
>  	ARM_OPT_ARCH_RUN(cfg)							\
> @@ -21,6 +24,10 @@ struct kvm_config_arch {
>  		     "updated to program CNTFRQ correctly*"),			\
>  	OPT_BOOLEAN('\0', "force-pci", &(cfg)->virtio_trans_pci,		\
>  		    "Force virtio devices to use PCI as their default "		\
> -		    "transport"),
> +		    "transport"),						\
> +        OPT_CALLBACK('\0', "irqchip", &(cfg)->irqchip,				\
> +		     "[gicv2|gicv3]",					\
> +		     "type of interrupt controller to emulate in the guest",	\
> +		     irqchip_parser, NULL),
>  
>  #endif /* ARM_COMMON__KVM_CONFIG_ARCH_H */
> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
> index a3344fa..aacc172 100644
> --- a/arm/kvm-cpu.c
> +++ b/arm/kvm-cpu.c
> @@ -144,6 +144,12 @@ bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
>  {
>  	int nr_redists = 0;
>  
> +	switch (vcpu->kvm->cfg.arch.irqchip) {
> +	case IRQCHIP_GICV3:
> +		nr_redists = vcpu->kvm->nrcpus;
> +		break;
> +	}
> +
>  	if (arm_addr_in_virtio_mmio_region(nr_redists, phys_addr)) {
>  		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>  	} else if (arm_addr_in_ioport_region(phys_addr)) {
> diff --git a/arm/kvm.c b/arm/kvm.c
> index f9685c2..2628d31 100644
> --- a/arm/kvm.c
> +++ b/arm/kvm.c
> @@ -82,6 +82,8 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
>  		MADV_MERGEABLE | MADV_HUGEPAGE);
>  
>  	/* Create the virtual GIC. */
> -	if (gic__create(kvm, IRQCHIP_GICV2))
> +	if (kvm->cfg.arch.irqchip == IRQCHIP_DEFAULT)
> +		kvm->cfg.arch.irqchip = IRQCHIP_GICV2;
> +	if (gic__create(kvm, kvm->cfg.arch.irqchip))
>  		die("Failed to create virtual GIC");
>  }
> 

I think you really need to revisit this the way you propagate the GIC
stuff outside of the GIC code. IRQCHIP_DEFAULT should be IRQCHIP_GICV2,
there is no reason for it to be otherwise. Same goes for the nr_redists
thing. Keep it localized as much as possible, and only export synthetic
information to the rest of kvmtool.

Thanks,

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

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 6/8] arm: prepare for instantiating different IRQ chip devices
  2015-06-10 17:21     ` Marc Zyngier
@ 2015-06-15 10:46       ` Andre Przywara
  -1 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-15 10:46 UTC (permalink / raw)
  To: Marc Zyngier, Will Deacon, penberg; +Cc: linux-arm-kernel, kvmarm, kvm

Hi Marc,

On 06/10/2015 06:21 PM, Marc Zyngier wrote:
> On 05/06/15 09:37, Andre Przywara wrote:
>> Extend the vGIC handling code to potentially deal with different IRQ
>> chip devices instead of hard-coding the GICv2 in.
>> We extend most vGIC functions to take a type parameter, but still put
>> GICv2 in at the top for the time being.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
...
>> @@ -26,21 +26,37 @@ static int gic__create_device(struct kvm *kvm)
>>  	};
>>  	struct kvm_device_attr dist_attr = {
>>  		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
>> -		.attr	= KVM_VGIC_V2_ADDR_TYPE_DIST,
>>  		.addr	= (u64)(unsigned long)&dist_addr,
>>  	};
>>  
>> +	switch (type) {
>> +	case IRQCHIP_GICV2:
>> +		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
>> +		break;
>> +	default:
>> +		return -ENODEV;
>> +	}
>> +
>>  	err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &gic_device);
>>  	if (err)
>>  		return err;
>>  
>>  	gic_fd = gic_device.fd;
>>  
>> -	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
>> +	switch (type) {
>> +	case IRQCHIP_GICV2:
>> +		dist_attr.attr = KVM_VGIC_V2_ADDR_TYPE_DIST;
> 
> You could move the structure patching in the first switch statement.
> 
>> +		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
>> +		break;
>> +	default:
>> +		return -ENODEV;
> 
> This default cannot be reached, as you've already caught the weird stuff
> above.

Tell that the compiler, not me ;-)
Will check if dropping IRQCHIP_DEFAULT will appease the compiler.

....

>> @@ -131,15 +156,26 @@ static int gic__init_gic(struct kvm *kvm)
>>  }
>>  late_init(gic__init_gic)
>>  
>> -void gic__generate_fdt_nodes(void *fdt, u32 phandle)
>> +void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
>>  {
>> +	const char *compatible;
>>  	u64 reg_prop[] = {
>> -		cpu_to_fdt64(ARM_GIC_DIST_BASE), cpu_to_fdt64(ARM_GIC_DIST_SIZE),
>> -		cpu_to_fdt64(ARM_GIC_CPUI_BASE), cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
>> +		cpu_to_fdt64(ARM_GIC_DIST_BASE),
>> +		cpu_to_fdt64(ARM_GIC_DIST_SIZE),
>> +		cpu_to_fdt64(ARM_GIC_CPUI_BASE),
>> +		cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
>>  	};
> 
> Any particular reason for this change? I found the original more readable...

80 characters. I will revert this.

Fixed the rest.

Thanks!
Andre.

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH v2 6/8] arm: prepare for instantiating different IRQ chip devices
@ 2015-06-15 10:46       ` Andre Przywara
  0 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-15 10:46 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Marc,

On 06/10/2015 06:21 PM, Marc Zyngier wrote:
> On 05/06/15 09:37, Andre Przywara wrote:
>> Extend the vGIC handling code to potentially deal with different IRQ
>> chip devices instead of hard-coding the GICv2 in.
>> We extend most vGIC functions to take a type parameter, but still put
>> GICv2 in at the top for the time being.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
...
>> @@ -26,21 +26,37 @@ static int gic__create_device(struct kvm *kvm)
>>  	};
>>  	struct kvm_device_attr dist_attr = {
>>  		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
>> -		.attr	= KVM_VGIC_V2_ADDR_TYPE_DIST,
>>  		.addr	= (u64)(unsigned long)&dist_addr,
>>  	};
>>  
>> +	switch (type) {
>> +	case IRQCHIP_GICV2:
>> +		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
>> +		break;
>> +	default:
>> +		return -ENODEV;
>> +	}
>> +
>>  	err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &gic_device);
>>  	if (err)
>>  		return err;
>>  
>>  	gic_fd = gic_device.fd;
>>  
>> -	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
>> +	switch (type) {
>> +	case IRQCHIP_GICV2:
>> +		dist_attr.attr = KVM_VGIC_V2_ADDR_TYPE_DIST;
> 
> You could move the structure patching in the first switch statement.
> 
>> +		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
>> +		break;
>> +	default:
>> +		return -ENODEV;
> 
> This default cannot be reached, as you've already caught the weird stuff
> above.

Tell that the compiler, not me ;-)
Will check if dropping IRQCHIP_DEFAULT will appease the compiler.

....

>> @@ -131,15 +156,26 @@ static int gic__init_gic(struct kvm *kvm)
>>  }
>>  late_init(gic__init_gic)
>>  
>> -void gic__generate_fdt_nodes(void *fdt, u32 phandle)
>> +void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
>>  {
>> +	const char *compatible;
>>  	u64 reg_prop[] = {
>> -		cpu_to_fdt64(ARM_GIC_DIST_BASE), cpu_to_fdt64(ARM_GIC_DIST_SIZE),
>> -		cpu_to_fdt64(ARM_GIC_CPUI_BASE), cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
>> +		cpu_to_fdt64(ARM_GIC_DIST_BASE),
>> +		cpu_to_fdt64(ARM_GIC_DIST_SIZE),
>> +		cpu_to_fdt64(ARM_GIC_CPUI_BASE),
>> +		cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
>>  	};
> 
> Any particular reason for this change? I found the original more readable...

80 characters. I will revert this.

Fixed the rest.

Thanks!
Andre.

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 7/8] arm: add support for supplying GICv3 redistributor addresses
  2015-06-10 17:40     ` Marc Zyngier
@ 2015-06-15 11:12       ` Andre Przywara
  -1 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-15 11:12 UTC (permalink / raw)
  To: Marc Zyngier, Will Deacon; +Cc: penberg, kvmarm, kvm, linux-arm-kernel

On 06/10/2015 06:40 PM, Marc Zyngier wrote:
> On 05/06/15 09:37, Andre Przywara wrote:
>> The code currently is assuming fixed sized memory regions for the
>> distributor and CPU interface. GICv3 needs a dynamic allocation of
>> its redistributor region, since its size depends on the number of
>> vCPUs.
>> Also add the necessary code to create a GICv3 IRQ chip instance.
>> This contains some defines which are not (yet) in the (32 bit) header
>> files to allow compilation for ARM.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  arm/gic.c                         | 37 +++++++++++++++++++++++++++++++++++--
>>  arm/include/arm-common/gic.h      |  2 +-
>>  arm/include/arm-common/kvm-arch.h | 18 ++++++++++++++----
>>  arm/kvm-cpu.c                     |  4 +++-
>>  4 files changed, 53 insertions(+), 8 deletions(-)
>>
>> diff --git a/arm/gic.c b/arm/gic.c
>> index 0ce40e4..c50d662 100644
>> --- a/arm/gic.c
>> +++ b/arm/gic.c
>> @@ -9,13 +9,24 @@
>>  #include <linux/kernel.h>
>>  #include <linux/kvm.h>
>>  
>> +/* Those names are not defined for ARM (yet) */
>> +#ifndef KVM_VGIC_V3_ADDR_TYPE_DIST
>> +#define KVM_VGIC_V3_ADDR_TYPE_DIST 2
>> +#endif
>> +
>> +#ifndef KVM_VGIC_V3_ADDR_TYPE_REDIST
>> +#define KVM_VGIC_V3_ADDR_TYPE_REDIST 3
>> +#endif
>> +
>>  static int gic_fd = -1;
>> +static int nr_redists;
> 
> Who sets this variable?
>>  
>>  static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>>  {
>>  	int err;
>>  	u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
>>  	u64 dist_addr = ARM_GIC_DIST_BASE;
>> +	u64 redist_addr = dist_addr - nr_redists * ARM_GIC_REDIST_SIZE;
> 
> You are doing a similar offsetting further down. Consider having a macro
> that computes the redist base from the dist base.
> 
>>  	struct kvm_create_device gic_device = {
>>  		.flags	= 0,
>>  	};
>> @@ -28,11 +39,19 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>>  		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
>>  		.addr	= (u64)(unsigned long)&dist_addr,
>>  	};
>> +	struct kvm_device_attr redist_attr = {
>> +		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +		.attr	= KVM_VGIC_V3_ADDR_TYPE_REDIST,
>> +		.addr	= (u64)(unsigned long)&redist_addr,
>> +	};
>>  
>>  	switch (type) {
>>  	case IRQCHIP_GICV2:
>>  		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
>>  		break;
>> +	case IRQCHIP_GICV3:
>> +		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V3;
>> +		break;
>>  	default:
>>  		return -ENODEV;
>>  	}
>> @@ -48,6 +67,10 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>>  		dist_attr.attr = KVM_VGIC_V2_ADDR_TYPE_DIST;
>>  		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
>>  		break;
>> +	case IRQCHIP_GICV3:
>> +		dist_attr.attr = KVM_VGIC_V3_ADDR_TYPE_DIST;
>> +		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &redist_attr);
>> +		break;
>>  	default:
>>  		return -ENODEV;
>>  	}
>> @@ -55,6 +78,8 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>>  		return err;
>>  
>>  	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
>> +	if (err)
>> +		return err;
> 
> Looks like a fairly useless statement...

Sorry, rebasing artefact, this gets amended in the next patch. I have
fixed it now in here.

>>  
>>  	return err;
>>  }
>> @@ -162,17 +187,25 @@ void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
>>  	u64 reg_prop[] = {
>>  		cpu_to_fdt64(ARM_GIC_DIST_BASE),
>>  		cpu_to_fdt64(ARM_GIC_DIST_SIZE),
>> -		cpu_to_fdt64(ARM_GIC_CPUI_BASE),
>> -		cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
>> +		0, 0,				/* to be filled */
>>  	};
>>  
>>  	switch (type) {
>>  	case IRQCHIP_GICV2:
>>  		compatible = "arm,cortex-a15-gic";
>> +		reg_prop[2] = ARM_GIC_CPUI_BASE;
>> +		reg_prop[3] = ARM_GIC_CPUI_SIZE;
>> +		break;
>> +	case IRQCHIP_GICV3:
>> +		compatible = "arm,gic-v3";
>> +		reg_prop[2] = ARM_GIC_DIST_BASE - nr_redists * ARM_GIC_REDIST_SIZE;
>> +		reg_prop[3] = ARM_GIC_REDIST_SIZE * nr_redists;
>>  		break;
>>  	default:
>>  		return;
>>  	}
>> +	reg_prop[2] = cpu_to_fdt64(reg_prop[2]);
>> +	reg_prop[3] = cpu_to_fdt64(reg_prop[3]);
> 
> I'd find it more readable if you did the cpu_to_fdt64() as part of the
> initial assignment.

Agreed, that looks much nicer now that I use a separate variable for the
GIC redist base address (instead of nr_redist).

>>  
>>  	_FDT(fdt_begin_node(fdt, "intc"));
>>  	_FDT(fdt_property_string(fdt, "compatible", compatible));
>> diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
>> index f5f6707..8d6ab01 100644
>> --- a/arm/include/arm-common/gic.h
>> +++ b/arm/include/arm-common/gic.h
>> @@ -21,7 +21,7 @@
>>  #define GIC_MAX_CPUS			8
>>  #define GIC_MAX_IRQ			255
>>  
>> -enum irqchip_type {IRQCHIP_DEFAULT, IRQCHIP_GICV2};
>> +enum irqchip_type {IRQCHIP_DEFAULT, IRQCHIP_GICV2, IRQCHIP_GICV3};
>>  
>>  struct kvm;
>>  
>> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
>> index 082131d..be66a76 100644
>> --- a/arm/include/arm-common/kvm-arch.h
>> +++ b/arm/include/arm-common/kvm-arch.h
>> @@ -17,10 +17,8 @@
>>  
>>  #define ARM_GIC_DIST_BASE	(ARM_AXI_AREA - ARM_GIC_DIST_SIZE)
>>  #define ARM_GIC_CPUI_BASE	(ARM_GIC_DIST_BASE - ARM_GIC_CPUI_SIZE)
>> -#define ARM_GIC_SIZE		(ARM_GIC_DIST_SIZE + ARM_GIC_CPUI_SIZE)
>>  
>>  #define ARM_IOPORT_SIZE		(ARM_MMIO_AREA - ARM_IOPORT_AREA)
>> -#define ARM_VIRTIO_MMIO_SIZE	(ARM_AXI_AREA - (ARM_MMIO_AREA + ARM_GIC_SIZE))
>>  #define ARM_PCI_CFG_SIZE	(1ULL << 24)
>>  #define ARM_PCI_MMIO_SIZE	(ARM_MEMORY_AREA - \
>>  				(ARM_AXI_AREA + ARM_PCI_CFG_SIZE))
>> @@ -30,6 +28,13 @@
>>  #define KVM_PCI_MMIO_AREA	(KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE)
>>  #define KVM_VIRTIO_MMIO_AREA	ARM_MMIO_AREA
>>  
>> +/*
>> + * On a GICv3 there must be one redistributor per vCPU.
>> + * The value here is the size for one, we multiply this at runtime with
>> + * the number of requested vCPUs to get the actual size.
>> + */
>> +#define ARM_GIC_REDIST_SIZE	0x20000
>> +
>>  #define KVM_IRQ_OFFSET		GIC_SPI_IRQ_BASE
>>  
>>  #define KVM_VM_TYPE		0
>> @@ -45,9 +50,14 @@ static inline bool arm_addr_in_ioport_region(u64 phys_addr)
>>  	return phys_addr >= KVM_IOPORT_AREA && phys_addr < limit;
>>  }
>>  
>> -static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
>> +static inline bool arm_addr_in_virtio_mmio_region(int nr_redists, u64 phys_addr)
>>  {
>> -	u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
>> +	u64 limit = ARM_AXI_AREA - ARM_GIC_DIST_SIZE;
>> +
>> +	if (nr_redists)
>> +		limit -= ARM_GIC_REDIST_SIZE * nr_redists;
>> +	else
>> +		limit -= ARM_GIC_CPUI_SIZE;
>>  	return phys_addr >= KVM_VIRTIO_MMIO_AREA && phys_addr < limit;
>>  }
>>  
>> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
>> index ab08815..a3344fa 100644
>> --- a/arm/kvm-cpu.c
>> +++ b/arm/kvm-cpu.c
>> @@ -142,7 +142,9 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
>>  bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
>>  			   u32 len, u8 is_write)
>>  {
>> -	if (arm_addr_in_virtio_mmio_region(phys_addr)) {
>> +	int nr_redists = 0;
>> +
>> +	if (arm_addr_in_virtio_mmio_region(nr_redists, phys_addr)) {
>>  		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>>  	} else if (arm_addr_in_ioport_region(phys_addr)) {
>>  		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
>>
> 
> Ouch. This feels really ugly. Why don't you have the GIC code export a
> structure that contains the boundaries of the GIC (irrespective of its
> type), and use that to compute the limit? I don't think we want this
> nr_redists to propagate beyond the GIC code at all.

Looking more closely at the code I wonder why we differentiate beyond
the IO port region anyway. I rewrote this now without actually checking
for the GIC region at all. This simplified a lot and allows us to get
rid of nr_redists completely.

Cheers,
Andre.

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH v2 7/8] arm: add support for supplying GICv3 redistributor addresses
@ 2015-06-15 11:12       ` Andre Przywara
  0 siblings, 0 replies; 34+ messages in thread
From: Andre Przywara @ 2015-06-15 11:12 UTC (permalink / raw)
  To: linux-arm-kernel

On 06/10/2015 06:40 PM, Marc Zyngier wrote:
> On 05/06/15 09:37, Andre Przywara wrote:
>> The code currently is assuming fixed sized memory regions for the
>> distributor and CPU interface. GICv3 needs a dynamic allocation of
>> its redistributor region, since its size depends on the number of
>> vCPUs.
>> Also add the necessary code to create a GICv3 IRQ chip instance.
>> This contains some defines which are not (yet) in the (32 bit) header
>> files to allow compilation for ARM.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  arm/gic.c                         | 37 +++++++++++++++++++++++++++++++++++--
>>  arm/include/arm-common/gic.h      |  2 +-
>>  arm/include/arm-common/kvm-arch.h | 18 ++++++++++++++----
>>  arm/kvm-cpu.c                     |  4 +++-
>>  4 files changed, 53 insertions(+), 8 deletions(-)
>>
>> diff --git a/arm/gic.c b/arm/gic.c
>> index 0ce40e4..c50d662 100644
>> --- a/arm/gic.c
>> +++ b/arm/gic.c
>> @@ -9,13 +9,24 @@
>>  #include <linux/kernel.h>
>>  #include <linux/kvm.h>
>>  
>> +/* Those names are not defined for ARM (yet) */
>> +#ifndef KVM_VGIC_V3_ADDR_TYPE_DIST
>> +#define KVM_VGIC_V3_ADDR_TYPE_DIST 2
>> +#endif
>> +
>> +#ifndef KVM_VGIC_V3_ADDR_TYPE_REDIST
>> +#define KVM_VGIC_V3_ADDR_TYPE_REDIST 3
>> +#endif
>> +
>>  static int gic_fd = -1;
>> +static int nr_redists;
> 
> Who sets this variable?
>>  
>>  static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>>  {
>>  	int err;
>>  	u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
>>  	u64 dist_addr = ARM_GIC_DIST_BASE;
>> +	u64 redist_addr = dist_addr - nr_redists * ARM_GIC_REDIST_SIZE;
> 
> You are doing a similar offsetting further down. Consider having a macro
> that computes the redist base from the dist base.
> 
>>  	struct kvm_create_device gic_device = {
>>  		.flags	= 0,
>>  	};
>> @@ -28,11 +39,19 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>>  		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
>>  		.addr	= (u64)(unsigned long)&dist_addr,
>>  	};
>> +	struct kvm_device_attr redist_attr = {
>> +		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +		.attr	= KVM_VGIC_V3_ADDR_TYPE_REDIST,
>> +		.addr	= (u64)(unsigned long)&redist_addr,
>> +	};
>>  
>>  	switch (type) {
>>  	case IRQCHIP_GICV2:
>>  		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
>>  		break;
>> +	case IRQCHIP_GICV3:
>> +		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V3;
>> +		break;
>>  	default:
>>  		return -ENODEV;
>>  	}
>> @@ -48,6 +67,10 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>>  		dist_attr.attr = KVM_VGIC_V2_ADDR_TYPE_DIST;
>>  		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
>>  		break;
>> +	case IRQCHIP_GICV3:
>> +		dist_attr.attr = KVM_VGIC_V3_ADDR_TYPE_DIST;
>> +		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &redist_attr);
>> +		break;
>>  	default:
>>  		return -ENODEV;
>>  	}
>> @@ -55,6 +78,8 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>>  		return err;
>>  
>>  	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
>> +	if (err)
>> +		return err;
> 
> Looks like a fairly useless statement...

Sorry, rebasing artefact, this gets amended in the next patch. I have
fixed it now in here.

>>  
>>  	return err;
>>  }
>> @@ -162,17 +187,25 @@ void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
>>  	u64 reg_prop[] = {
>>  		cpu_to_fdt64(ARM_GIC_DIST_BASE),
>>  		cpu_to_fdt64(ARM_GIC_DIST_SIZE),
>> -		cpu_to_fdt64(ARM_GIC_CPUI_BASE),
>> -		cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
>> +		0, 0,				/* to be filled */
>>  	};
>>  
>>  	switch (type) {
>>  	case IRQCHIP_GICV2:
>>  		compatible = "arm,cortex-a15-gic";
>> +		reg_prop[2] = ARM_GIC_CPUI_BASE;
>> +		reg_prop[3] = ARM_GIC_CPUI_SIZE;
>> +		break;
>> +	case IRQCHIP_GICV3:
>> +		compatible = "arm,gic-v3";
>> +		reg_prop[2] = ARM_GIC_DIST_BASE - nr_redists * ARM_GIC_REDIST_SIZE;
>> +		reg_prop[3] = ARM_GIC_REDIST_SIZE * nr_redists;
>>  		break;
>>  	default:
>>  		return;
>>  	}
>> +	reg_prop[2] = cpu_to_fdt64(reg_prop[2]);
>> +	reg_prop[3] = cpu_to_fdt64(reg_prop[3]);
> 
> I'd find it more readable if you did the cpu_to_fdt64() as part of the
> initial assignment.

Agreed, that looks much nicer now that I use a separate variable for the
GIC redist base address (instead of nr_redist).

>>  
>>  	_FDT(fdt_begin_node(fdt, "intc"));
>>  	_FDT(fdt_property_string(fdt, "compatible", compatible));
>> diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
>> index f5f6707..8d6ab01 100644
>> --- a/arm/include/arm-common/gic.h
>> +++ b/arm/include/arm-common/gic.h
>> @@ -21,7 +21,7 @@
>>  #define GIC_MAX_CPUS			8
>>  #define GIC_MAX_IRQ			255
>>  
>> -enum irqchip_type {IRQCHIP_DEFAULT, IRQCHIP_GICV2};
>> +enum irqchip_type {IRQCHIP_DEFAULT, IRQCHIP_GICV2, IRQCHIP_GICV3};
>>  
>>  struct kvm;
>>  
>> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
>> index 082131d..be66a76 100644
>> --- a/arm/include/arm-common/kvm-arch.h
>> +++ b/arm/include/arm-common/kvm-arch.h
>> @@ -17,10 +17,8 @@
>>  
>>  #define ARM_GIC_DIST_BASE	(ARM_AXI_AREA - ARM_GIC_DIST_SIZE)
>>  #define ARM_GIC_CPUI_BASE	(ARM_GIC_DIST_BASE - ARM_GIC_CPUI_SIZE)
>> -#define ARM_GIC_SIZE		(ARM_GIC_DIST_SIZE + ARM_GIC_CPUI_SIZE)
>>  
>>  #define ARM_IOPORT_SIZE		(ARM_MMIO_AREA - ARM_IOPORT_AREA)
>> -#define ARM_VIRTIO_MMIO_SIZE	(ARM_AXI_AREA - (ARM_MMIO_AREA + ARM_GIC_SIZE))
>>  #define ARM_PCI_CFG_SIZE	(1ULL << 24)
>>  #define ARM_PCI_MMIO_SIZE	(ARM_MEMORY_AREA - \
>>  				(ARM_AXI_AREA + ARM_PCI_CFG_SIZE))
>> @@ -30,6 +28,13 @@
>>  #define KVM_PCI_MMIO_AREA	(KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE)
>>  #define KVM_VIRTIO_MMIO_AREA	ARM_MMIO_AREA
>>  
>> +/*
>> + * On a GICv3 there must be one redistributor per vCPU.
>> + * The value here is the size for one, we multiply this at runtime with
>> + * the number of requested vCPUs to get the actual size.
>> + */
>> +#define ARM_GIC_REDIST_SIZE	0x20000
>> +
>>  #define KVM_IRQ_OFFSET		GIC_SPI_IRQ_BASE
>>  
>>  #define KVM_VM_TYPE		0
>> @@ -45,9 +50,14 @@ static inline bool arm_addr_in_ioport_region(u64 phys_addr)
>>  	return phys_addr >= KVM_IOPORT_AREA && phys_addr < limit;
>>  }
>>  
>> -static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
>> +static inline bool arm_addr_in_virtio_mmio_region(int nr_redists, u64 phys_addr)
>>  {
>> -	u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
>> +	u64 limit = ARM_AXI_AREA - ARM_GIC_DIST_SIZE;
>> +
>> +	if (nr_redists)
>> +		limit -= ARM_GIC_REDIST_SIZE * nr_redists;
>> +	else
>> +		limit -= ARM_GIC_CPUI_SIZE;
>>  	return phys_addr >= KVM_VIRTIO_MMIO_AREA && phys_addr < limit;
>>  }
>>  
>> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
>> index ab08815..a3344fa 100644
>> --- a/arm/kvm-cpu.c
>> +++ b/arm/kvm-cpu.c
>> @@ -142,7 +142,9 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
>>  bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
>>  			   u32 len, u8 is_write)
>>  {
>> -	if (arm_addr_in_virtio_mmio_region(phys_addr)) {
>> +	int nr_redists = 0;
>> +
>> +	if (arm_addr_in_virtio_mmio_region(nr_redists, phys_addr)) {
>>  		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>>  	} else if (arm_addr_in_ioport_region(phys_addr)) {
>>  		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
>>
> 
> Ouch. This feels really ugly. Why don't you have the GIC code export a
> structure that contains the boundaries of the GIC (irrespective of its
> type), and use that to compute the limit? I don't think we want this
> nr_redists to propagate beyond the GIC code at all.

Looking more closely at the code I wonder why we differentiate beyond
the IO port region anyway. I rewrote this now without actually checking
for the GIC region at all. This simplified a lot and allows us to get
rid of nr_redists completely.

Cheers,
Andre.

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 7/8] arm: add support for supplying GICv3 redistributor addresses
  2015-06-15 11:12       ` Andre Przywara
@ 2015-06-15 11:56         ` Marc Zyngier
  -1 siblings, 0 replies; 34+ messages in thread
From: Marc Zyngier @ 2015-06-15 11:56 UTC (permalink / raw)
  To: Andre Przywara, Will Deacon; +Cc: penberg, kvm, kvmarm, linux-arm-kernel

On 15/06/15 12:12, Andre Przywara wrote:
> On 06/10/2015 06:40 PM, Marc Zyngier wrote:
>> On 05/06/15 09:37, Andre Przywara wrote:
>>> The code currently is assuming fixed sized memory regions for the
>>> distributor and CPU interface. GICv3 needs a dynamic allocation of
>>> its redistributor region, since its size depends on the number of
>>> vCPUs.
>>> Also add the necessary code to create a GICv3 IRQ chip instance.
>>> This contains some defines which are not (yet) in the (32 bit) header
>>> files to allow compilation for ARM.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>  arm/gic.c                         | 37 +++++++++++++++++++++++++++++++++++--
>>>  arm/include/arm-common/gic.h      |  2 +-
>>>  arm/include/arm-common/kvm-arch.h | 18 ++++++++++++++----
>>>  arm/kvm-cpu.c                     |  4 +++-
>>>  4 files changed, 53 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/arm/gic.c b/arm/gic.c
>>> index 0ce40e4..c50d662 100644
>>> --- a/arm/gic.c
>>> +++ b/arm/gic.c
>>> @@ -9,13 +9,24 @@
>>>  #include <linux/kernel.h>
>>>  #include <linux/kvm.h>
>>>  
>>> +/* Those names are not defined for ARM (yet) */
>>> +#ifndef KVM_VGIC_V3_ADDR_TYPE_DIST
>>> +#define KVM_VGIC_V3_ADDR_TYPE_DIST 2
>>> +#endif
>>> +
>>> +#ifndef KVM_VGIC_V3_ADDR_TYPE_REDIST
>>> +#define KVM_VGIC_V3_ADDR_TYPE_REDIST 3
>>> +#endif
>>> +
>>>  static int gic_fd = -1;
>>> +static int nr_redists;
>>
>> Who sets this variable?
>>>  
>>>  static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>>>  {
>>>  	int err;
>>>  	u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
>>>  	u64 dist_addr = ARM_GIC_DIST_BASE;
>>> +	u64 redist_addr = dist_addr - nr_redists * ARM_GIC_REDIST_SIZE;
>>
>> You are doing a similar offsetting further down. Consider having a macro
>> that computes the redist base from the dist base.
>>
>>>  	struct kvm_create_device gic_device = {
>>>  		.flags	= 0,
>>>  	};
>>> @@ -28,11 +39,19 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>>>  		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
>>>  		.addr	= (u64)(unsigned long)&dist_addr,
>>>  	};
>>> +	struct kvm_device_attr redist_attr = {
>>> +		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
>>> +		.attr	= KVM_VGIC_V3_ADDR_TYPE_REDIST,
>>> +		.addr	= (u64)(unsigned long)&redist_addr,
>>> +	};
>>>  
>>>  	switch (type) {
>>>  	case IRQCHIP_GICV2:
>>>  		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
>>>  		break;
>>> +	case IRQCHIP_GICV3:
>>> +		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V3;
>>> +		break;
>>>  	default:
>>>  		return -ENODEV;
>>>  	}
>>> @@ -48,6 +67,10 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>>>  		dist_attr.attr = KVM_VGIC_V2_ADDR_TYPE_DIST;
>>>  		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
>>>  		break;
>>> +	case IRQCHIP_GICV3:
>>> +		dist_attr.attr = KVM_VGIC_V3_ADDR_TYPE_DIST;
>>> +		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &redist_attr);
>>> +		break;
>>>  	default:
>>>  		return -ENODEV;
>>>  	}
>>> @@ -55,6 +78,8 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>>>  		return err;
>>>  
>>>  	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
>>> +	if (err)
>>> +		return err;
>>
>> Looks like a fairly useless statement...
> 
> Sorry, rebasing artefact, this gets amended in the next patch. I have
> fixed it now in here.
> 
>>>  
>>>  	return err;
>>>  }
>>> @@ -162,17 +187,25 @@ void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
>>>  	u64 reg_prop[] = {
>>>  		cpu_to_fdt64(ARM_GIC_DIST_BASE),
>>>  		cpu_to_fdt64(ARM_GIC_DIST_SIZE),
>>> -		cpu_to_fdt64(ARM_GIC_CPUI_BASE),
>>> -		cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
>>> +		0, 0,				/* to be filled */
>>>  	};
>>>  
>>>  	switch (type) {
>>>  	case IRQCHIP_GICV2:
>>>  		compatible = "arm,cortex-a15-gic";
>>> +		reg_prop[2] = ARM_GIC_CPUI_BASE;
>>> +		reg_prop[3] = ARM_GIC_CPUI_SIZE;
>>> +		break;
>>> +	case IRQCHIP_GICV3:
>>> +		compatible = "arm,gic-v3";
>>> +		reg_prop[2] = ARM_GIC_DIST_BASE - nr_redists * ARM_GIC_REDIST_SIZE;
>>> +		reg_prop[3] = ARM_GIC_REDIST_SIZE * nr_redists;
>>>  		break;
>>>  	default:
>>>  		return;
>>>  	}
>>> +	reg_prop[2] = cpu_to_fdt64(reg_prop[2]);
>>> +	reg_prop[3] = cpu_to_fdt64(reg_prop[3]);
>>
>> I'd find it more readable if you did the cpu_to_fdt64() as part of the
>> initial assignment.
> 
> Agreed, that looks much nicer now that I use a separate variable for the
> GIC redist base address (instead of nr_redist).
> 
>>>  
>>>  	_FDT(fdt_begin_node(fdt, "intc"));
>>>  	_FDT(fdt_property_string(fdt, "compatible", compatible));
>>> diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
>>> index f5f6707..8d6ab01 100644
>>> --- a/arm/include/arm-common/gic.h
>>> +++ b/arm/include/arm-common/gic.h
>>> @@ -21,7 +21,7 @@
>>>  #define GIC_MAX_CPUS			8
>>>  #define GIC_MAX_IRQ			255
>>>  
>>> -enum irqchip_type {IRQCHIP_DEFAULT, IRQCHIP_GICV2};
>>> +enum irqchip_type {IRQCHIP_DEFAULT, IRQCHIP_GICV2, IRQCHIP_GICV3};
>>>  
>>>  struct kvm;
>>>  
>>> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
>>> index 082131d..be66a76 100644
>>> --- a/arm/include/arm-common/kvm-arch.h
>>> +++ b/arm/include/arm-common/kvm-arch.h
>>> @@ -17,10 +17,8 @@
>>>  
>>>  #define ARM_GIC_DIST_BASE	(ARM_AXI_AREA - ARM_GIC_DIST_SIZE)
>>>  #define ARM_GIC_CPUI_BASE	(ARM_GIC_DIST_BASE - ARM_GIC_CPUI_SIZE)
>>> -#define ARM_GIC_SIZE		(ARM_GIC_DIST_SIZE + ARM_GIC_CPUI_SIZE)
>>>  
>>>  #define ARM_IOPORT_SIZE		(ARM_MMIO_AREA - ARM_IOPORT_AREA)
>>> -#define ARM_VIRTIO_MMIO_SIZE	(ARM_AXI_AREA - (ARM_MMIO_AREA + ARM_GIC_SIZE))
>>>  #define ARM_PCI_CFG_SIZE	(1ULL << 24)
>>>  #define ARM_PCI_MMIO_SIZE	(ARM_MEMORY_AREA - \
>>>  				(ARM_AXI_AREA + ARM_PCI_CFG_SIZE))
>>> @@ -30,6 +28,13 @@
>>>  #define KVM_PCI_MMIO_AREA	(KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE)
>>>  #define KVM_VIRTIO_MMIO_AREA	ARM_MMIO_AREA
>>>  
>>> +/*
>>> + * On a GICv3 there must be one redistributor per vCPU.
>>> + * The value here is the size for one, we multiply this at runtime with
>>> + * the number of requested vCPUs to get the actual size.
>>> + */
>>> +#define ARM_GIC_REDIST_SIZE	0x20000
>>> +
>>>  #define KVM_IRQ_OFFSET		GIC_SPI_IRQ_BASE
>>>  
>>>  #define KVM_VM_TYPE		0
>>> @@ -45,9 +50,14 @@ static inline bool arm_addr_in_ioport_region(u64 phys_addr)
>>>  	return phys_addr >= KVM_IOPORT_AREA && phys_addr < limit;
>>>  }
>>>  
>>> -static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
>>> +static inline bool arm_addr_in_virtio_mmio_region(int nr_redists, u64 phys_addr)
>>>  {
>>> -	u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
>>> +	u64 limit = ARM_AXI_AREA - ARM_GIC_DIST_SIZE;
>>> +
>>> +	if (nr_redists)
>>> +		limit -= ARM_GIC_REDIST_SIZE * nr_redists;
>>> +	else
>>> +		limit -= ARM_GIC_CPUI_SIZE;
>>>  	return phys_addr >= KVM_VIRTIO_MMIO_AREA && phys_addr < limit;
>>>  }
>>>  
>>> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
>>> index ab08815..a3344fa 100644
>>> --- a/arm/kvm-cpu.c
>>> +++ b/arm/kvm-cpu.c
>>> @@ -142,7 +142,9 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
>>>  bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
>>>  			   u32 len, u8 is_write)
>>>  {
>>> -	if (arm_addr_in_virtio_mmio_region(phys_addr)) {
>>> +	int nr_redists = 0;
>>> +
>>> +	if (arm_addr_in_virtio_mmio_region(nr_redists, phys_addr)) {
>>>  		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>>>  	} else if (arm_addr_in_ioport_region(phys_addr)) {
>>>  		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
>>>
>>
>> Ouch. This feels really ugly. Why don't you have the GIC code export a
>> structure that contains the boundaries of the GIC (irrespective of its
>> type), and use that to compute the limit? I don't think we want this
>> nr_redists to propagate beyond the GIC code at all.
> 
> Looking more closely at the code I wonder why we differentiate beyond
> the IO port region anyway. I rewrote this now without actually checking
> for the GIC region at all. This simplified a lot and allows us to get
> rid of nr_redists completely.

Not sure about that. Returning to userspace on access to the vgic region
is a good indication that something went wrong (no GIC instantiated?).

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

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH v2 7/8] arm: add support for supplying GICv3 redistributor addresses
@ 2015-06-15 11:56         ` Marc Zyngier
  0 siblings, 0 replies; 34+ messages in thread
From: Marc Zyngier @ 2015-06-15 11:56 UTC (permalink / raw)
  To: linux-arm-kernel

On 15/06/15 12:12, Andre Przywara wrote:
> On 06/10/2015 06:40 PM, Marc Zyngier wrote:
>> On 05/06/15 09:37, Andre Przywara wrote:
>>> The code currently is assuming fixed sized memory regions for the
>>> distributor and CPU interface. GICv3 needs a dynamic allocation of
>>> its redistributor region, since its size depends on the number of
>>> vCPUs.
>>> Also add the necessary code to create a GICv3 IRQ chip instance.
>>> This contains some defines which are not (yet) in the (32 bit) header
>>> files to allow compilation for ARM.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>  arm/gic.c                         | 37 +++++++++++++++++++++++++++++++++++--
>>>  arm/include/arm-common/gic.h      |  2 +-
>>>  arm/include/arm-common/kvm-arch.h | 18 ++++++++++++++----
>>>  arm/kvm-cpu.c                     |  4 +++-
>>>  4 files changed, 53 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/arm/gic.c b/arm/gic.c
>>> index 0ce40e4..c50d662 100644
>>> --- a/arm/gic.c
>>> +++ b/arm/gic.c
>>> @@ -9,13 +9,24 @@
>>>  #include <linux/kernel.h>
>>>  #include <linux/kvm.h>
>>>  
>>> +/* Those names are not defined for ARM (yet) */
>>> +#ifndef KVM_VGIC_V3_ADDR_TYPE_DIST
>>> +#define KVM_VGIC_V3_ADDR_TYPE_DIST 2
>>> +#endif
>>> +
>>> +#ifndef KVM_VGIC_V3_ADDR_TYPE_REDIST
>>> +#define KVM_VGIC_V3_ADDR_TYPE_REDIST 3
>>> +#endif
>>> +
>>>  static int gic_fd = -1;
>>> +static int nr_redists;
>>
>> Who sets this variable?
>>>  
>>>  static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>>>  {
>>>  	int err;
>>>  	u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
>>>  	u64 dist_addr = ARM_GIC_DIST_BASE;
>>> +	u64 redist_addr = dist_addr - nr_redists * ARM_GIC_REDIST_SIZE;
>>
>> You are doing a similar offsetting further down. Consider having a macro
>> that computes the redist base from the dist base.
>>
>>>  	struct kvm_create_device gic_device = {
>>>  		.flags	= 0,
>>>  	};
>>> @@ -28,11 +39,19 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>>>  		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
>>>  		.addr	= (u64)(unsigned long)&dist_addr,
>>>  	};
>>> +	struct kvm_device_attr redist_attr = {
>>> +		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
>>> +		.attr	= KVM_VGIC_V3_ADDR_TYPE_REDIST,
>>> +		.addr	= (u64)(unsigned long)&redist_addr,
>>> +	};
>>>  
>>>  	switch (type) {
>>>  	case IRQCHIP_GICV2:
>>>  		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
>>>  		break;
>>> +	case IRQCHIP_GICV3:
>>> +		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V3;
>>> +		break;
>>>  	default:
>>>  		return -ENODEV;
>>>  	}
>>> @@ -48,6 +67,10 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>>>  		dist_attr.attr = KVM_VGIC_V2_ADDR_TYPE_DIST;
>>>  		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
>>>  		break;
>>> +	case IRQCHIP_GICV3:
>>> +		dist_attr.attr = KVM_VGIC_V3_ADDR_TYPE_DIST;
>>> +		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &redist_attr);
>>> +		break;
>>>  	default:
>>>  		return -ENODEV;
>>>  	}
>>> @@ -55,6 +78,8 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>>>  		return err;
>>>  
>>>  	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
>>> +	if (err)
>>> +		return err;
>>
>> Looks like a fairly useless statement...
> 
> Sorry, rebasing artefact, this gets amended in the next patch. I have
> fixed it now in here.
> 
>>>  
>>>  	return err;
>>>  }
>>> @@ -162,17 +187,25 @@ void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
>>>  	u64 reg_prop[] = {
>>>  		cpu_to_fdt64(ARM_GIC_DIST_BASE),
>>>  		cpu_to_fdt64(ARM_GIC_DIST_SIZE),
>>> -		cpu_to_fdt64(ARM_GIC_CPUI_BASE),
>>> -		cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
>>> +		0, 0,				/* to be filled */
>>>  	};
>>>  
>>>  	switch (type) {
>>>  	case IRQCHIP_GICV2:
>>>  		compatible = "arm,cortex-a15-gic";
>>> +		reg_prop[2] = ARM_GIC_CPUI_BASE;
>>> +		reg_prop[3] = ARM_GIC_CPUI_SIZE;
>>> +		break;
>>> +	case IRQCHIP_GICV3:
>>> +		compatible = "arm,gic-v3";
>>> +		reg_prop[2] = ARM_GIC_DIST_BASE - nr_redists * ARM_GIC_REDIST_SIZE;
>>> +		reg_prop[3] = ARM_GIC_REDIST_SIZE * nr_redists;
>>>  		break;
>>>  	default:
>>>  		return;
>>>  	}
>>> +	reg_prop[2] = cpu_to_fdt64(reg_prop[2]);
>>> +	reg_prop[3] = cpu_to_fdt64(reg_prop[3]);
>>
>> I'd find it more readable if you did the cpu_to_fdt64() as part of the
>> initial assignment.
> 
> Agreed, that looks much nicer now that I use a separate variable for the
> GIC redist base address (instead of nr_redist).
> 
>>>  
>>>  	_FDT(fdt_begin_node(fdt, "intc"));
>>>  	_FDT(fdt_property_string(fdt, "compatible", compatible));
>>> diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
>>> index f5f6707..8d6ab01 100644
>>> --- a/arm/include/arm-common/gic.h
>>> +++ b/arm/include/arm-common/gic.h
>>> @@ -21,7 +21,7 @@
>>>  #define GIC_MAX_CPUS			8
>>>  #define GIC_MAX_IRQ			255
>>>  
>>> -enum irqchip_type {IRQCHIP_DEFAULT, IRQCHIP_GICV2};
>>> +enum irqchip_type {IRQCHIP_DEFAULT, IRQCHIP_GICV2, IRQCHIP_GICV3};
>>>  
>>>  struct kvm;
>>>  
>>> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
>>> index 082131d..be66a76 100644
>>> --- a/arm/include/arm-common/kvm-arch.h
>>> +++ b/arm/include/arm-common/kvm-arch.h
>>> @@ -17,10 +17,8 @@
>>>  
>>>  #define ARM_GIC_DIST_BASE	(ARM_AXI_AREA - ARM_GIC_DIST_SIZE)
>>>  #define ARM_GIC_CPUI_BASE	(ARM_GIC_DIST_BASE - ARM_GIC_CPUI_SIZE)
>>> -#define ARM_GIC_SIZE		(ARM_GIC_DIST_SIZE + ARM_GIC_CPUI_SIZE)
>>>  
>>>  #define ARM_IOPORT_SIZE		(ARM_MMIO_AREA - ARM_IOPORT_AREA)
>>> -#define ARM_VIRTIO_MMIO_SIZE	(ARM_AXI_AREA - (ARM_MMIO_AREA + ARM_GIC_SIZE))
>>>  #define ARM_PCI_CFG_SIZE	(1ULL << 24)
>>>  #define ARM_PCI_MMIO_SIZE	(ARM_MEMORY_AREA - \
>>>  				(ARM_AXI_AREA + ARM_PCI_CFG_SIZE))
>>> @@ -30,6 +28,13 @@
>>>  #define KVM_PCI_MMIO_AREA	(KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE)
>>>  #define KVM_VIRTIO_MMIO_AREA	ARM_MMIO_AREA
>>>  
>>> +/*
>>> + * On a GICv3 there must be one redistributor per vCPU.
>>> + * The value here is the size for one, we multiply this at runtime with
>>> + * the number of requested vCPUs to get the actual size.
>>> + */
>>> +#define ARM_GIC_REDIST_SIZE	0x20000
>>> +
>>>  #define KVM_IRQ_OFFSET		GIC_SPI_IRQ_BASE
>>>  
>>>  #define KVM_VM_TYPE		0
>>> @@ -45,9 +50,14 @@ static inline bool arm_addr_in_ioport_region(u64 phys_addr)
>>>  	return phys_addr >= KVM_IOPORT_AREA && phys_addr < limit;
>>>  }
>>>  
>>> -static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
>>> +static inline bool arm_addr_in_virtio_mmio_region(int nr_redists, u64 phys_addr)
>>>  {
>>> -	u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
>>> +	u64 limit = ARM_AXI_AREA - ARM_GIC_DIST_SIZE;
>>> +
>>> +	if (nr_redists)
>>> +		limit -= ARM_GIC_REDIST_SIZE * nr_redists;
>>> +	else
>>> +		limit -= ARM_GIC_CPUI_SIZE;
>>>  	return phys_addr >= KVM_VIRTIO_MMIO_AREA && phys_addr < limit;
>>>  }
>>>  
>>> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
>>> index ab08815..a3344fa 100644
>>> --- a/arm/kvm-cpu.c
>>> +++ b/arm/kvm-cpu.c
>>> @@ -142,7 +142,9 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
>>>  bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
>>>  			   u32 len, u8 is_write)
>>>  {
>>> -	if (arm_addr_in_virtio_mmio_region(phys_addr)) {
>>> +	int nr_redists = 0;
>>> +
>>> +	if (arm_addr_in_virtio_mmio_region(nr_redists, phys_addr)) {
>>>  		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>>>  	} else if (arm_addr_in_ioport_region(phys_addr)) {
>>>  		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
>>>
>>
>> Ouch. This feels really ugly. Why don't you have the GIC code export a
>> structure that contains the boundaries of the GIC (irrespective of its
>> type), and use that to compute the limit? I don't think we want this
>> nr_redists to propagate beyond the GIC code at all.
> 
> Looking more closely at the code I wonder why we differentiate beyond
> the IO port region anyway. I rewrote this now without actually checking
> for the GIC region at all. This simplified a lot and allows us to get
> rid of nr_redists completely.

Not sure about that. Returning to userspace on access to the vgic region
is a good indication that something went wrong (no GIC instantiated?).

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

^ permalink raw reply	[flat|nested] 34+ messages in thread
end of thread, other threads:[~2015-06-15 11:56 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-05  8:37 [PATCH v2 0/8] kvmtool: arm64: GICv3 guest support Andre Przywara
2015-06-05  8:37 ` Andre Przywara
2015-06-05  8:37 ` [PATCH v2 1/8] AArch64: Reserve two 64k pages for GIC CPU interface Andre Przywara
2015-06-05  8:37   ` Andre Przywara
2015-06-05  8:37 ` [PATCH v2 2/8] AArch{32, 64}: use KVM_CREATE_DEVICE & co to instanciate the GIC Andre Przywara
2015-06-05  8:37   ` Andre Przywara
2015-06-05  8:37 ` [PATCH v2 3/8] irq: add irq__get_nr_allocated_lines Andre Przywara
2015-06-05  8:37   ` Andre Przywara
2015-06-05  8:37 ` [PATCH v2 4/8] AArch{32,64}: dynamically configure the number of GIC interrupts Andre Przywara
2015-06-05  8:37   ` [PATCH v2 4/8] AArch{32, 64}: " Andre Przywara
2015-06-05  8:37 ` [PATCH v2 5/8] arm: finish VGIC initialisation explicitly Andre Przywara
2015-06-05  8:37   ` Andre Przywara
2015-06-10 17:07   ` Marc Zyngier
2015-06-10 17:07     ` Marc Zyngier
2015-06-05  8:37 ` [PATCH v2 6/8] arm: prepare for instantiating different IRQ chip devices Andre Przywara
2015-06-05  8:37   ` Andre Przywara
2015-06-09 11:31   ` Andre Przywara
2015-06-09 11:31     ` Andre Przywara
2015-06-10 17:21   ` Marc Zyngier
2015-06-10 17:21     ` Marc Zyngier
2015-06-15 10:46     ` Andre Przywara
2015-06-15 10:46       ` Andre Przywara
2015-06-05  8:37 ` [PATCH v2 7/8] arm: add support for supplying GICv3 redistributor addresses Andre Przywara
2015-06-05  8:37   ` Andre Przywara
2015-06-10 17:40   ` Marc Zyngier
2015-06-10 17:40     ` Marc Zyngier
2015-06-15 11:12     ` Andre Przywara
2015-06-15 11:12       ` Andre Przywara
2015-06-15 11:56       ` Marc Zyngier
2015-06-15 11:56         ` Marc Zyngier
2015-06-05  8:37 ` [PATCH v2 8/8] arm: use new irqchip parameter to create different vGIC types Andre Przywara
2015-06-05  8:37   ` Andre Przywara
2015-06-10 17:48   ` Marc Zyngier
2015-06-10 17:48     ` Marc Zyngier

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.